Start calculating new combined NetworkChangeNotifier signal

net/base/network_change_notifier.cc

Index: net/base/network_change_notifier.cc
diff --git a/net/base/network_change_notifier.cc b/net/base/network_change_notifier.cc
index e8dd1208e4127713433df6c0dfbb9836d6d46a57..43c9247fb01962eb1cff36381d3f885f445b4c56 100644
--- a/net/base/network_change_notifier.cc
+++ b/net/base/network_change_notifier.cc
@@ -35,6 +35,7 @@ NetworkChangeNotifierFactory* g_network_change_notifier_factory = NULL;
 
 class MockNetworkChangeNotifier : public NetworkChangeNotifier {
  public:
+  MockNetworkChangeNotifier() : NetworkChangeNotifier(0, 0, 0, 0) {}
   virtual ConnectionType GetCurrentConnectionType() const OVERRIDE {
     return CONNECTION_UNKNOWN;
   }
@@ -46,12 +47,14 @@ class MockNetworkChangeNotifier : public NetworkChangeNotifier {
 class HistogramWatcher
     : public NetworkChangeNotifier::ConnectionTypeObserver,
       public NetworkChangeNotifier::IPAddressObserver,
-      public NetworkChangeNotifier::DNSObserver {
+      public NetworkChangeNotifier::DNSObserver,
+      public NetworkChangeNotifier::NetworkChangeObserver {
  public:
   HistogramWatcher()
       : last_ip_address_change_(base::TimeTicks::Now()),
         last_connection_change_(base::TimeTicks::Now()),
         last_dns_change_(base::TimeTicks::Now()),
+        last_network_change_(base::TimeTicks::Now()),
         last_connection_type_(NetworkChangeNotifier::CONNECTION_UNKNOWN),
         offline_packets_received_(0) {}
 
@@ -64,6 +67,7 @@ class HistogramWatcher
     NetworkChangeNotifier::AddConnectionTypeObserver(this);
     NetworkChangeNotifier::AddIPAddressObserver(this);
     NetworkChangeNotifier::AddDNSObserver(this);
+    NetworkChangeNotifier::AddNetworkChangeObserver(this);
   }
 
   virtual ~HistogramWatcher() {}
@@ -72,6 +76,9 @@ class HistogramWatcher
   virtual void OnIPAddressChanged() OVERRIDE {
     UMA_HISTOGRAM_MEDIUM_TIMES("NCN.IPAddressChange",
                                SinceLast(&last_ip_address_change_));
+    UMA_HISTOGRAM_MEDIUM_TIMES(
+        "NCN.ConnectionTypeChangeToIPAddressChange",
+        last_ip_address_change_ - last_connection_change_);
   }
 
   // NetworkChangeNotifier::ConnectionTypeObserver implementation.
@@ -98,6 +105,9 @@ class HistogramWatcher
       UMA_HISTOGRAM_MEDIUM_TIMES("NCN.OfflineChange",
                                  SinceLast(&last_connection_change_));
     }
+    UMA_HISTOGRAM_MEDIUM_TIMES(
+        "NCN.IPAddressChangeToConnectionTypeChange",
+        last_connection_change_ - last_ip_address_change_);
 
     offline_packets_received_ = 0;
     last_connection_type_ = type;
@@ -110,6 +120,18 @@ class HistogramWatcher
                                SinceLast(&last_dns_change_));
   }
 
+  // NetworkChangeNotifier::NetworkChangeObserver implementation.
+  virtual void OnNetworkChanged(
+      NetworkChangeNotifier::ConnectionType type) OVERRIDE {
+    if (type != NetworkChangeNotifier::CONNECTION_NONE) {
+      UMA_HISTOGRAM_MEDIUM_TIMES("NCN.NetworkOnlineChange",
+                                 SinceLast(&last_network_change_));
+    } else {
+      UMA_HISTOGRAM_MEDIUM_TIMES("NCN.NetworkOfflineChange",
+                                 SinceLast(&last_network_change_));
+    }
+  }
+
   // Record histogram data whenever we receive a packet but think we're
   // offline.  Should only be called from the network thread.
   void NotifyDataReceived(const GURL& source) {
@@ -153,6 +175,7 @@ class HistogramWatcher
   base::TimeTicks last_ip_address_change_;
   base::TimeTicks last_connection_change_;
   base::TimeTicks last_dns_change_;
+  base::TimeTicks last_network_change_;
   base::TimeTicks last_offline_packet_received_;
   base::TimeTicks last_polled_connection_;
   // |polling_interval_| is initialized by |OnConnectionTypeChanged| on our
@@ -193,6 +216,89 @@ class NetworkChangeNotifier::NetworkState {
   DnsConfig dns_config_;
 };
 
+// Calculates NetworkChange signal from IPAddress and ConnectionType signals.
+class NetworkChangeNotifier::NetworkChangeCalculator
+    : public ConnectionTypeObserver,
+      public IPAddressObserver {
+ public:
+  // Delay arguments control how many milliseconds to delay notifications
+  // so as to combine duplicates. |ip_address_offline_delay_ms| controls
+  // delay after OnIPAddressChanged when transitioning from an offline state.
+  // |ip_address_online_delay_ms| controls delay after OnIPAddressChanged when
+  // transitioning from an online state. |connection_type_offline_delay_ms|
+  // controls delay after OnConnectionTypeChanged when transitioning from an
+  // offline state. |connection_type_online_delay_ms| controls delay after
+  // OnConnectionTypeChanged when transitioning from an online state.

[szym, 2012/11/25 07:37:37]

This comment needs to explain that the notifications are not just delayed but
also "debounced" or "de-duped", i.e., if two notifications arrive within XXX,
then only one will be sent out.

+  NetworkChangeCalculator(uint32 ip_address_offline_delay_ms,
+                          uint32 ip_address_online_delay_ms,
+                          uint32 connection_type_offline_delay_ms,
+                          uint32 connection_type_online_delay_ms)
+      : ip_address_offline_delay_(base::TimeDelta::FromMilliseconds(
+            ip_address_offline_delay_ms)),
+        ip_address_online_delay_(base::TimeDelta::FromMilliseconds(
+            ip_address_online_delay_ms)),
+        connection_type_offline_delay_(base::TimeDelta::FromMilliseconds(
+            connection_type_offline_delay_ms)),
+        connection_type_online_delay_(base::TimeDelta::FromMilliseconds(
+            connection_type_online_delay_ms)),
+        have_announced_(false),
+        last_announced_connection_type_(CONNECTION_NONE),
+        pending_connection_type_(CONNECTION_NONE) {}
+  void Init() {
+    AddConnectionTypeObserver(this);
+    AddIPAddressObserver(this);
+  }
+  virtual ~NetworkChangeCalculator() {
+    RemoveConnectionTypeObserver(this);
+    RemoveIPAddressObserver(this);
+  }
+
+  // NetworkChangeNotifier::IPAddressObserver implementation.

[szym, 2012/11/25 07:37:37]

nit: no need for "implementation"

[pauljensen, 2012/11/27 21:47:42]

I see a lot of people using "implementation": URLRequestJobFactoryImpl and
ProfileImpl and MessageLoopProxyImpl.
I was copying HistogramWatcher's style.

+  virtual void OnIPAddressChanged() OVERRIDE {
+    base::TimeDelta delay = last_announced_connection_type_ == CONNECTION_NONE
+        ? ip_address_offline_delay_ : ip_address_online_delay_;
+    timer_.Start(FROM_HERE, delay, this, &NetworkChangeCalculator::Notify);

[szym, 2012/11/25 07:37:37]

Perhaps a note (a reminder really) that this cancels the previous timer.

+  }
+
+  // NetworkChangeNotifier::ConnectionTypeObserver implementation.
+  virtual void OnConnectionTypeChanged(ConnectionType type) OVERRIDE {
+    pending_connection_type_ = type;
+    base::TimeDelta delay = last_announced_connection_type_ == CONNECTION_NONE
+        ? connection_type_offline_delay_ : connection_type_online_delay_;
+    timer_.Start(FROM_HERE, delay, this, &NetworkChangeCalculator::Notify);
+  }
+
+ private:
+  void Notify() {
+    // Don't bother signaling about dead connections.
+    if (have_announced_ && last_announced_connection_type_ == CONNECTION_NONE &&

[szym, 2012/11/25 07:37:37]

suggest parens for readability

+        pending_connection_type_ == CONNECTION_NONE)

[szym, 2012/11/25 07:37:37]

Could the same logic be accomplished by initializing
last_announced_connection_type_ to CONNECTION_UNKNOWN, or are the timeouts
essential even for the first announcement?

[pauljensen, 2012/11/27 21:47:42]

The timeouts are essential for all announcements; the first is no different. 
The timeouts for offline->online are always longer than the timeouts for
online->offline so I wanted to initialize last_announced_connect_type_ to an
offline state to essentially prepare for the worst deduping circumstances.

+      return;

[szym, 2012/11/25 07:37:37]

use braces when if condition is multiline

+    have_announced_ = true;
+    last_announced_connection_type_ = pending_connection_type_;
+    // Immediately before sending out an online signal, send out an offline
+    // signal to perform any destructive actions before constructive actions.
+    if (pending_connection_type_ != CONNECTION_NONE)
+      NetworkChangeNotifier::NotifyObserversOfNetworkChange(CONNECTION_NONE);
+    NetworkChangeNotifier::NotifyObserversOfNetworkChange(
+        pending_connection_type_);
+  }
+
+  const base::TimeDelta ip_address_offline_delay_;
+  const base::TimeDelta ip_address_online_delay_;
+  const base::TimeDelta connection_type_offline_delay_;
+  const base::TimeDelta connection_type_online_delay_;
+
+  // Indicates if NotifyObserversOfNetworkChange has been called yet.
+  bool have_announced_;
+  // Last value passed to NotifyObserversOfNetworkChange.
+  ConnectionType last_announced_connection_type_;
+  // Value to pass to NotifyObserversOfNetworkChange when Notify is called.
+  ConnectionType pending_connection_type_;
+  // Used to delay notifications so duplicates can be combined.
+  base::OneShotTimer<NetworkChangeCalculator> timer_;
+};
+
 NetworkChangeNotifier::~NetworkChangeNotifier() {
   DCHECK_EQ(this, g_network_change_notifier);
   g_network_change_notifier = NULL;
@@ -346,6 +452,14 @@ void NetworkChangeNotifier::AddDNSObserver(DNSObserver* observer) {
   }
 }
 
+void NetworkChangeNotifier::AddNetworkChangeObserver(
+    NetworkChangeObserver* observer) {
+  if (g_network_change_notifier) {
+    g_network_change_notifier->network_change_observer_list_->AddObserver(
+        observer);
+  }
+}
+
 void NetworkChangeNotifier::RemoveIPAddressObserver(
     IPAddressObserver* observer) {
   if (g_network_change_notifier) {
@@ -369,7 +483,19 @@ void NetworkChangeNotifier::RemoveDNSObserver(DNSObserver* observer) {
   }
 }
 
-NetworkChangeNotifier::NetworkChangeNotifier()
+void NetworkChangeNotifier::RemoveNetworkChangeObserver(
+    NetworkChangeObserver* observer) {
+  if (g_network_change_notifier) {
+    g_network_change_notifier->network_change_observer_list_->RemoveObserver(
+        observer);
+  }
+}
+
+NetworkChangeNotifier::NetworkChangeNotifier(
+    uint32 ip_address_offline_delay_ms,
+    uint32 ip_address_online_delay_ms,
+    uint32 connection_type_offline_delay_ms,
+    uint32 connection_type_online_delay_ms)
     : ip_address_observer_list_(
         new ObserverListThreadSafe<IPAddressObserver>(
             ObserverListBase<IPAddressObserver>::NOTIFY_EXISTING_ONLY)),
@@ -379,10 +505,19 @@ NetworkChangeNotifier::NetworkChangeNotifier()
       resolver_state_observer_list_(
         new ObserverListThreadSafe<DNSObserver>(
             ObserverListBase<DNSObserver>::NOTIFY_EXISTING_ONLY)),
+      network_change_observer_list_(
+        new ObserverListThreadSafe<NetworkChangeObserver>(
+            ObserverListBase<NetworkChangeObserver>::NOTIFY_EXISTING_ONLY)),
       network_state_(new NetworkState()),
-      histogram_watcher_(new HistogramWatcher()) {
+      histogram_watcher_(new HistogramWatcher()),
+      network_change_calculator_(new NetworkChangeCalculator(
+          ip_address_offline_delay_ms,
+          ip_address_online_delay_ms,
+          connection_type_offline_delay_ms,
+          connection_type_online_delay_ms)) {
   DCHECK(!g_network_change_notifier);
   g_network_change_notifier = this;
+  network_change_calculator_->Init();
 }
 
 #if defined(OS_LINUX)
@@ -424,6 +559,15 @@ void NetworkChangeNotifier::NotifyObserversOfConnectionTypeChange() {
   }
 }
 
+void NetworkChangeNotifier::NotifyObserversOfNetworkChange(
+    ConnectionType type) {
+  if (g_network_change_notifier) {
+    g_network_change_notifier->network_change_observer_list_->Notify(
+        &NetworkChangeObserver::OnNetworkChanged,
+        type);
+  }
+}
+
 NetworkChangeNotifier::DisableForTest::DisableForTest()
     : network_change_notifier_(g_network_change_notifier) {
   DCHECK(g_network_change_notifier);