mobile: Implement a native C++ iOS network change monitor #42832
Conversation
Previously, a network change monitor existed in the Objective-C implementation. However, those using the C++ native APIs directly on iOS could not use the iOS network change monitor. This change adds support for network change monitoring in iOS through the native C++ APIs. This implementation ports the network change monitor that the Envoy Mobile team has been using in the YouTube app, so the implementation has been validated in a production setting. Once this change has been thoroughly tested, the Objective-C API only network change monitor can be removed. Signed-off-by: Ali Beyad <abeyad@google.com>
|
/assign @danzh2010 |
abeyad
force-pushed
the
ios-network-change-monitor
branch
6 times, most recently
from
599fbc7 to
274fd9e
Compare
Signed-off-by: Ali Beyad <abeyad@google.com>
abeyad
force-pushed
the
ios-network-change-monitor
branch
from
274fd9e to
aecf9a0
Compare
|
/retest |
danzh2010
left a comment
danzh2010
left a comment
There was a problem hiding this comment.
This PR actually adds another Objective-C network monitor implementation but initialized it from native engine. The PR header and description is a bit misleading as it sounds like the implementation is in native code.
It might worth clarifying that the PR actually is doing 2 things: 1) adding the ability of starting an Apple network monitor from CC engine; 2) adding a new Objective-C network monitor based on YT app's implementation.
And as the PR description mentions that the existing Apple network monitor impl will be deprecated, if so would Object-C API and swift API be lack of network monitor?
There was a problem hiding this comment.
Why this file is named as bridge? It looks like the real implementation interacting with Apple's nw_path_monitor_t
| * Construct an AppleNetworkChangeMonitor. | ||
| * @param engine reference to a NetworkChangeListener. | ||
| */ | ||
| explicit AppleNetworkChangeMonitor(NetworkChangeListener& engine); |
There was a problem hiding this comment.
The param name engine doesn't reflect its type. It seems that Envoy CC engine is an implementation of NetworkChangeListener, but this class doesn't really care which implementation the listener is.
| std::shared_ptr<Envoy::Platform::NetworkChangeListener> network_change_listener_ptr( | ||
| &network_change_listener_, [](NetworkChangeListener *) { | ||
| // Empty deleter - we don't own the network change listener | ||
| }); |
There was a problem hiding this comment.
This looks weird. Since NetworkChangeListener is a stand alone interface, can we pass down an implementation of it which wraps an engine object, so that it can be passed down using shared_ptr?
There was a problem hiding this comment.
Why is this test file empty?
There was a problem hiding this comment.
It's worth mentioning in the PR how this network monitor impl is different from the original [Objective-C impl] (
) which is started from the Objective-C engine:envoy/mobile/library/objective-c/EnvoyEngineImpl.mm
Lines 428 to 431 in 1f75115
Do we just need an API to initialize Apple network monitor from the common CC engine? If so, I think it makes sense to have the same monitor implementation, either the original one or the new one added here, to be used regardless of which language API is used to start the engine.
2 participants
Previously, a network change monitor existed in the Objective-C implementation. However, those using the C++ native APIs directly on iOS could not use the iOS network change monitor. This change adds support for network change monitoring in iOS through the native C++ APIs.
This implementation ports the network change monitor that the Envoy Mobile team has been using in the YouTube app, so the implementation has been validated in a production setting.
Once this change has been thoroughly tested, the Objective-C API only network change monitor can be removed.