Expose ECT to render frames, Blink and NetInfo

Expose ECT to render frames, Blink and NetInfo

Expose Effective Connection Type (ECT) from Network Quality Estimator
(NQE) to render frames. From there it is plumbed to the
NetworkStateNotifier and NetInfo API in Blink.

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

[tbansal1, 2017-05-13 22:07:31]

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

[tbansal1, 2017-05-13 22:07:35]

Patchset #1 (id:1) has been deleted

[commit-bot: I haz the power, 2017-05-13 22:07:41]

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

Description was changed from

==========
ECT to content

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

to

==========
ECT to content

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

[commit-bot: I haz the power, 2017-05-13 23:16:20]

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

[commit-bot: I haz the power, 2017-05-13 23:16:21]

Dry run: This issue passed the CQ dry run.

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

Description was changed from

==========
ECT to content

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

to

==========
Expose ECT to render frames

Expose Effective Connection Type (ECT) from Network Quality Estimator
(NQE) to render frames. This would be plumbed to Blink and via
Javascript APIs in forthcoming CLs.

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

[tbansal1, 2017-05-15 15:36:42]

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

[commit-bot: I haz the power, 2017-05-15 15:37:24]

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-15 16:01:18]

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

[commit-bot: I haz the power, 2017-05-15 16:01:53]

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-15 16:38:07]

Patchset #3 (id:60001) has been deleted

[tbansal1, 2017-05-15 16:40:22]

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

[tbansal1, 2017-05-15 16:40:26]

Patchset #2 (id:40001) has been deleted

[commit-bot: I haz the power, 2017-05-15 16:40: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-15 16:41:32]

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

[tbansal1, 2017-05-15 16:41:33]

nasko: ptal. Thanks.

[commit-bot: I haz the power, 2017-05-15 19:12:03]

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

[commit-bot: I haz the power, 2017-05-15 19:12:04]

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

[tbansal1, 2017-05-15 19:36:07]

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

[commit-bot: I haz the power, 2017-05-15 19:36: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-15 21:00:50]

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

[commit-bot: I haz the power, 2017-05-15 21:00:51]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

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

Patchset #3 (id:100001) has been deleted

[tbansal1, 2017-05-16 23:55:22]

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:56:16]

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 06:14:31]

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

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

Dry run: This issue passed the CQ dry run.

[tbansal1, 2017-05-22 20:51:20]

Patchset #2 (id:80001) has been deleted

[tbansal1, 2017-05-22 20:51:30]

Patchset #1 (id:20001) has been deleted

[tbansal1, 2017-05-22 21:15:01]

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

[commit-bot: I haz the power, 2017-05-22 21:15:38]

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-22 21:57:13]

Patchset #1 (id:120001) has been deleted

[tbansal1, 2017-05-22 21:57:39]

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

[tbansal1, 2017-05-22 21:57:40]

nasko: ping.
ryansturm: ptal at * or //net/nqe/. Thanks.

[tbansal1, 2017-05-22 21:58:00]

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

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

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 22:08:44]

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

[commit-bot: I haz the power, 2017-05-22 22:08:45]

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

[nasko, 2017-05-23 04:54:37]

Can you list the upcoming CL that will be using this information in the
renderer? Is there any reason why it is not contained in this CL? It is usually
much easier to review the end to end IPC communication in a single CL instead of
spreading it around.

Also, this CL seems to have lots of red bots.

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

https://codereview.chromium.org/2883763002/diff/140001/content/renderer/rende...
content/renderer/render_thread_impl.cc:2141: net::EffectiveConnectionType type,
This doesn't seem to be used at all.

[tbansal1, 2017-05-23 06:19:40]

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

[commit-bot: I haz the power, 2017-05-23 06:19: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-23 06:42:06]

Description was changed from

==========
Expose ECT to render frames

Expose Effective Connection Type (ECT) from Network Quality Estimator
(NQE) to render frames. This would be plumbed to Blink and via
Javascript APIs in forthcoming CLs.

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

to

==========
Expose ECT to render frames and Blink

Expose Effective Connection Type (ECT) from Network Quality Estimator
(NQE) to render frames. From there it is plumbed to the
NetworkStateNotifier in Blink. It would be exposed via
Javascript APIs in forthcoming CLs.

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

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

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

[commit-bot: I haz the power, 2017-05-23 08:05:12]

Dry run: This issue passed the CQ dry run.

[tbansal1, 2017-05-23 17:50:45]

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

[commit-bot: I haz the power, 2017-05-23 17:51:29]

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

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

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

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-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...)
  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-23 17:56:49]

Description was changed from

==========
Expose ECT to render frames and Blink

Expose Effective Connection Type (ECT) from Network Quality Estimator
(NQE) to render frames. From there it is plumbed to the
NetworkStateNotifier in Blink. It would be exposed via
Javascript APIs in forthcoming CLs.

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

to

==========
Expose ECT to render frames, Blink and NetInfo

Expose Effective Connection Type (ECT) from Network Quality Estimator
(NQE) to render frames. From there it is plumbed to the
NetworkStateNotifier and NetInfo API in Blink.

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

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

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

[tbansal1, 2017-05-23 18:03:09]

Patchset #3 (id:180001) has been deleted

[commit-bot: I haz the power, 2017-05-23 18:04:02]

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-23 18:08:41]

Patchset #2 (id:160001) has been deleted

[tbansal1, 2017-05-23 18:35:27]

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

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

nasko: ptal at content/*

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

dcheng: third_party/WebKit/Source/core/

Thanks.

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

https://codereview.chromium.org/2883763002/diff/140001/content/renderer/rende...
content/renderer/render_thread_impl.cc:2141: net::EffectiveConnectionType type,
On 2017/05/23 04:54:37, nasko wrote:
> This doesn't seem to be used at all.

I have added the plumbing to Blink.

[tbansal1, 2017-05-23 18:42:55]

On 2017/05/23 04:54:37, nasko wrote:
> Can you list the upcoming CL that will be using this information in the
> renderer? Is there any reason why it is not contained in this CL? It is
usually
> much easier to review the end to end IPC communication in a single CL instead
of
> spreading it around.
> 
This CL now includes the Blink and NetInfo changes.

[commit-bot: I haz the power, 2017-05-23 20:04:03]

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

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

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_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[nasko, 2017-05-23 21:42:05]

I'm sorry I missed an important piece in my previous review pass. When removing
something from an IPC file, it has to be removed from the real struct in C++
code too. This CL does only the IPC change without removing it from the C++
struct, which leads to inconsistency.

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

https://codereview.chromium.org/2883763002/diff/200001/content/browser/net_in...
content/browser/net_info_browsertest.cc:183: EXPECT_EQ("4g",
RunScriptExtractString("getEffectiveType()"));
Don't we have string constants defined for these?

https://codereview.chromium.org/2883763002/diff/200001/content/common/resourc...
File content/common/resource_messages.h (left):

https://codereview.chromium.org/2883763002/diff/200001/content/common/resourc...
content/common/resource_messages.h:223:
IPC_STRUCT_TRAITS_MEMBER(effective_connection_type)
Why is this removed from here and not the actual definition of
content::ResourceResponseInfo? This macro is supposed to describe the real C++
struct defined elsewhere and with this change they will mismatch.

[RyanSturm, 2017-05-23 22:12:41]

net/nqe: lgtm

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

ptal. Thanks.

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

https://codereview.chromium.org/2883763002/diff/200001/content/browser/net_in...
content/browser/net_info_browsertest.cc:183: EXPECT_EQ("4g",
RunScriptExtractString("getEffectiveType()"));
On 2017/05/23 21:42:04, nasko wrote:
> Don't we have string constants defined for these?

No, the strings come from NetworkInformation.cpp in Webkit. I can move the
constants to some common place where they are accessible to both content/ and
NetworkInformation.cpp, but that might be an overkill?

https://codereview.chromium.org/2883763002/diff/200001/content/common/resourc...
File content/common/resource_messages.h (left):

https://codereview.chromium.org/2883763002/diff/200001/content/common/resourc...
content/common/resource_messages.h:223:
IPC_STRUCT_TRAITS_MEMBER(effective_connection_type)
On 2017/05/23 21:42:04, nasko wrote:
> Why is this removed from here and not the actual definition of
> content::ResourceResponseInfo? This macro is supposed to describe the real C++
> struct defined elsewhere and with this change they will mismatch.

Thanks. Fixed.
Also, filed crbug.com/725680 for cleanup.

[nasko, 2017-05-23 22:45:39]

IPC LGTM

[tbansal1, 2017-05-23 22:54:39]

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

[tbansal1, 2017-05-23 22:54:40]

haraken:  third_party/WebKit/Source/platform/*

[dcheng, 2017-05-23 23:43:03]

Source/core lgtm

[tbansal1, 2017-05-24 00:58:33]

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

[commit-bot: I haz the power, 2017-05-24 00:59:52]

Dry run: CQ is trying da patch.

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

[haraken, 2017-05-24 01:06:19]

platform/ LGTM

[commit-bot: I haz the power, 2017-05-24 04:39:02]

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

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

Dry run: This issue passed the CQ dry run.

[tbansal1, 2017-05-24 17:23:43]

jkarlin: comments? Thanks.

[tbansal1, 2017-05-25 16:00:44]

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

[commit-bot: I haz the power, 2017-05-25 16:01:18]

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-25 16:04:38]

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

[commit-bot: I haz the power, 2017-05-25 16:04:39]

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-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  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_...)

[jkarlin, 2017-05-25 16:08:39]

lgtm, sorry for the delay

[tbansal1, 2017-05-25 16:21:17]

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

[commit-bot: I haz the power, 2017-05-25 16:21:53]

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-25 18:33:41]

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

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

Dry run: This issue passed the CQ dry run.

[tbansal1, 2017-05-25 18:44:24]

The CQ bit was checked by tbansal@chromium.org

[tbansal1, 2017-05-25 18:44:25]

The patchset sent to the CQ was uploaded after l-g-t-m from
ryansturm@chromium.org, haraken@chromium.org, jkarlin@chromium.org,
nasko@chromium.org, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2883763002/#ps240001
(title: "rebased")

[commit-bot: I haz the power, 2017-05-25 18:44:56]

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-25 18:53:12]

CQ is committing da patch.

Bot data: {"patchset_id": 240001, "attempt_start_ts": 1495737864520800,
"parent_rev": "5b72981a509569fa37fac74dc2488f57ddbc5930", "commit_rev":
"b612c5de3b9f415ff5feebd799ff954f1b4d7daa"}

[commit-bot: I haz the power, 2017-05-25 18:53:25]

Description was changed from

==========
Expose ECT to render frames, Blink and NetInfo

Expose Effective Connection Type (ECT) from Network Quality Estimator
(NQE) to render frames. From there it is plumbed to the
NetworkStateNotifier and NetInfo API in Blink.

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

to

==========
Expose ECT to render frames, Blink and NetInfo

Expose Effective Connection Type (ECT) from Network Quality Estimator
(NQE) to render frames. From there it is plumbed to the
NetworkStateNotifier and NetInfo API in Blink.

BUG=719108

Review-Url: https://codereview.chromium.org/2883763002
Cr-Commit-Position: refs/heads/master@{#474729}
Committed:
https://chromium.googlesource.com/chromium/src/+/b612c5de3b9f415ff5feebd799ff...
==========

[commit-bot: I haz the power, 2017-05-25 18:53:28]

Committed patchset #4 (id:240001) as
https://chromium.googlesource.com/chromium/src/+/b612c5de3b9f415ff5feebd799ff...

[foolip, 2017-05-29 11:39:39]

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

[foolip, 2017-05-29 11:39:40]

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

https://codereview.chromium.org/2883763002/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/netinfo/NetworkInformation.idl:38:
[MeasureAs=NetInfoEffectiveType] readonly attribute EffectiveConnectionType
effectiveType;
Looks like RuntimeEnabled=NetInfoEffectiveType was omitted accidentally here,
shipping effectiveType for M60. This may have been a problem since
https://codereview.chromium.org/2642873002/, not sure.

Filed https://crbug.com/727285 to sort it out.