PlzNavigate: Updated navigation cancel policy for renderer-initiated requests.

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 689 matching lines @@
 // PlzNavigate
 void NavigatorImpl::OnBeginNavigation(
     FrameTreeNode* frame_tree_node,
     const CommonNavigationParams& common_params,
     const BeginNavigationParams& begin_params,
     scoped_refptr<ResourceRequestBody> body) {
   CHECK(base::CommandLine::ForCurrentProcess()->HasSwitch(
       switches::kEnableBrowserSideNavigation));
   DCHECK(frame_tree_node);
 
-  // This is a renderer-initiated navigation, so generate a new
-  // NavigationRequest and store it in the map.
-  // TODO(clamy): Renderer-initiated navigations should not always cancel the
-  // current one.
+  NavigationRequest* ongoing_navigation_request =
+      navigation_request_map_.get(frame_tree_node->frame_tree_node_id());
+  if (ongoing_navigation_request &&
+      ongoing_navigation_request->begin_params().has_user_gesture &&

[clamy, 2015/02/11 17:22:29]

I don't think this currently works. In browser initiated navigations we
explicitly set has_user_gesture to false
(https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fr...).

The condition there would be (ongoing_navigation_request->browser_initiated() ||
ongoing_navigation_request->begin_params().has_user_gesture).

[carlosk, 2015/02/12 12:21:06]

And I even knew about it... So I need one more test!

Test added and bug fixed. It was also good to go over them once more because I
found a few issues with the existing tests.

+      !begin_params.has_user_gesture) {
+    // The renderer navigation request is canceled iff a) there is an ongoing

[clamy, 2015/02/11 17:22:30]

This does not cancel the renderer-initiated navigation, so I think that a better
phrasing would be: The ongoing navigation is not canceled iff...

[carlosk, 2015/02/12 12:21:05]

I did this instead: s/canceled/ignored

[clamy, 2015/02/12 18:08:06]

The change did not appear in the diff :)

[carlosk, 2015/02/13 13:44:28]

Done now. Sorry about that.

+    // request b) which is user-initiated and c) the renderer request is not
+    // user-initiated.
+    return;
+  }
+  // In all other cases the current navigation, if any, is canceled and a new

[clamy, 2015/02/11 17:22:29]

nit: add an empty line before the comment.

[carlosk, 2015/02/12 12:21:06]

Done.

+  // NavigationRequest is created and store it in the map.

[clamy, 2015/02/11 17:22:30]

s/store it/stored

[carlosk, 2015/02/12 12:21:06]

Done.

   scoped_ptr<NavigationRequest> navigation_request =
       NavigationRequest::CreateRendererInitiated(
           frame_tree_node, common_params, begin_params, body);
   navigation_request_map_.set(
       frame_tree_node->frame_tree_node_id(), navigation_request.Pass());
 
   if (frame_tree_node->IsMainFrame())
     navigation_data_.reset();
 
   BeginNavigation(frame_tree_node);
@@ skipping 193 matching lines @@
         "Navigation.TimeToCommit_ExistingRenderer_BeforeUnloadDiscounted",
         time_to_commit);
     UMA_HISTOGRAM_TIMES(
         "Navigation.TimeToURLJobStart_ExistingRenderer_BeforeUnloadDiscounted",
         time_to_network);
   }
   navigation_data_.reset();
 }
 
 }  // namespace content