ResourceRequestAllowedNotifier didn't work as expected

Following problems will be fixed by this change.
1) OnResourceRequestsAllowed() was invoked even if network was offline
2) ResourceRequestsAllowed() returned true even though network was offline
3) OnResourceRequestsAllowed() was not invoked even if network went online
See also http://crbug.com/249607 for each scenario in details.

BUG=249607
TEST=unit_tests --gtest_filter='ResourceRequestAllowedNotifierTest.*'

Review URL: https://chromiumcodereview.appspot.com/16841020

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@207199 0039d316-1c4b-4281-b951-d872f2087c98

4 files changed, 65 insertions, 21 deletions

diff --git a/chrome/browser/web_resource/resource_request_allowed_notifier.cc b/chrome/browser/web_resource/resource_request_allowed_notifier.cc
index 3650944..269c299 100644
--- a/chrome/browser/web_resource/resource_request_allowed_notifier.cc
+++ b/chrome/browser/web_resource/resource_request_allowed_notifier.cc
@@ -45,8 +45,9 @@ bool ResourceRequestAllowedNotifier::ResourceRequestsAllowed() {
   // The observer requested permission. Return the current criteria state and
   // set a flag to remind this class to notify the observer once the criteria
   // is met.
-  observer_requested_permission_ = true;
-  return !waiting_for_user_to_accept_eula_ && !waiting_for_network_;
+  observer_requested_permission_ = waiting_for_user_to_accept_eula_ ||
+                                   waiting_for_network_;
+  return !observer_requested_permission_;
 }
 
 void ResourceRequestAllowedNotifier::SetWaitingForNetworkForTesting(
@@ -59,6 +60,11 @@ void ResourceRequestAllowedNotifier::SetWaitingForEulaForTesting(
   waiting_for_user_to_accept_eula_ = waiting;
 }
 
+void ResourceRequestAllowedNotifier::SetObserverRequestedForTesting(
+    bool requested) {
+  observer_requested_permission_ = requested;
+}
+
 void ResourceRequestAllowedNotifier::MaybeNotifyObserver() {
   // Need to ensure that all criteria are met before notifying observers.
   if (observer_requested_permission_ && ResourceRequestsAllowed()) {
@@ -96,5 +102,9 @@ void ResourceRequestAllowedNotifier::OnConnectionTypeChanged(
     waiting_for_network_ = false;
     DVLOG(1) << "Network came back online.";
     MaybeNotifyObserver();
+  } else if (!waiting_for_network_ &&
+             type == net::NetworkChangeNotifier::CONNECTION_NONE) {
+    waiting_for_network_ = true;
+    DVLOG(1) << "Network went offline.";
   }
 }
diff --git a/chrome/browser/web_resource/resource_request_allowed_notifier.h b/chrome/browser/web_resource/resource_request_allowed_notifier.h
index 90d3df0..c28a9a0b 100644
--- a/chrome/browser/web_resource/resource_request_allowed_notifier.h
+++ b/chrome/browser/web_resource/resource_request_allowed_notifier.h
@@ -55,6 +55,7 @@ class ResourceRequestAllowedNotifier
 
   void SetWaitingForNetworkForTesting(bool waiting);
   void SetWaitingForEulaForTesting(bool waiting);
+  void SetObserverRequestedForTesting(bool requested);
 
  protected:
   // Notifies the observer if all criteria needed for resource requests are met.
diff --git a/chrome/browser/web_resource/resource_request_allowed_notifier_test_util.cc b/chrome/browser/web_resource/resource_request_allowed_notifier_test_util.cc
index 073c4bc..2845c53 100644
--- a/chrome/browser/web_resource/resource_request_allowed_notifier_test_util.cc
+++ b/chrome/browser/web_resource/resource_request_allowed_notifier_test_util.cc
@@ -25,22 +25,19 @@ void TestRequestAllowedNotifier::SetRequestsAllowedOverride(bool allowed) {
 }
 
 void TestRequestAllowedNotifier::NotifyObserver() {
-  // Force the allowed state to true. This forces MaybeNotifyObserver to always
-  // notify observers, as MaybeNotifyObserver checks ResourceRequestsAllowed.
+  // Force the allowed state and requested state to true. This forces
+  // MaybeNotifyObserver to always notify observers, as MaybeNotifyObserver
+  // checks ResourceRequestsAllowed and requested state.
   override_requests_allowed_ = true;
   requests_allowed_ = true;
+  SetObserverRequestedForTesting(true);
   MaybeNotifyObserver();
 }
 
 bool TestRequestAllowedNotifier::ResourceRequestsAllowed() {
-  // Call ResourceRequestAllowedNotifier::ResourceRequestsAllowed once to
-  // simulate that the user requested permission. Only return that result if
-  // the override flag was set.
-  bool requests_allowed =
-      ResourceRequestAllowedNotifier::ResourceRequestsAllowed();
   if (override_requests_allowed_)
     return requests_allowed_;
-  return requests_allowed;
+  return ResourceRequestAllowedNotifier::ResourceRequestsAllowed();
 }
 
 EulaAcceptedNotifier* TestRequestAllowedNotifier::CreateEulaNotifier() {
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 1c18b42..c8f8e034 100644
--- a/chrome/browser/web_resource/resource_request_allowed_notifier_unittest.cc
+++ b/chrome/browser/web_resource/resource_request_allowed_notifier_unittest.cc
@@ -103,9 +103,11 @@ class ResourceRequestAllowedNotifierTest
     network_notifier.SimulateNetworkConnectionChange(type);
   }
 
-  // Simulate a resource request from the test service.
-  void SimulateResourceRequest() {
-    resource_request_allowed_notifier_.ResourceRequestsAllowed();
+  // Simulate a resource request from the test service. It returns true if
+  // resource request is allowed. Otherwise returns false and will change the
+  // result of was_notified() to true when the request is allowed.
+  bool SimulateResourceRequest() {
+    return resource_request_allowed_notifier_.ResourceRequestsAllowed();
   }
 
   void SimulateEulaAccepted() {
@@ -152,31 +154,31 @@ class ResourceRequestAllowedNotifierTest
 };
 
 TEST_F(ResourceRequestAllowedNotifierTest, DoNotNotifyIfOffline) {
-  SimulateResourceRequest();
   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) {
-  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());
@@ -192,8 +194,10 @@ TEST_F(ResourceRequestAllowedNotifierTest, NoNotifyOnWardriving) {
 }
 
 TEST_F(ResourceRequestAllowedNotifierTest, NoNotifyOnFlakyConnection) {
-  SimulateResourceRequest();
+  // SimulateResourceRequest() returns true because network is online.
   SetWaitingForNetwork(false);
+  EXPECT_TRUE(SimulateResourceRequest());
+  // The callback is nerver invoked whatever happens on network connection.
   SimulateNetworkConnectionChange(
       net::NetworkChangeNotifier::CONNECTION_WIFI);
   EXPECT_FALSE(was_notified());
@@ -205,6 +209,38 @@ TEST_F(ResourceRequestAllowedNotifierTest, NoNotifyOnFlakyConnection) {
   EXPECT_FALSE(was_notified());
 }
 
+TEST_F(ResourceRequestAllowedNotifierTest, NotifyOnFlakyConnection) {
+  SetWaitingForNetwork(false);
+  EXPECT_TRUE(SimulateResourceRequest());
+  // Network goes online, but not notified because SimulateResourceRequest()
+  // returns true before.
+  SimulateNetworkConnectionChange(
+      net::NetworkChangeNotifier::CONNECTION_WIFI);
+  EXPECT_FALSE(was_notified());
+  SimulateNetworkConnectionChange(
+      net::NetworkChangeNotifier::CONNECTION_NONE);
+  EXPECT_FALSE(SimulateResourceRequest());
+  // Now, SimulateResourceRequest() returns false and will be notified later.
+  EXPECT_FALSE(was_notified());
+  SimulateNetworkConnectionChange(
+      net::NetworkChangeNotifier::CONNECTION_WIFI);
+  EXPECT_TRUE(was_notified());
+}
+
+TEST_F(ResourceRequestAllowedNotifierTest, NoNotifyOnEulaAfterGoOffline) {
+  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());
+}
+
 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
@@ -216,16 +252,16 @@ TEST_F(ResourceRequestAllowedNotifierTest, NoRequestNoNotify) {
 }
 
 TEST_F(ResourceRequestAllowedNotifierTest, EulaOnlyNetworkOffline) {
-  SimulateResourceRequest();
   DisableEulaAndNetwork();
+  EXPECT_FALSE(SimulateResourceRequest());
 
   SimulateEulaAccepted();
   EXPECT_FALSE(was_notified());
 }
 
 TEST_F(ResourceRequestAllowedNotifierTest, EulaFirst) {
-  SimulateResourceRequest();
   DisableEulaAndNetwork();
+  EXPECT_FALSE(SimulateResourceRequest());
 
   SimulateEulaAccepted();
   EXPECT_FALSE(was_notified());
@@ -236,8 +272,8 @@ TEST_F(ResourceRequestAllowedNotifierTest, EulaFirst) {
 }
 
 TEST_F(ResourceRequestAllowedNotifierTest, NetworkFirst) {
-  SimulateResourceRequest();
   DisableEulaAndNetwork();
+  EXPECT_FALSE(SimulateResourceRequest());
 
   SimulateNetworkConnectionChange(
       net::NetworkChangeNotifier::CONNECTION_WIFI);