NetInfo network quality extension: Add callbacks and Layout tests

third_party/WebKit/Source/platform/network/NetworkStateNotifier.cpp

Index: third_party/WebKit/Source/platform/network/NetworkStateNotifier.cpp
diff --git a/third_party/WebKit/Source/platform/network/NetworkStateNotifier.cpp b/third_party/WebKit/Source/platform/network/NetworkStateNotifier.cpp
index 46d6824a532edc1fb752bf0acf1708e31ab9f588..6aedb982960bb7d365d2ede825577ae35c9d5d4c 100644
--- a/third_party/WebKit/Source/platform/network/NetworkStateNotifier.cpp
+++ b/third_party/WebKit/Source/platform/network/NetworkStateNotifier.cpp
@@ -146,9 +146,10 @@ void NetworkStateNotifier::RemoveOnLineObserver(
   RemoveObserver(on_line_state_observers_, observer, std::move(task_runner));
 }
 
-void NetworkStateNotifier::SetOverride(bool on_line,
-                                       WebConnectionType type,
-                                       double max_bandwidth_mbps) {
+void NetworkStateNotifier::SetNetworkConnectionInfoOverride(
+    bool on_line,
+    WebConnectionType type,
+    double max_bandwidth_mbps) {
   DCHECK(IsMainThread());
   ScopedNotifier notifier(*this);
   {
@@ -162,6 +163,26 @@ void NetworkStateNotifier::SetOverride(bool on_line,
   }
 }
 
+void NetworkStateNotifier::SetNetworkQualityInfoOverride(

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

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]

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?

[dcheng, 2017/05/29 08:06:14]

The browser-side network change/quality notifier should probably be swapped out
in test mode with a testing implementation to avoid these issues.
I do think we should fix this, but a subsequent CL is fine.

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

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.

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

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

+    WebEffectiveConnectionType effective_type,
+    unsigned long transport_rtt_msec,
+    double downlink_throughput_mbps) {
+  DCHECK(IsMainThread());
+  ScopedNotifier notifier(*this);
+  {
+    MutexLocker locker(mutex_);
+    has_override_ = true;
+    override_.on_line_initialized = true;
+    override_.connection_initialized = true;
+    override_.effective_type = effective_type;
+    override_.transport_rtt =
+        base::TimeDelta::FromMilliseconds(transport_rtt_msec);
+    override_.downlink_throughput_mbps = base::nullopt;
+    if (downlink_throughput_mbps >= 0)
+      override_.downlink_throughput_mbps = downlink_throughput_mbps;
+  }
+}
+
 void NetworkStateNotifier::ClearOverride() {
   DCHECK(IsMainThread());
   ScopedNotifier notifier(*this);