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..ba2780e9610b4ae54ad642df3ca9228c8f4dbeab 100644
--- a/content/public/browser/navigation_handle.h
+++ b/content/public/browser/navigation_handle.h
@@ -6,33 +6,89 @@
 #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;
+class WebContents;
 
 // 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;

[Charlie Reis, 2015/09/11 00:06:49]

From http://www.chromium.org/developers/content-module/content-api:
"don't add the const identifier to interfaces. For interfaces implemented by the
embedder, we can't make assumptions about what the embedder needs to implement
it. For interfaces implemented by content, the implementation details doesn't
have to be exposed."

John has wanted to be fairly strict on this, so we should update these previous
ones as well as not introduce new consts.

[clamy, 2015/09/16 01:03:22]

Done.

 
-  // 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;

[Charlie Reis, 2015/09/11 00:06:48]

It's not initially clear to me what this refers to.  What kind of sanitization
or validation?  Why is it needed?

This matters because other code is going to have to choose between calling this
and calling GetURL() (which most people will do without thinking about).  I'm
hesitant to expose both without a strong reason.

[clamy, 2015/09/11 13:30:44]

When transitioning the InterceptNavigationResourceThrottle, they filter the url
using RenderProcessHost::FilterURL. This returns the url filtered by the same
method. We had a discussion with nasko@ about whether GetURL should just always
returns the filtered url, but I was worried that some future users of the
interface, such as SafeBrowsing might need the original unfiltered url.

What we could do is have GetURL return the filtered url by default, and add a
GetUnfilteredURL method that returns the original url if needed. Wdyt?

[nasko, 2015/09/14 22:00:04]

I am all for only having one accessor, as I've stated before. Let's not
anticipate what we might need and just expose what we really need. I'm fine
exposing only the filtered URL, which is likely what we do in other places.

[Charlie Reis, 2015/09/16 00:09:24]

Yes, let's try to have only GetURL which returns a validated URL.  FilterURL is
intended to be applied to all incoming URLs immediately, before they're seen by
any other code.  Hopefully we can avoid any cases where the unfiltered URL is
needed.  (If those come up, please let us know.)

[clamy, 2015/09/16 01:03:22]

Done.

 
   // Whether the navigation is taking place in the main frame or in a subframe.
+  // This remains constant over the navigation lifetime.
   virtual bool IsInMainFrame() const = 0;
 
+  // The WebContents the navigation is taking place in.
+  virtual WebContents* GetWebContents() const = 0;
+
+  // Parameters available at network request start time ------------------------
+
+  // The following parameters are only available when the network request is
+  // made for the navigation (or at commit time if no network request is made).
+  // 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
+  // navigation when encountering a server redirect.
+  virtual bool IsPost() const = 0;
+
+  // Returns a sanitized version of the referrer for this request.
+  virtual const Referrer& GetReferrer() const = 0;
+
+  // Whether the navigation was initiated by a user gesture. Note that this
+  // will return false for browser-initiated navigations.
+  // TODO(clamy): when PlzNavigate launches, this should return true for
+  // browser-initiated navigations.
+  virtual bool HasUserGesture() const = 0;

[Charlie Reis, 2015/09/11 00:06:49]

I'm not sure why many of these are inaccessible until network request start
time.  Aren't things like GetReferrer, HasUserGesture, etc known at navigation
start time?Actually, all of them seem like they would be.  What is the
distinction for?

[clamy, 2015/09/11 13:30:44]

It's for supporting the current architecture without too many additional
changes. DidStartNavigation is executed following a DidStartProvisionalLoad IPC.
In order to know all those parameters at that time, I'd need to add more
parameters to the DidStartProvisionalLoad IPC.

+
+  // Returns the page transition type.
+  virtual ui::PageTransition GetPageTransition() const = 0;
+
+  // Whether the target URL cannot be handled by the browser's internal protocol
+  // 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 tests. The throttle can
+  // modify the request, pause the request or cancel the request. This will
+  // take ownership of the NavigationThrottle.
+  // Note: in non-test cases, NavigationThrottles should not be added directly
+  // but returned by the implementation of
+  // ContentBrowserClient::AddNavigationThrottles. This ensures proper ordering

[Charlie Reis, 2015/09/11 00:06:48]

Stale name?

[clamy, 2015/09/16 01:03:22]

Done.

+  // of the throttles.
+  virtual void RegisterThrottleForTesting(

[Charlie Reis, 2015/09/11 00:06:48]

Everything so far has been an accessor with no state changes.  Maybe this
belongs in a separate comment-delimited section?

[clamy, 2015/09/16 01:03:22]

Well I did group it with the last 3 accessors in a Navigation control flow
section, should it still be separated?

+      scoped_ptr<NavigationThrottle> navigation_throttle) = 0;
 };
 
 }  // namespace content