Provide NetworkChangeNotifierAndroid with the actual initial connection type.

net/android/network_change_notifier_android_unittest.cc

Index: net/android/network_change_notifier_android_unittest.cc
diff --git a/net/android/network_change_notifier_android_unittest.cc b/net/android/network_change_notifier_android_unittest.cc
index b66d6474c025720fb9996e0a86d2eefad9ba0407..77acafa74e6912d18c1e517e840a62818355ac33 100644
--- a/net/android/network_change_notifier_android_unittest.cc
+++ b/net/android/network_change_notifier_android_unittest.cc
@@ -66,25 +66,27 @@ class BaseNetworkChangeNotifierAndroidTest : public testing::Test {
     EXPECT_EQ(NetworkChangeNotifier::CONNECTION_UNKNOWN,
               connection_type_getter.Run());
 
-    ForceConnectivityState(NetworkChangeNotifierDelegateAndroid::OFFLINE);
+    SetGlobalConnectivityStateAsOnline(false);
     EXPECT_EQ(1, times_connection_type_changed_callback.Run());
     EXPECT_EQ(NetworkChangeNotifier::CONNECTION_NONE,
               connection_type_getter.Run());
 
-    ForceConnectivityState(NetworkChangeNotifierDelegateAndroid::OFFLINE);
+    SetGlobalConnectivityStateAsOnline(false);
     EXPECT_EQ(1, times_connection_type_changed_callback.Run());
     EXPECT_EQ(NetworkChangeNotifier::CONNECTION_NONE,
               connection_type_getter.Run());
 
-    ForceConnectivityState(NetworkChangeNotifierDelegateAndroid::ONLINE);
+    SetGlobalConnectivityStateAsOnline(true);

[Ryan Sleevi, 2012/12/21 19:26:07]

nit: I actually found the original code cleaner, in this case. It's not
immediately obvious from reading this that the bool here is used to control the
online/offline state, and not some secondary argument.

While there is more typing involved, I think it's clearer. An alternative is
just two helper methods "SetOffline()" & "SetOnline" [the
'globalconnectivitystate' is a bit redundant/unnecessary]

[Philippe, 2012/12/26 10:33:25]

I chose the SetOnline/SetOffline alternative. I had made this change initially
after I moved the ConnectivityState enum to the class' private section. I
removed this enum now.

     EXPECT_EQ(2, times_connection_type_changed_callback.Run());
     EXPECT_EQ(NetworkChangeNotifier::CONNECTION_UNKNOWN,
               connection_type_getter.Run());
   }
 
-  void ForceConnectivityState(
-      NetworkChangeNotifierDelegateAndroid::ConnectivityState state) {
-    delegate_.ForceConnectivityState(state);
+  void SetGlobalConnectivityStateAsOnline(bool is_online) {
+    delegate_.ForceJavaSideConnectivityState(
+        is_online ?
+            NetworkChangeNotifierDelegateAndroid::ONLINE :
+            NetworkChangeNotifierDelegateAndroid::OFFLINE);
     // Note that this is needed because ObserverListThreadSafe uses PostTask().
     MessageLoop::current()->RunUntilIdle();
   }
@@ -92,6 +94,35 @@ class BaseNetworkChangeNotifierAndroidTest : public testing::Test {
   NetworkChangeNotifierDelegateAndroid delegate_;
 };
 
+// Tests that NetworkChangeNotifierDelegateAndroid is initialized with the
+// actual connection type rather than a hardcoded one (e.g.
+// CONNECTION_UNKNOWN). Initializing the connection type to CONNECTION_UNKNOWN
+// and relying on the first network change notification to set it correctly can
+// be problematic in case there is a long delay between the delegate's
+// construction and the notification.
+// This addresses http://crbug.com/166883.

[Ryan Sleevi, 2012/12/21 19:26:07]

I'd drop this comment. It'd be one thing if it was some highly specific
regression test, but this isn't - it's testing that Android implements the same
guarantees that (Linux, Mac, Win) do - which is that the initial state is
immediately acquired, rather than waiting for the first transition state.

[Philippe, 2012/12/26 10:33:25]

Done.

+TEST_F(BaseNetworkChangeNotifierAndroidTest,
+       DelegateIsInitializedWithCurrentConnectionType) {
+  SetGlobalConnectivityStateAsOnline(true);
+  ASSERT_EQ(NetworkChangeNotifier::CONNECTION_UNKNOWN,
+            delegate_.GetCurrentConnectionType());

[Ryan Sleevi, 2012/12/21 19:26:07]

Just from a naieve reading, these two lines combined with the comments
explaining the test (line 97-99) are quite confusing.

You're wanting to test that it's initialized with the right state, but then you
immediately test CONNECT_UNKNOWN after initializing, which you suggest is the
wrong state.

Hopefully you see the confusion here.

[Philippe, 2012/12/26 10:33:25]

I hope it's less confusing now.

+  // Instantiate another delegate to validate that it uses the actual
+  // connection type at construction.
+  scoped_ptr<NetworkChangeNotifierDelegateAndroid> other_delegate(
+      new NetworkChangeNotifierDelegateAndroid());
+  EXPECT_EQ(NetworkChangeNotifier::CONNECTION_UNKNOWN,
+            other_delegate->GetCurrentConnectionType());
+
+  // Toggle the global connectivity state and instantiate another delegate
+  // again.
+  SetGlobalConnectivityStateAsOnline(false);
+  ASSERT_EQ(NetworkChangeNotifier::CONNECTION_NONE,
+            delegate_.GetCurrentConnectionType());
+  other_delegate.reset(new NetworkChangeNotifierDelegateAndroid());
+  EXPECT_EQ(NetworkChangeNotifier::CONNECTION_NONE,
+            other_delegate->GetCurrentConnectionType());
+}
+
 class NetworkChangeNotifierDelegateAndroidTest
     : public BaseNetworkChangeNotifierAndroidTest {
  protected: