Switch from OnIPAddressChanged and OnConnectionTypeChange to OnNetworkChanged

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 cf5557c601a47dd54da64c705a55919889f0fa20..9ff38c25e07339b7033cad6c207164a7028fef55 100644
--- a/net/base/network_change_notifier.cc
+++ b/net/base/network_change_notifier.cc
@@ -43,18 +43,18 @@ class MockNetworkChangeNotifier : public NetworkChangeNotifier {
 }  // namespace
 
 // The main observer class that records UMAs for network events.
-class HistogramWatcher
-    : public NetworkChangeNotifier::ConnectionTypeObserver,
-      public NetworkChangeNotifier::IPAddressObserver,
-      public NetworkChangeNotifier::DNSObserver,
-      public NetworkChangeNotifier::NetworkChangeObserver {
+class NetworkChangeNotifier::HistogramWatcher
+    : public ConnectionTypeObserver,
+      public IPAddressObserver,
+      public DNSObserver,
+      public 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),
+        last_connection_type_(CONNECTION_UNKNOWN),
         offline_packets_received_(0) {}
 
   // Registers our three Observer implementations.  This is called from the
@@ -63,15 +63,15 @@ class HistogramWatcher
   // because the only other interface, |NotifyDataReceived| is also
   // only called from the network thread.
   void Init() {
-    NetworkChangeNotifier::AddConnectionTypeObserver(this);
-    NetworkChangeNotifier::AddIPAddressObserver(this);
-    NetworkChangeNotifier::AddDNSObserver(this);
-    NetworkChangeNotifier::AddNetworkChangeObserver(this);
+    Deprecated::AddConnectionTypeObserver(this);
+    Deprecated::AddIPAddressObserver(this);
+    AddDNSObserver(this);
+    AddNetworkChangeObserver(this);
   }
 
   virtual ~HistogramWatcher() {}
 
-  // NetworkChangeNotifier::IPAddressObserver implementation.
+  // IPAddressObserver implementation.
   virtual void OnIPAddressChanged() OVERRIDE {
     UMA_HISTOGRAM_MEDIUM_TIMES("NCN.IPAddressChange",
                                SinceLast(&last_ip_address_change_));
@@ -80,10 +80,9 @@ class HistogramWatcher
         last_ip_address_change_ - last_connection_change_);
   }
 
-  // NetworkChangeNotifier::ConnectionTypeObserver implementation.
-  virtual void OnConnectionTypeChanged(
-      NetworkChangeNotifier::ConnectionType type) OVERRIDE {
-    if (type != NetworkChangeNotifier::CONNECTION_NONE) {
+  // ConnectionTypeObserver implementation.
+  virtual void OnConnectionTypeChanged(ConnectionType type) OVERRIDE {
+    if (type != CONNECTION_NONE) {
       UMA_HISTOGRAM_MEDIUM_TIMES("NCN.OnlineChange",
                                  SinceLast(&last_connection_change_));
 
@@ -113,16 +112,15 @@ class HistogramWatcher
     polling_interval_ = base::TimeDelta::FromSeconds(1);
   }
 
-  // NetworkChangeNotifier::DNSObserver implementation.
+  // DNSObserver implementation.
   virtual void OnDNSChanged() OVERRIDE {
     UMA_HISTOGRAM_MEDIUM_TIMES("NCN.DNSConfigChange",
                                SinceLast(&last_dns_change_));
   }
 
-  // NetworkChangeNotifier::NetworkChangeObserver implementation.
-  virtual void OnNetworkChanged(
-      NetworkChangeNotifier::ConnectionType type) OVERRIDE {
-    if (type != NetworkChangeNotifier::CONNECTION_NONE) {
+  // NetworkChangeObserver implementation.
+  virtual void OnNetworkChanged(ConnectionType type) OVERRIDE {
+    if (type != CONNECTION_NONE) {
       UMA_HISTOGRAM_MEDIUM_TIMES("NCN.NetworkOnlineChange",
                                  SinceLast(&last_network_change_));
     } else {
@@ -134,7 +132,7 @@ class HistogramWatcher
   // 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) {
-    if (last_connection_type_ != NetworkChangeNotifier::CONNECTION_NONE ||
+    if (last_connection_type_ != CONNECTION_NONE ||
         IsLocalhost(source.host()) ||
         !(source.SchemeIs("http") || source.SchemeIs("https"))) {
       return;
@@ -150,14 +148,12 @@ class HistogramWatcher
       polling_interval_ *= 2;
       last_polled_connection_ = current_time;
       base::TimeTicks started_get_connection_type = base::TimeTicks::Now();
-      last_polled_connection_type_ =
-          NetworkChangeNotifier::GetConnectionType();
+      last_polled_connection_type_ = GetConnectionType();
       UMA_HISTOGRAM_TIMES("NCN.GetConnectionTypeTime",
                           base::TimeTicks::Now() -
                           started_get_connection_type);
     }
-    if (last_polled_connection_type_ ==
-        NetworkChangeNotifier::CONNECTION_NONE) {
+    if (last_polled_connection_type_ == CONNECTION_NONE) {
       UMA_HISTOGRAM_MEDIUM_TIMES("NCN.PollingOfflineDataRecv",
                                  current_time - last_connection_change_);
     }
@@ -180,15 +176,15 @@ class HistogramWatcher
   // |polling_interval_| is initialized by |OnConnectionTypeChanged| on our
   // first transition to offline and on subsequent transitions.  Once offline,
   // |polling_interval_| doubles as offline data is received and we poll
-  // with |NetworkChangeNotifier::GetConnectionType| to verify the connection
+  // with |GetConnectionType| to verify the connection
   // state.
   base::TimeDelta polling_interval_;
   // |last_connection_type_| is the last value passed to
   // |OnConnectionTypeChanged|.
-  NetworkChangeNotifier::ConnectionType last_connection_type_;
+  ConnectionType last_connection_type_;
   // |last_polled_connection_type_| is last result from calling
-  // |NetworkChangeNotifier::GetConnectionType| in |NotifyDataReceived|.
-  NetworkChangeNotifier::ConnectionType last_polled_connection_type_;
+  // |GetConnectionType| in |NotifyDataReceived|.
+  ConnectionType last_polled_connection_type_;
   int32 offline_packets_received_;
 
   DISALLOW_COPY_AND_ASSIGN(HistogramWatcher);
@@ -228,11 +224,11 @@ class NetworkChangeNotifier::NetworkChangeCalculator
       : params_(params),
         have_announced_(false),
         last_announced_connection_type_(CONNECTION_NONE),
-        pending_connection_type_(CONNECTION_NONE) {}
+        pending_connection_type_initialized_(false) {}
 
   void Init() {
-    AddConnectionTypeObserver(this);
-    AddIPAddressObserver(this);
+    Deprecated::AddConnectionTypeObserver(this);
+    Deprecated::AddIPAddressObserver(this);
   }
 
   virtual ~NetworkChangeCalculator() {
@@ -242,6 +238,10 @@ class NetworkChangeNotifier::NetworkChangeCalculator
 
   // NetworkChangeNotifier::IPAddressObserver implementation.
   virtual void OnIPAddressChanged() OVERRIDE {
+    if (!pending_connection_type_initialized_) {
+      pending_connection_type_initialized_ = true;
+      pending_connection_type_ = GetConnectionType();
+    }
     base::TimeDelta delay = last_announced_connection_type_ == CONNECTION_NONE
         ? params_.ip_address_offline_delay_ : params_.ip_address_online_delay_;
     // Cancels any previous timer.
@@ -250,6 +250,7 @@ class NetworkChangeNotifier::NetworkChangeCalculator
 
   // NetworkChangeNotifier::ConnectionTypeObserver implementation.
   virtual void OnConnectionTypeChanged(ConnectionType type) OVERRIDE {
+    pending_connection_type_initialized_ = true;
     pending_connection_type_ = type;
     base::TimeDelta delay = last_announced_connection_type_ == CONNECTION_NONE
         ? params_.connection_type_offline_delay_
@@ -260,6 +261,7 @@ class NetworkChangeNotifier::NetworkChangeCalculator
 
  private:
   void Notify() {
+    DCHECK(pending_connection_type_initialized_);
     // Don't bother signaling about dead connections.
     if (have_announced_ &&
         (last_announced_connection_type_ == CONNECTION_NONE) &&
@@ -284,6 +286,8 @@ class NetworkChangeNotifier::NetworkChangeCalculator
   ConnectionType last_announced_connection_type_;
   // Value to pass to NotifyObserversOfNetworkChange when Notify is called.
   ConnectionType pending_connection_type_;
+  // Indicates if pending_connection_type_ has been initialized.
+  bool pending_connection_type_initialized_;
   // Used to delay notifications so duplicates can be combined.
   base::OneShotTimer<NetworkChangeCalculator> timer_;
 };
@@ -421,14 +425,17 @@ NetworkChangeNotifier* NetworkChangeNotifier::CreateMock() {
   return new MockNetworkChangeNotifier();
 }
 
-void NetworkChangeNotifier::AddIPAddressObserver(IPAddressObserver* observer) {
+void NetworkChangeNotifier::Deprecated::AddIPAddressObserver(
+    IPAddressObserver* observer) {
   if (g_network_change_notifier)
     g_network_change_notifier->ip_address_observer_list_->AddObserver(observer);
 }
 
-void NetworkChangeNotifier::AddConnectionTypeObserver(
+void NetworkChangeNotifier::Deprecated::AddConnectionTypeObserver(
     ConnectionTypeObserver* observer) {
   if (g_network_change_notifier) {
+    // AddObserver does nothing without a valid MessageLoop
+    DCHECK(MessageLoop::current());
     g_network_change_notifier->connection_type_observer_list_->AddObserver(
         observer);
   }
@@ -436,6 +443,8 @@ void NetworkChangeNotifier::AddConnectionTypeObserver(
 
 void NetworkChangeNotifier::AddDNSObserver(DNSObserver* observer) {
   if (g_network_change_notifier) {
+    // AddObserver does nothing without a valid MessageLoop
+    DCHECK(MessageLoop::current());
     g_network_change_notifier->resolver_state_observer_list_->AddObserver(
         observer);
   }
@@ -444,6 +453,8 @@ void NetworkChangeNotifier::AddDNSObserver(DNSObserver* observer) {
 void NetworkChangeNotifier::AddNetworkChangeObserver(
     NetworkChangeObserver* observer) {
   if (g_network_change_notifier) {
+    // AddObserver does nothing without a valid MessageLoop
+    DCHECK(MessageLoop::current());
     g_network_change_notifier->network_change_observer_list_->AddObserver(
         observer);
   }
@@ -562,4 +573,10 @@ NetworkChangeNotifier::DisableForTest::~DisableForTest() {
   g_network_change_notifier = network_change_notifier_;
 }
 
+void NetworkChangeNotifier::ResetNetworkChangeCalculatorParamsForTest(
+    NetworkChangeCalculatorParams params) {
+  network_change_calculator_.reset(new NetworkChangeCalculator(params));

[szym, 2013/01/20 06:52:08]

How thread-safe is this? It seems that this has to be called on the thread where
the NCN constructor was called. You should probably enforce that.

+  network_change_calculator_->Init();
+}
+
 }  // namespace net