Create network change notifier.

Create network change notifier and pass the event to NetworkManager

BUG=

Committed: https://crrev.com/023f3ef0296511f12897c503d6fc2ed063712474
Cr-Commit-Position: refs/heads/master@{#10325}

[honghaiz3, 2015-10-08 22:03:47]

Patchset #1 (id:1) has been deleted

[honghaiz3, 2015-10-08 23:13:33]

Patchset #1 (id:20001) has been deleted

[honghaiz3, 2015-10-08 23:13:41]

Patchset #1 (id:40001) has been deleted

[honghaiz3, 2015-10-08 23:13:47]

Patchset #1 (id:60001) has been deleted

[honghaiz3, 2015-10-08 23:13:52]

Patchset #1 (id:80001) has been deleted

[honghaiz3, 2015-10-08 23:14:01]

Patchset #1 (id:100001) has been deleted

[honghaiz3, 2015-10-08 23:23:31]

Patchset #1 (id:120001) has been deleted

[honghaiz3, 2015-10-08 23:23:39]

Patchset #1 (id:140001) has been deleted

[honghaiz3, 2015-10-08 23:33:54]

honghaiz@webrtc.org changed reviewers:
+ pthatcher@webrtc.org

[honghaiz3, 2015-10-08 23:33:54]

I largely borrowed the java file from the Chromium file, so I used the name in
the Chromium file. Let me know if you want a different name. 
I try to remove (from the Chromium class) the part that is not directly related,
but still keep some that may be useful very soon, such as the net id. 
Working on the tests. 

Please take a look at the CL.

[honghaiz3, 2015-10-09 00:46:27]

Patchset #1 (id:160001) has been deleted

[pthatcher1, 2015-10-09 23:11:30]

https://codereview.webrtc.org/1391703003/diff/220001/webrtc/base/network.cc
File webrtc/base/network.cc (right):

https://codereview.webrtc.org/1391703003/diff/220001/webrtc/base/network.cc#n...
webrtc/base/network.cc:66: const uint32 kUpdateNetworksMessage = 1;
Similarly, kUpdateNetworksContinuallyMessage

https://codereview.webrtc.org/1391703003/diff/220001/webrtc/base/network.cc#n...
webrtc/base/network.cc:334: void BasicNetworkManager::OnNetworkChangeNotified()
{
I think OnNetworksChanged would be a better name.

https://codereview.webrtc.org/1391703003/diff/220001/webrtc/base/network.cc#n...
webrtc/base/network.cc:696:
network_change_notifier_.reset(factory->CreateNotifier());
Why do we have GetFactory()->CreateNotifier()?  Why not just call
NetworkChangeNotifier::Create()?

https://codereview.webrtc.org/1391703003/diff/220001/webrtc/base/network.h
File webrtc/base/network.h (right):

https://codereview.webrtc.org/1391703003/diff/220001/webrtc/base/network.h#ne...
webrtc/base/network.h:235: // Only updates the network; do not reschedule the
next update.
do not => does not

https://codereview.webrtc.org/1391703003/diff/220001/webrtc/base/network.h#ne...
webrtc/base/network.h:236: void DoUpdateNetworks();
I think a better name pair would be UpdateNetworksContinually/UpdateNetworksOnce
rather than UpdateNetworks/DoUpdateNetworks.

https://codereview.webrtc.org/1391703003/diff/220001/webrtc/base/networkchang...
File webrtc/base/networkchangenotifier.cc (right):

https://codereview.webrtc.org/1391703003/diff/220001/webrtc/base/networkchang...
webrtc/base/networkchangenotifier.cc:16: rtc::NetworkChangeNotifierFactory*
network_change_notifier_factory = nullptr;
Can we create the Java object from C++ and avoid needing a global variable?

https://codereview.webrtc.org/1391703003/diff/220001/webrtc/base/networkchang...
File webrtc/base/networkchangenotifier.h (right):

https://codereview.webrtc.org/1391703003/diff/220001/webrtc/base/networkchang...
webrtc/base/networkchangenotifier.h:71: };
This seems really complex.  Let's try and simplify it.

[honghaiz3, 2015-10-13 23:16:32]

Patchset #4 (id:240001) has been deleted

[honghaiz3, 2015-10-13 23:16:37]

Patchset #4 (id:260001) has been deleted

[honghaiz3, 2015-10-13 23:16:47]

Patchset #4 (id:280001) has been deleted

[honghaiz3, 2015-10-13 23:19:56]

Removed delegate but kept factory for testing and for not exposing jni namespace
in rtc namespace. 
PTAL.

https://codereview.webrtc.org/1391703003/diff/220001/webrtc/base/network.cc
File webrtc/base/network.cc (right):

https://codereview.webrtc.org/1391703003/diff/220001/webrtc/base/network.cc#n...
webrtc/base/network.cc:66: const uint32 kUpdateNetworksMessage = 1;
On 2015/10/09 23:11:29, pthatcher1 wrote:
> Similarly, kUpdateNetworksContinuallyMessage

I kept the message name because now I do not need to add a new message here. The
reason to move that message from here to the NetworkMonitor is to make the
NetwrkMonitor handle threading more safely.

https://codereview.webrtc.org/1391703003/diff/220001/webrtc/base/network.cc#n...
webrtc/base/network.cc:334: void BasicNetworkManager::OnNetworkChangeNotified()
{
On 2015/10/09 23:11:29, pthatcher1 wrote:
> I think OnNetworksChanged would be a better name.

Done.

https://codereview.webrtc.org/1391703003/diff/220001/webrtc/base/network.cc#n...
webrtc/base/network.cc:696:
network_change_notifier_.reset(factory->CreateNotifier());
On 2015/10/09 23:11:29, pthatcher1 wrote:
> Why do we have GetFactory()->CreateNotifier()?  Why not just call
> NetworkChangeNotifier::Create()?
I keep the factory as it helps unittests and allow this class not calling
something in the webrtc_jni namespace in the rtc namespace.

https://codereview.webrtc.org/1391703003/diff/220001/webrtc/base/network.h
File webrtc/base/network.h (right):

https://codereview.webrtc.org/1391703003/diff/220001/webrtc/base/network.h#ne...
webrtc/base/network.h:235: // Only updates the network; do not reschedule the
next update.
On 2015/10/09 23:11:29, pthatcher1 wrote:
> do not => does not

Done.

https://codereview.webrtc.org/1391703003/diff/220001/webrtc/base/network.h#ne...
webrtc/base/network.h:236: void DoUpdateNetworks();
On 2015/10/09 23:11:29, pthatcher1 wrote:
> I think a better name pair would be
UpdateNetworksContinually/UpdateNetworksOnce
> rather than UpdateNetworks/DoUpdateNetworks.

Done.

https://codereview.webrtc.org/1391703003/diff/220001/webrtc/base/networkchang...
File webrtc/base/networkchangenotifier.cc (right):

https://codereview.webrtc.org/1391703003/diff/220001/webrtc/base/networkchang...
webrtc/base/networkchangenotifier.cc:16: rtc::NetworkChangeNotifierFactory*
network_change_notifier_factory = nullptr;
On 2015/10/09 23:11:29, pthatcher1 wrote:
> Can we create the Java object from C++ and avoid needing a global variable? 
Now we create the Java object from c++. 
I still keep this factory to make it easier to inject a different factory in the
unittest and so that when creating the monitor here, it need not access the
namespace webrtc_jni.

https://codereview.webrtc.org/1391703003/diff/220001/webrtc/base/networkchang...
File webrtc/base/networkchangenotifier.h (right):

https://codereview.webrtc.org/1391703003/diff/220001/webrtc/base/networkchang...
webrtc/base/networkchangenotifier.h:71: };
On 2015/10/09 23:11:29, pthatcher1 wrote:
> This seems really complex.  Let's try and simplify it.
> 
> 

Done.

[honghaiz3, 2015-10-13 23:35:28]

Patchset #4 (id:300001) has been deleted

[honghaiz3, 2015-10-13 23:42:20]

Patchset #4 (id:320001) has been deleted

[pthatcher1, 2015-10-15 08:11:44]

I still need to look over the Java files, but it's looking much better on the
C++ side.

By the way, can you add an Android-experienced person, perhaps Alex, to review
the Java?

https://codereview.webrtc.org/1391703003/diff/340001/talk/app/webrtc/java/and...
File talk/app/webrtc/java/android/org/webrtc/NetworkMonitorAutoDetect.java
(right):

https://codereview.webrtc.org/1391703003/diff/340001/talk/app/webrtc/java/and...
talk/app/webrtc/java/android/org/webrtc/NetworkMonitorAutoDetect.java:73:
private final int subtype;
Later on these are defined by ConnectivityManager.TYPE_X and
TelephonyManager.NETWORK_TYPE_X.  Can you comment on that up here?  That would
make the values expected here much more clear.

https://codereview.webrtc.org/1391703003/diff/340001/talk/app/webrtc/java/jni...
File talk/app/webrtc/java/jni/peerconnection_jni.cc (right):

https://codereview.webrtc.org/1391703003/diff/340001/talk/app/webrtc/java/jni...
talk/app/webrtc/java/jni/peerconnection_jni.cc:1079: }
Shouldn't this be more like

if (network_monitor_factory_ ==  rtc::NetworkMonitorFactory::GetFactory()) {
  rtc::NetworkMonitorFactory::SetFactory(nullptr);
}

In case someone else called SetFactory?


And isn't that potentially thread unsafe?  Shouldn't that instead be something
like this?

if (network_monitory_factory_) {
  rtc::NetworkMonitoryFactory::RelaseFactory(network_monitor_factory_);
}

https://codereview.webrtc.org/1391703003/diff/340001/talk/app/webrtc/java/jni...
talk/app/webrtc/java/jni/peerconnection_jni.cc:1119: network_monitor_factory =
AndroidNetworkMonitorFactory::CreateFactory();
I think this would be more clear as:

network_monitor_factory = new AndroidNetworkMonitorFactory();
NetworkMonitorFactory::SetFactory(network_monitor_factory);

https://codereview.webrtc.org/1391703003/diff/340001/webrtc/base/network.cc
File webrtc/base/network.cc (right):

https://codereview.webrtc.org/1391703003/diff/340001/webrtc/base/network.cc#n...
webrtc/base/network.cc:701: this, &BasicNetworkManager::OnNetworksChanged);
Shouldn't we connect before starting?

https://codereview.webrtc.org/1391703003/diff/340001/webrtc/base/network.h
File webrtc/base/network.h (right):

https://codereview.webrtc.org/1391703003/diff/340001/webrtc/base/network.h#ne...
webrtc/base/network.h:226: // Creates a network monitor and starts to listen to
the update.
"and starts to listen to the update" => "and listens for network updates"

https://codereview.webrtc.org/1391703003/diff/340001/webrtc/base/network.h#ne...
webrtc/base/network.h:230: // Called when it receives the update from the
network monitor.
receives the update => receives updates

https://codereview.webrtc.org/1391703003/diff/340001/webrtc/base/network.h#ne...
webrtc/base/network.h:244: // It monitors the network changes and it may be NULL
if it is not injected.
"if it is not injected" is confusing, especially since it can be null if it was
never started.

I think you can just remove the whole comment.  The method comments are enough.

https://codereview.webrtc.org/1391703003/diff/340001/webrtc/base/networkmonit...
File webrtc/base/networkmonitor.cc (right):

https://codereview.webrtc.org/1391703003/diff/340001/webrtc/base/networkmonit...
webrtc/base/networkmonitor.cc:18: // A global network monitor factory injected
by the peer connection client.
I don't think you need to say "by the peer connection client" here.  Code not
related to PeerConnection, such as ORTC, might use this.

In fact, I think you could replace the whole line with "This is set by
NetworkMonitor::SetFactory"

https://codereview.webrtc.org/1391703003/diff/340001/webrtc/base/networkmonit...
webrtc/base/networkmonitor.cc:20: // no network monitor will be created. If a
client injected a factory, it is
Isn't it up to callers to decided what to do if there is no factory set?  Why
even comment on that here in this file?  I think you could remove these whole
comment with "The caller of NetworkMonitor::SetFactory must is responsible for
calling SetFactory(nullptr) when the factory is destroyed".

https://codereview.webrtc.org/1391703003/diff/340001/webrtc/base/networkmonit...
webrtc/base/networkmonitor.cc:40: network_monitor_factory = factory;
What's the thread safety on this?  Do we have to say it's thread unsafe?

https://codereview.webrtc.org/1391703003/diff/340001/webrtc/base/networkmonit...
File webrtc/base/networkmonitor.h (right):

https://codereview.webrtc.org/1391703003/diff/340001/webrtc/base/networkmonit...
webrtc/base/networkmonitor.h:37: class NetworkMonitor : public MessageHandler,
public sigslot::has_slots<> {
It seems like this should be called NetworkMontiorBase and it should implement
NetworkMonitorInterface.

https://codereview.webrtc.org/1391703003/diff/340001/webrtc/base/networkmonit...
webrtc/base/networkmonitor.h:47: virtual void StopMonitoring() = 0;
Why not just Start() and Stop()?  And I don't think the comments are necessary.

https://codereview.webrtc.org/1391703003/diff/340001/webrtc/base/networkmonit...
webrtc/base/networkmonitor.h:49: // Called when it receives a network-change
event from the network monitor.
The network monitor method is called when it receives a network change from the
network monitor?  That seems confusing.

Why not just say "implementations should call this method on the base when
networks change, and the base will fire SignalNetworksChanged on the right
thread".

[honghaiz3, 2015-10-15 19:02:42]

Addressed comments.

https://codereview.webrtc.org/1391703003/diff/340001/talk/app/webrtc/java/and...
File talk/app/webrtc/java/android/org/webrtc/NetworkMonitorAutoDetect.java
(right):

https://codereview.webrtc.org/1391703003/diff/340001/talk/app/webrtc/java/and...
talk/app/webrtc/java/android/org/webrtc/NetworkMonitorAutoDetect.java:73:
private final int subtype;
On 2015/10/15 08:11:43, pthatcher1 wrote:
> Later on these are defined by ConnectivityManager.TYPE_X and
> TelephonyManager.NETWORK_TYPE_X.  Can you comment on that up here?  That would
> make the values expected here much more clear.

Done.

https://codereview.webrtc.org/1391703003/diff/340001/talk/app/webrtc/java/jni...
File talk/app/webrtc/java/jni/peerconnection_jni.cc (right):

https://codereview.webrtc.org/1391703003/diff/340001/talk/app/webrtc/java/jni...
talk/app/webrtc/java/jni/peerconnection_jni.cc:1079: }
On 2015/10/15 08:11:43, pthatcher1 wrote:
> Shouldn't this be more like
> 
> if (network_monitor_factory_ ==  rtc::NetworkMonitorFactory::GetFactory()) {
>   rtc::NetworkMonitorFactory::SetFactory(nullptr);
> }
> 
> In case someone else called SetFactory?
> 
> 
> And isn't that potentially thread unsafe?  Shouldn't that instead be something
> like this?
> 
> if (network_monitory_factory_) {
>   rtc::NetworkMonitoryFactory::RelaseFactory(network_monitor_factory_);
> }

Done.

https://codereview.webrtc.org/1391703003/diff/340001/talk/app/webrtc/java/jni...
talk/app/webrtc/java/jni/peerconnection_jni.cc:1119: network_monitor_factory =
AndroidNetworkMonitorFactory::CreateFactory();
On 2015/10/15 08:11:43, pthatcher1 wrote:
> I think this would be more clear as:
> 
> network_monitor_factory = new AndroidNetworkMonitorFactory();
> NetworkMonitorFactory::SetFactory(network_monitor_factory);

Done.

https://codereview.webrtc.org/1391703003/diff/340001/webrtc/base/network.cc
File webrtc/base/network.cc (right):

https://codereview.webrtc.org/1391703003/diff/340001/webrtc/base/network.cc#n...
webrtc/base/network.cc:701: this, &BasicNetworkManager::OnNetworksChanged);
On 2015/10/15 08:11:43, pthatcher1 wrote:
> Shouldn't we connect before starting? 

Done.

https://codereview.webrtc.org/1391703003/diff/340001/webrtc/base/network.h
File webrtc/base/network.h (right):

https://codereview.webrtc.org/1391703003/diff/340001/webrtc/base/network.h#ne...
webrtc/base/network.h:226: // Creates a network monitor and starts to listen to
the update.
On 2015/10/15 08:11:43, pthatcher1 wrote:
> "and starts to listen to the update" => "and listens for network updates"

Done.

https://codereview.webrtc.org/1391703003/diff/340001/webrtc/base/network.h#ne...
webrtc/base/network.h:230: // Called when it receives the update from the
network monitor.
On 2015/10/15 08:11:43, pthatcher1 wrote:
> receives the update => receives updates

Done.

https://codereview.webrtc.org/1391703003/diff/340001/webrtc/base/network.h#ne...
webrtc/base/network.h:244: // It monitors the network changes and it may be NULL
if it is not injected.
On 2015/10/15 08:11:43, pthatcher1 wrote:
> "if it is not injected" is confusing, especially since it can be null if it
was
> never started.
> 
> I think you can just remove the whole comment.  The method comments are
enough.

Done.

https://codereview.webrtc.org/1391703003/diff/340001/webrtc/base/networkmonit...
File webrtc/base/networkmonitor.cc (right):

https://codereview.webrtc.org/1391703003/diff/340001/webrtc/base/networkmonit...
webrtc/base/networkmonitor.cc:18: // A global network monitor factory injected
by the peer connection client.
On 2015/10/15 08:11:43, pthatcher1 wrote:
> I don't think you need to say "by the peer connection client" here.  Code not
> related to PeerConnection, such as ORTC, might use this.
> 
> In fact, I think you could replace the whole line with "This is set by
> NetworkMonitor::SetFactory"

Done.

https://codereview.webrtc.org/1391703003/diff/340001/webrtc/base/networkmonit...
webrtc/base/networkmonitor.cc:20: // no network monitor will be created. If a
client injected a factory, it is
On 2015/10/15 08:11:43, pthatcher1 wrote:
> Isn't it up to callers to decided what to do if there is no factory set?  Why
> even comment on that here in this file?  I think you could remove these whole
> comment with "The caller of NetworkMonitor::SetFactory must is responsible for
> calling SetFactory(nullptr) when the factory is destroyed".

Done.

https://codereview.webrtc.org/1391703003/diff/340001/webrtc/base/networkmonit...
webrtc/base/networkmonitor.cc:40: network_monitor_factory = factory;
On 2015/10/15 08:11:43, pthatcher1 wrote:
> What's the thread safety on this?  Do we have to say it's thread unsafe?
Added comments.

https://codereview.webrtc.org/1391703003/diff/340001/webrtc/base/networkmonit...
File webrtc/base/networkmonitor.h (right):

https://codereview.webrtc.org/1391703003/diff/340001/webrtc/base/networkmonit...
webrtc/base/networkmonitor.h:37: class NetworkMonitor : public MessageHandler,
public sigslot::has_slots<> {
On 2015/10/15 08:11:43, pthatcher1 wrote:
> It seems like this should be called NetworkMontiorBase and it should implement
> NetworkMonitorInterface.  

Done.

https://codereview.webrtc.org/1391703003/diff/340001/webrtc/base/networkmonit...
webrtc/base/networkmonitor.h:47: virtual void StopMonitoring() = 0;
On 2015/10/15 08:11:43, pthatcher1 wrote:
> Why not just Start() and Stop()?  And I don't think the comments are
necessary.

Done.

https://codereview.webrtc.org/1391703003/diff/340001/webrtc/base/networkmonit...
webrtc/base/networkmonitor.h:49: // Called when it receives a network-change
event from the network monitor.
On 2015/10/15 08:11:43, pthatcher1 wrote:
> The network monitor method is called when it receives a network change from
the
> network monitor?  That seems confusing.
> 
> Why not just say "implementations should call this method on the base when
> networks change, and the base will fire SignalNetworksChanged on the right
> thread".

Done.

[honghaiz3, 2015-10-15 19:07:01]

honghaiz@webrtc.org changed reviewers:
+ glaznev@webrtc.org

[honghaiz3, 2015-10-15 19:07:02]

Alex, 

Can you take a look at the CL, especially on the Java part of the code? It was
largely borrowed from Chromium class NetworkChangeNotifier but renamed.

[honghaiz3, 2015-10-15 22:48:45]

Patchset #6 (id:380001) has been deleted

[honghaiz3, 2015-10-16 03:25:17]

Patchset #6 (id:400001) has been deleted

[honghaiz3, 2015-10-16 03:25:35]

Patchset #6 (id:420001) has been deleted

[honghaiz3, 2015-10-16 17:38:21]

Patchset #7 (id:460001) has been deleted

[honghaiz3, 2015-10-16 17:38:33]

Patchset #7 (id:480001) has been deleted

[honghaiz3, 2015-10-16 18:51:54]

Patchset #8 (id:520001) has been deleted

[honghaiz3, 2015-10-16 18:52:41]

The CQ bit was checked by honghaiz@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2015-10-16 18:52:58]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1391703003/540001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1391703003/540001

[AlexG, 2015-10-16 19:25:42]

https://codereview.webrtc.org/1391703003/diff/500001/talk/app/webrtc/java/and...
File talk/app/webrtc/java/android/org/webrtc/NetworkMonitor.java (right):

https://codereview.webrtc.org/1391703003/diff/500001/talk/app/webrtc/java/and...
talk/app/webrtc/java/android/org/webrtc/NetworkMonitor.java:62: private final
ArrayList<NetworkObserver> networkObservers;
Are you planning to use Java observers? For now this looks like unused code

https://codereview.webrtc.org/1391703003/diff/540001/talk/app/webrtc/java/and...
File talk/app/webrtc/java/android/org/webrtc/NetworkMonitor.java (right):

https://codereview.webrtc.org/1391703003/diff/540001/talk/app/webrtc/java/and...
talk/app/webrtc/java/android/org/webrtc/NetworkMonitor.java:94: public static
NetworkMonitor getInstance() {
Is it used anywhere now?

https://codereview.webrtc.org/1391703003/diff/540001/talk/app/webrtc/java/and...
talk/app/webrtc/java/android/org/webrtc/NetworkMonitor.java:111: public void
startMonitoring(long nativeObserver) {
Make it private so it can be called from native code only?

https://codereview.webrtc.org/1391703003/diff/540001/talk/app/webrtc/java/and...
talk/app/webrtc/java/android/org/webrtc/NetworkMonitor.java:118: public void
stopMonitoring(long nativeObserver) {
ditto - make it private?

[commit-bot: I haz the power, 2015-10-16 20:53:15]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-10-16 20:53:16]

Dry run: Try jobs failed on following builders:
  android_arm64_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  android_dbg on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  android_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL)

[honghaiz3, 2015-10-16 21:54:28]

Thanks. PTAL. 
Note that I have put the createMonitorTest in the UI thread (as done in Chrome
NetworkChangeNotifier).

https://codereview.webrtc.org/1391703003/diff/500001/talk/app/webrtc/java/and...
File talk/app/webrtc/java/android/org/webrtc/NetworkMonitor.java (right):

https://codereview.webrtc.org/1391703003/diff/500001/talk/app/webrtc/java/and...
talk/app/webrtc/java/android/org/webrtc/NetworkMonitor.java:62: private final
ArrayList<NetworkObserver> networkObservers;
On 2015/10/16 19:25:42, AlexG wrote:
> Are you planning to use Java observers? For now this looks like unused code
Yes, app can use this to observe the network change event.

https://codereview.webrtc.org/1391703003/diff/540001/talk/app/webrtc/java/and...
File talk/app/webrtc/java/android/org/webrtc/NetworkMonitor.java (right):

https://codereview.webrtc.org/1391703003/diff/540001/talk/app/webrtc/java/and...
talk/app/webrtc/java/android/org/webrtc/NetworkMonitor.java:94: public static
NetworkMonitor getInstance() {
On 2015/10/16 19:25:42, AlexG wrote:
> Is it used anywhere now?
Not yet. But app can use this to get access to the Monitor and observe the
network change events. 
app can also use this to check network state.

https://codereview.webrtc.org/1391703003/diff/540001/talk/app/webrtc/java/and...
talk/app/webrtc/java/android/org/webrtc/NetworkMonitor.java:111: public void
startMonitoring(long nativeObserver) {
On 2015/10/16 19:25:42, AlexG wrote:
> Make it private so it can be called from native code only?

Done.

https://codereview.webrtc.org/1391703003/diff/540001/talk/app/webrtc/java/and...
talk/app/webrtc/java/android/org/webrtc/NetworkMonitor.java:118: public void
stopMonitoring(long nativeObserver) {
On 2015/10/16 19:25:42, AlexG wrote:
> ditto - make it private?

Done.

[honghaiz3, 2015-10-16 21:54:36]

The CQ bit was checked by honghaiz@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2015-10-16 21:54:41]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1391703003/560001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1391703003/560001

[AlexG, 2015-10-16 22:10:33]

lgtm

[pthatcher1, 2015-10-16 22:37:07]

lgtm, assuming you change the name to NetworkMonitorInterface

https://codereview.webrtc.org/1391703003/diff/580001/webrtc/base/networkmonit...
File webrtc/base/networkmonitor.h (right):

https://codereview.webrtc.org/1391703003/diff/580001/webrtc/base/networkmonit...
webrtc/base/networkmonitor.h:39: class NetworkMonitor {
This should be called NetworkMonitorInterface

https://codereview.webrtc.org/1391703003/diff/580001/webrtc/base/networkmonit...
webrtc/base/networkmonitor.h:49: // Implementation should call this method on
the base when networks change,
Implementation => Implementations

[honghaiz3, 2015-10-16 23:00:33]

https://codereview.webrtc.org/1391703003/diff/580001/webrtc/base/networkmonit...
File webrtc/base/networkmonitor.h (right):

https://codereview.webrtc.org/1391703003/diff/580001/webrtc/base/networkmonit...
webrtc/base/networkmonitor.h:39: class NetworkMonitor {
On 2015/10/16 22:37:07, pthatcher1 wrote:
> This should be called NetworkMonitorInterface

Done.

https://codereview.webrtc.org/1391703003/diff/580001/webrtc/base/networkmonit...
webrtc/base/networkmonitor.h:49: // Implementation should call this method on
the base when networks change,
On 2015/10/16 22:37:07, pthatcher1 wrote:
> Implementation => Implementations

Done.

[honghaiz3, 2015-10-16 23:00:42]

The CQ bit was checked by honghaiz@webrtc.org

[honghaiz3, 2015-10-16 23:00:43]

The patchset sent to the CQ was uploaded after l-g-t-m from glaznev@webrtc.org,
pthatcher@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1391703003/#ps600001 (title:
"Changed base class name to NetworkMonitorInterface")

[commit-bot: I haz the power, 2015-10-16 23:00:58]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1391703003/600001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1391703003/600001

[honghaiz3, 2015-10-16 23:45:31]

The CQ bit was unchecked by honghaiz@webrtc.org

[honghaiz3, 2015-10-19 06:04:26]

I added an option to disable network monitor if needed. We need to disable
network monitor in PeerConnectionClientTest.java (otherwise it caused some test
errors due to some special settings of that test ).

[honghaiz3, 2015-10-19 16:38:02]

The CQ bit was checked by honghaiz@webrtc.org

[honghaiz3, 2015-10-19 16:38:02]

The patchset sent to the CQ was uploaded after l-g-t-m from glaznev@webrtc.org,
pthatcher@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1391703003/#ps640001 (title:
"Changed comments")

[commit-bot: I haz the power, 2015-10-19 16:38:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1391703003/640001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1391703003/640001

[commit-bot: I haz the power, 2015-10-19 16:39:36]

Committed patchset #13 (id:640001)

[commit-bot: I haz the power, 2015-10-19 16:39:48]

Patchset 13 (id:??) landed as
https://crrev.com/023f3ef0296511f12897c503d6fc2ed063712474
Cr-Commit-Position: refs/heads/master@{#10325}

[AlexG, 2015-10-19 17:28:44]

On 2015/10/19 06:04:26, honghaiz3 wrote:
> I added an option to disable network monitor if needed. We need to disable
> network monitor in PeerConnectionClientTest.java (otherwise it caused some
test
> errors due to some special settings of that test ).

Did we understand the root case why PeerConnectionClientTest requires network
monitor disabling? This test is supposed to exercise loopback mode - it should
not depend on network monitor enabling/disabling.

[honghaiz3, 2015-10-19 17:50:42]

Patchset #14 (id:660001) has been deleted

[honghaiz3, 2015-10-19 19:12:35]

On 2015/10/19 17:28:44, AlexG wrote:
> On 2015/10/19 06:04:26, honghaiz3 wrote:
> > I added an option to disable network monitor if needed. We need to disable
> > network monitor in PeerConnectionClientTest.java (otherwise it caused some
> test
> > errors due to some special settings of that test ).
> 
> Did we understand the root case why PeerConnectionClientTest requires network
> monitor disabling? This test is supposed to exercise loopback mode - it should
> not depend on network monitor enabling/disabling.
The error message was:
W/System.err(32447): java.lang.SecurityException: Given caller package
org.appspot.apprtc.test is not running in process ProcessRecord{16731df5
32447:org.appspot.apprtc/u0a114}
W/System.err(32447):  at android.os.Parcel.readException(Parcel.java:1546)
W/System.err(32447):  at android.os.Parcel.readException(Parcel.java:1499)
W/System.err(32447):  at
android.app.ActivityManagerProxy.registerReceiver(ActivityManagerNative.java:2782)
W/System.err(32447):  at
android.app.ContextImpl.registerReceiverInternal(ContextImpl.java:1655)
W/System.err(32447):  at
android.app.ContextImpl.registerReceiver(ContextImpl.java:1623)
W/System.err(32447):  at
android.app.ContextImpl.registerReceiver(ContextImpl.java:1617)
W/System.err(32447):  at
org.webrtc.NetworkMonitorAutoDetect$WifiManagerDelegate.getWifiSSID(NetworkMonitorAutoDetect.java:222)
....
The cause is that the AppRTCDemoTest loaded AppRTCDemo.apk and when registering
the BroadcastReceiver in AppRTCDemo, the context has 
package org.appspt.apprtc.test but the the App does not contain that package. It
raised a security exception in Android 4 and above. 
There are similar discussions here:
https://groups.google.com/forum/#!topic/android-developers/axLHaXJtonk

[AlexG, 2015-10-19 19:37:51]

On 2015/10/19 19:12:35, honghaiz3 wrote:
> On 2015/10/19 17:28:44, AlexG wrote:
> > On 2015/10/19 06:04:26, honghaiz3 wrote:
> > > I added an option to disable network monitor if needed. We need to disable
> > > network monitor in PeerConnectionClientTest.java (otherwise it caused some
> > test
> > > errors due to some special settings of that test ).
> > 
> > Did we understand the root case why PeerConnectionClientTest requires
network
> > monitor disabling? This test is supposed to exercise loopback mode - it
should
> > not depend on network monitor enabling/disabling.
> The error message was:
> W/System.err(32447): java.lang.SecurityException: Given caller package
> org.appspot.apprtc.test is not running in process ProcessRecord{16731df5
> 32447:org.appspot.apprtc/u0a114}
> W/System.err(32447):  at android.os.Parcel.readException(Parcel.java:1546)
> W/System.err(32447):  at android.os.Parcel.readException(Parcel.java:1499)
> W/System.err(32447):  at
>
android.app.ActivityManagerProxy.registerReceiver(ActivityManagerNative.java:2782)
> W/System.err(32447):  at
> android.app.ContextImpl.registerReceiverInternal(ContextImpl.java:1655)
> W/System.err(32447):  at
> android.app.ContextImpl.registerReceiver(ContextImpl.java:1623)
> W/System.err(32447):  at
> android.app.ContextImpl.registerReceiver(ContextImpl.java:1617)
> W/System.err(32447):  at
>
org.webrtc.NetworkMonitorAutoDetect$WifiManagerDelegate.getWifiSSID(NetworkMonitorAutoDetect.java:222)
> ....
> The cause is that the AppRTCDemoTest loaded AppRTCDemo.apk and when
registering
> the BroadcastReceiver in AppRTCDemo, the context has 
> package org.appspt.apprtc.test but the the App does not contain that package.
It
> raised a security exception in Android 4 and above. 
> There are similar discussions here:
> https://groups.google.com/forum/#!topic/android-developers/axLHaXJtonk

Shouldn't this exception need to be caught in NetworkMonitorAutoDetect.java?
I am not sure it is worth to add special public configuration options for this
case so in order to run the tests properly we need to do some extra
configurations.

[honghaiz3, 2015-10-19 22:26:27]

On 2015/10/19 19:37:51, AlexG wrote:
> On 2015/10/19 19:12:35, honghaiz3 wrote:
> > On 2015/10/19 17:28:44, AlexG wrote:
> > > On 2015/10/19 06:04:26, honghaiz3 wrote:
> > > > I added an option to disable network monitor if needed. We need to
disable
> > > > network monitor in PeerConnectionClientTest.java (otherwise it caused
some
> > > test
> > > > errors due to some special settings of that test ).
> > > 
> > > Did we understand the root case why PeerConnectionClientTest requires
> network
> > > monitor disabling? This test is supposed to exercise loopback mode - it
> should
> > > not depend on network monitor enabling/disabling.
> > The error message was:
> > W/System.err(32447): java.lang.SecurityException: Given caller package
> > org.appspot.apprtc.test is not running in process ProcessRecord{16731df5
> > 32447:org.appspot.apprtc/u0a114}
> > W/System.err(32447):  at android.os.Parcel.readException(Parcel.java:1546)
> > W/System.err(32447):  at android.os.Parcel.readException(Parcel.java:1499)
> > W/System.err(32447):  at
> >
>
android.app.ActivityManagerProxy.registerReceiver(ActivityManagerNative.java:2782)
> > W/System.err(32447):  at
> > android.app.ContextImpl.registerReceiverInternal(ContextImpl.java:1655)
> > W/System.err(32447):  at
> > android.app.ContextImpl.registerReceiver(ContextImpl.java:1623)
> > W/System.err(32447):  at
> > android.app.ContextImpl.registerReceiver(ContextImpl.java:1617)
> > W/System.err(32447):  at
> >
>
org.webrtc.NetworkMonitorAutoDetect$WifiManagerDelegate.getWifiSSID(NetworkMonitorAutoDetect.java:222)
> > ....
> > The cause is that the AppRTCDemoTest loaded AppRTCDemo.apk and when
> registering
> > the BroadcastReceiver in AppRTCDemo, the context has 
> > package org.appspt.apprtc.test but the the App does not contain that
package.
> It
> > raised a security exception in Android 4 and above. 
> > There are similar discussions here:
> > https://groups.google.com/forum/#!topic/android-developers/axLHaXJtonk
> 
> Shouldn't this exception need to be caught in NetworkMonitorAutoDetect.java?
> I am not sure it is worth to add special public configuration options for this
> case so in order to run the tests properly we need to do some extra
> configurations.

Had an offline discussion and we agree to keep this as is because that may
potentially
catch and ignore other SecurityExceptions that we don't want to ignore.  
Thanks for the suggestion!
which we