PlzNavigate: send Javascript urls synchronously to the renderer

content/browser/frame_host/navigator_impl.cc

Index: content/browser/frame_host/navigator_impl.cc
diff --git a/content/browser/frame_host/navigator_impl.cc b/content/browser/frame_host/navigator_impl.cc
index 8219a3d79518bc95ca9ff998a620c0f149838a86..9fcf875bc4be509509a0c90517241ba54c035076 100644
--- a/content/browser/frame_host/navigator_impl.cc
+++ b/content/browser/frame_host/navigator_impl.cc
@@ -821,11 +821,18 @@ void NavigatorImpl::RequestNavigation(
                                                 navigation_start, controller_);
   frame_tree_node->SetNavigationRequest(navigation_request.Pass());
 

[Fabrice (no longer in Chrome), 2015/05/29 12:37:08]

nit: empty line here

[clamy, 2015/05/29 16:02:06]

Done.

+  frame_tree_node->navigation_request()->SetWaitingForRendererResponse();
+
   // Have the current renderer execute its beforeUnload event if needed. If it
   // is not needed (eg. the renderer is not live), BeginNavigation should get
-  // called.
-  frame_tree_node->navigation_request()->SetWaitingForRendererResponse();
+  // called. If the navigation does not need to make a network request, then
+  // start it synchronously. This is needed for Javascript urls.
+  if (NavigationRequest::ShouldMakeNetworkRequest(

[nasko, 2015/05/29 14:17:46]

In this particular codepath, it isn't really about whether a network request
must be made or not. It is more about whether the navigation can go
cross-process. If it can, then we need to run the beforeunload handler. It kind
of fits nicely with the network request bit, but maybe a comment might be
worthwhile.

[clamy, 2015/05/29 16:02:06]

Updated the comment.

+          frame_tree_node->navigation_request()->common_params().url)) {
   frame_tree_node->current_frame_host()->DispatchBeforeUnload(true);

[Fabrice (no longer in Chrome), 2015/05/29 12:37:08]

nit: indent is wrong.

[clamy, 2015/05/29 16:02:06]

Done.

+  } else {
+    BeginNavigation(frame_tree_node);

[nasko, 2015/05/29 14:17:46]

Why did data URLs work without this?

[clamy, 2015/05/29 16:02:06]

Apparently no test tries to do a browser-intiated navigation to a data url. In
fact, I think content_shell won't even navigate to a data url that is typed in
its address bar.

[nasko, 2015/06/01 20:38:22]

Maybe we should add one : ). I don't see a reason why we can't have data URLs in
the omnibox, especially as it definitely works in regular Chrome. Try it : ).

[clamy, 2015/06/02 12:28:16]

So after further investigation, it appears that we do currently support
navigation to data urls (see BookmarkletTest::NavigateToStartPage for test
support), however they are not synchronous in the current implementation, nor
are they even with this patch in PlzNavigate. I suspect this is due to something
in Blink's implementation of LoadDataURL.

+  }
 }
 
 void NavigatorImpl::BeginNavigation(FrameTreeNode* frame_tree_node) {