[DevTools] Network.emulateNetworkConditions now affects NetworkStateNotifier.

[DevTools] Network.emulateNetworkConditions now affects NetworkStateNotifier.

This patch replaces test-only update mechanism in NetworkStateNotifier with
setOverride/clearOverride methods and uses them in InspectorNetworkAgent.

Unfortunately for emulation, NetworkStateNotifier is global and thus all
pages will observe the new network state. We can make it per-frame, but
that would introduce a lot of complexity. Current state is an acceptable trade-off
for now.

This also adds connectionType parameter to Network.emulateNetworkConditions and
makes it fall-through to the renderer.

BUG=567613, 586696, 581334
Committed: https://crrev.com/6ca18e32edb9bad244588538ea2bb2d1be9fb1ff
Cr-Commit-Position: refs/heads/master@{#402897}

[dgozman, 2016-06-22 17:05:08]

Description was changed from

==========
[DevTools] Network.emulateNetworkConditions now affects NetworkStateNotifier.

This patch replaces test-only update mechanism in NetworkStateNotifier with
setOverride/clearOverride methods and uses them in InspectorNetworkAgent.

Unfortunately for emulation, NetworkStateNotifier is global and thus all
pages will observe the new network state. We can make it per-frame, but
that would introduce a lot of complexity. Current state is an acceptable
trade-off
for now.

BUG=567613,586696,581334
==========

to

==========
[DevTools] Network.emulateNetworkConditions now affects NetworkStateNotifier.

This patch replaces test-only update mechanism in NetworkStateNotifier with
setOverride/clearOverride methods and uses them in InspectorNetworkAgent.

Unfortunately for emulation, NetworkStateNotifier is global and thus all
pages will observe the new network state. We can make it per-frame, but
that would introduce a lot of complexity. Current state is an acceptable
trade-off
for now.

This also adds connectionType parameter to Network.emulateNetworkConditions.

BUG=567613,586696,581334
==========

[dgozman, 2016-06-22 19:43:16]

Description was changed from

==========
[DevTools] Network.emulateNetworkConditions now affects NetworkStateNotifier.

This patch replaces test-only update mechanism in NetworkStateNotifier with
setOverride/clearOverride methods and uses them in InspectorNetworkAgent.

Unfortunately for emulation, NetworkStateNotifier is global and thus all
pages will observe the new network state. We can make it per-frame, but
that would introduce a lot of complexity. Current state is an acceptable
trade-off
for now.

This also adds connectionType parameter to Network.emulateNetworkConditions.

BUG=567613,586696,581334
==========

to

==========
[DevTools] Network.emulateNetworkConditions now affects NetworkStateNotifier.

This patch replaces test-only update mechanism in NetworkStateNotifier with
setOverride/clearOverride methods and uses them in InspectorNetworkAgent.

Unfortunately for emulation, NetworkStateNotifier is global and thus all
pages will observe the new network state. We can make it per-frame, but
that would introduce a lot of complexity. Current state is an acceptable
trade-off
for now.

This also adds connectionType parameter to Network.emulateNetworkConditions and
makes it fall-through to the renderer.

BUG=567613,586696,581334
==========

[dgozman, 2016-06-23 01:48:39]

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

[commit-bot: I haz the power, 2016-06-23 01:49:15]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2087293003/40001

[dgozman, 2016-06-23 01:50:35]

dgozman@chromium.org changed reviewers:
+ allada@chromium.org, caseq@chromium.org, jkarlin@chromium.org

[dgozman, 2016-06-23 01:50:35]

Hi,

Could you please take a look:
- caseq, allada at the whole patch;
- jkarlin at NetworkStateNotifier.

Thanks,
Dmitry

[commit-bot: I haz the power, 2016-06-23 02:24:44]

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

[commit-bot: I haz the power, 2016-06-23 02:24:45]

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

[jkarlin, 2016-06-23 12:53:46]

https://codereview.chromium.org/2087293003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/page/NetworkStateNotifier.cpp (right):

https://codereview.chromium.org/2087293003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/NetworkStateNotifier.cpp:152: double
maxBandwidthMbps = maxBandwidth();
The underlying values of type and bandwidth might change between the above two
calls. In fact it might change before notifyObservers is run. notifyObservers
should take the connection type and max bandwidth as arguments.

[dgozman, 2016-06-23 15:00:22]

https://codereview.chromium.org/2087293003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/page/NetworkStateNotifier.cpp (right):

https://codereview.chromium.org/2087293003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/NetworkStateNotifier.cpp:152: double
maxBandwidthMbps = maxBandwidth();
On 2016/06/23 12:53:46, jkarlin wrote:
> The underlying values of type and bandwidth might change between the above two
> calls. In fact it might change before notifyObservers is run. notifyObservers
> should take the connection type and max bandwidth as arguments.

How is that possible? Both notifyObservers and all state-mutating methods check
that they run on main thread, and we call notifyObservers synchronously.

[dgozman, 2016-06-23 18:19:08]

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

[commit-bot: I haz the power, 2016-06-23 18:19:44]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2087293003/60001

[commit-bot: I haz the power, 2016-06-23 19:06:03]

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

[commit-bot: I haz the power, 2016-06-23 19:06:04]

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

[dgozman, 2016-06-23 19:39:14]

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

[commit-bot: I haz the power, 2016-06-23 19:40:22]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2087293003/80001

[commit-bot: I haz the power, 2016-06-23 20:25:51]

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

[commit-bot: I haz the power, 2016-06-23 20:25:52]

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

[dgozman, 2016-06-23 21:43:38]

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

[commit-bot: I haz the power, 2016-06-23 21:44:16]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2087293003/100001

[commit-bot: I haz the power, 2016-06-23 23:31:17]

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

[commit-bot: I haz the power, 2016-06-23 23:31:18]

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

[jkarlin, 2016-06-24 14:47:50]

https://codereview.chromium.org/2087293003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/page/NetworkStateNotifier.cpp (right):

https://codereview.chromium.org/2087293003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/NetworkStateNotifier.cpp:152: double
maxBandwidthMbps = maxBandwidth();
On 2016/06/23 15:00:22, dgozman wrote:
> On 2016/06/23 12:53:46, jkarlin wrote:
> > The underlying values of type and bandwidth might change between the above
two
> > calls. In fact it might change before notifyObservers is run.
notifyObservers
> > should take the connection type and max bandwidth as arguments.
> 
> How is that possible? Both notifyObservers and all state-mutating methods
check
> that they run on main thread, and we call notifyObservers synchronously.

Oh right, good point about this all running on the main thread. Never mind.

https://codereview.chromium.org/2087293003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/page/NetworkStateNotifier.cpp (right):

https://codereview.chromium.org/2087293003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/NetworkStateNotifier.cpp:131:
Page::networkStateChanged(onLine());
setOnline and setWebConnection check if anything has actually changed before
sending out notification. This should as well.

https://codereview.chromium.org/2087293003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/NetworkStateNotifier.cpp:144:
Page::networkStateChanged(onLine());
setOnline and setWebConnection check if anything has actually changed before
sending out notification. This should as well.

[dgozman, 2016-06-24 15:13:44]

https://codereview.chromium.org/2087293003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/page/NetworkStateNotifier.cpp (right):

https://codereview.chromium.org/2087293003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/NetworkStateNotifier.cpp:131:
Page::networkStateChanged(onLine());
On 2016/06/24 14:47:50, jkarlin wrote:
> setOnline and setWebConnection check if anything has actually changed before
> sending out notification. This should as well.

I think this would be undesirable. Consider two cases it's used:
- Tests. Here we cannot rely on the previous values (they can be arbitrary). And
if the test expects an event to be fired, event should fire no matter what.
- DevTools emulation. Here user expects an event to fire after an explicit
action. If it fires at some times and not the others, it would be perceived as
unreliable.

[jkarlin, 2016-06-24 19:20:16]

https://codereview.chromium.org/2087293003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/page/NetworkStateNotifier.cpp (right):

https://codereview.chromium.org/2087293003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/NetworkStateNotifier.cpp:131:
Page::networkStateChanged(onLine());
On 2016/06/24 15:13:44, dgozman wrote:
> On 2016/06/24 14:47:50, jkarlin wrote:
> > setOnline and setWebConnection check if anything has actually changed before
> > sending out notification. This should as well.
> 
> I think this would be undesirable. Consider two cases it's used:
> - Tests. Here we cannot rely on the previous values (they can be arbitrary).
And
> if the test expects an event to be fired, event should fire no matter what.

The clients of NetworkStateNotifier (e.g., the online event) don't expect to get
repeat values. Firing onLine twice without first calling offLine isn't spec'd
behavior.

If tests need an event, they can set it to one value and then another, the
second will trigger an event. That's what the existing tests do.

> - DevTools emulation. Here user expects an event to fire after an explicit
> action. If it fires at some times and not the others, it would be perceived as
> unreliable.

Do they? If my device is onLine and I change to 2G, I wouldn't expect to receive
an onLine event.

[dgozman, 2016-06-27 21:13:12]

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

[commit-bot: I haz the power, 2016-06-27 21:14:14]

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

[dgozman, 2016-06-27 23:03:49]

Please take another look. I've refactored a bit to ensure proper notifications.

https://codereview.chromium.org/2087293003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/page/NetworkStateNotifier.cpp (right):

https://codereview.chromium.org/2087293003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/NetworkStateNotifier.cpp:131:
Page::networkStateChanged(onLine());
On 2016/06/24 19:20:16, jkarlin wrote:
> On 2016/06/24 15:13:44, dgozman wrote:
> > On 2016/06/24 14:47:50, jkarlin wrote:
> > > setOnline and setWebConnection check if anything has actually changed
before
> > > sending out notification. This should as well.
> > 
> > I think this would be undesirable. Consider two cases it's used:
> > - Tests. Here we cannot rely on the previous values (they can be arbitrary).
> And
> > if the test expects an event to be fired, event should fire no matter what.
> 
> The clients of NetworkStateNotifier (e.g., the online event) don't expect to
get
> repeat values. Firing onLine twice without first calling offLine isn't spec'd
> behavior.
Alright, you convinced me. Can't go against the spec.

> 
> If tests need an event, they can set it to one value and then another, the
> second will trigger an event. That's what the existing tests do.
> 
> > - DevTools emulation. Here user expects an event to fire after an explicit
> > action. If it fires at some times and not the others, it would be perceived
as
> > unreliable.
> 
> Do they? If my device is onLine and I change to 2G, I wouldn't expect to
receive
> an onLine event.
>

[commit-bot: I haz the power, 2016-06-28 00:12:59]

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

[commit-bot: I haz the power, 2016-06-28 00:13:00]

Dry run: This issue passed the CQ dry run.

[jkarlin, 2016-06-28 14:23:41]

https://codereview.chromium.org/2087293003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/page/NetworkStateNotifier.cpp (right):

https://codereview.chromium.org/2087293003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/NetworkStateNotifier.cpp:49:
m_notifier.m_mutex.lock();
It's not obvious that a "notifier" would acquire a lock. Suggest putting the
lock back into the calling functions.

https://codereview.chromium.org/2087293003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/NetworkStateNotifier.cpp:53:
NetworkStateNotifier::ScopedNotifier::~ScopedNotifier()
Add unittests that set the connection (and online state) to the value twice in a
row to verify that the second doesn't send a duplicate notification.

https://codereview.chromium.org/2087293003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/NetworkStateNotifier.cpp:56: if (after.type
!= m_before.type || after.maxBandwidthMbps != m_before.maxBandwidthMbps ||
(after.connectionInitialized && !m_before.connectionInitialized))
|| (after.connectionInitialized && !m_before.connectionInitialized)

is a change in behavior. This fires the event the first time the type is set,
which is not spec. It's supposed to fire when the type changes. 

The purpose of the "connectionInitialized" variable is just to verify that
nobody uses the NetworkStateNotifier before the browser process sends a message
to initialize it.

https://codereview.chromium.org/2087293003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/NetworkStateNotifier.cpp:59: bool
onLineChanged = after.onLine != m_before.onLine || (after.onLineInitialized &&
!m_before.onLineInitialized);
Ditto

https://codereview.chromium.org/2087293003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/NetworkStateNotifier.cpp:125: void
NetworkStateNotifier::clearOverride()
Please add the thread checks back to these functions. I realize the
ScopedNotifier does it but it also serves as documentation.

https://codereview.chromium.org/2087293003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/page/NetworkStateNotifier.h (right):

https://codereview.chromium.org/2087293003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/NetworkStateNotifier.h:135: struct Data {
How about a slightly more descriptive name such as NetworkState?

https://codereview.chromium.org/2087293003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/NetworkStateNotifier.h:145: bool
onLineInitialized;
Please initialize the members here instead of in the constructor.

https://codereview.chromium.org/2087293003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/NetworkStateNotifier.h:152: class
ScopedNotifier {
This class needs documentation

https://codereview.chromium.org/2087293003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/NetworkStateNotifier.h:154: explicit
ScopedNotifier(NetworkStateNotifier&);
Why not use a raw pointer?

[dgozman, 2016-06-28 14:34:35]

See some comments. I will change the rest.

https://codereview.chromium.org/2087293003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/page/NetworkStateNotifier.cpp (right):

https://codereview.chromium.org/2087293003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/NetworkStateNotifier.cpp:49:
m_notifier.m_mutex.lock();
On 2016/06/28 14:23:41, jkarlin wrote:
> It's not obvious that a "notifier" would acquire a lock. Suggest putting the
> lock back into the calling functions.

It's not obvious, but everyone working on blink can figure it out when changing
this code.
The downside is:
- we would still need a lock in notifier to access members of notifier;
- we'll have additional locks in mutating functions, which hurts performance;
- it's uglier.

https://codereview.chromium.org/2087293003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/NetworkStateNotifier.cpp:56: if (after.type
!= m_before.type || after.maxBandwidthMbps != m_before.maxBandwidthMbps ||
(after.connectionInitialized && !m_before.connectionInitialized))
On 2016/06/28 14:23:41, jkarlin wrote:
> || (after.connectionInitialized && !m_before.connectionInitialized)
> 
> is a change in behavior. This fires the event the first time the type is set,
> which is not spec. It's supposed to fire when the type changes. 
I can do that, but how can we rely on changing from arbitrary start value to
something? For example, onLine defaults to true in our code. That means, we will
fire "offline" event when initializing, but never fire "online" event. Is that
per spec?

> 
> The purpose of the "connectionInitialized" variable is just to verify that
> nobody uses the NetworkStateNotifier before the browser process sends a
message
> to initialize it.

[dgozman, 2016-06-28 17:14:24]

Please take another look.

https://codereview.chromium.org/2087293003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/page/NetworkStateNotifier.cpp (right):

https://codereview.chromium.org/2087293003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/NetworkStateNotifier.cpp:53:
NetworkStateNotifier::ScopedNotifier::~ScopedNotifier()
On 2016/06/28 14:23:41, jkarlin wrote:
> Add unittests that set the connection (and online state) to the value twice in
a
> row to verify that the second doesn't send a duplicate notification.

Note that I didn't change the behavior here, but I can add a test for this as
well.

https://codereview.chromium.org/2087293003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/NetworkStateNotifier.cpp:125: void
NetworkStateNotifier::clearOverride()
On 2016/06/28 14:23:41, jkarlin wrote:
> Please add the thread checks back to these functions. I realize the
> ScopedNotifier does it but it also serves as documentation.

Done.

https://codereview.chromium.org/2087293003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/page/NetworkStateNotifier.h (right):

https://codereview.chromium.org/2087293003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/NetworkStateNotifier.h:135: struct Data {
On 2016/06/28 14:23:41, jkarlin wrote:
> How about a slightly more descriptive name such as NetworkState?

Done.

https://codereview.chromium.org/2087293003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/NetworkStateNotifier.h:145: bool
onLineInitialized;
On 2016/06/28 14:23:41, jkarlin wrote:
> Please initialize the members here instead of in the constructor.

Done.

https://codereview.chromium.org/2087293003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/NetworkStateNotifier.h:152: class
ScopedNotifier {
On 2016/06/28 14:23:41, jkarlin wrote:
> This class needs documentation

Done.

https://codereview.chromium.org/2087293003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/NetworkStateNotifier.h:154: explicit
ScopedNotifier(NetworkStateNotifier&);
On 2016/06/28 14:23:41, jkarlin wrote:
> Why not use a raw pointer?

Because it's never null.

[jkarlin, 2016-06-28 17:57:12]

https://codereview.chromium.org/2087293003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/page/NetworkStateNotifier.cpp (right):

https://codereview.chromium.org/2087293003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/NetworkStateNotifier.cpp:49:
m_notifier.m_mutex.lock();
On 2016/06/28 14:34:35, dgozman wrote:
> On 2016/06/28 14:23:41, jkarlin wrote:
> > It's not obvious that a "notifier" would acquire a lock. Suggest putting the
> > lock back into the calling functions.
> 
> It's not obvious, but everyone working on blink can figure it out when
changing
> this code.
> The downside is:
> - we would still need a lock in notifier to access members of notifier;

I'm not sure which notifier you're suggesting would need a lock, ScopedNotifier?
It doesn't need a lock since it's on the main thread.

ScopedNotifier verges on being too clever, and I feel like a name change would
help, but I'm having a hard time finding a better one. NetworkStateMutator is
what I want it to be, but the struct doesn't actually do the mutating.

Anyway, this seems fine to me.

https://codereview.chromium.org/2087293003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/NetworkStateNotifier.cpp:53:
NetworkStateNotifier::ScopedNotifier::~ScopedNotifier()
On 2016/06/28 17:14:23, dgozman wrote:
> On 2016/06/28 14:23:41, jkarlin wrote:
> > Add unittests that set the connection (and online state) to the value twice
in
> a
> > row to verify that the second doesn't send a duplicate notification.
> 
> Note that I didn't change the behavior here, but I can add a test for this as
> well.

Thanks.

https://codereview.chromium.org/2087293003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/NetworkStateNotifier.cpp:56: if (after.type
!= m_before.type || after.maxBandwidthMbps != m_before.maxBandwidthMbps ||
(after.connectionInitialized && !m_before.connectionInitialized))
On 2016/06/28 14:34:35, dgozman wrote:
> On 2016/06/28 14:23:41, jkarlin wrote:
> > || (after.connectionInitialized && !m_before.connectionInitialized)
> > 
> > is a change in behavior. This fires the event the first time the type is
set,
> > which is not spec. It's supposed to fire when the type changes. 
> I can do that, but how can we rely on changing from arbitrary start value to
> something? For example, onLine defaults to true in our code. That means, we
will
> fire "offline" event when initializing, but never fire "online" event. Is that
> per spec?
> 
> > 
> > The purpose of the "connectionInitialized" variable is just to verify that
> > nobody uses the NetworkStateNotifier before the browser process sends a
> message
> > to initialize it.
> 

Good point. We shouldn't fire a change event on initialization.

https://codereview.chromium.org/2087293003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/page/NetworkStateNotifier.h (right):

https://codereview.chromium.org/2087293003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/NetworkStateNotifier.h:154: explicit
ScopedNotifier(NetworkStateNotifier&);
On 2016/06/28 17:14:23, dgozman wrote:
> On 2016/06/28 14:23:41, jkarlin wrote:
> > Why not use a raw pointer?
> 
> Because it's never null.

Ah, sorry, Chrome style hiccup.

https://codereview.chromium.org/2087293003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/page/NetworkStateNotifier.cpp (right):

https://codereview.chromium.org/2087293003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/NetworkStateNotifier.cpp:50: m_before =
m_notifier.m_hasOverride ? m_notifier.m_override : m_notifier.m_state;
m_before can be set before acquiring the mutex since we're on the main thread.
Slight reduction in time that the lock has to be held.

https://codereview.chromium.org/2087293003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/NetworkStateNotifier.cpp:58: bool
onLineValue = after.onLine;
Why make this copy? The value won't change while in this function since we're on
the main thread.

https://codereview.chromium.org/2087293003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/NetworkStateNotifier.cpp:60:
m_notifier.m_mutex.unlock();
Can you add a comment somewhere around here that we must unlock before calling
Page::NetworkStateChanged to prevent deadlock? That, or, preferably PostTask the
networkStateChanged call so we don't have to worry about it.

https://codereview.chromium.org/2087293003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/page/NetworkStateNotifier.h (right):

https://codereview.chromium.org/2087293003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/NetworkStateNotifier.h:107: // When called,
successive setWebConnectionType/setOnLine calls are ignored,
Hmm, not quite ignored, since they're stored and will be used again once
clearOverride() is called.

Perhaps:

"are stored, and supplied overridden values are used instead until
clearOverride() is called."

https://codereview.chromium.org/2087293003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/NetworkStateNotifier.h:113: // mode, all
state will be reset to default values.
This last sentence is no longer true.

https://codereview.chromium.org/2087293003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/NetworkStateNotifier.h:146: // It's only
possible to mutate the state on main thread.
s/on main thread/on the main thread/

[allada, 2016-06-28 22:34:15]

Just a few minor things.

https://codereview.chromium.org/2087293003/diff/140001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/netinfo/resources/netinfo_common.js (right):

https://codereview.chromium.org/2087293003/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/netinfo/resources/netinfo_common.js:14:
window.addEventListener('beforeunload', function() {
(please fix here too)

https://codereview.chromium.org/2087293003/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/netinfo/resources/netinfo_common.js:20: return
type != 'none';
nit: double quotes

https://codereview.chromium.org/2087293003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.cpp (right):

https://codereview.chromium.org/2087293003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.cpp:249: if
(connectionType == protocol::Network::ConnectionTypeEnum::None)
Can we use a switch/case here instead?

https://codereview.chromium.org/2087293003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/sdk/NetworkManager.js (right):

https://codereview.chromium.org/2087293003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/sdk/NetworkManager.js:102: types =
WebInspector.NetworkManager._connectionTypes;
I think these should be classified in: WebInspector.NetworkManager.Conditions
(WebInspector.NetworkConditionsSelector._presets) this way we can keep the code
a bit cleaner rather than have them hard codded and having to update various
places when a new speed comes out.

[dgozman, 2016-06-29 01:16:54]

Patchset #10 (id:180001) has been deleted

[dgozman, 2016-06-29 01:17:33]

Please take another look.

https://codereview.chromium.org/2087293003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/page/NetworkStateNotifier.cpp (right):

https://codereview.chromium.org/2087293003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/NetworkStateNotifier.cpp:49:
m_notifier.m_mutex.lock();
On 2016/06/28 17:57:11, jkarlin wrote:
> On 2016/06/28 14:34:35, dgozman wrote:
> > On 2016/06/28 14:23:41, jkarlin wrote:
> > > It's not obvious that a "notifier" would acquire a lock. Suggest putting
the
> > > lock back into the calling functions.
> > 
> > It's not obvious, but everyone working on blink can figure it out when
> changing
> > this code.
> > The downside is:
> > - we would still need a lock in notifier to access members of notifier;
> 
> I'm not sure which notifier you're suggesting would need a lock,
ScopedNotifier?
> It doesn't need a lock since it's on the main thread.
> 
> ScopedNotifier verges on being too clever, and I feel like a name change would
> help, but I'm having a hard time finding a better one. NetworkStateMutator is
> what I want it to be, but the struct doesn't actually do the mutating.
> 
> Anyway, this seems fine to me.

That's a good point about main thread :-)
In this case I will go back to lock in mutators.

https://codereview.chromium.org/2087293003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/NetworkStateNotifier.cpp:56: if (after.type
!= m_before.type || after.maxBandwidthMbps != m_before.maxBandwidthMbps ||
(after.connectionInitialized && !m_before.connectionInitialized))
On 2016/06/28 17:57:11, jkarlin wrote:
> On 2016/06/28 14:34:35, dgozman wrote:
> > On 2016/06/28 14:23:41, jkarlin wrote:
> > > || (after.connectionInitialized && !m_before.connectionInitialized)
> > > 
> > > is a change in behavior. This fires the event the first time the type is
> set,
> > > which is not spec. It's supposed to fire when the type changes. 
> > I can do that, but how can we rely on changing from arbitrary start value to
> > something? For example, onLine defaults to true in our code. That means, we
> will
> > fire "offline" event when initializing, but never fire "online" event. Is
that
> > per spec?
> > 
> > > 
> > > The purpose of the "connectionInitialized" variable is just to verify that
> > > nobody uses the NetworkStateNotifier before the browser process sends a
> > message
> > > to initialize it.
> > 
> 
> Good point. We shouldn't fire a change event on initialization.

Done. Added a test for that.

https://codereview.chromium.org/2087293003/diff/140001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/netinfo/resources/netinfo_common.js (right):

https://codereview.chromium.org/2087293003/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/netinfo/resources/netinfo_common.js:20: return
type != 'none';
On 2016/06/28 22:34:14, Blaise wrote:
> nit: double quotes

Single quotes are the common style in netinfo tests. We should match the
surroundings.

https://codereview.chromium.org/2087293003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.cpp (right):

https://codereview.chromium.org/2087293003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.cpp:249: if
(connectionType == protocol::Network::ConnectionTypeEnum::None)
On 2016/06/28 22:34:15, Blaise wrote:
> Can we use a switch/case here instead?

Unfortunately, it's a string.

https://codereview.chromium.org/2087293003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/page/NetworkStateNotifier.cpp (right):

https://codereview.chromium.org/2087293003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/NetworkStateNotifier.cpp:50: m_before =
m_notifier.m_hasOverride ? m_notifier.m_override : m_notifier.m_state;
On 2016/06/28 17:57:12, jkarlin wrote:
> m_before can be set before acquiring the mutex since we're on the main thread.
> Slight reduction in time that the lock has to be held.

Done.

https://codereview.chromium.org/2087293003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/NetworkStateNotifier.cpp:58: bool
onLineValue = after.onLine;
On 2016/06/28 17:57:11, jkarlin wrote:
> Why make this copy? The value won't change while in this function since we're
on
> the main thread.

Done.

https://codereview.chromium.org/2087293003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/NetworkStateNotifier.cpp:60:
m_notifier.m_mutex.unlock();
On 2016/06/28 17:57:11, jkarlin wrote:
> Can you add a comment somewhere around here that we must unlock before calling
> Page::NetworkStateChanged to prevent deadlock? That, or, preferably PostTask
the
> networkStateChanged call so we don't have to worry about it. 

I've added a comment about this. Doing a PostTask is unnecessary, so I'd rather
avoid it.

https://codereview.chromium.org/2087293003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/page/NetworkStateNotifier.h (right):

https://codereview.chromium.org/2087293003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/NetworkStateNotifier.h:107: // When called,
successive setWebConnectionType/setOnLine calls are ignored,
On 2016/06/28 17:57:12, jkarlin wrote:
> Hmm, not quite ignored, since they're stored and will be used again once
> clearOverride() is called.
> 
> Perhaps:
> 
> "are stored, and supplied overridden values are used instead until
> clearOverride() is called."

Done.

https://codereview.chromium.org/2087293003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/NetworkStateNotifier.h:113: // mode, all
state will be reset to default values.
On 2016/06/28 17:57:12, jkarlin wrote:
> This last sentence is no longer true.

Good catch! Done.

https://codereview.chromium.org/2087293003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/NetworkStateNotifier.h:146: // It's only
possible to mutate the state on main thread.
On 2016/06/28 17:57:12, jkarlin wrote:
> s/on main thread/on the main thread/
> 

Done.

https://codereview.chromium.org/2087293003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/devtools/front_end/sdk/NetworkManager.js (right):

https://codereview.chromium.org/2087293003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/devtools/front_end/sdk/NetworkManager.js:102: types =
WebInspector.NetworkManager._connectionTypes;
On 2016/06/28 22:34:15, Blaise wrote:
> I think these should be classified in: WebInspector.NetworkManager.Conditions
> (WebInspector.NetworkConditionsSelector._presets) this way we can keep the
code
> a bit cleaner rather than have them hard codded and having to update various
> places when a new speed comes out. 

Added a TODO.

[jkarlin, 2016-06-29 14:00:18]

NetworkStateNotifier.h/cpp/unittest lgtm with nit

https://codereview.chromium.org/2087293003/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/page/NetworkStateNotifier.cpp (right):

https://codereview.chromium.org/2087293003/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/NetworkStateNotifier.cpp:53: {
DCHECK(isMainThread());

[dgozman, 2016-06-29 16:11:19]

Thanks for review!

@allada: take another look please.

https://codereview.chromium.org/2087293003/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/page/NetworkStateNotifier.cpp (right):

https://codereview.chromium.org/2087293003/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/page/NetworkStateNotifier.cpp:53: {
On 2016/06/29 14:00:18, jkarlin wrote:
> DCHECK(isMainThread());

Done.

[allada, 2016-06-29 17:49:14]

lgtm

[dgozman, 2016-06-29 17:54:44]

The CQ bit was checked by dgozman@chromium.org

[dgozman, 2016-06-29 17:54:45]

The patchset sent to the CQ was uploaded after l-g-t-m from jkarlin@chromium.org
Link to the patchset: https://codereview.chromium.org/2087293003/#ps220001
(title: "dcheck")

[commit-bot: I haz the power, 2016-06-29 17:55:33]

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, 2016-06-29 19:30:44]

Description was changed from

==========
[DevTools] Network.emulateNetworkConditions now affects NetworkStateNotifier.

This patch replaces test-only update mechanism in NetworkStateNotifier with
setOverride/clearOverride methods and uses them in InspectorNetworkAgent.

Unfortunately for emulation, NetworkStateNotifier is global and thus all
pages will observe the new network state. We can make it per-frame, but
that would introduce a lot of complexity. Current state is an acceptable
trade-off
for now.

This also adds connectionType parameter to Network.emulateNetworkConditions and
makes it fall-through to the renderer.

BUG=567613,586696,581334
==========

to

==========
[DevTools] Network.emulateNetworkConditions now affects NetworkStateNotifier.

This patch replaces test-only update mechanism in NetworkStateNotifier with
setOverride/clearOverride methods and uses them in InspectorNetworkAgent.

Unfortunately for emulation, NetworkStateNotifier is global and thus all
pages will observe the new network state. We can make it per-frame, but
that would introduce a lot of complexity. Current state is an acceptable
trade-off
for now.

This also adds connectionType parameter to Network.emulateNetworkConditions and
makes it fall-through to the renderer.

BUG=567613,586696,581334
==========

[commit-bot: I haz the power, 2016-06-29 19:30:46]

Committed patchset #11 (id:220001)

[commit-bot: I haz the power, 2016-06-29 19:31:18]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-06-29 19:32:44]

Description was changed from

==========
[DevTools] Network.emulateNetworkConditions now affects NetworkStateNotifier.

This patch replaces test-only update mechanism in NetworkStateNotifier with
setOverride/clearOverride methods and uses them in InspectorNetworkAgent.

Unfortunately for emulation, NetworkStateNotifier is global and thus all
pages will observe the new network state. We can make it per-frame, but
that would introduce a lot of complexity. Current state is an acceptable
trade-off
for now.

This also adds connectionType parameter to Network.emulateNetworkConditions and
makes it fall-through to the renderer.

BUG=567613,586696,581334
==========

to

==========
[DevTools] Network.emulateNetworkConditions now affects NetworkStateNotifier.

This patch replaces test-only update mechanism in NetworkStateNotifier with
setOverride/clearOverride methods and uses them in InspectorNetworkAgent.

Unfortunately for emulation, NetworkStateNotifier is global and thus all
pages will observe the new network state. We can make it per-frame, but
that would introduce a lot of complexity. Current state is an acceptable
trade-off
for now.

This also adds connectionType parameter to Network.emulateNetworkConditions and
makes it fall-through to the renderer.

BUG=567613,586696,581334

Committed: https://crrev.com/6ca18e32edb9bad244588538ea2bb2d1be9fb1ff
Cr-Commit-Position: refs/heads/master@{#402897}
==========

[commit-bot: I haz the power, 2016-06-29 19:32:46]

Patchset 11 (id:??) landed as
https://crrev.com/6ca18e32edb9bad244588538ea2bb2d1be9fb1ff
Cr-Commit-Position: refs/heads/master@{#402897}