ResourceRequestAllowedNotifier didn't work as expected

chrome/browser/web_resource/resource_request_allowed_notifier_unittest.cc

Index: chrome/browser/web_resource/resource_request_allowed_notifier_unittest.cc
diff --git a/chrome/browser/web_resource/resource_request_allowed_notifier_unittest.cc b/chrome/browser/web_resource/resource_request_allowed_notifier_unittest.cc
index 1c18b42458e3bc6053c1f5a17d6f345aa1a0ac27..0bf618a29edf23ad8b91103a9d091ccbd6a07a02 100644
--- a/chrome/browser/web_resource/resource_request_allowed_notifier_unittest.cc
+++ b/chrome/browser/web_resource/resource_request_allowed_notifier_unittest.cc
@@ -104,8 +104,8 @@ class ResourceRequestAllowedNotifierTest
   }
 
   // Simulate a resource request from the test service.

[Alexei Svitkine (slow), 2013/06/14 16:47:59]

Nit: Update comment to explain return value.

[Takashi Toyoshima, 2013/06/17 07:04:40]

Done.

-  void SimulateResourceRequest() {
-    resource_request_allowed_notifier_.ResourceRequestsAllowed();
+  bool SimulateResourceRequest() {
+    return resource_request_allowed_notifier_.ResourceRequestsAllowed();
   }
 
   void SimulateEulaAccepted() {
@@ -152,31 +152,31 @@ class ResourceRequestAllowedNotifierTest
 };
 
 TEST_F(ResourceRequestAllowedNotifierTest, DoNotNotifyIfOffline) {
-  SimulateResourceRequest();

[Takashi Toyoshima, 2013/06/14 08:27:29]

In this order, ResourceRequest always return true internally, and never
notified. We should change network and EULA conditions before calling it.

   SetWaitingForNetwork(true);
+  EXPECT_FALSE(SimulateResourceRequest());
   SimulateNetworkConnectionChange(net::NetworkChangeNotifier::CONNECTION_NONE);
   EXPECT_FALSE(was_notified());
 }
 
 TEST_F(ResourceRequestAllowedNotifierTest, DoNotNotifyIfOnlineToOnline) {
-  SimulateResourceRequest();
   SetWaitingForNetwork(false);
+  EXPECT_TRUE(SimulateResourceRequest());
   SimulateNetworkConnectionChange(
       net::NetworkChangeNotifier::CONNECTION_ETHERNET);
   EXPECT_FALSE(was_notified());
 }
 
 TEST_F(ResourceRequestAllowedNotifierTest, NotifyOnReconnect) {

[Takashi Toyoshima, 2013/06/14 08:27:29]

This test is problem 2) of crbug.
But doesn't work correctly because SetWaitingForNetwork is called after
SimulateResourceRequest().
Revised test doesn't work without my patch.

-  SimulateResourceRequest();
   SetWaitingForNetwork(true);
+  EXPECT_FALSE(SimulateResourceRequest());
   SimulateNetworkConnectionChange(
       net::NetworkChangeNotifier::CONNECTION_ETHERNET);
   EXPECT_TRUE(was_notified());
 }
 
 TEST_F(ResourceRequestAllowedNotifierTest, NoNotifyOnWardriving) {
-  SimulateResourceRequest();
   SetWaitingForNetwork(false);
+  EXPECT_TRUE(SimulateResourceRequest());
   SimulateNetworkConnectionChange(
       net::NetworkChangeNotifier::CONNECTION_WIFI);
   EXPECT_FALSE(was_notified());
@@ -191,17 +191,32 @@ TEST_F(ResourceRequestAllowedNotifierTest, NoNotifyOnWardriving) {
   EXPECT_FALSE(was_notified());
 }
 
-TEST_F(ResourceRequestAllowedNotifierTest, NoNotifyOnFlakyConnection) {
-  SimulateResourceRequest();
+TEST_F(ResourceRequestAllowedNotifierTest, NotifyOnFlakyConnection) {

[Alexei Svitkine (slow), 2013/06/14 16:47:59]

This test makes sense.

I think it's different from the old test, which we should also have.

The old test was testing the case where we get true from
SimulateResourceRequest() first, so that we're not expecting a notification. And
then the connection toggles back and forth (without another
SimulateResourceRequest() in between), and this toggling doesn't generate any
notifications (since we weren't waiting for any). Can you re-add such a test?

[Takashi Toyoshima, 2013/06/17 07:04:40]

Oh, I see.
I'll leave original one separately.

   SetWaitingForNetwork(false);
+  EXPECT_TRUE(SimulateResourceRequest());
   SimulateNetworkConnectionChange(
       net::NetworkChangeNotifier::CONNECTION_WIFI);
   EXPECT_FALSE(was_notified());

[Takashi Toyoshima, 2013/06/14 08:27:29]

Network goes online, but not notified because SimulateResourceRequest() returns
true before.

[Alexei Svitkine (slow), 2013/06/14 16:47:59]

Add it as a comment in the test.

[Takashi Toyoshima, 2013/06/17 07:04:40]

Done.

   SimulateNetworkConnectionChange(
       net::NetworkChangeNotifier::CONNECTION_NONE);
+  EXPECT_FALSE(SimulateResourceRequest());

[Takashi Toyoshima, 2013/06/14 08:27:29]

Now, SimulateResourceRequest() returns false and will be notified later.

[Alexei Svitkine (slow), 2013/06/14 16:47:59]

Add it as a comment in the test.

[Takashi Toyoshima, 2013/06/17 07:04:40]

Done.

   EXPECT_FALSE(was_notified());
   SimulateNetworkConnectionChange(
       net::NetworkChangeNotifier::CONNECTION_WIFI);
+  EXPECT_TRUE(was_notified());

[Takashi Toyoshima, 2013/06/14 08:27:29]

Yep, this time, it must be notified.

+}
+
+TEST_F(ResourceRequestAllowedNotifierTest, NoNotifyOnEulaAfterGoOffline) {

[Takashi Toyoshima, 2013/06/14 08:27:29]

This test is problem 1) of crbug.

+  DisableEulaAndNetwork();
+  EXPECT_FALSE(SimulateResourceRequest());
+
+  SimulateNetworkConnectionChange(
+      net::NetworkChangeNotifier::CONNECTION_WIFI);
+  EXPECT_FALSE(was_notified());
+  SimulateNetworkConnectionChange(
+      net::NetworkChangeNotifier::CONNECTION_NONE);
+  EXPECT_FALSE(was_notified());
+  SimulateEulaAccepted();
   EXPECT_FALSE(was_notified());
 }
 
@@ -216,16 +231,16 @@ TEST_F(ResourceRequestAllowedNotifierTest, NoRequestNoNotify) {
 }
 
 TEST_F(ResourceRequestAllowedNotifierTest, EulaOnlyNetworkOffline) {
-  SimulateResourceRequest();
   DisableEulaAndNetwork();
+  EXPECT_FALSE(SimulateResourceRequest());
 
   SimulateEulaAccepted();
   EXPECT_FALSE(was_notified());
 }
 
 TEST_F(ResourceRequestAllowedNotifierTest, EulaFirst) {

[Takashi Toyoshima, 2013/06/14 08:27:29]

This test is problem 3) of crbug.
Also doesn't work without my patch.

-  SimulateResourceRequest();
   DisableEulaAndNetwork();
+  EXPECT_FALSE(SimulateResourceRequest());
 
   SimulateEulaAccepted();
   EXPECT_FALSE(was_notified());
@@ -236,8 +251,8 @@ TEST_F(ResourceRequestAllowedNotifierTest, EulaFirst) {
 }
 
 TEST_F(ResourceRequestAllowedNotifierTest, NetworkFirst) {
-  SimulateResourceRequest();
   DisableEulaAndNetwork();
+  EXPECT_FALSE(SimulateResourceRequest());
 
   SimulateNetworkConnectionChange(
       net::NetworkChangeNotifier::CONNECTION_WIFI);