Add a NavigationThrottle to the public content/ interface

content/public/browser/navigation_handle.h

Index: content/public/browser/navigation_handle.h
diff --git a/content/public/browser/navigation_handle.h b/content/public/browser/navigation_handle.h
index 49d0884de43256e204ad3159903db42e889d1af6..0c1a9117a8be5c87a8db9a8e73479de24dfb0ad4 100644
--- a/content/public/browser/navigation_handle.h
+++ b/content/public/browser/navigation_handle.h
@@ -6,33 +6,79 @@
 #define CONTENT_PUBLIC_BROWSER_NAVIGATION_HANDLE_H_
 
 #include "content/common/content_export.h"
+#include "content/public/common/referrer.h"
 #include "net/base/net_errors.h"
+#include "ui/base/page_transition_types.h"
 
 class GURL;
 
 namespace content {
+class NavigationThrottle;
 
 // A NavigationHandle tracks information related to a single navigation.
 class CONTENT_EXPORT NavigationHandle {
  public:
   virtual ~NavigationHandle() {}
 
+  // Parameters available at navigation start time -----------------------------
+
+  // These parameters are always available during the navigation. Note that
+  // some may change during navigation (e.g. due to server redirects).
+
   // The URL the frame is navigating to. This may change during the navigation
   // when encountering a server redirect.
   virtual const GURL& GetURL() const = 0;
 
-  // The net error code if an error happened prior to commit. Otherwise it will
-  // be net::OK.
-  virtual net::Error GetNetErrorCode() const = 0;
+  // A sanitized version of the URL the frame is navigating to. It may change
+  // during the navigation when encountering a server redirect.
+  virtual const GURL& GetValidatedURL() const = 0;

[nasko, 2015/08/31 23:25:15]

Is there any reason to return non-validated URL?

[clamy, 2015/09/03 15:30:52]

I think some observers/throttles may be interested in the original one. I'm
thinking of devtools in particular.

[nasko, 2015/09/04 23:36:49]

If we need to keep this, I'd flip it around such that we have GetURL that
returns the validated one and an oddly named one that returns the raw value.

 
   // Whether the navigation is taking place in the main frame or in a subframe.
+  // This reamins constant over the navigation.

[nasko, 2015/08/31 23:25:15]

nit: navigation lifetime.

[Avi (use Gerrit), 2015/09/01 16:38:47]

typo: remains

[clamy, 2015/09/03 15:30:52]

Done.

   virtual bool IsInMainFrame() const = 0;
 
+  // Parameters available at network request start time ------------------------
+
+  // The following parameters are only available when the a network request is

[Avi (use Gerrit), 2015/09/01 16:38:47]

"when the network request"

[clamy, 2015/09/03 15:30:52]

Done.

+  // made for the navigation (or commit time if not network request is made).

[Avi (use Gerrit), 2015/09/01 16:38:47]

"or at commit time if no network request is made"

[clamy, 2015/09/03 15:30:52]

Done.

+  // This corresponds to NavigationThrottle::WillSendRequest. They should not
+  // be queried before that.
+
+  // Whether the navigation is a post or not. This may change during the

[nasko, 2015/08/31 23:25:15]

nit: s/post/POST/

[clamy, 2015/09/03 15:30:52]

Done.

+  // navigation when encountering a server redirect.
+  virtual bool IsPost() const = 0;
+
+  // Returns a sanitized version of the referrer for this request.
+  virtual const Referrer& GetSanitizedReferrer() const = 0;

[nasko, 2015/08/31 23:25:15]

Since there is no other referrer that we are returning, why not just
GetReferrer()? The comment should still say it is "sanitized", so it is clear to
whoever is reading about the method.

[clamy, 2015/09/03 15:30:52]

Done.

+
+  // Whether the navigation was initiated by a user gesture. Note that this
+  // will return false for browser-initiated navigations.
+  virtual bool HasUserGesture() const = 0;

[nasko, 2015/08/31 23:25:15]

Usually, browser initiated navigations are in response to something coming from
the user. Should that be factored into account?

[clamy, 2015/09/03 15:30:52]

It's possible to do that for PlzNavigate, but difficult for current navigations.
I'm not terribly thrilled about having this interface return two different
things whether PlzNavigate is enabled or not, so I included a TODO to change
that when PlzNavigate launches.

+
+  // Returns the page transition type.
+  virtual ui::PageTransition GetPageTransition() const = 0;
+
+  // Whether the target URL cannot be handled by Chrome's internal protocol

[nasko, 2015/08/31 23:25:15]

nit: s/Chrome/the browser/. Chrome is a branded version of Chromium, and we are
the content layer, so we shouldn't be using Chrome directly.

[clamy, 2015/09/03 15:30:52]

Done.

+  // handlers.
+  virtual bool IsExternalProtocol() const = 0;
+
+  // Navigation control flow --------------------------------------------------
+
+  // The net error code if an error happened prior to commit. Otherwise it will
+  // be net::OK.
+  virtual net::Error GetNetErrorCode() const = 0;
+
   // Whether the navigation has successfully committed a document.
   virtual bool HasCommittedDocument() const = 0;
 
   // Whether an error page has committed for the navigation.
   virtual bool HasCommittedErrorPage() const = 0;
+
+  // Registers a NavigationThrottle for this navigation. The throttle can
+  // modify the request, pause the request or cancel the request. This will
+  // take ownership of the NavigationThrottle.
+  virtual void RegisterThrottle(
+      scoped_ptr<NavigationThrottle> navigation_throttle) = 0;

[davidben, 2015/09/01 21:55:18]

I wonder if this would be better done as the ResourceThrottle-style API where
you have one change to staple all your NavigationThrottles. Thoughts? Right now
you can add them at any point, which is kinda weird.

[clamy, 2015/09/03 15:30:52]

Well the goal was to have that happen in just one point in the content browser
client, where we add all throttles in one go (using this precise method). I
moved this method to the implementation and have the content browser client
return a vector of throttles. This way there will be less possibility of
mistakes. I'm keeping a test version of this method though, since it's handy for
testing throttles.

 };
 
 }  // namespace content