NetInfo network quality extension: Add callbacks and Layout tests

NetInfo network quality extension: Add callbacks and Layout tests

"change" event is fired from NetworkInformation.cpp when there
is a change in the network quality.

Also, layout tests have been added for the network quality extension
to the NetInfo API.

BUG=725282, 726762
Review-Url: https://codereview.chromium.org/2903493002
Cr-Commit-Position: refs/heads/master@{#476595}
Committed: https://chromium.googlesource.com/chromium/src/+/f51122ff533e752a86a27193ca7539c450b3670e

[tbansal1, 2017-05-23 01:05:25]

Description was changed from

==========
w

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

to

==========
NetInfo network quality estension: Add callbacks and Layout tests

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

[tbansal1, 2017-05-23 01:05:33]

Description was changed from

==========
NetInfo network quality estension: Add callbacks and Layout tests

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

to

==========
NetInfo network quality extension: Add callbacks and Layout tests

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

[tbansal1, 2017-05-23 01:05:38]

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

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

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 01:17:36]

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

[commit-bot: I haz the power, 2017-05-23 01:17:37]

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

[tbansal1, 2017-05-23 05:04:34]

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

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

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:20:45]

Patchset #1 (id:1) has been deleted

[commit-bot: I haz the power, 2017-05-23 06:48:35]

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

[commit-bot: I haz the power, 2017-05-23 06:48:36]

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

[tbansal1, 2017-05-23 09:26:16]

Patchset #2 (id:40001) has been deleted

[tbansal1, 2017-05-23 09:26:36]

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

[commit-bot: I haz the power, 2017-05-23 09:26:45]

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 09:26:48]

Patchset #1 (id:20001) has been deleted

[commit-bot: I haz the power, 2017-05-23 09:34:25]

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

[commit-bot: I haz the power, 2017-05-23 09:34:26]

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_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...)
  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-23 14:26:52]

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

[commit-bot: I haz the power, 2017-05-23 14:27: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-23 15:19:01]

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

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

Dry run: This issue passed the CQ dry run.

[tbansal1, 2017-05-23 19:49:35]

Patchset #2 (id:80001) has been deleted

[tbansal1, 2017-05-23 20:19:02]

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

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

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

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

[commit-bot: I haz the power, 2017-05-23 22:26:27]

Dry run: This issue passed the CQ dry run.

[tbansal1, 2017-05-23 22:28:15]

Patchset #1 (id:60001) has been deleted

[tbansal1, 2017-05-24 00:37:02]

Patchset #2 (id:120001) has been deleted

[tbansal1, 2017-05-24 00:37:19]

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:39: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-05-24 03:31:28]

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

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

Dry run: This issue passed the CQ dry run.

[tbansal1, 2017-05-24 04:36:28]

Patchset #1 (id:100001) has been deleted

[tbansal1, 2017-05-26 16:58:43]

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

[commit-bot: I haz the power, 2017-05-26 16:59:25]

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-26 17:15:36]

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

[tbansal1, 2017-05-26 17:15:42]

Patchset #2 (id:150001) has been deleted

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

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-26 17:16:49]

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

[tbansal1, 2017-05-26 17:16:58]

Patchset #1 (id:50013) has been deleted

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

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

Description was changed from

==========
NetInfo network quality extension: Add callbacks and Layout tests

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

to

==========
NetInfo network quality extension: Add callbacks and Layout tests

"change" event is fired from NetworkInformation.cpp when there
is a change in the network quality.

Also, layout tests have been added for the network quality extension
to the NetInfo API.

BUG=725282, 726762
==========

[tbansal1, 2017-05-26 17:25:36]

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

[tbansal1, 2017-05-26 17:25:38]

dcheng: third_party/WebKit/Source/core/*
haraken: third_party/WebKit/Source/modules/*,
third_party/WebKit/Source/platform/*

Thanks.

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

dcheng: third_party/WebKit/Source/core/*
haraken: third_party/WebKit/Source/modules/*,
third_party/WebKit/Source/platform/*

Thanks.

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

dcheng: third_party/WebKit/Source/core/*
haraken: third_party/WebKit/Source/modules/*,
third_party/WebKit/Source/platform/*

Thanks.

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

dcheng: third_party/WebKit/Source/core/*
haraken: third_party/WebKit/Source/modules/*,
third_party/WebKit/Source/platform/*

Thanks.

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

dcheng: third_party/WebKit/Source/core/*
haraken: third_party/WebKit/Source/modules/*,
third_party/WebKit/Source/platform/*

Thanks.

[commit-bot: I haz the power, 2017-05-26 19:45:46]

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

[commit-bot: I haz the power, 2017-05-26 19:45:49]

Dry run: This issue passed the CQ dry run.

[dcheng, 2017-05-27 08:47:47]

https://codereview.chromium.org/2903493002/diff/170001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/testing/Internals.idl (right):

https://codereview.chromium.org/2903493002/diff/170001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/testing/Internals.idl:317: [RaisesException] void
setNetworkQualityInfoOverride(DOMString effective_type, unsigned long
transport_rtt_msec, double downlink_throughput_mbps);
Out of curiosity, is it possible to use enums in the WebIDL for this?

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

https://codereview.chromium.org/2903493002/diff/170001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/network/NetworkStateNotifier.cpp:166: void
NetworkStateNotifier::SetNetworkQualityInfoOverride(
I think the layering here may not be quite right: in Blink, we usually mock out
objects at the web layer for unit tests: e.g. see WebMockClipboard. The mock web
layer impl will replace the standard impl, and only callback connection info
parameters that we set. That way, we only pay for this branch in test builds.

Can we adopt a similar strategy for testing here instead of using has_override_?

[tbansal1, 2017-05-28 04:32:16]

ptal. Thanks for the comments.

https://codereview.chromium.org/2903493002/diff/170001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/testing/Internals.idl (right):

https://codereview.chromium.org/2903493002/diff/170001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/testing/Internals.idl:317: [RaisesException] void
setNetworkQualityInfoOverride(DOMString effective_type, unsigned long
transport_rtt_msec, double downlink_throughput_mbps);
On 2017/05/27 08:47:47, dcheng wrote:
> Out of curiosity, is it possible to use enums in the WebIDL for this?

I did try, but it gave a compilation error.  That also explains why
setNetworkConnectionInfoOverride() above also uses DOMString for |type|.

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

https://codereview.chromium.org/2903493002/diff/170001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/network/NetworkStateNotifier.cpp:166: void
NetworkStateNotifier::SetNetworkQualityInfoOverride(
On 2017/05/27 08:47:47, dcheng wrote:
> I think the layering here may not be quite right: in Blink, we usually mock
out
> objects at the web layer for unit tests: e.g. see WebMockClipboard. The mock
web
> layer impl will replace the standard impl, and only callback connection info
> parameters that we set. That way, we only pay for this branch in test builds.
> 
> Can we adopt a similar strategy for testing here instead of using
has_override_?

I think the problem here is that on actual test devices, the browser network
change notifier and the browser network quality estimator would keep sending
notifications to the NetworkStateNotifier on the Blink side. We need some way to
tell NetworkStateNotifier to discard those notifications.
So, IIUC, at least one extra bit of storage and one method to set that bit and
the override connection type, RTT, throughput is needed in
WebNetworkStateNotifier for testing purposes.

Also, NetworkStateNotifier is a static object which might be difficult to
override in tests. I need to learn a bit more about Blink testing.

If you feel strongly about this, may I do the refactor of
SetNetworkQualityInfoOverride and the existing
NetworkStateNotifier::SetOverride() method in a subsequent CL? I need to merge
this one back to M-60, so I would like to touch minimal code. WDYT?

[haraken, 2017-05-29 01:08:14]

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

[haraken, 2017-05-29 01:08:15]

+bashi

https://codereview.chromium.org/2903493002/diff/170001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/testing/Internals.idl (right):

https://codereview.chromium.org/2903493002/diff/170001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/testing/Internals.idl:317: [RaisesException] void
setNetworkQualityInfoOverride(DOMString effective_type, unsigned long
transport_rtt_msec, double downlink_throughput_mbps);
On 2017/05/28 04:32:16, tbansal1 wrote:
> On 2017/05/27 08:47:47, dcheng wrote:
> > Out of curiosity, is it possible to use enums in the WebIDL for this?
> 
> I did try, but it gave a compilation error.  That also explains why
> setNetworkConnectionInfoOverride() above also uses DOMString for |type|.

Do you have the compile error? We should use an enum here.

[tbansal1, 2017-05-29 04:45:22]

https://codereview.chromium.org/2903493002/diff/170001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/testing/Internals.idl (right):

https://codereview.chromium.org/2903493002/diff/170001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/testing/Internals.idl:317: [RaisesException] void
setNetworkQualityInfoOverride(DOMString effective_type, unsigned long
transport_rtt_msec, double downlink_throughput_mbps);
On 2017/05/29 01:08:14, haraken wrote:
> On 2017/05/28 04:32:16, tbansal1 wrote:
> > On 2017/05/27 08:47:47, dcheng wrote:
> > > Out of curiosity, is it possible to use enums in the WebIDL for this?
> > 
> > I did try, but it gave a compilation error.  That also explains why
> > setNetworkConnectionInfoOverride() above also uses DOMString for |type|.
> 
> Do you have the compile error? We should use an enum here.

When I build webkit tests, I get this:
gen/blink/bindings/core/v8/V8Internals.cpp:4475:3: error: unknown type name
'WebEffectiveConnectionType'
  WebEffectiveConnectionType* effective_type;
  ^
gen/blink/bindings/core/v8/V8Internals.cpp:4478:20: error: use of undeclared
identifier 'V8WebEffectiveConnectionType'
  effective_type =
V8WebEffectiveConnectionType::toImplWithTypeCheck(info.GetIsolate(), info[0]);

[tbansal1, 2017-05-29 04:53:29]

https://codereview.chromium.org/2903493002/diff/170001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/testing/Internals.idl (right):

https://codereview.chromium.org/2903493002/diff/170001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/testing/Internals.idl:317: [RaisesException] void
setNetworkQualityInfoOverride(DOMString effective_type, unsigned long
transport_rtt_msec, double downlink_throughput_mbps);
On 2017/05/29 04:45:22, tbansal1 wrote:
> On 2017/05/29 01:08:14, haraken wrote:
> > On 2017/05/28 04:32:16, tbansal1 wrote:
> > > On 2017/05/27 08:47:47, dcheng wrote:
> > > > Out of curiosity, is it possible to use enums in the WebIDL for this?
> > > 
> > > I did try, but it gave a compilation error.  That also explains why
> > > setNetworkConnectionInfoOverride() above also uses DOMString for |type|.
> > 
> > Do you have the compile error? We should use an enum here.
> 
> When I build webkit tests, I get this:
> gen/blink/bindings/core/v8/V8Internals.cpp:4475:3: error: unknown type name
> 'WebEffectiveConnectionType'
>   WebEffectiveConnectionType* effective_type;
>   ^
> gen/blink/bindings/core/v8/V8Internals.cpp:4478:20: error: use of undeclared
> identifier 'V8WebEffectiveConnectionType'
>   effective_type =
> V8WebEffectiveConnectionType::toImplWithTypeCheck(info.GetIsolate(), info[0]);

Is it possible to generate the binding only for tests? The binding is not needed
outside of tests. The spec requires effective connection type to be exposed as a
string (and not enum).

[dcheng, 2017-05-29 08:06:15]

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

https://codereview.chromium.org/2903493002/diff/170001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/network/NetworkStateNotifier.cpp:166: void
NetworkStateNotifier::SetNetworkQualityInfoOverride(
On 2017/05/28 04:32:16, tbansal1 wrote:
> On 2017/05/27 08:47:47, dcheng wrote:
> > I think the layering here may not be quite right: in Blink, we usually mock
> out
> > objects at the web layer for unit tests: e.g. see WebMockClipboard. The mock
> web
> > layer impl will replace the standard impl, and only callback connection info
> > parameters that we set. That way, we only pay for this branch in test
builds.
> > 
> > Can we adopt a similar strategy for testing here instead of using
> has_override_?
> 
> I think the problem here is that on actual test devices, the browser network
> change notifier and the browser network quality estimator would keep sending
> notifications to the NetworkStateNotifier on the Blink side. We need some way
to
> tell NetworkStateNotifier to discard those notifications.
> So, IIUC, at least one extra bit of storage and one method to set that bit and
> the override connection type, RTT, throughput is needed in
> WebNetworkStateNotifier for testing purposes.

The browser-side network change/quality notifier should probably be swapped out
in test mode with a testing implementation to avoid these issues.

> 
> Also, NetworkStateNotifier is a static object which might be difficult to
> override in tests. I need to learn a bit more about Blink testing.
> 
> If you feel strongly about this, may I do the refactor of
> SetNetworkQualityInfoOverride and the existing
> NetworkStateNotifier::SetOverride() method in a subsequent CL? I need to merge
> this one back to M-60, so I would like to touch minimal code. WDYT?

I do think we should fix this, but a subsequent CL is fine.

[bashi, 2017-05-29 10:43:32]

https://codereview.chromium.org/2903493002/diff/170001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/testing/Internals.idl (right):

https://codereview.chromium.org/2903493002/diff/170001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/testing/Internals.idl:317: [RaisesException] void
setNetworkQualityInfoOverride(DOMString effective_type, unsigned long
transport_rtt_msec, double downlink_throughput_mbps);
On 2017/05/29 04:53:29, tbansal1 wrote:
> On 2017/05/29 04:45:22, tbansal1 wrote:
> > On 2017/05/29 01:08:14, haraken wrote:
> > > On 2017/05/28 04:32:16, tbansal1 wrote:
> > > > On 2017/05/27 08:47:47, dcheng wrote:
> > > > > Out of curiosity, is it possible to use enums in the WebIDL for this?
> > > > 
> > > > I did try, but it gave a compilation error.  That also explains why
> > > > setNetworkConnectionInfoOverride() above also uses DOMString for |type|.
> > > 
> > > Do you have the compile error? We should use an enum here.
> > 
> > When I build webkit tests, I get this:
> > gen/blink/bindings/core/v8/V8Internals.cpp:4475:3: error: unknown type name
> > 'WebEffectiveConnectionType'
> >   WebEffectiveConnectionType* effective_type;
> >   ^
> > gen/blink/bindings/core/v8/V8Internals.cpp:4478:20: error: use of undeclared
> > identifier 'V8WebEffectiveConnectionType'
> >   effective_type =
> > V8WebEffectiveConnectionType::toImplWithTypeCheck(info.GetIsolate(),
info[0]);
> 
> Is it possible to generate the binding only for tests? The binding is not
needed
> outside of tests. The spec requires effective connection type to be exposed as
a
> string (and not enum).

Could you elaborate how you defined EffectiveConnectionType and where? Using an
enum in internals.idl should work and `blink_tests` target should be able to
build.

[tbansal1, 2017-05-29 19:36:12]

On 2017/05/29 10:43:32, bashi wrote:
>
https://codereview.chromium.org/2903493002/diff/170001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/testing/Internals.idl (right):
> 
>
https://codereview.chromium.org/2903493002/diff/170001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/testing/Internals.idl:317: [RaisesException]
void
> setNetworkQualityInfoOverride(DOMString effective_type, unsigned long
> transport_rtt_msec, double downlink_throughput_mbps);
> On 2017/05/29 04:53:29, tbansal1 wrote:
> > On 2017/05/29 04:45:22, tbansal1 wrote:
> > > On 2017/05/29 01:08:14, haraken wrote:
> > > > On 2017/05/28 04:32:16, tbansal1 wrote:
> > > > > On 2017/05/27 08:47:47, dcheng wrote:
> > > > > > Out of curiosity, is it possible to use enums in the WebIDL for
this?
> > > > > 
> > > > > I did try, but it gave a compilation error.  That also explains why
> > > > > setNetworkConnectionInfoOverride() above also uses DOMString for
|type|.
> > > > 
> > > > Do you have the compile error? We should use an enum here.
> > > 
> > > When I build webkit tests, I get this:
> > > gen/blink/bindings/core/v8/V8Internals.cpp:4475:3: error: unknown type
name
> > > 'WebEffectiveConnectionType'
> > >   WebEffectiveConnectionType* effective_type;
> > >   ^
> > > gen/blink/bindings/core/v8/V8Internals.cpp:4478:20: error: use of
undeclared
> > > identifier 'V8WebEffectiveConnectionType'
> > >   effective_type =
> > > V8WebEffectiveConnectionType::toImplWithTypeCheck(info.GetIsolate(),
> info[0]);
> > 
> > Is it possible to generate the binding only for tests? The binding is not
> needed
> > outside of tests. The spec requires effective connection type to be exposed
as
> a
> > string (and not enum).
> 
> Could you elaborate how you defined EffectiveConnectionType and where? Using
an
> enum in internals.idl should work and `blink_tests` target should be able to
> build.

https://cs.chromium.org/chromium/src/third_party/WebKit/public/platform/WebEf...
Thanks for the help!!

[kinuko, 2017-05-30 02:38:22]

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

https://codereview.chromium.org/2903493002/diff/170001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/network/NetworkStateNotifier.cpp:166: void
NetworkStateNotifier::SetNetworkQualityInfoOverride(
On 2017/05/29 08:06:14, dcheng wrote:
> On 2017/05/28 04:32:16, tbansal1 wrote:
> > On 2017/05/27 08:47:47, dcheng wrote:
> > > I think the layering here may not be quite right: in Blink, we usually
mock
> > out
> > > objects at the web layer for unit tests: e.g. see WebMockClipboard. The
mock
> > web
> > > layer impl will replace the standard impl, and only callback connection
info
> > > parameters that we set. That way, we only pay for this branch in test
> builds.
> > > 
> > > Can we adopt a similar strategy for testing here instead of using
> > has_override_?
> > 
> > I think the problem here is that on actual test devices, the browser network
> > change notifier and the browser network quality estimator would keep sending
> > notifications to the NetworkStateNotifier on the Blink side. We need some
way
> to
> > tell NetworkStateNotifier to discard those notifications.
> > So, IIUC, at least one extra bit of storage and one method to set that bit
and
> > the override connection type, RTT, throughput is needed in
> > WebNetworkStateNotifier for testing purposes.
> 
> The browser-side network change/quality notifier should probably be swapped
out
> in test mode with a testing implementation to avoid these issues.
> 
> > 
> > Also, NetworkStateNotifier is a static object which might be difficult to
> > override in tests. I need to learn a bit more about Blink testing.
> > 
> > If you feel strongly about this, may I do the refactor of
> > SetNetworkQualityInfoOverride and the existing
> > NetworkStateNotifier::SetOverride() method in a subsequent CL? I need to
merge
> > this one back to M-60, so I would like to touch minimal code. WDYT?
> 
> I do think we should fix this, but a subsequent CL is fine.

By the way existing SetOverride is also used for device/network emulation by
Inspector. Would we want to plumb this there too eventually? (I suspect we
might)  If so keep it in mind that this override may not be for testing only.

[dcheng, 2017-05-30 03:24:17]

On 2017/05/30 02:38:22, kinuko wrote:
>
https://codereview.chromium.org/2903493002/diff/170001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/platform/network/NetworkStateNotifier.cpp
> (right):
> 
>
https://codereview.chromium.org/2903493002/diff/170001/third_party/WebKit/Sou...
> third_party/WebKit/Source/platform/network/NetworkStateNotifier.cpp:166: void
> NetworkStateNotifier::SetNetworkQualityInfoOverride(
> On 2017/05/29 08:06:14, dcheng wrote:
> > On 2017/05/28 04:32:16, tbansal1 wrote:
> > > On 2017/05/27 08:47:47, dcheng wrote:
> > > > I think the layering here may not be quite right: in Blink, we usually
> mock
> > > out
> > > > objects at the web layer for unit tests: e.g. see WebMockClipboard. The
> mock
> > > web
> > > > layer impl will replace the standard impl, and only callback connection
> info
> > > > parameters that we set. That way, we only pay for this branch in test
> > builds.
> > > > 
> > > > Can we adopt a similar strategy for testing here instead of using
> > > has_override_?
> > > 
> > > I think the problem here is that on actual test devices, the browser
network
> > > change notifier and the browser network quality estimator would keep
sending
> > > notifications to the NetworkStateNotifier on the Blink side. We need some
> way
> > to
> > > tell NetworkStateNotifier to discard those notifications.
> > > So, IIUC, at least one extra bit of storage and one method to set that bit
> and
> > > the override connection type, RTT, throughput is needed in
> > > WebNetworkStateNotifier for testing purposes.
> > 
> > The browser-side network change/quality notifier should probably be swapped
> out
> > in test mode with a testing implementation to avoid these issues.
> > 
> > > 
> > > Also, NetworkStateNotifier is a static object which might be difficult to
> > > override in tests. I need to learn a bit more about Blink testing.
> > > 
> > > If you feel strongly about this, may I do the refactor of
> > > SetNetworkQualityInfoOverride and the existing
> > > NetworkStateNotifier::SetOverride() method in a subsequent CL? I need to
> merge
> > > this one back to M-60, so I would like to touch minimal code. WDYT?
> > 
> > I do think we should fix this, but a subsequent CL is fine.
> 
> By the way existing SetOverride is also used for device/network emulation by
> Inspector. Would we want to plumb this there too eventually? (I suspect we
> might)  If so keep it in mind that this override may not be for testing only.

Hmm... that's a good point. I thought the inspector thing only applied to tabs
the inspector is open in though? I got the impression that a lot of this network
state stuff is static right now, which would make it process global.

[tbansal1, 2017-05-30 04:10:59]

On 2017/05/30 03:24:17, dcheng wrote:
> On 2017/05/30 02:38:22, kinuko wrote:
> >
>
https://codereview.chromium.org/2903493002/diff/170001/third_party/WebKit/Sou...
> > File third_party/WebKit/Source/platform/network/NetworkStateNotifier.cpp
> > (right):
> > 
> >
>
https://codereview.chromium.org/2903493002/diff/170001/third_party/WebKit/Sou...
> > third_party/WebKit/Source/platform/network/NetworkStateNotifier.cpp:166:
void
> > NetworkStateNotifier::SetNetworkQualityInfoOverride(
> > On 2017/05/29 08:06:14, dcheng wrote:
> > > On 2017/05/28 04:32:16, tbansal1 wrote:
> > > > On 2017/05/27 08:47:47, dcheng wrote:
> > > > > I think the layering here may not be quite right: in Blink, we usually
> > mock
> > > > out
> > > > > objects at the web layer for unit tests: e.g. see WebMockClipboard.
The
> > mock
> > > > web
> > > > > layer impl will replace the standard impl, and only callback
connection
> > info
> > > > > parameters that we set. That way, we only pay for this branch in test
> > > builds.
> > > > > 
> > > > > Can we adopt a similar strategy for testing here instead of using
> > > > has_override_?
> > > > 
> > > > I think the problem here is that on actual test devices, the browser
> network
> > > > change notifier and the browser network quality estimator would keep
> sending
> > > > notifications to the NetworkStateNotifier on the Blink side. We need
some
> > way
> > > to
> > > > tell NetworkStateNotifier to discard those notifications.
> > > > So, IIUC, at least one extra bit of storage and one method to set that
bit
> > and
> > > > the override connection type, RTT, throughput is needed in
> > > > WebNetworkStateNotifier for testing purposes.
> > > 
> > > The browser-side network change/quality notifier should probably be
swapped
> > out
> > > in test mode with a testing implementation to avoid these issues.
> > > 
> > > > 
> > > > Also, NetworkStateNotifier is a static object which might be difficult
to
> > > > override in tests. I need to learn a bit more about Blink testing.
> > > > 
> > > > If you feel strongly about this, may I do the refactor of
> > > > SetNetworkQualityInfoOverride and the existing
> > > > NetworkStateNotifier::SetOverride() method in a subsequent CL? I need to
> > merge
> > > > this one back to M-60, so I would like to touch minimal code. WDYT?
> > > 
> > > I do think we should fix this, but a subsequent CL is fine.
> > 
> > By the way existing SetOverride is also used for device/network emulation by
> > Inspector. Would we want to plumb this there too eventually? (I suspect we
> > might)  If so keep it in mind that this override may not be for testing
only.
> 
> Hmm... that's a good point. I thought the inspector thing only applied to tabs
> the inspector is open in though? I got the impression that a lot of this
network
> state stuff is static right now, which would make it process global.

Yeah, the story there is confusing. There have 
been discussions to make the existing network throttles
from devtools global since it is virtually
impossible to accurately throttle network requests from only
a single tab. 

Also, the current devtools network emulation is also partly
global in a way. If I override the network type using devtools
in one tab, it also affects the value of network type in
devtools in other tabs.

[kinuko, 2017-05-30 04:50:31]

On 2017/05/30 04:10:59, tbansal1 wrote:
> On 2017/05/30 03:24:17, dcheng wrote:
> > On 2017/05/30 02:38:22, kinuko wrote:
> > >
> >
>
https://codereview.chromium.org/2903493002/diff/170001/third_party/WebKit/Sou...
> > > File third_party/WebKit/Source/platform/network/NetworkStateNotifier.cpp
> > > (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2903493002/diff/170001/third_party/WebKit/Sou...
> > > third_party/WebKit/Source/platform/network/NetworkStateNotifier.cpp:166:
> void
> > > NetworkStateNotifier::SetNetworkQualityInfoOverride(
> > > On 2017/05/29 08:06:14, dcheng wrote:
> > > > On 2017/05/28 04:32:16, tbansal1 wrote:
> > > > > On 2017/05/27 08:47:47, dcheng wrote:
> > > > > > I think the layering here may not be quite right: in Blink, we
usually
> > > mock
> > > > > out
> > > > > > objects at the web layer for unit tests: e.g. see WebMockClipboard.
> The
> > > mock
> > > > > web
> > > > > > layer impl will replace the standard impl, and only callback
> connection
> > > info
> > > > > > parameters that we set. That way, we only pay for this branch in
test
> > > > builds.
> > > > > > 
> > > > > > Can we adopt a similar strategy for testing here instead of using
> > > > > has_override_?
> > > > > 
> > > > > I think the problem here is that on actual test devices, the browser
> > network
> > > > > change notifier and the browser network quality estimator would keep
> > sending
> > > > > notifications to the NetworkStateNotifier on the Blink side. We need
> some
> > > way
> > > > to
> > > > > tell NetworkStateNotifier to discard those notifications.
> > > > > So, IIUC, at least one extra bit of storage and one method to set that
> bit
> > > and
> > > > > the override connection type, RTT, throughput is needed in
> > > > > WebNetworkStateNotifier for testing purposes.
> > > > 
> > > > The browser-side network change/quality notifier should probably be
> swapped
> > > out
> > > > in test mode with a testing implementation to avoid these issues.
> > > > 
> > > > > 
> > > > > Also, NetworkStateNotifier is a static object which might be difficult
> to
> > > > > override in tests. I need to learn a bit more about Blink testing.
> > > > > 
> > > > > If you feel strongly about this, may I do the refactor of
> > > > > SetNetworkQualityInfoOverride and the existing
> > > > > NetworkStateNotifier::SetOverride() method in a subsequent CL? I need
to
> > > merge
> > > > > this one back to M-60, so I would like to touch minimal code. WDYT?
> > > > 
> > > > I do think we should fix this, but a subsequent CL is fine.
> > > 
> > > By the way existing SetOverride is also used for device/network emulation
by
> > > Inspector. Would we want to plumb this there too eventually? (I suspect we
> > > might)  If so keep it in mind that this override may not be for testing
> only.
> > 
> > Hmm... that's a good point. I thought the inspector thing only applied to
tabs
> > the inspector is open in though? I got the impression that a lot of this
> network
> > state stuff is static right now, which would make it process global.
> 
> Yeah, the story there is confusing. There have 
> been discussions to make the existing network throttles
> from devtools global since it is virtually
> impossible to accurately throttle network requests from only
> a single tab. 
> 
> Also, the current devtools network emulation is also partly
> global in a way. If I override the network type using devtools
> in one tab, it also affects the value of network type in
> devtools in other tabs.

Yep, per-tab network emulation is broken in several ways and I think we're
probably to make this (more explicitly) process global.

[tbansal1, 2017-05-30 21:57:20]

ptal. Thanks.

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

https://codereview.chromium.org/2903493002/diff/170001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/network/NetworkStateNotifier.cpp:166: void
NetworkStateNotifier::SetNetworkQualityInfoOverride(
On 2017/05/30 02:38:22, kinuko wrote:
> On 2017/05/29 08:06:14, dcheng wrote:
> > On 2017/05/28 04:32:16, tbansal1 wrote:
> > > On 2017/05/27 08:47:47, dcheng wrote:
> > > > I think the layering here may not be quite right: in Blink, we usually
> mock
> > > out
> > > > objects at the web layer for unit tests: e.g. see WebMockClipboard. The
> mock
> > > web
> > > > layer impl will replace the standard impl, and only callback connection
> info
> > > > parameters that we set. That way, we only pay for this branch in test
> > builds.
> > > > 
> > > > Can we adopt a similar strategy for testing here instead of using
> > > has_override_?
> > > 
> > > I think the problem here is that on actual test devices, the browser
network
> > > change notifier and the browser network quality estimator would keep
sending
> > > notifications to the NetworkStateNotifier on the Blink side. We need some
> way
> > to
> > > tell NetworkStateNotifier to discard those notifications.
> > > So, IIUC, at least one extra bit of storage and one method to set that bit
> and
> > > the override connection type, RTT, throughput is needed in
> > > WebNetworkStateNotifier for testing purposes.
> > 
> > The browser-side network change/quality notifier should probably be swapped
> out
> > in test mode with a testing implementation to avoid these issues.
> > 
> > > 
> > > Also, NetworkStateNotifier is a static object which might be difficult to
> > > override in tests. I need to learn a bit more about Blink testing.
> > > 
> > > If you feel strongly about this, may I do the refactor of
> > > SetNetworkQualityInfoOverride and the existing
> > > NetworkStateNotifier::SetOverride() method in a subsequent CL? I need to
> merge
> > > this one back to M-60, so I would like to touch minimal code. WDYT?
> > 
> > I do think we should fix this, but a subsequent CL is fine.
> 
> By the way existing SetOverride is also used for device/network emulation by
> Inspector. Would we want to plumb this there too eventually? (I suspect we
> might)  If so keep it in mind that this override may not be for testing only.

filed https://crbug.com/727911 for changing the value of |rtt| and |downlink|
attributes here when connection is throttled.

[tbansal1, 2017-05-31 04:16:02]

https://codereview.chromium.org/2903493002/diff/170001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/testing/Internals.idl (right):

https://codereview.chromium.org/2903493002/diff/170001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/testing/Internals.idl:317: [RaisesException] void
setNetworkQualityInfoOverride(DOMString effective_type, unsigned long
transport_rtt_msec, double downlink_throughput_mbps);
On 2017/05/29 10:43:32, bashi wrote:
> On 2017/05/29 04:53:29, tbansal1 wrote:
> > On 2017/05/29 04:45:22, tbansal1 wrote:
> > > On 2017/05/29 01:08:14, haraken wrote:
> > > > On 2017/05/28 04:32:16, tbansal1 wrote:
> > > > > On 2017/05/27 08:47:47, dcheng wrote:
> > > > > > Out of curiosity, is it possible to use enums in the WebIDL for
this?
> > > > > 
> > > > > I did try, but it gave a compilation error.  That also explains why
> > > > > setNetworkConnectionInfoOverride() above also uses DOMString for
|type|.
> > > > 
> > > > Do you have the compile error? We should use an enum here.
> > > 
> > > When I build webkit tests, I get this:
> > > gen/blink/bindings/core/v8/V8Internals.cpp:4475:3: error: unknown type
name
> > > 'WebEffectiveConnectionType'
> > >   WebEffectiveConnectionType* effective_type;
> > >   ^
> > > gen/blink/bindings/core/v8/V8Internals.cpp:4478:20: error: use of
undeclared
> > > identifier 'V8WebEffectiveConnectionType'
> > >   effective_type =
> > > V8WebEffectiveConnectionType::toImplWithTypeCheck(info.GetIsolate(),
> info[0]);
> > 
> > Is it possible to generate the binding only for tests? The binding is not
> needed
> > outside of tests. The spec requires effective connection type to be exposed
as
> a
> > string (and not enum).
> 
> Could you elaborate how you defined EffectiveConnectionType and where? Using
an
> enum in internals.idl should work and `blink_tests` target should be able to
> build.

I replied elsewhere in the CL, but this is where EffectiveConnectionType is
defined:
https://cs.chromium.org/chromium/src/third_party/WebKit/public/platform/WebEf...

It's also defined here (although that would probably result in an invalid
dependency):
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/netinf...


It seems that to be usable as an enum here, it should be defined in an .idl
file. Are there any other examples of enum used here? I clicked through a few of
them, but could not find any enum. Thanks.

[foolip, 2017-05-31 14:31:15]

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

[foolip, 2017-05-31 14:31:16]

https://codereview.chromium.org/2903493002/diff/170001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/netinfo/network-quality.html (right):

https://codereview.chromium.org/2903493002/diff/170001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/netinfo/network-quality.html:36:
internals.setNetworkQualityInfoOverride(values[i][0], values[i][1],
values[i][2]);
I haven't looked at the whole CL, but at least this test seems to be for things
that are required by the spec but impossible to test without internals. (As
opposed to behavior that's deliberately left undefined but where we want to test
our choices.)

Can you file an issue blocking issue 707649 for what it would take to share a
test like this across implementations? If this is the only test then it's
probably not worth the effort, but if web developers could also make use of
WebDriver commands to test their web apps, maybe it is.

[tbansal1, 2017-05-31 17:06:29]

On 2017/05/31 14:31:16, foolip wrote:
>
https://codereview.chromium.org/2903493002/diff/170001/third_party/WebKit/Lay...
> File third_party/WebKit/LayoutTests/netinfo/network-quality.html (right):
> 
>
https://codereview.chromium.org/2903493002/diff/170001/third_party/WebKit/Lay...
> third_party/WebKit/LayoutTests/netinfo/network-quality.html:36:
> internals.setNetworkQualityInfoOverride(values[i][0], values[i][1],
> values[i][2]);
> I haven't looked at the whole CL, but at least this test seems to be for
things
> that are required by the spec but impossible to test without internals. (As
> opposed to behavior that's deliberately left undefined but where we want to
test
> our choices.)
> 
> Can you file an issue blocking issue 707649 for what it would take to share a
> test like this across implementations? If this is the only test then it's
> probably not worth the effort, but if web developers could also make use of
> WebDriver commands to test their web apps, maybe it is.

Done. https://bugs.chromium.org/p/chromium/issues/detail?id=728218

[tbansal1, 2017-05-31 17:06:40]

dcheng, haraken: ping. Thanks.

[dcheng, 2017-05-31 17:30:23]

On 2017/05/31 17:06:40, tbansal1 wrote:
> dcheng, haraken: ping. Thanks.

I don't see the new patchset that uses enums: did I miss something?

[dcheng, 2017-05-31 17:31:30]

On 2017/05/31 17:30:23, dcheng wrote:
> On 2017/05/31 17:06:40, tbansal1 wrote:
> > dcheng, haraken: ping. Thanks.
> 
> I don't see the new patchset that uses enums: did I miss something?

Oh I see, you weren't able to get it working. I guess I was waiting for the
definitive answer on that. The rest of the CL LGTM, but let's try to figure out
the bindings thing before landing.

[tbansal1, 2017-05-31 17:33:51]

On 2017/05/31 17:31:30, dcheng wrote:
> On 2017/05/31 17:30:23, dcheng wrote:
> > On 2017/05/31 17:06:40, tbansal1 wrote:
> > > dcheng, haraken: ping. Thanks.
> > 
> > I don't see the new patchset that uses enums: did I miss something?
> 
> Oh I see, you weren't able to get it working. I guess I was waiting for the
> definitive answer on that. The rest of the CL LGTM, but let's try to figure
out
> the bindings thing before landing.

I do not have a definite answer. I was unable to find an existing example.
Help appreciated, or let me know who is the best person to contact for this.
Thanks!

[bashi, 2017-06-01 00:05:28]

On 2017/05/31 04:16:02, tbansal1 wrote:
>
https://codereview.chromium.org/2903493002/diff/170001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/testing/Internals.idl (right):
> 
>
https://codereview.chromium.org/2903493002/diff/170001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/testing/Internals.idl:317: [RaisesException]
void
> setNetworkQualityInfoOverride(DOMString effective_type, unsigned long
> transport_rtt_msec, double downlink_throughput_mbps);
> On 2017/05/29 10:43:32, bashi wrote:
> > On 2017/05/29 04:53:29, tbansal1 wrote:
> > > On 2017/05/29 04:45:22, tbansal1 wrote:
> > > > On 2017/05/29 01:08:14, haraken wrote:
> > > > > On 2017/05/28 04:32:16, tbansal1 wrote:
> > > > > > On 2017/05/27 08:47:47, dcheng wrote:
> > > > > > > Out of curiosity, is it possible to use enums in the WebIDL for
> this?
> > > > > > 
> > > > > > I did try, but it gave a compilation error.  That also explains why
> > > > > > setNetworkConnectionInfoOverride() above also uses DOMString for
> |type|.
> > > > > 
> > > > > Do you have the compile error? We should use an enum here.
> > > > 
> > > > When I build webkit tests, I get this:
> > > > gen/blink/bindings/core/v8/V8Internals.cpp:4475:3: error: unknown type
> name
> > > > 'WebEffectiveConnectionType'
> > > >   WebEffectiveConnectionType* effective_type;
> > > >   ^
> > > > gen/blink/bindings/core/v8/V8Internals.cpp:4478:20: error: use of
> undeclared
> > > > identifier 'V8WebEffectiveConnectionType'
> > > >   effective_type =
> > > > V8WebEffectiveConnectionType::toImplWithTypeCheck(info.GetIsolate(),
> > info[0]);
> > > 
> > > Is it possible to generate the binding only for tests? The binding is not
> > needed
> > > outside of tests. The spec requires effective connection type to be
exposed
> as
> > a
> > > string (and not enum).
> > 
> > Could you elaborate how you defined EffectiveConnectionType and where? Using
> an
> > enum in internals.idl should work and `blink_tests` target should be able to
> > build.
> 
> I replied elsewhere in the CL, but this is where EffectiveConnectionType is
> defined:
>
https://cs.chromium.org/chromium/src/third_party/WebKit/public/platform/WebEf...
> 
> It's also defined here (although that would probably result in an invalid
> dependency):
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/netinf...
> 
> 
> It seems that to be usable as an enum here, it should be defined in an .idl
> file. Are there any other examples of enum used here? I clicked through a few
of
> them, but could not find any enum. Thanks.

IDL's enum is a different thing from C++ enum. In WebIDL enums are Strings of
which values are restricted[1]. You need to define an enum for
EffectiveConnectionType in .idl file and then in C++ you need to convert String
to WebEffectiveConnectionType.

[1] https://heycam.github.io/webidl/#idl-enumeration

[tbansal1, 2017-06-01 22:21:20]

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

[commit-bot: I haz the power, 2017-06-01 22:22:20]

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-06-01 23:38:16]

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

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

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-06-01 23:48:56]

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

[commit-bot: I haz the power, 2017-06-01 23:49:29]

Dry run: CQ is trying da patch.

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

[tbansal1, 2017-06-01 23:56:37]

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

[tbansal1, 2017-06-01 23:56:38]

Patchset #2 (id:190001) has been deleted

[commit-bot: I haz the power, 2017-06-01 23:57:12]

Dry run: CQ is trying da patch.

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

[tbansal1, 2017-06-02 01:24:32]

haraken: ptal. Thanks everyone!!

https://codereview.chromium.org/2903493002/diff/170001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/testing/Internals.idl (right):

https://codereview.chromium.org/2903493002/diff/170001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/testing/Internals.idl:317: [RaisesException] void
setNetworkQualityInfoOverride(DOMString effective_type, unsigned long
transport_rtt_msec, double downlink_throughput_mbps);
On 2017/05/31 04:16:02, tbansal1 wrote:
> On 2017/05/29 10:43:32, bashi wrote:
> > On 2017/05/29 04:53:29, tbansal1 wrote:
> > > On 2017/05/29 04:45:22, tbansal1 wrote:
> > > > On 2017/05/29 01:08:14, haraken wrote:
> > > > > On 2017/05/28 04:32:16, tbansal1 wrote:
> > > > > > On 2017/05/27 08:47:47, dcheng wrote:
> > > > > > > Out of curiosity, is it possible to use enums in the WebIDL for
> this?
> > > > > > 
> > > > > > I did try, but it gave a compilation error.  That also explains why
> > > > > > setNetworkConnectionInfoOverride() above also uses DOMString for
> |type|.
> > > > > 
> > > > > Do you have the compile error? We should use an enum here.
> > > > 
> > > > When I build webkit tests, I get this:
> > > > gen/blink/bindings/core/v8/V8Internals.cpp:4475:3: error: unknown type
> name
> > > > 'WebEffectiveConnectionType'
> > > >   WebEffectiveConnectionType* effective_type;
> > > >   ^
> > > > gen/blink/bindings/core/v8/V8Internals.cpp:4478:20: error: use of
> undeclared
> > > > identifier 'V8WebEffectiveConnectionType'
> > > >   effective_type =
> > > > V8WebEffectiveConnectionType::toImplWithTypeCheck(info.GetIsolate(),
> > info[0]);
> > > 
> > > Is it possible to generate the binding only for tests? The binding is not
> > needed
> > > outside of tests. The spec requires effective connection type to be
exposed
> as
> > a
> > > string (and not enum).
> > 
> > Could you elaborate how you defined EffectiveConnectionType and where? Using
> an
> > enum in internals.idl should work and `blink_tests` target should be able to
> > build.
> 
> I replied elsewhere in the CL, but this is where EffectiveConnectionType is
> defined:
>
https://cs.chromium.org/chromium/src/third_party/WebKit/public/platform/WebEf...
> 
> It's also defined here (although that would probably result in an invalid
> dependency):
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/netinf...
> 
> 
> It seems that to be usable as an enum here, it should be defined in an .idl
> file. Are there any other examples of enum used here? I clicked through a few
of
> them, but could not find any enum. Thanks.

Done. Thanks for the help!!

[haraken, 2017-06-02 04:03:57]

LGTM with comments.

https://codereview.chromium.org/2903493002/diff/210001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/testing/NetInfo.cpp (right):

https://codereview.chromium.org/2903493002/diff/210001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/testing/NetInfo.cpp:3: // found in the LICENSE
file.

Remove this file.

https://codereview.chromium.org/2903493002/diff/210001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/testing/NetInfo.h (right):

https://codereview.chromium.org/2903493002/diff/210001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/testing/NetInfo.h:3: // found in the LICENSE
file.

Remove this file.

https://codereview.chromium.org/2903493002/diff/210001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/testing/NetInfo.idl (right):

https://codereview.chromium.org/2903493002/diff/210001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/testing/NetInfo.idl:5: enum
EffectiveConnectionType {

Can we move this enum to Internals.idl?

We don't need to define the NetInfo interface.

[commit-bot: I haz the power, 2017-06-02 04:06:43]

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

[commit-bot: I haz the power, 2017-06-02 04:06:45]

Dry run: This issue passed the CQ dry run.

[tbansal1, 2017-06-02 04:41:18]

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

[commit-bot: I haz the power, 2017-06-02 04:41:33]

Dry run: CQ is trying da patch.

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

[tbansal1, 2017-06-02 04:42:11]

https://codereview.chromium.org/2903493002/diff/210001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/testing/NetInfo.cpp (right):

https://codereview.chromium.org/2903493002/diff/210001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/testing/NetInfo.cpp:3: // found in the LICENSE
file.
On 2017/06/02 04:03:56, haraken wrote:
> 
> Remove this file.

Done.

https://codereview.chromium.org/2903493002/diff/210001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/testing/NetInfo.h (right):

https://codereview.chromium.org/2903493002/diff/210001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/testing/NetInfo.h:3: // found in the LICENSE
file.
On 2017/06/02 04:03:56, haraken wrote:
> 
> Remove this file.

Done.

https://codereview.chromium.org/2903493002/diff/210001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/testing/NetInfo.idl (right):

https://codereview.chromium.org/2903493002/diff/210001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/testing/NetInfo.idl:5: enum
EffectiveConnectionType {
On 2017/06/02 04:03:56, haraken wrote:
> 
> Can we move this enum to Internals.idl?
> 
> We don't need to define the NetInfo interface.

Done.

[commit-bot: I haz the power, 2017-06-02 06:47:57]

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

[commit-bot: I haz the power, 2017-06-02 06:47:59]

Dry run: This issue passed the CQ dry run.

[tbansal1, 2017-06-02 06:55:37]

The CQ bit was checked by tbansal@chromium.org

[tbansal1, 2017-06-02 06:55:37]

The patchset sent to the CQ was uploaded after l-g-t-m from dcheng@chromium.org,
haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/2903493002/#ps230001
(title: "haraken comments")

[commit-bot: I haz the power, 2017-06-02 06:55:59]

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-06-02 07:00:42]

CQ is committing da patch.

Bot data: {"patchset_id": 230001, "attempt_start_ts": 1496386537182750,
"parent_rev": "b84dddac87497b87173c8860647477337d611031", "commit_rev":
"f51122ff533e752a86a27193ca7539c450b3670e"}

[commit-bot: I haz the power, 2017-06-02 07:00:55]

Description was changed from

==========
NetInfo network quality extension: Add callbacks and Layout tests

"change" event is fired from NetworkInformation.cpp when there
is a change in the network quality.

Also, layout tests have been added for the network quality extension
to the NetInfo API.

BUG=725282, 726762
==========

to

==========
NetInfo network quality extension: Add callbacks and Layout tests

"change" event is fired from NetworkInformation.cpp when there
is a change in the network quality.

Also, layout tests have been added for the network quality extension
to the NetInfo API.

BUG=725282, 726762

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

[commit-bot: I haz the power, 2017-06-02 07:00:58]

Committed patchset #3 (id:230001) as
https://chromium.googlesource.com/chromium/src/+/f51122ff533e752a86a27193ca75...

[kinuko, 2017-06-02 08:06:02]

Description was changed from

==========
NetInfo network quality extension: Add callbacks and Layout tests

"change" event is fired from NetworkInformation.cpp when there
is a change in the network quality.

Also, layout tests have been added for the network quality extension
to the NetInfo API.

BUG=725282, 726762

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

to

==========
NetInfo network quality extension: Add callbacks and Layout tests

"change" event is fired from NetworkInformation.cpp when there
is a change in the network quality.

Also, layout tests have been added for the network quality extension
to the NetInfo API.

BUG=725282, 726762

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

[kinuko, 2017-06-02 08:06:03]

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