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..290e2144743e85e0f50a45a16c23f0be9571363a 100644
--- a/net/base/network_change_notifier.cc
+++ b/net/base/network_change_notifier.cc
@@ -46,12 +46,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 +66,7 @@ class HistogramWatcher
     NetworkChangeNotifier::AddConnectionTypeObserver(this);
     NetworkChangeNotifier::AddIPAddressObserver(this);
     NetworkChangeNotifier::AddDNSObserver(this);
+    NetworkChangeNotifier::AddNetworkChangeObserver(this);
   }
 
   virtual ~HistogramWatcher() {}
@@ -72,6 +75,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 +104,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 +119,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 +174,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 +215,79 @@ class NetworkChangeNotifier::NetworkState {
   DnsConfig dns_config_;
 };
 
+NetworkChangeNotifier::NetworkChangeCalculatorParams::
+    NetworkChangeCalculatorParams() {
+}
+
+// Calculates NetworkChange signal from IPAddress and ConnectionType signals.
+class NetworkChangeNotifier::NetworkChangeCalculator
+    : public ConnectionTypeObserver,
+      public IPAddressObserver {
+ public:
+  NetworkChangeCalculator(const NetworkChangeCalculatorParams& params)
+      : params_(params),
+        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.
+  virtual void OnIPAddressChanged() OVERRIDE {
+    base::TimeDelta delay = last_announced_connection_type_ == CONNECTION_NONE
+        ? params_.ip_address_offline_delay_ : params_.ip_address_online_delay_;
+    // Cancels any previous timer.
+    timer_.Start(FROM_HERE, delay, this, &NetworkChangeCalculator::Notify);
+  }
+
+  // NetworkChangeNotifier::ConnectionTypeObserver implementation.
+  virtual void OnConnectionTypeChanged(ConnectionType type) OVERRIDE {
+    pending_connection_type_ = type;
+    base::TimeDelta delay = last_announced_connection_type_ == CONNECTION_NONE
+        ? params_.connection_type_offline_delay_
+        : params_.connection_type_online_delay_;
+    // Cancels any previous timer.
+    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) &&
+        (pending_connection_type_ == CONNECTION_NONE)) {
+      return;
+    }
+    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 NetworkChangeCalculatorParams params_;
+
+  // 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 +441,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,20 +472,42 @@ void NetworkChangeNotifier::RemoveDNSObserver(DNSObserver* observer) {
   }
 }
 
-NetworkChangeNotifier::NetworkChangeNotifier()
-    : ip_address_observer_list_(
-        new ObserverListThreadSafe<IPAddressObserver>(
-            ObserverListBase<IPAddressObserver>::NOTIFY_EXISTING_ONLY)),
-      connection_type_observer_list_(
-        new ObserverListThreadSafe<ConnectionTypeObserver>(
-            ObserverListBase<ConnectionTypeObserver>::NOTIFY_EXISTING_ONLY)),
-      resolver_state_observer_list_(
-        new ObserverListThreadSafe<DNSObserver>(
-            ObserverListBase<DNSObserver>::NOTIFY_EXISTING_ONLY)),
-      network_state_(new NetworkState()),
-      histogram_watcher_(new HistogramWatcher()) {
+void NetworkChangeNotifier::RemoveNetworkChangeObserver(
+    NetworkChangeObserver* observer) {
+  if (g_network_change_notifier) {
+    g_network_change_notifier->network_change_observer_list_->RemoveObserver(
+        observer);
+  }
+}
+
+NetworkChangeNotifier::NetworkChangeNotifier() {
+  CommonConstructor(NetworkChangeCalculatorParams());
+}
+
+NetworkChangeNotifier::NetworkChangeNotifier(
+    const NetworkChangeCalculatorParams& params) {
+  CommonConstructor(params);
+}
+
+void NetworkChangeNotifier::CommonConstructor(
+    const NetworkChangeCalculatorParams& params) {
+  ip_address_observer_list_ = new ObserverListThreadSafe<IPAddressObserver>(
+      ObserverListBase<IPAddressObserver>::NOTIFY_EXISTING_ONLY);

[szym, 2012/12/07 17:56:24]

I wish there was a better way. Where's C++11 when we need it?

In this case I'd actually suggest default argument:
NetworkChangeNotifier(const NetworkChangeCalculatorParams& params =
NetowkrChangeCalculatorParams()) : ...

It's one violation of codestyle vs. one semi-violation (overrides are seen as
effective default args), de-consting and extra code.

BTW: Maybe you could express |new
ObserverListThreadSafe<T>(ObserverListBase<T>::NOTIFY_EXISTING_ONLY)| as
MakeObserverList<T>().

+  connection_type_observer_list_ =
+      new ObserverListThreadSafe<ConnectionTypeObserver>(
+          ObserverListBase<ConnectionTypeObserver>::NOTIFY_EXISTING_ONLY);
+  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_.reset(new NetworkState());
+  histogram_watcher_.reset(new HistogramWatcher());
+  network_change_calculator_.reset(new NetworkChangeCalculator(params));
+
   DCHECK(!g_network_change_notifier);
   g_network_change_notifier = this;
+  network_change_calculator_->Init();
 }
 
 #if defined(OS_LINUX)
@@ -424,6 +549,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);