Make browser_handles_top_level_requests synchronous.

content/renderer/render_view_impl.cc

Index: content/renderer/render_view_impl.cc
diff --git a/content/renderer/render_view_impl.cc b/content/renderer/render_view_impl.cc
index 8103dc5789f5a0a289609077a5e79c0c31aa752c..0fa2f1e27d2c68dc419a652b6d67e0edb327e9a2 100644
--- a/content/renderer/render_view_impl.cc
+++ b/content/renderer/render_view_impl.cc
@@ -2325,18 +2325,24 @@ WebNavigationPolicy RenderViewImpl::decidePolicyForNavigation(
 
   // 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;
     }
   }
 
@@ -5156,8 +5162,14 @@ bool RenderViewImpl::IsNonLocalTopLevelNavigation(
   // 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 &&