Adding platform support check for NetworkObserver

Adding platform support check for NetworkObserver

Committed: https://crrev.com/c94943b28b4876071a8d509f478e4c63ebb75ca2
Cr-Commit-Position: refs/heads/master@{#363020}

[Jana, 2015-11-19 01:52:57]

jri@chromium.org changed reviewers:
+ pauljensen@chromium.org

[Jana, 2015-11-19 01:52:58]

I thought of adding a boolean return to AddNetworkObserver, but the semantics of
that would not be clean, since AddNetworkObserver does not add an observer if
the singleton is not yet instantiated. Feel free to suggest a different method
of doing this.

[Jana, 2015-11-19 01:58:26]

Paul: Are there other observers for which such a method could/should be added?
What about IPAddressObserver?

[Ryan Hamilton, 2015-11-19 02:19:04]

rch@chromium.org changed reviewers:
+ rch@chromium.org

[Ryan Hamilton, 2015-11-19 02:19:04]

Cool. But my opinion is not important; Paul's it.

https://codereview.chromium.org/1454313002/diff/20001/net/base/network_change...
File net/base/network_change_notifier.cc (right):

https://codereview.chromium.org/1454313002/diff/20001/net/base/network_change...
net/base/network_change_notifier.cc:810: return true;
FWIW, you can just do:

return base::android::BuildInfo::GetInstance()->sdk_int() >=
       base::android::SDK_VERSION_LOLLIPOP;

[Jana, 2015-11-19 03:25:20]

https://codereview.chromium.org/1454313002/diff/20001/net/base/network_change...
File net/base/network_change_notifier.cc (right):

https://codereview.chromium.org/1454313002/diff/20001/net/base/network_change...
net/base/network_change_notifier.cc:810: return true;
On 2015/11/19 02:19:04, Ryan Hamilton wrote:
> FWIW, you can just do:
> 
> return base::android::BuildInfo::GetInstance()->sdk_int() >=
>        base::android::SDK_VERSION_LOLLIPOP;

Done.

[pauljensen, 2015-11-19 04:12:55]

So I think we need to make some changes to this:
1. Broaden the API to not just cover NetworkObservers but also the other APIs I
added:
       static void GetConnectedNetworks(NetworkList* network_list);
       static ConnectionType GetNetworkConnectionType(NetworkHandle network);
       static NetworkHandle GetDefaultNetwork();
2. Make this a virtual function with a default implementation that returns
false.  NetworkChangeNotifierAndroid can offer an implementation with the
Android version check.

[pauljensen, 2015-11-19 18:37:14]

Jana and I discussed on VC:
1. I think changing the API name to AreNetworkHandlesSupported()
2. I think we should make all 5 functions covered by this API include
DCHECK(AreNetworkHandlesSupported())

[Jana, 2015-11-20 23:08:48]

Paul: I think this patch does what you want to do. PTAL.

[Ryan Hamilton, 2015-11-20 23:36:18]

rch@chromium.org changed reviewers:
- pauljensen@chromium.org

[Ryan Hamilton, 2015-11-20 23:36:18]

Looks good, but Paul knows for sure

[pauljensen, 2015-11-30 20:23:47]

pauljensen@chromium.org changed reviewers:
+ pauljensen@chromium.org

[pauljensen, 2015-11-30 20:23:48]

https://codereview.chromium.org/1454313002/diff/60001/net/android/network_cha...
File net/android/network_change_notifier_android.cc (right):

https://codereview.chromium.org/1454313002/diff/60001/net/android/network_cha...
net/android/network_change_notifier_android.cc:166: // Notifications for API
using NetworkHandles only implemented for Android
Notifications->Notifications/querying

https://codereview.chromium.org/1454313002/diff/60001/net/android/network_cha...
net/android/network_change_notifier_android.cc:166: // Notifications for API
using NetworkHandles only implemented for Android
API->APIs

https://codereview.chromium.org/1454313002/diff/60001/net/base/network_change...
File net/base/network_change_notifier.cc (right):

https://codereview.chromium.org/1454313002/diff/60001/net/base/network_change...
net/base/network_change_notifier.cc:851: void
NetworkChangeNotifier::AddNetworkObserver(NetworkObserver* observer) {
DCHECK(AreNetworkHandlesSupported());

https://codereview.chromium.org/1454313002/diff/60001/net/base/network_change...
net/base/network_change_notifier.cc:896: void
NetworkChangeNotifier::RemoveNetworkObserver(NetworkObserver* observer) {
DCHECK(AreNetworkHandlesSupported());

https://codereview.chromium.org/1454313002/diff/60001/net/base/network_change...
File net/base/network_change_notifier.h (right):

https://codereview.chromium.org/1454313002/diff/60001/net/base/network_change...
net/base/network_change_notifier.h:294: // Returns true if the platform supports
use of API based on NetworkHandles.
API->APIs

https://codereview.chromium.org/1454313002/diff/60001/net/base/network_change...
net/base/network_change_notifier.h:296: // GetNetworkConnectionType(), and
GetDefaultNetwork().
please also mention NetworkObserver

https://codereview.chromium.org/1454313002/diff/60001/net/base/network_change...
net/base/network_change_notifier.h:301: // empty when unimplemented. Requires
NetworkHandles support.
support->support, see AreNetworkHandlesSupported().

https://codereview.chromium.org/1454313002/diff/60001/net/base/network_change...
net/base/network_change_notifier.h:308: // CONNECTION_UNKNOWN when
unimplemented. Requires NetworkHandles support.
ditto

https://codereview.chromium.org/1454313002/diff/60001/net/base/network_change...
net/base/network_change_notifier.h:318: // Requires NetworkHandles support.
ditto

[Jana, 2015-11-30 22:31:06]

Thanks, Paul, comments addressed.

https://codereview.chromium.org/1454313002/diff/60001/net/android/network_cha...
File net/android/network_change_notifier_android.cc (right):

https://codereview.chromium.org/1454313002/diff/60001/net/android/network_cha...
net/android/network_change_notifier_android.cc:166: // Notifications for API
using NetworkHandles only implemented for Android
On 2015/11/30 20:23:48, pauljensen wrote:
> API->APIs

Done.

https://codereview.chromium.org/1454313002/diff/60001/net/android/network_cha...
net/android/network_change_notifier_android.cc:166: // Notifications for API
using NetworkHandles only implemented for Android
On 2015/11/30 20:23:48, pauljensen wrote:
> Notifications->Notifications/querying

Modified comment -- see if it's better.

https://codereview.chromium.org/1454313002/diff/60001/net/base/network_change...
File net/base/network_change_notifier.cc (right):

https://codereview.chromium.org/1454313002/diff/60001/net/base/network_change...
net/base/network_change_notifier.cc:851: void
NetworkChangeNotifier::AddNetworkObserver(NetworkObserver* observer) {
On 2015/11/30 20:23:48, pauljensen wrote:
> DCHECK(AreNetworkHandlesSupported());

Done.

https://codereview.chromium.org/1454313002/diff/60001/net/base/network_change...
net/base/network_change_notifier.cc:896: void
NetworkChangeNotifier::RemoveNetworkObserver(NetworkObserver* observer) {
On 2015/11/30 20:23:48, pauljensen wrote:
> DCHECK(AreNetworkHandlesSupported());

Done.

https://codereview.chromium.org/1454313002/diff/60001/net/base/network_change...
File net/base/network_change_notifier.h (right):

https://codereview.chromium.org/1454313002/diff/60001/net/base/network_change...
net/base/network_change_notifier.h:296: // GetNetworkConnectionType(), and
GetDefaultNetwork().
On 2015/11/30 20:23:48, pauljensen wrote:
> please also mention NetworkObserver

Done.

https://codereview.chromium.org/1454313002/diff/60001/net/base/network_change...
net/base/network_change_notifier.h:301: // empty when unimplemented. Requires
NetworkHandles support.
On 2015/11/30 20:23:48, pauljensen wrote:
> support->support, see AreNetworkHandlesSupported().

Done.

https://codereview.chromium.org/1454313002/diff/60001/net/base/network_change...
net/base/network_change_notifier.h:308: // CONNECTION_UNKNOWN when
unimplemented. Requires NetworkHandles support.
On 2015/11/30 20:23:48, pauljensen wrote:
> ditto

Done.

https://codereview.chromium.org/1454313002/diff/60001/net/base/network_change...
net/base/network_change_notifier.h:318: // Requires NetworkHandles support.
On 2015/11/30 20:23:48, pauljensen wrote:
> ditto

Done.

[pauljensen, 2015-12-01 12:34:44]

lgtm modulo my one comment

https://codereview.chromium.org/1454313002/diff/80001/net/base/network_change...
File net/base/network_change_notifier.h (right):

https://codereview.chromium.org/1454313002/diff/80001/net/base/network_change...
net/base/network_change_notifier.h:297: // RemoveNetworkObserver, and all public
NetworkObserver methods.
RemoveNetworkObserver->RemoveNetworkObserver()

[Jana, 2015-12-01 16:24:01]

Thanks, Paul. Fixed, and submitting.

[Jana, 2015-12-01 16:24:06]

The CQ bit was checked by jri@chromium.org

[Jana, 2015-12-01 16:24:07]

The patchset sent to the CQ was uploaded after l-g-t-m from
pauljensen@chromium.org
Link to the patchset: https://codereview.chromium.org/1454313002/#ps100001
(title: "comment addressed")

[commit-bot: I haz the power, 2015-12-01 16:25:42]

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

[commit-bot: I haz the power, 2015-12-01 16:38:37]

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

[commit-bot: I haz the power, 2015-12-01 16:38:38]

Try jobs failed on following builders:
  android_compile_dbg on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_compile...)

[Jana, 2015-12-01 17:22:52]

The CQ bit was checked by jri@chromium.org

[Jana, 2015-12-01 17:22:53]

The patchset sent to the CQ was uploaded after l-g-t-m from
pauljensen@chromium.org
Link to the patchset: https://codereview.chromium.org/1454313002/#ps120001
(title: "trying to fix compile error")

[commit-bot: I haz the power, 2015-12-01 17:23:09]

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

[commit-bot: I haz the power, 2015-12-01 18:42:05]

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

[commit-bot: I haz the power, 2015-12-01 18:42:06]

Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)

[Jana, 2015-12-01 18:44:38]

The CQ bit was checked by jri@chromium.org

[commit-bot: I haz the power, 2015-12-01 18:45:36]

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

[Jana, 2015-12-01 20:41:37]

The CQ bit was unchecked by jri@chromium.org

[Jana, 2015-12-01 20:41:38]

The CQ bit was checked by jri@chromium.org

[commit-bot: I haz the power, 2015-12-01 20:51:41]

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

[commit-bot: I haz the power, 2015-12-01 20:51:42]

Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)

[pauljensen, 2015-12-01 22:56:04]

The CQ bit was checked by pauljensen@chromium.org

[commit-bot: I haz the power, 2015-12-01 22:57:26]

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

[commit-bot: I haz the power, 2015-12-02 02:49:32]

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

[commit-bot: I haz the power, 2015-12-02 02:49:34]

Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)

[Jana, 2015-12-02 03:54:06]

Paul: Per our offline conversation, I've added the set method for tests. PTAL

[pauljensen, 2015-12-02 13:43:31]

That's not how I envisioned this.  Furthermore I don't think the change works: 
the test explicitly instantiates a NCNAndroid which overrides
AreNetworkHandlesSupported() so mucking with the parent class won't affect this
(or fix the test).

Here's a simpler fix that doesn't affect the NCN API or involve the NCN
singleton:
1. add private NCNAndroid::ForceNetworkHandlesSupportedForTesting() {
      force_network_handles_supported_for_testing_ = true;
    }
2. add protected NCNAndroidTest::ForceNetworkHandlesSupportedForTesting() {
      notifier_->ForceNetworkHandlesSupportedForTesting();
    }
3. have the test call ForceNetworkHandlesSupportedForTesting()
4. add to the begining of NCNAndroid::AreNetworkHandlesSupported
      if (force_network_handles_supported_for_testing_)
        return true;
5. add private bool NCNAndroid::force_network_handles_supported_for_testing_

[Jana, 2015-12-02 18:13:09]

Paul: I think I'm going to need to do the same for tests later using MockNCN, so
I think it's useful to have the forcing in NCN instead of just AndroidNCN. I've
made some mods, PTAL. (I like your names better, so I'm using those)

[pauljensen, 2015-12-02 18:16:14]

On 2015/12/02 18:13:09, Jana wrote:
> Paul: I think I'm going to need to do the same for tests later using MockNCN,
so
> I think it's useful to have the forcing in NCN instead of just AndroidNCN.
I've
> made some mods, PTAL. (I like your names better, so I'm using those)

I don't think this fixes the test as I said in
https://codereview.chromium.org/1454313002/#msg38
In the case of later tests using MockNCN, I'd much prefer we modify
MockNCN::AreNetworkHandlesSupported() rather than the production NCN.

[pauljensen, 2015-12-02 18:19:32]

On 2015/12/02 18:16:14, pauljensen wrote:
> On 2015/12/02 18:13:09, Jana wrote:
> > Paul: I think I'm going to need to do the same for tests later using
MockNCN,
> so
> > I think it's useful to have the forcing in NCN instead of just AndroidNCN.
> I've
> > made some mods, PTAL. (I like your names better, so I'm using those)
> 
> I don't think this fixes the test as I said in
> https://codereview.chromium.org/1454313002/#msg38
> In the case of later tests using MockNCN, I'd much prefer we modify
> MockNCN::AreNetworkHandlesSupported() rather than the production NCN.

Ah I see you adjusted NCNAndroid to call the parent override.  I still would
like this moved out of NCN as it's not necessary and involves the hideous global
singleton.

[Jana, 2015-12-02 21:35:30]

I'll do it in AndroidNCN, but I was trying to point out that I'll have to do
something similar in MockNCN later. That seems fine, and I'll make it happen.

[Jana, 2015-12-02 21:50:54]

Ok -- support added per your earlier comment. PTAL.

[pauljensen, 2015-12-03 03:52:45]

lgtm

[pauljensen, 2015-12-03 03:52:53]

The CQ bit was checked by pauljensen@chromium.org

[commit-bot: I haz the power, 2015-12-03 03:53:13]

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

[commit-bot: I haz the power, 2015-12-03 06:13:04]

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

[commit-bot: I haz the power, 2015-12-03 06:13:05]

Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)

[Jana, 2015-12-03 17:15:45]

The CQ bit was checked by jri@chromium.org

[commit-bot: I haz the power, 2015-12-03 17:16:57]

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

[commit-bot: I haz the power, 2015-12-03 18:50:33]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2015-12-03 18:51:22]

Description was changed from

==========
Adding platform support check for NetworkObserver
==========

to

==========
Adding platform support check for NetworkObserver

Committed: https://crrev.com/c94943b28b4876071a8d509f478e4c63ebb75ca2
Cr-Commit-Position: refs/heads/master@{#363020}
==========

[commit-bot: I haz the power, 2015-12-03 18:51:23]

Patchset 10 (id:??) landed as
https://crrev.com/c94943b28b4876071a8d509f478e4c63ebb75ca2
Cr-Commit-Position: refs/heads/master@{#363020}