Fix initial offline state response handling

net/base/network_change_notifier_linux.cc

Index: net/base/network_change_notifier_linux.cc
diff --git a/net/base/network_change_notifier_linux.cc b/net/base/network_change_notifier_linux.cc
index 95f54e24e5070e32ec0e06ef24fc2292c18e1c32..fbefffb70558ce6aa9e62bd32dc151aa5f01bff3 100644
--- a/net/base/network_change_notifier_linux.cc
+++ b/net/base/network_change_notifier_linux.cc
@@ -88,16 +88,8 @@ class NetworkManagerApi {
 
  private:
   // Callbacks for D-Bus API.
-  void OnStateChanged(dbus::Message* message);
-
-  void OnResponse(dbus::Response* response) {
-    OnStateChanged(response);
-    offline_state_initialized_.Signal();
-  }
-
-  void OnSignaled(dbus::Signal* signal) {
-    OnStateChanged(signal);
-  }
+  void OnResponse(dbus::Response* response);
+  void OnSignaled(dbus::Signal* signal);
 
   void OnConnected(const std::string&, const std::string&, bool success) {
     if (!success) {
@@ -162,17 +154,35 @@ void NetworkManagerApi::CleanUp() {
   ptr_factory_.InvalidateWeakPtrs();
 }
 
-void NetworkManagerApi::OnStateChanged(dbus::Message* message) {
+void NetworkManagerApi::OnResponse(dbus::Response* response) {
   DCHECK_EQ(helper_thread_id_, base::PlatformThread::CurrentId());
-  if (!message) {
+  if (!response) {
     DLOG(WARNING) << "No response received for initial state request";
+    offline_state_initialized_.Signal();
     return;
   }
-  dbus::MessageReader reader(message);
+  dbus::MessageReader reader(response);
   uint32 state = 0;
-  if (!reader.HasMoreData() || !reader.PopUint32(&state)) {
+  if (!reader.HasMoreData() || !reader.PopVariantOfUint32(&state)) {

[satorux1, 2011/11/17 17:40:15]

You don't need !reader.HasMoreData(). PopVariantOfUint32() will return false, if
the content is empty.

[adamk, 2011/11/17 18:58:24]

Good to know, done.

     DLOG(WARNING) << "Unexpected response for NetworkManager State request: "
-                  << message->ToString();
+                  << response->ToString();
+    offline_state_initialized_.Signal();
+    return;
+  }
+  {
+    base::AutoLock lock(is_offline_lock_);
+    is_offline_ = StateIsOffline(state);
+  }
+  offline_state_initialized_.Signal();

[satorux1, 2011/11/17 17:40:15]

Calling offline_state_initialized_.Signal() from three places looks a bit
error-prone. You might want to introduce OnResponseInternal() to do most of the
stuff, and then change the function to be

  OnResponseInternal(response);
  offline_state_initialized_.Signal();

[adamk, 2011/11/17 18:58:24]

Yeah, I agree; this is covered by the unittests, but if someone were to add
another exit point it'd be bad.  I've renamed the callback to OnInitialResponse
and the parser to HandleResponse.

+}
+
+void NetworkManagerApi::OnSignaled(dbus::Signal* signal) {
+  DCHECK_EQ(helper_thread_id_, base::PlatformThread::CurrentId());
+  dbus::MessageReader reader(signal);
+  uint32 state = 0;
+  if (!reader.HasMoreData() || !reader.PopUint32(&state)) {

[satorux1, 2011/11/17 17:40:15]

You can remove !reader.HasMoreData() from here too.

[adamk, 2011/11/17 18:58:24]

Done.

+    DLOG(WARNING) << "Unexpected signal for NetworkManager StateChanged: "
+                  << signal->ToString();
     return;
   }
   bool new_is_offline = StateIsOffline(state);
@@ -183,8 +193,7 @@ void NetworkManagerApi::OnStateChanged(dbus::Message* message) {
     else
       return;
   }
-  if (offline_state_initialized_.IsSignaled())
-    notification_callback_.Run();
+  notification_callback_.Run();
 }
 
 bool NetworkManagerApi::StateIsOffline(uint32 state) {