Allow asynchronous deferral in NavigationSimulator

content/public/test/navigation_simulator.h

Index: content/public/test/navigation_simulator.h
diff --git a/content/public/test/navigation_simulator.h b/content/public/test/navigation_simulator.h
index 8c37ab7a34c3b1f147a1bec853128ee82a81a79c..2314a80819d2cfb8be191612f42dcb16a98a81fe 100644
--- a/content/public/test/navigation_simulator.h
+++ b/content/public/test/navigation_simulator.h
@@ -7,6 +7,9 @@
 
 #include <memory>
 
+#include "base/callback.h"
+#include "base/optional.h"
+#include "content/public/browser/navigation_throttle.h"
 #include "content/public/browser/web_contents_observer.h"
 #include "content/public/common/referrer.h"
 #include "content/public/test/navigation_simulator.h"
@@ -21,6 +24,10 @@ class RenderFrameHost;
 class TestRenderFrameHost;
 struct Referrer;
 
+// To avoid DEPS violation by bringing in navigation_handle_impl.h.
+using ThrottleChecksFinishedCallback =
+    base::Callback<void(NavigationThrottle::ThrottleCheckResult)>;
+
 // An interface for simulating a navigation in unit tests. Currently this only
 // supports renderer-initiated navigations.
 // Note: this should not be used in browser tests.
@@ -64,19 +71,25 @@ class NavigationSimulator : public WebContentsObserver {
   //       NavigationSimulator::CreateRendererInitiated(
   //           original_url, render_frame_host);
   //   simulator->SetTransition(ui::PAGE_TRANSITION_LINK);
-  //   simulator->Start();
-  //   for (GURL redirect_url : redirects)
-  //     simulator->Redirect(redirect_url);
-  //   simulator->Commit();
+  //   if (simulator->Start() == NavigationThrottle::DEFER)

[clamy, 2017/03/07 15:18:14]

So I had a chat with nasko, and we agreed that having all callers need to check
the return value of Start, etc.. was not something we wanted to have.

Instead we suggest to have two kind of methods: Start and StartAndExpectDeferral
(same with Redirect, etc... as neded). This way users who don't care about
running another TaskRunner in the background will just called the simple Start
method, while more advanced users can use StartAndExpectDeferral.

Then we'd have a somewhat generic method called Resume that people can call to
have whatever was deferred resume. I'm not quite sure on whether it should
return the precise throttle check, or we should just query it from the
NavigationSimulator, probably the latter.

Does that make sense to you?

[Charlie Harrison, 2017/03/07 15:23:33]

Yes this API design sounds good to me. I agree checking each method is tedious
and maybe the wrong abstraction level for a lot of tests.

Regarding "just query it [the throttle check] from the NavigationSimulator",
were you thinking of an API like:

simulator->StartAndExpectDeferral()
<dostuff>
simulator->Resume()
EXPECT_EQ(NavigationThrottle::CANCEL, simulator->GetLastThrottleCheckResult());

[clamy, 2017/03/07 15:25:12]

Yes. That seems quite easy to use IMO.

+  //     simulator->WaitForThrottleChecksComplete();
+  //   for (GURL redirect_url : redirects) {
+  //     EXPECT_EQ(NavigationThrottle::PROCEED,
+  //               simulator->Redirect(redirect_url));
+  //   }
+  //   EXPECT_EQ(NavigationThrottle::PROCEED, simulator->Commit());
   //
   // Example of usage for a failed renderer-initiated navigation:
   //   unique_ptr<NavigationSimulator> simulator =
   //       NavigationSimulator::CreateRendererInitiated(
   //           original_url, render_frame_host);
   //   simulator->SetTransition(ui::PAGE_TRANSITION_LINK);
-  //   simulator->Start();
-  //   for (GURL redirect_url : redirects)
-  //     simulator->Redirect(redirect_url);
+  //   EXPECT_EQ(NavigationThrottle::PROCEED, simulator->Start());
+  //   for (GURL redirect_url : redirects) {
+  //     EXPECT_EQ(NavigationThrottle::DEFER,
+  //               simulator->Redirect(redirect_url));
+  //     simulator->WaitForThrottleChecksComplete();
+  //   }
   //   simulator->Fail(net::ERR_TIMED_OUT);
   //   simulator->CommitErrorPage();
   //
@@ -87,13 +100,13 @@ class NavigationSimulator : public WebContentsObserver {
   //   simulator->CommitSamePage();
 
   // Simulates the start of the navigation.
-  virtual void Start();
+  virtual NavigationThrottle::ThrottleCheckResult Start();
 
   // Simulates a redirect to |new_url| for the navigation.
-  virtual void Redirect(const GURL& new_url);
+  virtual NavigationThrottle::ThrottleCheckResult Redirect(const GURL& new_url);
 
   // Simulates the commit of the navigation in the RenderFrameHost.
-  virtual void Commit();
+  virtual NavigationThrottle::ThrottleCheckResult Commit();
 
   // Simulates the navigation failing with the error code |error_code|.
   virtual void Fail(int error_code);
@@ -122,6 +135,10 @@ class NavigationSimulator : public WebContentsObserver {
   // |Redirect|.
   virtual void SetReferrer(const Referrer& referrer);
 
+  // This method will block waiting for throttle checks to complete.
+  virtual NavigationThrottle::ThrottleCheckResult
+  WaitForThrottleChecksComplete();
+
  private:
   // WebContentsObserver:
   void DidStartNavigation(NavigationHandle* navigation_handle) override;
@@ -133,6 +150,28 @@ class NavigationSimulator : public WebContentsObserver {
   void OnWillRedirectRequest();
   void OnWillProcessResponse();
 
+  // Called after all the throttle checks are finished for these steps.
+  void FinishStart(NavigationThrottle::ThrottleCheckResult result);
+  void FinishRedirect(int previous_num_will_redirect_request_called,
+                      int previous_did_redirect_navigation_called,
+                      NavigationThrottle::ThrottleCheckResult result);
+  void FinishCommit(NavigationThrottle::ThrottleCheckResult result);
+
+  // Returns the sync result of the throttle checks if it is available, or DEFER
+  // if it is not.
+  NavigationThrottle::ThrottleCheckResult RunOrStoreThrottleCompleteCallback(
+      ThrottleChecksFinishedCallback finish_callback);
+
+  void OnThrottleChecksComplete(NavigationThrottle::ThrottleCheckResult result);
+
+  // Helper method to set the OnThrottleChecksComplete callback on the
+  // NavigationHandle.
+  void PrepareCompleteCallbackOnHandle();
+
+  // Helper method for places in the code which expect the navigation to start
+  // successfuly (e.g. when calling Commit() before Start()).
+  void StartAndExpectToProceed();
+
   enum State {
     INITIALIZATION,
     STARTED,
@@ -162,6 +201,18 @@ class NavigationSimulator : public WebContentsObserver {
   int num_ready_to_commit_called_ = 0;
   int num_did_finish_navigation_called_ = 0;
 
+  // Holds the last ThrottleCheckResult calculated by the navigation's
+  // throttles. Will be unset before WillStartRequest is finished. Will be unset
+  // while throttles are being run, but before they finish.
+  base::Optional<NavigationThrottle::ThrottleCheckResult>
+      last_throttle_check_result_;
+
+  // Used to execute the Finish* methods, after the throttles have all finished.
+  ThrottleChecksFinishedCallback complete_callback_;
+
+  // Closure that is set when WaitForThrottleChecksComplete is called.
+  base::Closure throttle_checks_wait_closure_;
+
   base::WeakPtrFactory<NavigationSimulator> weak_factory_;
 };