Expose RTT and downlink bandwidth using experimental Javascript API

Expose RTT and downlink bandwidth using experimental Javascript API

Initial draft spec is here: https://cdn.rawgit.com/WICG/netinfo/ect/index.html

The API is runtime enabled feature, and is currently set to
"experimental".

This CL only implements "rtt" and "downlink" attributes. "ect" and
callback (when there is a change in network quality) will be 
implemented in forthcoming CLs.

BUG=719108
Review-Url: https://codereview.chromium.org/2863973003
Cr-Commit-Position: refs/heads/master@{#473649}
Committed: https://chromium.googlesource.com/chromium/src/+/99d8aeb59f72b051b8f4b4af8a55b54c22dedfda

[tbansal1, 2017-05-05 22:00:31]

Patchset #1 (id:1) has been deleted

[tbansal1, 2017-05-05 22:00:52]

Description was changed from

==========
NQE JS

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

to

==========
NQE JS API along with content and Webkit changes

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

[tbansal1, 2017-05-05 22:00:56]

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

[commit-bot: I haz the power, 2017-05-05 22:03:10]

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-05-05 22:10:49]

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

[commit-bot: I haz the power, 2017-05-05 22:10:50]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[tbansal1, 2017-05-11 20:00:56]

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

[commit-bot: I haz the power, 2017-05-11 20:01:48]

Dry run: CQ is trying da patch.

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

[tbansal1, 2017-05-11 20:12:20]

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

[tbansal1, 2017-05-11 20:12:26]

Patchset #2 (id:40001) has been deleted

[commit-bot: I haz the power, 2017-05-11 20:12:44]

Dry run: CQ is trying da patch.

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

[tbansal1, 2017-05-11 21:42:47]

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

[tbansal1, 2017-05-11 21:42:50]

Patchset #3 (id:80001) has been deleted

[commit-bot: I haz the power, 2017-05-11 21:44:16]

Dry run: CQ is trying da patch.

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

[tbansal1, 2017-05-11 22:20:37]

Description was changed from

==========
NQE JS API along with content and Webkit changes

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

to

==========
Expose RTT and downlink bandwidth using experimental Javascript API

Initial spec is here: https://cdn.rawgit.com/WICG/netinfo/ect/index.html

The API is runtime enabled feature, and this CL sets its status to 
"experimental"

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

[tbansal1, 2017-05-11 22:21:02]

Description was changed from

==========
Expose RTT and downlink bandwidth using experimental Javascript API

Initial spec is here: https://cdn.rawgit.com/WICG/netinfo/ect/index.html

The API is runtime enabled feature, and this CL sets its status to 
"experimental"

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

to

==========
Expose RTT and downlink bandwidth using experimental Javascript API

Initial draft spec is here: https://cdn.rawgit.com/WICG/netinfo/ect/index.html

The API is runtime enabled feature, and this CL sets its status to 
"experimental"

BUG=719108
==========

[tbansal1, 2017-05-11 22:21:12]

Description was changed from

==========
Expose RTT and downlink bandwidth using experimental Javascript API

Initial draft spec is here: https://cdn.rawgit.com/WICG/netinfo/ect/index.html

The API is runtime enabled feature, and this CL sets its status to 
"experimental"

BUG=719108
==========

to

==========
Expose RTT and downlink bandwidth using experimental Javascript API

Initial draft spec is here: https://cdn.rawgit.com/WICG/netinfo/ect/index.html

The API is runtime enabled feature, and this CL sets its status to 
"experimental".

BUG=719108
==========

[tbansal1, 2017-05-11 22:21:23]

Patchset #1 (id:20001) has been deleted

[tbansal1, 2017-05-11 22:21:35]

Patchset #1 (id:60001) has been deleted

[tbansal1, 2017-05-11 22:21:47]

Patchset #1 (id:100001) has been deleted

[tbansal1, 2017-05-11 22:33:12]

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

[tbansal1, 2017-05-11 22:33:18]

Patchset #1 (id:120001) has been deleted

[tbansal1, 2017-05-11 22:33:38]

Description was changed from

==========
Expose RTT and downlink bandwidth using experimental Javascript API

Initial draft spec is here: https://cdn.rawgit.com/WICG/netinfo/ect/index.html

The API is runtime enabled feature, and this CL sets its status to 
"experimental".

BUG=719108
==========

to

==========
Expose RTT and downlink bandwidth using experimental Javascript API

Initial draft spec is here: https://cdn.rawgit.com/WICG/netinfo/ect/index.html

The API is runtime enabled feature, and is currently set to
"experimental".

BUG=719108
==========

[tbansal1, 2017-05-11 22:34:00]

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

[tbansal1, 2017-05-11 22:34:01]

ryansturm: ptal. Thanks.

[commit-bot: I haz the power, 2017-05-11 22:35:32]

Dry run: CQ is trying da patch.

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

[tbansal1, 2017-05-11 22:43:25]

tbansal@chromium.org changed reviewers:
+ bengr@chromium.org
- ryansturm@chromium.org

[tbansal1, 2017-05-11 22:43:26]

bengr: ptal. Thanks.

[tbansal1, 2017-05-11 22:44:12]

Description was changed from

==========
Expose RTT and downlink bandwidth using experimental Javascript API

Initial draft spec is here: https://cdn.rawgit.com/WICG/netinfo/ect/index.html

The API is runtime enabled feature, and is currently set to
"experimental".

BUG=719108
==========

to

==========
Expose RTT and downlink bandwidth using experimental Javascript API

Initial draft spec is here: https://cdn.rawgit.com/WICG/netinfo/ect/index.html

The API is runtime enabled feature, and is currently set to
"experimental".

This CL only implements "rtt" and "downlink" attributes. "ect" will
be implemented in a different CL.

BUG=719108
==========

[commit-bot: I haz the power, 2017-05-11 23:09:30]

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

[commit-bot: I haz the power, 2017-05-11 23:09:31]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[tbansal1, 2017-05-11 23:17:59]

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

[tbansal1, 2017-05-11 23:18:02]

Patchset #1 (id:140001) has been deleted

[commit-bot: I haz the power, 2017-05-11 23:18:43]

Dry run: CQ is trying da patch.

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

[tbansal1, 2017-05-11 23:44:00]

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

[tbansal1, 2017-05-11 23:44:03]

Patchset #1 (id:160001) has been deleted

[commit-bot: I haz the power, 2017-05-11 23:44:56]

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-05-12 01:01:51]

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

[commit-bot: I haz the power, 2017-05-12 01:01:52]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[tbansal1, 2017-05-12 01:20:59]

Patchset #2 (id:200001) has been deleted

[tbansal1, 2017-05-12 01:21:03]

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

[commit-bot: I haz the power, 2017-05-12 01:21:40]

Dry run: CQ is trying da patch.

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

[tbansal1, 2017-05-12 01:22:02]

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

[commit-bot: I haz the power, 2017-05-12 01:22:55]

Dry run: CQ is trying da patch.

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

[tbansal1, 2017-05-12 02:26:37]

Patchset #1 (id:180001) has been deleted

[commit-bot: I haz the power, 2017-05-12 02:49:27]

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

[commit-bot: I haz the power, 2017-05-12 02:49:28]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[tbansal1, 2017-05-12 06:50:59]

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

[commit-bot: I haz the power, 2017-05-12 06:51:26]

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-05-12 09:30:00]

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

[commit-bot: I haz the power, 2017-05-12 09:30:00]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[tbansal1, 2017-05-12 15:35:41]

Patchset #1 (id:220001) has been deleted

[tbansal1, 2017-05-12 15:39:45]

Description was changed from

==========
Expose RTT and downlink bandwidth using experimental Javascript API

Initial draft spec is here: https://cdn.rawgit.com/WICG/netinfo/ect/index.html

The API is runtime enabled feature, and is currently set to
"experimental".

This CL only implements "rtt" and "downlink" attributes. "ect" will
be implemented in a different CL.

BUG=719108
==========

to

==========
Expose RTT and downlink bandwidth using experimental Javascript API

Initial draft spec is here: https://cdn.rawgit.com/WICG/netinfo/ect/index.html

The API is runtime enabled feature, and is currently set to
"experimental".

This CL only implements "rtt" and "downlink" attributes. "ect" and
callback (when there is a change in network quality) will be 
implemented in forthcoming CLs.

BUG=719108
==========

[tbansal1, 2017-05-12 16:11:57]

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

[tbansal1, 2017-05-12 16:11:57]

jkarlin: third_party/WebKit/Source/modules/netinfo/* and 
 third_party/WebKit/Source/platform/network/*.

Thanks.

[tbansal1, 2017-05-12 16:12:47]

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

[tbansal1, 2017-05-12 16:12:47]

nasko: ptal at content/*. Thanks.

[bengr, 2017-05-12 19:09:55]

https://codereview.chromium.org/2863973003/diff/240001/content/browser/net_in...
File content/browser/net_info_browsertest.cc (right):

https://codereview.chromium.org/2863973003/diff/240001/content/browser/net_in...
content/browser/net_info_browsertest.cc:197:
base::TimeDelta::FromMilliseconds(123),
I would also test value of -1 since that's how you initialize. 0 and max int
might be good too.

https://codereview.chromium.org/2863973003/diff/240001/content/browser/net_in...
content/browser/net_info_browsertest.cc:216: // Make sure the minor changes in
the network quality are not notified.
Say how minor is minor, or at least say how to determine that.

https://codereview.chromium.org/2863973003/diff/240001/net/nqe/network_qualit...
File net/nqe/network_quality_estimator.cc (right):

https://codereview.chromium.org/2863973003/diff/240001/net/nqe/network_qualit...
net/nqe/network_quality_estimator.cc:1617: for (auto& observer :
throughput_observer_list_) {
I'd prefer that you didn't add test support into non-test code. Can you instead
put  the call to observer.OnThroughputObservation into its own virtualized
method? Alternatively, could you subclass the observer, virtualize
onThroughputObservation, and pass a subclass instance into NQE?

https://codereview.chromium.org/2863973003/diff/240001/net/nqe/network_qualit...
net/nqe/network_quality_estimator.cc:1691: // TODO(tbansal): Add hysteresis in
the notification.
What kind of hysteresis? Consider making the hysteresis part of the observer.

https://codereview.chromium.org/2863973003/diff/240001/net/nqe/network_qualit...
net/nqe/network_quality_estimator.cc:1699: if
(suppress_notifications_for_testing_)
Yeah, this is gross. Virtualize.

https://codereview.chromium.org/2863973003/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/netinfo/NetworkInformation.h (right):

https://codereview.chromium.org/2863973003/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/netinfo/NetworkInformation.h:40: double
downlink_throughput_mbps) override;
Names are weird. Either do s/downlink_max_mbps/downlink_throughput_max_mbps/ or
s/downlink_throughput_mbps/downlink_mbps/.

https://codereview.chromium.org/2863973003/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/netinfo/NetworkInformation.h:74: double
downlink_throughput_mbps_;
Names are similarly weird.

https://codereview.chromium.org/2863973003/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/netinfo/NetworkInformation.idl (right):

https://codereview.chromium.org/2863973003/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/netinfo/NetworkInformation.idl:19: typedef
unrestricted double MilliSeconds;
Should 'S' really be capitalized?

[tbansal1, 2017-05-12 22:08:23]

Patchset #2 (id:250001) has been deleted

[tbansal1, 2017-05-12 22:18:04]

Patchset #2 (id:270001) has been deleted

[tbansal1, 2017-05-12 22:18:17]

ptal. Thanks.

https://codereview.chromium.org/2863973003/diff/240001/content/browser/net_in...
File content/browser/net_info_browsertest.cc (right):

https://codereview.chromium.org/2863973003/diff/240001/content/browser/net_in...
content/browser/net_info_browsertest.cc:197:
base::TimeDelta::FromMilliseconds(123),
On 2017/05/12 19:09:54, bengr wrote:
> I would also test value of -1 since that's how you initialize. 0 and max int
> might be good too.

Added -1 and 0 check.

https://codereview.chromium.org/2863973003/diff/240001/content/browser/net_in...
content/browser/net_info_browsertest.cc:216: // Make sure the minor changes in
the network quality are not notified.
On 2017/05/12 19:09:54, bengr wrote:
> Say how minor is minor, or at least say how to determine that.

Done.

https://codereview.chromium.org/2863973003/diff/240001/net/nqe/network_qualit...
File net/nqe/network_quality_estimator.cc (right):

https://codereview.chromium.org/2863973003/diff/240001/net/nqe/network_qualit...
net/nqe/network_quality_estimator.cc:1617: for (auto& observer :
throughput_observer_list_) {
On 2017/05/12 19:09:54, bengr wrote:
> I'd prefer that you didn't add test support into non-test code. Can you
instead
> put  the call to observer.OnThroughputObservation into its own virtualized
> method? Alternatively, could you subclass the observer, virtualize
> onThroughputObservation, and pass a subclass instance into NQE?

Done.

https://codereview.chromium.org/2863973003/diff/240001/net/nqe/network_qualit...
net/nqe/network_quality_estimator.cc:1691: // TODO(tbansal): Add hysteresis in
the notification.
On 2017/05/12 19:09:55, bengr wrote:
> What kind of hysteresis? Consider making the hysteresis part of the observer.

Removed this. We can consider adding hysteresis in the observer interface at
some point. Currently, hysteresis already exists for the notifications sent from
the browser to the render processes.

https://codereview.chromium.org/2863973003/diff/240001/net/nqe/network_qualit...
net/nqe/network_quality_estimator.cc:1699: if
(suppress_notifications_for_testing_)
On 2017/05/12 19:09:55, bengr wrote:
> Yeah, this is gross. Virtualize.

Done.

https://codereview.chromium.org/2863973003/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/netinfo/NetworkInformation.h (right):

https://codereview.chromium.org/2863973003/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/netinfo/NetworkInformation.h:40: double
downlink_throughput_mbps) override;
On 2017/05/12 19:09:55, bengr wrote:
> Names are weird. Either do s/downlink_max_mbps/downlink_throughput_max_mbps/
or
> s/downlink_throughput_mbps/downlink_mbps/.

Done.

https://codereview.chromium.org/2863973003/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/netinfo/NetworkInformation.h:74: double
downlink_throughput_mbps_;
On 2017/05/12 19:09:55, bengr wrote:
> Names are similarly weird.

Done.

https://codereview.chromium.org/2863973003/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/netinfo/NetworkInformation.idl (right):

https://codereview.chromium.org/2863973003/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/netinfo/NetworkInformation.idl:19: typedef
unrestricted double MilliSeconds;
On 2017/05/12 19:09:55, bengr wrote:
> Should 'S' really be capitalized?

Done.

[tbansal1, 2017-05-12 22:18:20]

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

[commit-bot: I haz the power, 2017-05-12 22:19:57]

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-05-12 22:26:27]

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

[commit-bot: I haz the power, 2017-05-12 22:26:29]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[nasko, 2017-05-12 23:29:54]

I would leave the more in-depth review of the content test to bengr@, but left
some comments about general test structure.

https://codereview.chromium.org/2863973003/diff/290001/content/browser/net_in...
File content/browser/net_info_browsertest.cc (right):

https://codereview.chromium.org/2863973003/diff/290001/content/browser/net_in...
content/browser/net_info_browsertest.cc:161: NavigateToURL(shell(),
content::GetTestUrl("", "net_info.html"));
Why not load this through the embedded_test_server()? Does it need to be a
file:// based navigation? If yes, please add a comment to ensure it is known
why.

https://codereview.chromium.org/2863973003/diff/290001/content/browser/net_in...
content/browser/net_info_browsertest.cc:162: base::RunLoop().RunUntilIdle();
Why do we need this? NavigateToURL is waiting for the navigation to complete. If
you need to wait for something else, it should be an explicit wait for that
event.

https://codereview.chromium.org/2863973003/diff/290001/content/browser/net_in...
content/browser/net_info_browsertest.cc:182: base::RunLoop().RunUntilIdle();
Same here, let's wait for something explicit.

[tbansal1, 2017-05-13 22:44:40]

Patchset #3 (id:310001) has been deleted

[tbansal1, 2017-05-13 22:44:48]

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

[commit-bot: I haz the power, 2017-05-13 22:44:52]

Dry run: CQ is trying da patch.

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

[tbansal1, 2017-05-13 23:31:18]

Patchset #4 (id:350001) has been deleted

[tbansal1, 2017-05-13 23:31:31]

Patchset #3 (id:330001) has been deleted

[tbansal1, 2017-05-13 23:32:39]

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

[commit-bot: I haz the power, 2017-05-13 23:32:44]

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-05-14 01:22:22]

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

[commit-bot: I haz the power, 2017-05-14 01:22:24]

Dry run: This issue passed the CQ dry run.

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

bengr, jkarlin, nasko: ptal. Thanks.

https://codereview.chromium.org/2863973003/diff/290001/content/browser/net_in...
File content/browser/net_info_browsertest.cc (right):

https://codereview.chromium.org/2863973003/diff/290001/content/browser/net_in...
content/browser/net_info_browsertest.cc:161: NavigateToURL(shell(),
content::GetTestUrl("", "net_info.html"));
On 2017/05/12 23:29:54, nasko wrote:
> Why not load this through the embedded_test_server()? Does it need to be a
> file:// based navigation? If yes, please add a comment to ensure it is known
> why.

Done.

https://codereview.chromium.org/2863973003/diff/290001/content/browser/net_in...
content/browser/net_info_browsertest.cc:162: base::RunLoop().RunUntilIdle();
On 2017/05/12 23:29:54, nasko wrote:
> Why do we need this? NavigateToURL is waiting for the navigation to complete.
If
> you need to wait for something else, it should be an explicit wait for that
> event.

Done. There are still couple of RunUntilIdle() but they are at places where they
are not preceded by NavigateToURL().

https://codereview.chromium.org/2863973003/diff/290001/content/browser/net_in...
content/browser/net_info_browsertest.cc:182: base::RunLoop().RunUntilIdle();
On 2017/05/12 23:29:54, nasko wrote:
> Same here, let's wait for something explicit.

Done.

[bengr, 2017-05-15 21:20:01]

lgtm

[jkarlin, 2017-05-16 14:32:59]

Looks good!

https://codereview.chromium.org/2863973003/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/netinfo/NetworkInformation.cpp (right):

https://codereview.chromium.org/2863973003/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/netinfo/NetworkInformation.cpp:44: int
roundRtt(int rtt_msec) {
RoundRtt

https://codereview.chromium.org/2863973003/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/netinfo/NetworkInformation.cpp:49: double
roundMbps(double downlink_mbps) {
RoundMbps

https://codereview.chromium.org/2863973003/diff/370001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/netinfo/NetworkInformation.idl (right):

https://codereview.chromium.org/2863973003/diff/370001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/netinfo/NetworkInformation.idl:19: typedef
unrestricted double Milliseconds;
The spec says unsigned long long for milliseconds

https://codereview.chromium.org/2863973003/diff/370001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/network/NetworkStateNotifierTest.cpp
(right):

https://codereview.chromium.org/2863973003/diff/370001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/network/NetworkStateNotifierTest.cpp:213:
EXPECT_EQ(observer.CallbackCount(), 2);
Can you verify that changing just the connection type or just the throughput/rtt
incurs an increment of 1 to callback count?

[tbansal1, 2017-05-16 18:22:26]

Patchset #4 (id:390001) has been deleted

[tbansal1, 2017-05-16 18:23:28]

jkarlin: ptal at
 third_party/WebKit/Source/modules/netinfo/* and 
 third_party/WebKit/Source/platform/network/*.
I have also added myself to netinfo OWNERS, but let me know if that's not cool.
Thanks.

https://codereview.chromium.org/2863973003/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/netinfo/NetworkInformation.cpp (right):

https://codereview.chromium.org/2863973003/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/netinfo/NetworkInformation.cpp:44: int
roundRtt(int rtt_msec) {
On 2017/05/16 14:32:59, jkarlin wrote:
> RoundRtt

Done.

https://codereview.chromium.org/2863973003/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/netinfo/NetworkInformation.cpp:49: double
roundMbps(double downlink_mbps) {
On 2017/05/16 14:32:59, jkarlin wrote:
> RoundMbps

Done.

https://codereview.chromium.org/2863973003/diff/370001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/netinfo/NetworkInformation.idl (right):

https://codereview.chromium.org/2863973003/diff/370001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/netinfo/NetworkInformation.idl:19: typedef
unrestricted double Milliseconds;
On 2017/05/16 14:32:59, jkarlin wrote:
> The spec says unsigned long long for milliseconds

Done.

https://codereview.chromium.org/2863973003/diff/370001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/network/NetworkStateNotifierTest.cpp
(right):

https://codereview.chromium.org/2863973003/diff/370001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/network/NetworkStateNotifierTest.cpp:213:
EXPECT_EQ(observer.CallbackCount(), 2);
On 2017/05/16 14:32:59, jkarlin wrote:
> Can you verify that changing just the connection type or just the
throughput/rtt
> incurs an increment of 1 to callback count?

Done.

[tbansal1, 2017-05-16 18:28:08]

tbansal@chromium.org changed reviewers:
+ dcheng@chromium.org, kinuko@chromium.org

[tbansal1, 2017-05-16 18:28:09]

PTAL.
kinuko: third_party/WebKit/Source/platform/*
dcheng: third_party/WebKit/Source/core/frame/*
Thanks.

[tbansal1, 2017-05-16 18:29:25]

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

[commit-bot: I haz the power, 2017-05-16 18:31:19]

Dry run: CQ is trying da patch.

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

[jkarlin, 2017-05-16 18:37:19]

https://codereview.chromium.org/2863973003/diff/370001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/netinfo/NetworkInformation.idl (right):

https://codereview.chromium.org/2863973003/diff/370001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/netinfo/NetworkInformation.idl:19: typedef
unrestricted double Milliseconds;
On 2017/05/16 18:23:28, tbansal1 wrote:
> On 2017/05/16 14:32:59, jkarlin wrote:
> > The spec says unsigned long long for milliseconds
> 
> Done.

Why remove the typedef? Might as well have it as it's in the spec IDL.

https://codereview.chromium.org/2863973003/diff/410001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/netinfo/NetworkInformation.cpp (right):

https://codereview.chromium.org/2863973003/diff/410001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/netinfo/NetworkInformation.cpp:46: return
rtt_msec;
This is returning a negative value but rtt() is supposed to be unsigned long
long?

https://codereview.chromium.org/2863973003/diff/410001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/netinfo/NetworkInformation.cpp:87: long
NetworkInformation::rtt() const {
unsigned long long?

[tbansal1, 2017-05-16 20:13:50]

Patchset #6 (id:450001) has been deleted

[tbansal1, 2017-05-16 21:34:30]

jkarlin: Thanks for the comments. PTAL. I have also update
NetworkStateNotifier to use Optional and TimeDelta to store RTTs (instead of
using integers/long).

https://codereview.chromium.org/2863973003/diff/370001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/netinfo/NetworkInformation.idl (right):

https://codereview.chromium.org/2863973003/diff/370001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/netinfo/NetworkInformation.idl:19: typedef
unrestricted double Milliseconds;
On 2017/05/16 18:37:19, jkarlin wrote:
> On 2017/05/16 18:23:28, tbansal1 wrote:
> > On 2017/05/16 14:32:59, jkarlin wrote:
> > > The spec says unsigned long long for milliseconds
> > 
> > Done.
> 
> Why remove the typedef? Might as well have it as it's in the spec IDL.

Done.

https://codereview.chromium.org/2863973003/diff/410001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/netinfo/NetworkInformation.cpp (right):

https://codereview.chromium.org/2863973003/diff/410001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/netinfo/NetworkInformation.cpp:46: return
rtt_msec;
On 2017/05/16 18:37:19, jkarlin wrote:
> This is returning a negative value but rtt() is supposed to be unsigned long
> long?

Done.

https://codereview.chromium.org/2863973003/diff/410001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/netinfo/NetworkInformation.cpp:87: long
NetworkInformation::rtt() const {
On 2017/05/16 18:37:19, jkarlin wrote:
> unsigned long long?

Done.

[tbansal1, 2017-05-16 23:45:10]

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

[commit-bot: I haz the power, 2017-05-16 23:46:30]

Dry run: CQ is trying da patch.

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

[kinuko, 2017-05-17 02:05:45]

https://codereview.chromium.org/2863973003/diff/470001/content/renderer/rende...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/2863973003/diff/470001/content/renderer/rende...
content/renderer/render_thread_impl.cc:2130: downlink_throughput_kbps);
So we're downcasting double to int here?  Why don't we sent them in
base::TimeDelta?

https://codereview.chromium.org/2863973003/diff/470001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/network/NetworkStateNotifier.h (right):

https://codereview.chromium.org/2863973003/diff/470001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/network/NetworkStateNotifier.h:149: void
SetWebNetworkQuality(int http_rtt_msec,
Just SetNetworkQuality ? 'WebNetworkQuality' doesn't seem to make a lot sense.

[commit-bot: I haz the power, 2017-05-17 02:40:07]

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

[commit-bot: I haz the power, 2017-05-17 02:40:08]

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

[tbansal1, 2017-05-17 05:25:27]

kinuko. dcheng: ptal. Thanks.

https://codereview.chromium.org/2863973003/diff/470001/content/renderer/rende...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/2863973003/diff/470001/content/renderer/rende...
content/renderer/render_thread_impl.cc:2130: downlink_throughput_kbps);
On 2017/05/17 02:05:45, kinuko wrote:
> So we're downcasting double to int here?  Why don't we sent them in
> base::TimeDelta?

Done. Thanks!

https://codereview.chromium.org/2863973003/diff/470001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/network/NetworkStateNotifier.h (right):

https://codereview.chromium.org/2863973003/diff/470001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/network/NetworkStateNotifier.h:149: void
SetWebNetworkQuality(int http_rtt_msec,
On 2017/05/17 02:05:45, kinuko wrote:
> Just SetNetworkQuality ? 'WebNetworkQuality' doesn't seem to make a lot sense.

Done.

[tbansal1, 2017-05-17 05:26:13]

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

[commit-bot: I haz the power, 2017-05-17 05:26:56]

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-05-17 05:55:05]

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

[commit-bot: I haz the power, 2017-05-17 05:55:06]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[dcheng, 2017-05-17 12:27:47]

https://codereview.chromium.org/2863973003/diff/490001/content/common/rendere...
File content/common/renderer.mojom (right):

https://codereview.chromium.org/2863973003/diff/490001/content/common/rendere...
content/common/renderer.mojom:166:
OnNetworkQualityChanged(mojo.common.mojom.TimeDelta http_rtt,
mojo.common.mojom.TimeDelta transport_rtt, double bandwidth_kbps);
Nit: 80 characters

https://codereview.chromium.org/2863973003/diff/490001/net/nqe/network_qualit...
File net/nqe/network_quality_estimator_test_util.h (right):

https://codereview.chromium.org/2863973003/diff/490001/net/nqe/network_qualit...
net/nqe/network_quality_estimator_test_util.h:57: bool
suppress_notifications_for_testing,
FWIW, this is a lot of bools. I guess it's just extending the existing ones, but
it may be useful to consider policy enums/constants to control test behavior at
some point.

https://codereview.chromium.org/2863973003/diff/490001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/platform/linux/fast/dom/Window/property-access-on-cached-window-after-frame-navigated-expected.txt
(right):

https://codereview.chromium.org/2863973003/diff/490001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/platform/linux/fast/dom/Window/property-access-on-cached-window-after-frame-navigated-expected.txt:1:
CONSOLE WARNING: line 108: 'window.webkitStorageInfo' is deprecated. Please use
'navigator.webkitTemporaryStorage' or 'navigator.webkitPersistentStorage'
instead.
FWIW, I think the linux-specific versions of these expected files shouldn't be
needed anymore after https://codereview.chromium.org/2706923002/.

https://codereview.chromium.org/2863973003/diff/490001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/network/NetworkStateNotifier.cpp
(right):

https://codereview.chromium.org/2863973003/diff/490001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/network/NetworkStateNotifier.cpp:106:
http_rtt.InMilliseconds() < 0 ? Optional<TimeDelta>() : http_rtt;
Nit: does nullopt instead of Optional<TimeDelta>() work here? Same question
below.

https://codereview.chromium.org/2863973003/diff/490001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/WebNetworkStateNotifier.h (right):

https://codereview.chromium.org/2863973003/diff/490001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebNetworkStateNotifier.h:34: #include
"platform/wtf/Time.h"
I'm not sure we can use platform/wtf/Time.h in a public header. =/

Normally, we'd just IPC directly to a Mojo interface in Blink, but I'm not sure
if that's an option here--there's another network connection IPC that needs to
be in content/renderer (since it notifies observers), and it'd be weird to
define only one of them in Blink...

[jkarlin, 2017-05-17 12:59:15]

https://codereview.chromium.org/2863973003/diff/490001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/netinfo/NetworkInformation.cpp (right):

https://codereview.chromium.org/2863973003/diff/490001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/netinfo/NetworkInformation.cpp:47: return 0;
This behavior isn't mentioned in the spec.

https://codereview.chromium.org/2863973003/diff/490001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/netinfo/NetworkInformation.cpp:49: 
DCHECK that rtt is <= max int size.

https://codereview.chromium.org/2863973003/diff/490001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/netinfo/NetworkInformation.cpp:60: return
std::numeric_limits<double>::infinity();
This behavior isn't mentioned in the spec.

[tbansal1, 2017-05-18 00:10:30]

ptal. Thanks.

https://codereview.chromium.org/2863973003/diff/490001/content/common/rendere...
File content/common/renderer.mojom (right):

https://codereview.chromium.org/2863973003/diff/490001/content/common/rendere...
content/common/renderer.mojom:166:
OnNetworkQualityChanged(mojo.common.mojom.TimeDelta http_rtt,
mojo.common.mojom.TimeDelta transport_rtt, double bandwidth_kbps);
On 2017/05/17 12:27:46, dcheng (in AEST) wrote:
> Nit: 80 characters

Done. I assumed git cl format would have fixed it.

https://codereview.chromium.org/2863973003/diff/490001/net/nqe/network_qualit...
File net/nqe/network_quality_estimator_test_util.h (right):

https://codereview.chromium.org/2863973003/diff/490001/net/nqe/network_qualit...
net/nqe/network_quality_estimator_test_util.h:57: bool
suppress_notifications_for_testing,
On 2017/05/17 12:27:46, dcheng (in AEST) wrote:
> FWIW, this is a lot of bools. I guess it's just extending the existing ones,
but
> it may be useful to consider policy enums/constants to control test behavior
at
> some point.

Agreed. I will fix it, but I will prefer to do that in a separate CL.

https://codereview.chromium.org/2863973003/diff/490001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/platform/linux/fast/dom/Window/property-access-on-cached-window-after-frame-navigated-expected.txt
(right):

https://codereview.chromium.org/2863973003/diff/490001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/platform/linux/fast/dom/Window/property-access-on-cached-window-after-frame-navigated-expected.txt:1:
CONSOLE WARNING: line 108: 'window.webkitStorageInfo' is deprecated. Please use
'navigator.webkitTemporaryStorage' or 'navigator.webkitPersistentStorage'
instead.
On 2017/05/17 12:27:46, dcheng (in AEST) wrote:
> FWIW, I think the linux-specific versions of these expected files shouldn't be
> needed anymore after https://codereview.chromium.org/2706923002/.

Acknowledged.

https://codereview.chromium.org/2863973003/diff/490001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/netinfo/NetworkInformation.cpp (right):

https://codereview.chromium.org/2863973003/diff/490001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/netinfo/NetworkInformation.cpp:47: return 0;
On 2017/05/17 12:59:14, jkarlin wrote:
> This behavior isn't mentioned in the spec.

Good point. As discussed offline, this should be okay.

https://codereview.chromium.org/2863973003/diff/490001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/netinfo/NetworkInformation.cpp:49: 
On 2017/05/17 12:59:14, jkarlin wrote:
> DCHECK that rtt is <= max int size.

Done.

https://codereview.chromium.org/2863973003/diff/490001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/netinfo/NetworkInformation.cpp:60: return
std::numeric_limits<double>::infinity();
On 2017/05/17 12:59:14, jkarlin wrote:
> This behavior isn't mentioned in the spec.

same as above.

https://codereview.chromium.org/2863973003/diff/490001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/network/NetworkStateNotifier.cpp
(right):

https://codereview.chromium.org/2863973003/diff/490001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/network/NetworkStateNotifier.cpp:106:
http_rtt.InMilliseconds() < 0 ? Optional<TimeDelta>() : http_rtt;
On 2017/05/17 12:27:46, dcheng (in AEST) wrote:
> Nit: does nullopt instead of Optional<TimeDelta>() work here? Same question
> below.

Done.

https://codereview.chromium.org/2863973003/diff/490001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/WebNetworkStateNotifier.h (right):

https://codereview.chromium.org/2863973003/diff/490001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebNetworkStateNotifier.h:34: #include
"platform/wtf/Time.h"
On 2017/05/17 12:27:46, dcheng (in AEST) wrote:
> I'm not sure we can use platform/wtf/Time.h in a public header. =/
> 
> Normally, we'd just IPC directly to a Mojo interface in Blink, but I'm not
sure
> if that's an option here--there's another network connection IPC that needs to
> be in content/renderer (since it notifies observers), and it'd be weird to
> define only one of them in Blink...

This should have been from the //base. Fixed.

[tbansal1, 2017-05-18 10:33:20]

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

[commit-bot: I haz the power, 2017-05-18 10:33:43]

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-05-18 15:55:06]

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

[commit-bot: I haz the power, 2017-05-18 15:55:08]

Dry run: This issue passed the CQ dry run.

[tbansal1, 2017-05-19 03:46:21]

kinuko, dcheng: ping. Thanks!!

[kinuko, 2017-05-19 15:09:55]

lgtm

[haraken, 2017-05-19 16:10:07]

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

[haraken, 2017-05-19 16:10:08]

https://codereview.chromium.org/2863973003/diff/510001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/netinfo/OWNERS (right):

https://codereview.chromium.org/2863973003/diff/510001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/netinfo/OWNERS:2: tbansal@chromium.org

Nit: Blink normally doesn't add a person to OWNERs until he/she commits a
certain amount of patches and gets familiar with the code base.

(Sorry, I'm saying this because people sometimes added their names to OWNERS in
the first patch and that had regressed the code quality of modules/.)

[tbansal1, 2017-05-19 17:18:40]

On 2017/05/19 16:10:08, haraken wrote:
>
https://codereview.chromium.org/2863973003/diff/510001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/modules/netinfo/OWNERS (right):
> 
>
https://codereview.chromium.org/2863973003/diff/510001/third_party/WebKit/Sou...
> third_party/WebKit/Source/modules/netinfo/OWNERS:2:
mailto:tbansal@chromium.org
> 
> Nit: Blink normally doesn't add a person to OWNERs until he/she commits a
> certain amount of patches and gets familiar with the code base.
> 
> (Sorry, I'm saying this because people sometimes added their names to OWNERS
in
> the first patch and that had regressed the code quality of modules/.)

Thanks, that makes sense. Done.

[tbansal1, 2017-05-19 17:19:00]

jkarlin, nasko: ptal. Thanks.

[jkarlin, 2017-05-19 17:29:07]

lgtm with comment

https://codereview.chromium.org/2863973003/diff/510001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/netinfo/NetworkInformation.cpp (right):

https://codereview.chromium.org/2863973003/diff/510001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/netinfo/NetworkInformation.cpp:49: return 0;
Please create an issue on github to get this (and the RoundMbps) behavior in the
spec.

[haraken, 2017-05-19 17:29:52]

WebKit LGTM

[dcheng, 2017-05-19 19:19:18]

https://codereview.chromium.org/2863973003/diff/530001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/exported/WebNetworkStateNotifier.cpp
(right):

https://codereview.chromium.org/2863973003/diff/530001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/exported/WebNetworkStateNotifier.cpp:46: void
WebNetworkStateNotifier::SetNetworkQuality(TimeDelta http_rtt,
The header declares this as base::TimeDelta while this is WTF::TimeDelta. While
this happens to work today because it's the same type, doing this is depending
on internal implementation details of WTF:TimeDelta.

I guess it's OK in this case since we plan on allowing subsets of base in Blink
soon though?

[tbansal1, 2017-05-19 19:28:59]

https://codereview.chromium.org/2863973003/diff/530001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/exported/WebNetworkStateNotifier.cpp
(right):

https://codereview.chromium.org/2863973003/diff/530001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/exported/WebNetworkStateNotifier.cpp:46: void
WebNetworkStateNotifier::SetNetworkQuality(TimeDelta http_rtt,
On 2017/05/19 19:19:18, dcheng (in AEST) wrote:
> The header declares this as base::TimeDelta while this is WTF::TimeDelta.
While
> this happens to work today because it's the same type, doing this is depending
> on internal implementation details of WTF:TimeDelta.
> 
> I guess it's OK in this case since we plan on allowing subsets of base in
Blink
> soon though?

I am not sure. The alternative is to pass the base::TimeDelta as int32_t, and
convert to WTF::TimeDelta. Please let me know.

[nasko, 2017-05-19 19:36:29]

content/ mostly rubberstamp LGTM. Deferring to bengr@ on the correctness of the
tests.

https://codereview.chromium.org/2863973003/diff/530001/content/common/rendere...
File content/common/renderer.mojom (right):

https://codereview.chromium.org/2863973003/diff/530001/content/common/rendere...
content/common/renderer.mojom:167: mojo.common.mojom.TimeDelta transport_rtt,
No need for an empty line here.

[dcheng, 2017-05-19 19:40:14]

https://codereview.chromium.org/2863973003/diff/530001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/exported/WebNetworkStateNotifier.cpp
(right):

https://codereview.chromium.org/2863973003/diff/530001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/exported/WebNetworkStateNotifier.cpp:46: void
WebNetworkStateNotifier::SetNetworkQuality(TimeDelta http_rtt,
On 2017/05/19 19:28:59, tbansal1 wrote:
> On 2017/05/19 19:19:18, dcheng (in AEST) wrote:
> > The header declares this as base::TimeDelta while this is WTF::TimeDelta.
> While
> > this happens to work today because it's the same type, doing this is
depending
> > on internal implementation details of WTF:TimeDelta.
> > 
> > I guess it's OK in this case since we plan on allowing subsets of base in
> Blink
> > soon though?
> 
> I am not sure. The alternative is to pass the base::TimeDelta as int32_t, and
> convert to WTF::TimeDelta. Please let me know.

Sorry to be clear, this question is aimed for haraken

[haraken, 2017-05-19 20:11:34]

On 2017/05/19 19:40:14, dcheng (in AEST) wrote:
>
https://codereview.chromium.org/2863973003/diff/530001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/platform/exported/WebNetworkStateNotifier.cpp
> (right):
> 
>
https://codereview.chromium.org/2863973003/diff/530001/third_party/WebKit/Sou...
> third_party/WebKit/Source/platform/exported/WebNetworkStateNotifier.cpp:46:
void
> WebNetworkStateNotifier::SetNetworkQuality(TimeDelta http_rtt,
> On 2017/05/19 19:28:59, tbansal1 wrote:
> > On 2017/05/19 19:19:18, dcheng (in AEST) wrote:
> > > The header declares this as base::TimeDelta while this is WTF::TimeDelta.
> > While
> > > this happens to work today because it's the same type, doing this is
> depending
> > > on internal implementation details of WTF:TimeDelta.
> > > 
> > > I guess it's OK in this case since we plan on allowing subsets of base in
> > Blink
> > > soon though?

Yeah, this will be fine. (Sorry for the delay for sending the proposal of
allowing //base types in Blink. It was blocked by the Onion Soup 2.0 design. I
think I can send the proposal next week.)

> > 
> > I am not sure. The alternative is to pass the base::TimeDelta as int32_t,
and
> > convert to WTF::TimeDelta. Please let me know.
> 
> Sorry to be clear, this question is aimed for haraken

[tbansal1, 2017-05-19 20:16:28]

https://codereview.chromium.org/2863973003/diff/530001/content/common/rendere...
File content/common/renderer.mojom (right):

https://codereview.chromium.org/2863973003/diff/530001/content/common/rendere...
content/common/renderer.mojom:167: mojo.common.mojom.TimeDelta transport_rtt,
On 2017/05/19 19:36:29, nasko wrote:
> No need for an empty line here.

Assuming I understand the comment correctly -- There is no empty line here. It
looks like there is one due to the way the diffs are shown.

[tbansal1, 2017-05-19 20:18:24]

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

[commit-bot: I haz the power, 2017-05-19 20:19:30]

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-05-19 20:23:05]

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

[commit-bot: I haz the power, 2017-05-19 20:23:06]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[tbansal1, 2017-05-19 20:58:11]

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

[commit-bot: I haz the power, 2017-05-19 20:58:57]

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-05-19 23:19:22]

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

[commit-bot: I haz the power, 2017-05-19 23:19:24]

Dry run: This issue passed the CQ dry run.

[tbansal1, 2017-05-22 04:23:26]

dcheng: comments? Thanks.

[dcheng, 2017-05-22 09:36:32]

sorry was travelling

lgtm

[tbansal1, 2017-05-22 14:25:09]

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

[commit-bot: I haz the power, 2017-05-22 14:25: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-05-22 14:27:56]

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

[commit-bot: I haz the power, 2017-05-22 14:27:58]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[tbansal1, 2017-05-22 16:35:39]

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

[commit-bot: I haz the power, 2017-05-22 16:36:05]

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-05-22 18:58:19]

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

[commit-bot: I haz the power, 2017-05-22 18:58:20]

Dry run: This issue passed the CQ dry run.

[tbansal1, 2017-05-22 19:06:08]

The CQ bit was checked by tbansal@chromium.org

[tbansal1, 2017-05-22 19:06:08]

The patchset sent to the CQ was uploaded after l-g-t-m from bengr@chromium.org,
kinuko@chromium.org, jkarlin@chromium.org, nasko@chromium.org,
haraken@chromium.org, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2863973003/#ps590001
(title: "Rebased")

[commit-bot: I haz the power, 2017-05-22 19:06:34]

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-05-22 19:13:02]

CQ is committing da patch.

Bot data: {"patchset_id": 590001, "attempt_start_ts": 1495479968099750,
"parent_rev": "e9d3b3997673274a86176c159bf453012b747196", "commit_rev":
"99d8aeb59f72b051b8f4b4af8a55b54c22dedfda"}

[commit-bot: I haz the power, 2017-05-22 19:13:18]

Description was changed from

==========
Expose RTT and downlink bandwidth using experimental Javascript API

Initial draft spec is here: https://cdn.rawgit.com/WICG/netinfo/ect/index.html

The API is runtime enabled feature, and is currently set to
"experimental".

This CL only implements "rtt" and "downlink" attributes. "ect" and
callback (when there is a change in network quality) will be 
implemented in forthcoming CLs.

BUG=719108
==========

to

==========
Expose RTT and downlink bandwidth using experimental Javascript API

Initial draft spec is here: https://cdn.rawgit.com/WICG/netinfo/ect/index.html

The API is runtime enabled feature, and is currently set to
"experimental".

This CL only implements "rtt" and "downlink" attributes. "ect" and
callback (when there is a change in network quality) will be 
implemented in forthcoming CLs.

BUG=719108

Review-Url: https://codereview.chromium.org/2863973003
Cr-Commit-Position: refs/heads/master@{#473649}
Committed:
https://chromium.googlesource.com/chromium/src/+/99d8aeb59f72b051b8f4b4af8a55...
==========

[commit-bot: I haz the power, 2017-05-22 19:13:21]

Committed patchset #12 (id:590001) as
https://chromium.googlesource.com/chromium/src/+/99d8aeb59f72b051b8f4b4af8a55...