PlzNavigate: Move navigation-level mixed content checks to the browser.

content/browser/frame_host/navigation_handle_impl.cc

Index: content/browser/frame_host/navigation_handle_impl.cc
diff --git a/content/browser/frame_host/navigation_handle_impl.cc b/content/browser/frame_host/navigation_handle_impl.cc
index 8ed60813cfc8592fe75bd843f6931c59c078c1ba..069d04391395b006bb8d91785b583e3a3fdadab0 100644
--- a/content/browser/frame_host/navigation_handle_impl.cc
+++ b/content/browser/frame_host/navigation_handle_impl.cc
@@ -14,6 +14,7 @@
 #include "content/browser/frame_host/ancestor_throttle.h"
 #include "content/browser/frame_host/debug_urls.h"
 #include "content/browser/frame_host/frame_tree_node.h"
+#include "content/browser/frame_host/mixed_content_navigation_throttle.h"
 #include "content/browser/frame_host/navigator.h"
 #include "content/browser/frame_host/navigator_delegate.h"
 #include "content/browser/loader/resource_dispatcher_host_impl.h"
@@ -30,7 +31,6 @@
 #include "content/public/common/url_constants.h"
 #include "net/base/net_errors.h"
 #include "net/url_request/redirect_info.h"
-#include "third_party/WebKit/public/platform/WebMixedContentContextType.h"
 #include "url/gurl.h"
 #include "url/url_constants.h"
 
@@ -788,6 +788,10 @@ void NavigationHandleImpl::RegisterNavigationThrottles() {
   // GetNavigationThrottles is not assigned to throttles_ directly because it
   // would overwrite any throttle previously added with
   // RegisterThrottleForTesting.
+  if (IsBrowserSideNavigationEnabled()) {
+    throttles_.insert(throttles_.end(),
+                      new MixedContentNavigationThrottle(this));

[nasko, 2017/01/12 18:32:37]

Does the ordering of when we add this matter? Would DevTools need to know and be
able to log the navigation before it is potentially blocked by the mixed content
checker?

[carlosk, 2017/01/21 02:54:59]

I don't know the answer for this.

As this is a security check it seemed appropriate to move it up the chain but
that's just me guessing. I created a new never-to-be-committed CL [1] where I
moved this registration to be the last one and no trybots ever complained about
it.

I also noticed the established pattern here of concealing the creation decision
from each throttle within their classes by having a CreateThrottleForNavigation
method called from here. I updated my code to do the same.

Another thing I'm usure about is if I should register directly to |throttles_|
or through |throttles_to_register| as both cases exist. I chose the latter
because 2 out of 3 cases do that...? :O


[1] https://codereview.chromium.org/2630043002

[nasko, 2017/01/23 22:32:37]

Let's keep it first then, until we hear something breaking.
Cool, thanks! That's definitely nice.
 
Yeah, what you did is good. We should follow up and make them all consistent.
 

[carlosk, 2017/02/08 02:59:02]

OK, I'm keeping as is but just reaffirming: trybots didn't complain when I moved
it to being the last one.

I also added a TODO to make this code a bit cleaner.

[nasko, 2017/02/10 01:08:20]

Acknowledged.

+  }
   ScopedVector<NavigationThrottle> throttles_to_register =
       GetDelegate()->CreateThrottlesForNavigation(this);
   std::unique_ptr<NavigationThrottle> devtools_throttle =