ResourceRequestAllowedNotifier didn't work as expected

chrome/browser/web_resource/resource_request_allowed_notifier_test_util.cc

Index: chrome/browser/web_resource/resource_request_allowed_notifier_test_util.cc
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 073c4bcffa38c913db5ce2dcddce22a0a2e2cb66..1b76afe0a2c02d3bc5441c2313c3432f0d42051f 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
@@ -29,6 +29,9 @@ void TestRequestAllowedNotifier::NotifyObserver() {
   // notify observers, as MaybeNotifyObserver checks ResourceRequestsAllowed.
   override_requests_allowed_ = true;
   requests_allowed_ = true;
+  SetWaitingForNetworkForTesting(true);
+  ResourceRequestAllowedNotifier::ResourceRequestsAllowed();
+  SetWaitingForNetworkForTesting(false);

[Alexei Svitkine (slow), 2013/06/17 17:36:13]

So I think ResourceRequestsAllowed() should already be returning true due to
override_requests_allowed_ here.

So I'm guessing the only reason you're calling the base class version is to make
sure observer_requested_permission_ gets set (because with your change below, it
won't get called anymore).

If you revert the change below, then this change won't be necessary.
Alternatively, make a *ForTesting method to set override_requests_allowed_.

[Takashi Toyoshima, 2013/06/18 07:21:05]

OK, I'll add ForTesting method.

   MaybeNotifyObserver();
 }
 
@@ -36,11 +39,9 @@ bool TestRequestAllowedNotifier::ResourceRequestsAllowed() {
   // Call ResourceRequestAllowedNotifier::ResourceRequestsAllowed once to
   // simulate that the user requested permission. Only return that result if
   // the override flag was set.

[Alexei Svitkine (slow), 2013/06/17 17:36:13]

With this change, this comment becomes incorrect.

I'm not sure it makes sense to change the semantics of this. given that current
callers may be relying on the underlying one being set?

Could you explain why this is necessary? What happens that causes the test to
fail?

[Takashi Toyoshima, 2013/06/18 07:21:05]

I fixed this function because it had two problems.

1) Even if it calls ResourceRequestsAllowed(), MaybeNotifyObserver() doesn't
notify if the ResourceRequestsAllowed() returns true here.
This is the failed test scenario and was hidden by the bug I fixed in this
change.

2) When both override_requests_allowed_ and  requests_allowed_ are set to true,
and ResourceRequestAllowedNotifier::ResourceRequestsAllowed() returns false
here, it invokes the callback later even though this function returns true. This
was inconsistent and a potential bug.

-  bool requests_allowed =
-      ResourceRequestAllowedNotifier::ResourceRequestsAllowed();
   if (override_requests_allowed_)
     return requests_allowed_;
-  return requests_allowed;
+  return ResourceRequestAllowedNotifier::ResourceRequestsAllowed();
 }
 
 EulaAcceptedNotifier* TestRequestAllowedNotifier::CreateEulaNotifier() {