[DevTools] Network.emulateNetworkConditions now affects NetworkStateNotifier.

third_party/WebKit/Source/core/page/NetworkStateNotifier.cpp

Index: third_party/WebKit/Source/core/page/NetworkStateNotifier.cpp
diff --git a/third_party/WebKit/Source/core/page/NetworkStateNotifier.cpp b/third_party/WebKit/Source/core/page/NetworkStateNotifier.cpp
index 628b377de6a04f3f2954bf2f6754bb7eb38ceb58..76598a32141cca33fbf6db84e492993462b94ad1 100644
--- a/third_party/WebKit/Source/core/page/NetworkStateNotifier.cpp
+++ b/third_party/WebKit/Source/core/page/NetworkStateNotifier.cpp
@@ -42,28 +42,39 @@ NetworkStateNotifier& networkStateNotifier()
     return networkStateNotifier;
 }
 
-void NetworkStateNotifier::setOnLine(bool onLine)
+NetworkStateNotifier::ScopedNotifier::ScopedNotifier(NetworkStateNotifier& notifier)
+    : m_notifier(notifier)
 {
-    ASSERT(isMainThread());
-
-    {
-        MutexLocker locker(m_mutex);
-        if (m_isOnLine == onLine)
-            return;
+    DCHECK(isMainThread());
+    m_notifier.m_mutex.lock();

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

It's not obvious that a "notifier" would acquire a lock. Suggest putting the
lock back into the calling functions.

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

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.

[jkarlin, 2016/06/28 17:57:11]

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.

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

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

+    m_before = m_notifier.m_hasOverride ? m_notifier.m_overrideData : m_notifier.m_data;
+}
 
-        m_isOnLine = onLine;
-    }
+NetworkStateNotifier::ScopedNotifier::~ScopedNotifier()

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

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.

[dgozman, 2016/06/28 17:14:23]

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

[jkarlin, 2016/06/28 17:57:11]

Thanks.

+{
+    const Data& after = m_notifier.m_hasOverride ? m_notifier.m_overrideData : m_notifier.m_data;
+    if (after.type != m_before.type || after.maxBandwidthMbps != m_before.maxBandwidthMbps || (after.connectionInitialized && !m_before.connectionInitialized))

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

|| (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.

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

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?

[jkarlin, 2016/06/28 17:57:11]

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

[dgozman, 2016/06/29 01:17:32]

Done. Added a test for that.

+        m_notifier.notifyObservers(after.type, after.maxBandwidthMbps);
+    bool onLineValue = after.onLine;
+    bool onLineChanged = after.onLine != m_before.onLine || (after.onLineInitialized && !m_before.onLineInitialized);

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

Ditto

+    m_notifier.m_mutex.unlock();
+    if (onLineChanged)
+        Page::networkStateChanged(onLineValue);
+}
 
-    Page::networkStateChanged(onLine);
+void NetworkStateNotifier::setOnLine(bool onLine)
+{
+    ScopedNotifier notifier(*this);
+    m_data.onLineInitialized = true;
+    m_data.onLine = onLine;
 }
 
 void NetworkStateNotifier::setWebConnection(WebConnectionType type, double maxBandwidthMbps)
 {
-    ASSERT(isMainThread());
-    if (m_testUpdatesOnly)
-        return;
-
-    setWebConnectionImpl(type, maxBandwidthMbps);
+    ScopedNotifier notifier(*this);
+    m_data.connectionInitialized = true;
+    m_data.type = type;
+    m_data.maxBandwidthMbps = maxBandwidthMbps;
 }
 
 void NetworkStateNotifier::addObserver(NetworkStateObserver* observer, ExecutionContext* context)
@@ -100,40 +111,26 @@ void NetworkStateNotifier::removeObserver(NetworkStateObserver* observer, Execut
         collectZeroedObservers(observerList, context);
 }
 
-void NetworkStateNotifier::setTestUpdatesOnly(bool updatesOnly)
+void NetworkStateNotifier::setOverride(bool onLine, WebConnectionType type, double maxBandwidthMbps)
 {
-    ASSERT(isMainThread());
-    MutexLocker locker(m_mutex);
-
-    // Reset state to default when entering or leaving test mode.
-    if (updatesOnly != m_testUpdatesOnly) {
-        m_isOnLine = true;
-        m_type = WebConnectionTypeOther;
-        m_maxBandwidthMbps = std::numeric_limits<double>::infinity();
-    }
-
-    m_testUpdatesOnly = updatesOnly;
+    ScopedNotifier notifier(*this);
+    m_hasOverride = true;
+    m_overrideData.onLineInitialized = true;
+    m_overrideData.onLine = onLine;
+    m_overrideData.connectionInitialized = true;
+    m_overrideData.type = type;
+    m_overrideData.maxBandwidthMbps = maxBandwidthMbps;
 }
 
-void NetworkStateNotifier::setWebConnectionForTest(WebConnectionType type, double maxBandwidthMbps)
+void NetworkStateNotifier::clearOverride()

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

Please add the thread checks back to these functions. I realize the
ScopedNotifier does it but it also serves as documentation.

[dgozman, 2016/06/28 17:14:23]

Done.

 {
-    ASSERT(isMainThread());
-    ASSERT(m_testUpdatesOnly);
-    setWebConnectionImpl(type, maxBandwidthMbps);
+    ScopedNotifier notifier(*this);
+    m_hasOverride = false;
 }
 
-void NetworkStateNotifier::setWebConnectionImpl(WebConnectionType type, double maxBandwidthMbps)
+void NetworkStateNotifier::notifyObservers(WebConnectionType type, double maxBandwidthMbps)
 {
-    ASSERT(isMainThread());
-
-    MutexLocker locker(m_mutex);
-    m_initialized = true;
-
-    if (m_type == type && m_maxBandwidthMbps == maxBandwidthMbps)
-        return;
-    m_type = type;
-    m_maxBandwidthMbps = maxBandwidthMbps;
-
+    DCHECK(isMainThread());
     for (const auto& entry : m_observers) {
         ExecutionContext* context = entry.key;
         context->postTask(BLINK_FROM_HERE, createCrossThreadTask(&NetworkStateNotifier::notifyObserversOfConnectionChangeOnContext, crossThreadUnretained(this), type, maxBandwidthMbps));