ResourceRequestAllowedNotifier didn't work as expected

chrome/browser/web_resource/resource_request_allowed_notifier_test_util.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 "chrome/browser/web_resource/resource_request_allowed_notifier_test_util.h"
 
 TestRequestAllowedNotifier::TestRequestAllowedNotifier()
     : override_requests_allowed_(false),
       requests_allowed_(true) {
 }
@@ skipping 11 matching lines @@
 void TestRequestAllowedNotifier::SetRequestsAllowedOverride(bool allowed) {
   override_requests_allowed_ = true;
   requests_allowed_ = allowed;
 }
 
 void TestRequestAllowedNotifier::NotifyObserver() {
   // Force the allowed state to true. This forces MaybeNotifyObserver to always
   // 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();
 }
 
 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() {
   return test_eula_notifier_.release();
 }