PlzNavigate: send Javascript urls synchronously to the renderer

content/browser/frame_host/navigator_impl.cc

 // Copyright 2013 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/browser/frame_host/navigator_impl.h"
 
 #include "base/command_line.h"
 #include "base/metrics/histogram.h"
 #include "base/time/time.h"
 #include "content/browser/frame_host/frame_tree.h"
@@ skipping 802 matching lines @@
   CHECK(base::CommandLine::ForCurrentProcess()->HasSwitch(
       switches::kEnableBrowserSideNavigation));
   DCHECK(frame_tree_node);
   FrameMsg_Navigate_Type::Value navigation_type =
       GetNavigationType(controller_->GetBrowserContext(), entry, reload_type);
   scoped_ptr<NavigationRequest> navigation_request =
       NavigationRequest::CreateBrowserInitiated(frame_tree_node, entry,
                                                 navigation_type,
                                                 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) {
   NavigationRequest* navigation_request = frame_tree_node->navigation_request();
 
   // A browser-initiated navigation could have been cancelled while it was
   // waiting for the BeforeUnload event to execute.
   if (!navigation_request)
     return;
 
@@ skipping 54 matching lines @@
         "Navigation.TimeToCommit_ExistingRenderer_BeforeUnloadDiscounted",
         time_to_commit);
     UMA_HISTOGRAM_TIMES(
         "Navigation.TimeToURLJobStart_ExistingRenderer_BeforeUnloadDiscounted",
         time_to_network);
   }
   navigation_data_.reset();
 }
 
 }  // namespace content