ResourceRequestAllowedNotifier didn't work as expected

chrome/browser/web_resource/resource_request_allowed_notifier_unittest.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "base/prefs/testing_pref_service.h"
 #include "chrome/browser/web_resource/eula_accepted_notifier.h"
 #include "chrome/browser/web_resource/resource_request_allowed_notifier_test_util.h"
 #include "chrome/common/chrome_notification_types.h"
 #include "chrome/test/base/testing_browser_process.h"
 #include "content/public/browser/notification_service.h"
@@ skipping 85 matching lines @@
   // Network manipulation methods:
   void SetWaitingForNetwork(bool waiting) {
     resource_request_allowed_notifier_.SetWaitingForNetworkForTesting(waiting);
   }
 
   void SimulateNetworkConnectionChange(
       net::NetworkChangeNotifier::ConnectionType type) {
     network_notifier.SimulateNetworkConnectionChange(type);
   }
 
   // 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() {
     eula_notifier_->SimulateEulaAccepted();
   }
 
   // Eula manipulation methods:
   void SetNeedsEulaAcceptance(bool needs_acceptance) {
     eula_notifier_->SetEulaAcceptedForTesting(!needs_acceptance);
   }
@@ skipping 26 matching lines @@
   content::TestBrowserThread ui_thread;
   TestNetworkChangeNotifier network_notifier;
   TestRequestAllowedNotifier resource_request_allowed_notifier_;
   TestEulaAcceptedNotifier* eula_notifier_;  // Weak, owned by RRAN.
   bool was_notified_;
 
   DISALLOW_COPY_AND_ASSIGN(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());
   SimulateNetworkConnectionChange(
       net::NetworkChangeNotifier::CONNECTION_3G);
   EXPECT_FALSE(was_notified());
   SimulateNetworkConnectionChange(
       net::NetworkChangeNotifier::CONNECTION_4G);
   EXPECT_FALSE(was_notified());
   SimulateNetworkConnectionChange(
       net::NetworkChangeNotifier::CONNECTION_WIFI);
   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());
-  SimulateNetworkConnectionChange(
-      net::NetworkChangeNotifier::CONNECTION_WIFI);
+  SimulateEulaAccepted();
   EXPECT_FALSE(was_notified());
 }
 
 TEST_F(ResourceRequestAllowedNotifierTest, NoRequestNoNotify) {
   // Ensure that if the observing service does not request access, it does not
   // get notified, even if the criteria is met. Note that this is done by not
   // calling SimulateResourceRequest here.
   SetWaitingForNetwork(true);
   SimulateNetworkConnectionChange(
       net::NetworkChangeNotifier::CONNECTION_ETHERNET);
   EXPECT_FALSE(was_notified());
 }
 
 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());
 
   SimulateNetworkConnectionChange(
       net::NetworkChangeNotifier::CONNECTION_WIFI);
   EXPECT_TRUE(was_notified());
 }
 
 TEST_F(ResourceRequestAllowedNotifierTest, NetworkFirst) {
-  SimulateResourceRequest();
   DisableEulaAndNetwork();
+  EXPECT_FALSE(SimulateResourceRequest());
 
   SimulateNetworkConnectionChange(
       net::NetworkChangeNotifier::CONNECTION_WIFI);
   EXPECT_FALSE(was_notified());
 
   SimulateEulaAccepted();
   EXPECT_TRUE(was_notified());
 }
 
 TEST_F(ResourceRequestAllowedNotifierTest, NoRequestNoNotifyEula) {
   // Ensure that if the observing service does not request access, it does not
   // get notified, even if the criteria is met. Note that this is done by not
   // calling SimulateResourceRequest here.
   DisableEulaAndNetwork();
 
   SimulateNetworkConnectionChange(
       net::NetworkChangeNotifier::CONNECTION_WIFI);
   EXPECT_FALSE(was_notified());
 
   SimulateEulaAccepted();
   EXPECT_FALSE(was_notified());
 }