Make browser_handles_top_level_requests synchronous.

content/renderer/render_view_impl.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/renderer/render_view_impl.h"
 
 #include <algorithm>
 #include <cmath>
 #include <string>
 #include <vector>
@@ skipping 2307 matching lines @@
     // TODO(cevans): revisit whether this origin check is still necessary once
     // crbug.com/101395 is fixed.
     if (frame_url.GetOrigin() != url.GetOrigin()) {
       OpenURL(frame, url, referrer, default_policy);
       return WebKit::WebNavigationPolicyIgnore;
     }
   }
 
   // If the browser is interested, then give it a chance to look at top level
   // navigations.
-  if (is_content_initiated) {

[Charlie Reis, 2012/04/05 17:06:53]

I thought this was here so it didn't send browser-initiated navigations right
back to the browser.  It looks like ExternalTabContainer will now intercept
those and cause us to ignore them, so I'm puzzled if this works.  Do we get here
for browser-initiated navigations?

[mkosiba (inactive), 2012/04/10 17:58:37]

The change is here so that Chrome on *Android* doesn't send navigations right
back to the browser (sorry, I should have made this clear). I don't intend to
change the way ExternalTabContainer works in this CL.

The way this currently works is:
1) user clicks on a link/some action takes place
2) the navigation comes in to render_view_impl::decidePolicyForNavigation
3) since the navigation was not initiated via WebFrame::LoadRequest
is_content_initiated == true
4) we cancel the navigation and send a message to the browser
5) navigation is re-initiated browser side,
6) navigation is now initiated via WebFrame::LoadRequest, so
is_content_initiated == false,
7) since !is_content_initiated we skip over this if block and continue to
loading the page

The way ExternalTabContainer works with the change is:
1) user clicks on link
2) the navigation comes in to render_view_impl::decidePolicyForNavigation
3) is_content_initiated == true for the same reasons
4) we send a synchronous IPC to ExternalTabContainer
5) in ExternalTabContainer we see content_initiated == true so we start a new
browser-initiated navigation (the new navigation request will wait in the
message queue until we've finished processing the current navigation) and tell
the render to ignore the current one
6) the new browser-initiated navigation comes in, is_content_initiated == false
7) we send a synchronous message to ExternalTabContainer, but since
is_content_initiated == false the navigation proceeds as normal

Now, what I've described under 7) isn't what the code does
(ExternalTabContainer::ShouldIgnoreNavigation should finish with return
is_content_initiated instead of always returning  true). I'll change the code to
do that, but this also means that (contrary to what I expected) running ui tests
on the win trybot doesn't excercise this code...

-    bool browser_handles_top_level_requests =
-        renderer_preferences_.browser_handles_top_level_requests &&
-        IsNonLocalTopLevelNavigation(url, frame, type);
-    if (browser_handles_top_level_requests ||
+  if (renderer_preferences_.browser_handles_top_level_requests) {
+    if (IsNonLocalTopLevelNavigation(url, frame, type) ||
         renderer_preferences_.browser_handles_all_requests) {
       // Reset these counters as the RenderView could be reused for the next
       // navigation.
       page_id_ = -1;
       last_page_id_sent_to_browser_ = -1;
-      OpenURL(frame, url, referrer, default_policy);
-      return WebKit::WebNavigationPolicyIgnore;  // Suppress the load here.
+      bool ignore_navigation = false;
+      Send(new ViewHostMsg_ShouldIgnoreNavigation(
+          routing_id_,
+          url,
+          referrer,
+          NavigationPolicyToDisposition(default_policy),
+          frame->identifier(),
+          is_content_initiated,
+          &ignore_navigation));
+      if (ignore_navigation)
+        return WebKit::WebNavigationPolicyIgnore;
     }
   }
 
   // Detect when we're crossing a permission-based boundary (e.g. into or out of
   // an extension or app origin, leaving a WebUI page, etc). We only care about
   // top-level navigations (not iframes). But we sometimes navigate to
   // about:blank to clear a tab, and we want to still allow that.
   //
   // Note: this is known to break POST submissions when crossing process
   // boundaries until http://crbug.com/101395 is fixed.  This is better for
@@ skipping 2799 matching lines @@
   // Must be a top level frame.
   if (frame->parent() != NULL)
     return false;
 
   // Navigations initiated within Webkit are not sent out to the external host
   // in the following cases.
   // 1. The url scheme is not http/https
   // 2. The origin of the url and the opener is the same in which case the
   //    opener relationship is maintained.
   // 3. Reloads/form submits/back forward navigations
+#if defined(OS_ANDROID)
+  // It is possible for applications to register handlers for URLs (like
+  // http://maps.google.com/), so we do need to consider http:// and https://
+  // URLs as well as anything else.
+#else

[Charlie Reis, 2012/04/05 17:06:53]

This also makes me nervous.  I'd rather have a well-tested path that works for
both Android and desktop Chrome if possible, than the possibility of
Android-specific bugs from the ifdef.

[groby-ooo-7-16, 2012/04/05 22:51:19]

Specifically, unless I misread the intent - this doesn't start considering
http/https. It stops treating non-http/https as local navigations.

On 2012/04/05 17:06:53, creis wrote:

[mkosiba (inactive), 2012/04/10 17:58:37]

Agreed. My bad for sticking this in to the CL - this actually is a hack to work
around our current lack of a platform_util:OpenExternal implementation. I'll
remove this bit.

   if (!url.SchemeIs(chrome::kHttpScheme) && !url.SchemeIs(chrome::kHttpsScheme))
     return false;
+#endif
 
   // Not interested in reloads/form submits/resubmits/back forward navigations.
   if (type != WebKit::WebNavigationTypeReload &&
       type != WebKit::WebNavigationTypeFormSubmitted &&
       type != WebKit::WebNavigationTypeFormResubmitted &&
       type != WebKit::WebNavigationTypeBackForward) {
     // The opener relationship between the new window and the parent allows the
     // new window to script the parent and vice versa. This is not allowed if
     // the origins of the two domains are different. This can be treated as a
     // top level navigation and routed back to the host.
@@ skipping 62 matching lines @@
 bool RenderViewImpl::WebWidgetHandlesCompositorScheduling() const {
   return !!RenderThreadImpl::current()->compositor_thread();
 }
 
 void RenderViewImpl::OnJavaBridgeInit() {
   DCHECK(!java_bridge_dispatcher_.get());
 #if defined(ENABLE_JAVA_BRIDGE)
   java_bridge_dispatcher_.reset(new JavaBridgeDispatcher(this));
 #endif
 }