Add a NavigationThrottle to the public content/ interface

content/browser/frame_host/navigation_handle_impl.h

Index: content/browser/frame_host/navigation_handle_impl.h
diff --git a/content/browser/frame_host/navigation_handle_impl.h b/content/browser/frame_host/navigation_handle_impl.h
index 33e51ddab278cf281b89fe998d32159acc2210b0..9aff56c0ef975cb1f73dbe612fe14b26c2f1caae 100644
--- a/content/browser/frame_host/navigation_handle_impl.h
+++ b/content/browser/frame_host/navigation_handle_impl.h
@@ -9,7 +9,9 @@
 
 #include "base/macros.h"
 #include "base/memory/scoped_ptr.h"
+#include "base/memory/scoped_vector.h"
 #include "content/common/content_export.h"
+#include "content/public/browser/navigation_throttle.h"
 #include "url/gurl.h"
 
 namespace content {
@@ -52,18 +54,22 @@ struct NavigationRequestInfo;
 // the RenderFrameHost still apply.
 class CONTENT_EXPORT NavigationHandleImpl : public NavigationHandle {
  public:
-  static scoped_ptr<NavigationHandleImpl> Create(const GURL& url,
-                                                 const bool is_main_frame,
-                                                 NavigatorDelegate* delegate);
-
   ~NavigationHandleImpl() override;
 
   // NavigationHandle implementation:
-  const GURL& GetURL() const override;
-  net::Error GetNetErrorCode() const override;
-  bool IsInMainFrame() const override;
-  bool HasCommittedDocument() const override;
-  bool HasCommittedErrorPage() const override;
+  const GURL& GetURL() override;
+  WebContents* GetWebContents() override;
+  bool IsInMainFrame() override;
+  bool IsPost() override;
+  const Referrer& GetReferrer() override;
+  bool HasUserGesture() override;
+  ui::PageTransition GetPageTransition() override;
+  bool IsExternalProtocol() override;
+  net::Error GetNetErrorCode() override;
+  bool HasCommittedDocument() override;
+  bool HasCommittedErrorPage() override;
+  void RegisterThrottleForTesting(
+      scoped_ptr<NavigationThrottle> navigation_throttle) override;
 
   void set_net_error_code(net::Error net_error_code) {
     net_error_code_ = net_error_code;
@@ -78,6 +84,25 @@ class CONTENT_EXPORT NavigationHandleImpl : public NavigationHandle {
     is_transferring_ = is_transferring;
   }
 
+  void set_web_contents(WebContents* web_contents) {
+    web_contents_ = web_contents;
+  }
+
+  // Called when the URLRequest will start in the network stack.
+  NavigationThrottle::ThrottleCheckResult WillStartRequest(
+      bool is_post,
+      const Referrer& sanitized_referrer,
+      bool has_user_gesture,
+      ui::PageTransition transition,
+      bool is_external_protocol);
+
+  // Called when the URLRequest will be redirected in the network stack.
+  NavigationThrottle::ThrottleCheckResult WillRedirectRequest(
+      const GURL& new_url,
+      bool new_method_is_post,
+      const GURL& new_referrer_url,
+      bool new_is_external_protocol);
+
   // Called when the navigation was redirected. This will update the |url_| and
   // inform the delegate.
   void DidRedirectNavigation(const GURL& new_url);
@@ -87,9 +112,13 @@ class CONTENT_EXPORT NavigationHandleImpl : public NavigationHandle {
   void DidCommitNavigation();
 
  private:
+  friend class NavigationHandleFactory;
+  friend class TestNavigationHandle;
+
   // Used to track the state the navigation is currently in.
   enum State {
     DID_START = 0,
+    WILL_SEND_REQUEST,
     DID_COMMIT,
     DID_COMMIT_ERROR_PAGE,
   };
@@ -100,9 +129,16 @@ class CONTENT_EXPORT NavigationHandleImpl : public NavigationHandle {
 
   // See NavigationHandle for a description of those member variables.
   GURL url_;
+  const bool is_main_frame_;
+  bool is_post_;
+  Referrer sanitized_referrer_;
+  bool has_user_gesture_;
+  ui::PageTransition transition_;
+  bool is_external_protocol_;
   net::Error net_error_code_;
+
+  // The state the navigation is in.
   State state_;
-  const bool is_main_frame_;
 
   // Whether the navigation is in the middle of a transfer. Set to false when
   // the DidStartProvisionalLoad is received from the new renderer.
@@ -112,6 +148,22 @@ class CONTENT_EXPORT NavigationHandleImpl : public NavigationHandle {
   // navigation.
   NavigatorDelegate* delegate_;
 
+  // A pointer to the WebContents the navigation is taking place in.
+  //
+  // The NavigationHandle is either owned by a NavigationRequest or a
+  // RenderFrameHost, both of which are owned by a FrameTreeNode, which is
+  // ultimately owned by the WebContents. Therefore, the NavigationHandle
+  // cannot outlive the WebContents and it is safe to use a raw pointer.
+  //
+  // Note that it makes sense for the NavigationHandleImpl to keep a pointer to
+  // the WebContents because the NavigationHandle interface is closely tied to
+  // a WebContents (it is passed outside content/ through WebContentsObserver
+  // methods).

[nasko, 2015/09/16 20:33:39]

It feels like a bit of a layering violation, but I think it is better than
passing the two always together.

Charlie, what is your take?

[Charlie Reis, 2015/09/16 20:46:07]

It's definitely a layering violation.  :)

That said, Camille made a good point that NavigationHandle and
WebContentsObserver are tightly coupled, and I haven't yet been able to come up
with a case where the NavigationHandle doesn't have a WebContents.  I'm somewhat
ok with allowing it, and hence my request for this comment.

Just to explore the alternative-- it would be better from a layering perspective
for frame_host classes not to depend on WebContents.  If we went that route,
NavigationHandle wouldn't know its WebContents, and callers would have to make
that association on their own.  This is doable but a bit clumsy and presents a
risk of bugs if they get it wrong.

Neither approach seems great, so perhaps the minor layering violation is better
to avoid the potential bugs.  Worst case, some NavigationHandles won't have a
WebContents.

+  WebContents* web_contents_;
+
+  // A list of Throttles registered for this navigation.
+  ScopedVector<NavigationThrottle> throttles_;
+
   DISALLOW_COPY_AND_ASSIGN(NavigationHandleImpl);
 };