NetInfo network quality extension: Add callbacks and Layout tests

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

 /*
  * Copyright (C) 2008 Apple Inc. All Rights Reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
  * are met:
  * 1. Redistributions of source code must retain the above copyright
  *    notice, this list of conditions and the following disclaimer.
  * 2. Redistributions in binary form must reproduce the above copyright
  *    notice, this list of conditions and the following disclaimer in the
@@ skipping 128 matching lines @@
     PassRefPtr<WebTaskRunner> task_runner) {
   RemoveObserver(connection_observers_, observer, std::move(task_runner));
 }
 
 void NetworkStateNotifier::RemoveOnLineObserver(
     NetworkStateObserver* observer,
     PassRefPtr<WebTaskRunner> task_runner) {
   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);
   {
     MutexLocker locker(mutex_);
     has_override_ = true;
     override_.on_line_initialized = true;
     override_.on_line = on_line;
     override_.connection_initialized = true;
     override_.type = type;
     override_.max_bandwidth_mbps = max_bandwidth_mbps;
   }
 }
 
+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);
   {
     MutexLocker locker(mutex_);
     has_override_ = false;
   }
 }
 
 void NetworkStateNotifier::NotifyObservers(ObserverListMap& map,
@@ skipping 111 matching lines @@
 
   list->zeroed_observers.clear();
 
   if (list->observers.IsEmpty()) {
     MutexLocker locker(mutex_);
     map.erase(task_runner);  // deletes list
   }
 }
 
 }  // namespace blink