Expose connection subtype from NetworkChangeNotifier

Expose connection subtype from NetworkChangeNotifier

It will be consumed by NQE in the next CL.

BUG=704998
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester

Review-Url: https://codereview.chromium.org/2776523003
Cr-Commit-Position: refs/heads/master@{#459638}
Committed: https://chromium.googlesource.com/chromium/src/+/880e49e7bf7d6d11f23d7d4dcd8e2e21b9c142b5

[tbansal1, 2017-03-23 23:12:51]

Description was changed from

==========
Expose connection subtype

BUG=
==========

to

==========
Expose connection subtype

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
==========

[tbansal1, 2017-03-23 23:13:04]

The CQ bit was checked by tbansal@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-23 23:14:00]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tbansal1, 2017-03-23 23:17:39]

The CQ bit was checked by tbansal@chromium.org to run a CQ dry run

[tbansal1, 2017-03-23 23:17:46]

Patchset #2 (id:20001) has been deleted

[tbansal1, 2017-03-23 23:17:52]

Patchset #1 (id:1) has been deleted

[commit-bot: I haz the power, 2017-03-23 23:18:38]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-23 23:33:56]

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

[commit-bot: I haz the power, 2017-03-23 23:33:57]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)

[tbansal1, 2017-03-24 00:06:43]

The CQ bit was checked by tbansal@chromium.org to run a CQ dry run

[tbansal1, 2017-03-24 00:06:54]

Patchset #1 (id:40001) has been deleted

[commit-bot: I haz the power, 2017-03-24 00:07:22]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-24 00:29:34]

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

[commit-bot: I haz the power, 2017-03-24 00:29:35]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[tbansal1, 2017-03-24 05:31:43]

The CQ bit was checked by tbansal@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-24 05:32:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-24 07:22:15]

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

[commit-bot: I haz the power, 2017-03-24 07:22:16]

Dry run: This issue passed the CQ dry run.

[tbansal1, 2017-03-24 17:49:52]

Patchset #1 (id:60001) has been deleted

[tbansal1, 2017-03-24 18:02:15]

Description was changed from

==========
Expose connection subtype

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
==========

to

==========
Expose connection subtype from NetworkChangeNotifier

It will be consumed by NQE in the next CL.

BUG=704998
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
==========

[tbansal1, 2017-03-24 18:02:44]

Patchset #1 (id:80001) has been deleted

[tbansal1, 2017-03-24 18:03:05]

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

[tbansal1, 2017-03-24 18:03:06]

pauljensen: ptal. Thanks.

[tbansal1, 2017-03-24 18:03:13]

The CQ bit was checked by tbansal@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-24 18:04:46]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[pauljensen, 2017-03-24 18:38:59]

are you aware this will only allow differentiating between cellular connections,
and not WiFi or Ethernet or Bluetooth connection sub-types?

https://codereview.chromium.org/2776523003/diff/100001/net/base/network_chang...
File net/base/network_change_notifier.h (right):

https://codereview.chromium.org/2776523003/diff/100001/net/base/network_chang...
net/base/network_change_notifier.h:279: // Returns the connection subtype of the
network the client is connected to.
"the connection subtype of the network the client is connected to"->"the
device's current default active network connection's subtype"

https://codereview.chromium.org/2776523003/diff/100001/net/base/network_chang...
net/base/network_change_notifier.h:283: // the internet, the connection type is
CONNECTION_WIFI.
there is no sub-type for WiFi

https://codereview.chromium.org/2776523003/diff/100001/net/base/network_chang...
net/base/network_change_notifier.h:283: // the internet, the connection type is
CONNECTION_WIFI.
The returned value only describes the connection currently used by the
device, and does not take into account other machines on the network. For
example, if the device is connected using Wifi to a 3G gateway to access the
internet, the connection type is CONNECTION_WIFI.
->
The returned value only describes the first-hop connection, for example if the
device is connected via WiFi to a 4G hotspot, the returned value will reflect
WiFi, not 4G.

(Also please make this same replacement on line 273, but use
CONNECTION_WIFI_there.)

[commit-bot: I haz the power, 2017-03-24 19:21:18]

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

[commit-bot: I haz the power, 2017-03-24 19:21:19]

Dry run: This issue passed the CQ dry run.

[tbansal1, 2017-03-24 23:22:24]

ptal. Thanks.

https://codereview.chromium.org/2776523003/diff/100001/net/base/network_chang...
File net/base/network_change_notifier.h (right):

https://codereview.chromium.org/2776523003/diff/100001/net/base/network_chang...
net/base/network_change_notifier.h:279: // Returns the connection subtype of the
network the client is connected to.
On 2017/03/24 18:38:58, pauljensen wrote:
> "the connection subtype of the network the client is connected to"->"the
> device's current default active network connection's subtype"

Done.

https://codereview.chromium.org/2776523003/diff/100001/net/base/network_chang...
net/base/network_change_notifier.h:283: // the internet, the connection type is
CONNECTION_WIFI.
On 2017/03/24 18:38:59, pauljensen wrote:
> The returned value only describes the connection currently used by the
> device, and does not take into account other machines on the network. For
> example, if the device is connected using Wifi to a 3G gateway to access the
> internet, the connection type is CONNECTION_WIFI.
> ->
> The returned value only describes the first-hop connection, for example if the
> device is connected via WiFi to a 4G hotspot, the returned value will reflect
> WiFi, not 4G.
> 
> (Also please make this same replacement on line 273, but use
> CONNECTION_WIFI_there.)

Done.

https://codereview.chromium.org/2776523003/diff/100001/net/base/network_chang...
net/base/network_change_notifier.h:283: // the internet, the connection type is
CONNECTION_WIFI.
On 2017/03/24 18:38:58, pauljensen wrote:
> there is no sub-type for WiFi

Thanks, I am more interested in cellular. If it proves useful, I may work on
getting the subtype for WiFi, although IIRC, there are some issues there due to
Android permissions.

I have added one more statement to explain that this may return UNKNOWN more
frequently. Let me know if it needs to be reworded.

[pauljensen, 2017-03-25 00:55:09]

lgtm, just please add a tiny test in network_change_notifier_unittest.cc that
just calls this new API to make sure it doesn't crash.

[tbansal1, 2017-03-25 01:11:28]

The CQ bit was checked by tbansal@chromium.org to run a CQ dry run

[tbansal1, 2017-03-25 01:11:40]

Added the test. Thanks for the review.

[commit-bot: I haz the power, 2017-03-25 01:11:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-25 02:36:35]

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

[commit-bot: I haz the power, 2017-03-25 02:36:36]

Dry run: This issue passed the CQ dry run.

[tbansal1, 2017-03-25 04:37:38]

The CQ bit was checked by tbansal@chromium.org

[tbansal1, 2017-03-25 04:37:38]

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/2776523003/#ps140001
(title: "Add test")

[commit-bot: I haz the power, 2017-03-25 04:37:50]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-25 04:44:29]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1490416658437430,
"parent_rev": "69536d1f656f6e6258f8508929aaa57ae7831d48", "commit_rev":
"880e49e7bf7d6d11f23d7d4dcd8e2e21b9c142b5"}

[commit-bot: I haz the power, 2017-03-25 04:45:47]

Description was changed from

==========
Expose connection subtype from NetworkChangeNotifier

It will be consumed by NQE in the next CL.

BUG=704998
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
==========

to

==========
Expose connection subtype from NetworkChangeNotifier

It will be consumed by NQE in the next CL.

BUG=704998
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester

Review-Url: https://codereview.chromium.org/2776523003
Cr-Commit-Position: refs/heads/master@{#459638}
Committed:
https://chromium.googlesource.com/chromium/src/+/880e49e7bf7d6d11f23d7d4dcd8e...
==========

[commit-bot: I haz the power, 2017-03-25 04:45:48]

Committed patchset #3 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/880e49e7bf7d6d11f23d7d4dcd8e...

[pauljensen, 2017-05-10 16:47:07]

https://codereview.chromium.org/2776523003/diff/140001/net/android/network_ch...
File net/android/network_change_notifier_android.cc (right):

https://codereview.chromium.org/2776523003/diff/140001/net/android/network_ch...
net/android/network_change_notifier_android.cc:165: return
delegate_->GetCurrentConnectionSubtype();
Hmm this is a potential threading violation.  NCN::GetConnectionSubtype() is
advertised as thread-safe, but here we're implementing it by calling a function
that can only be called on the NCN's thread.  In Chrome this will be the UI
thread, in Cronet this will be an obscure initialization thread.

[tbansal1, 2017-05-11 01:08:52]

https://codereview.chromium.org/2776523003/diff/140001/net/android/network_ch...
File net/android/network_change_notifier_android.cc (right):

https://codereview.chromium.org/2776523003/diff/140001/net/android/network_ch...
net/android/network_change_notifier_android.cc:165: return
delegate_->GetCurrentConnectionSubtype();
On 2017/05/10 16:47:07, pauljensen wrote:
> Hmm this is a potential threading violation.  NCN::GetConnectionSubtype() is
> advertised as thread-safe, but here we're implementing it by calling a
function
> that can only be called on the NCN's thread.  In Chrome this will be the UI
> thread, in Cronet this will be an obscure initialization thread.

I will send a CL tomorrow to fix this. Thanks for noticing this.