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 685 matching lines @@
   else
     CancelNavigation(frame_tree_node);
 }
 
 // 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(

[Charlie Reis, 2015/02/17 23:48:29]

Let's put a comment at the top of this function saying "This is a
renderer-initiated navigation."  That part is implicit in the comment below, but
it's worthwhile to call it out.

[carlosk, 2015/02/18 15:28:25]

I thought it was better to actually rename the method. Should I also rename the
IPC and its handler method in RFHI?

[Charlie Reis, 2015/02/19 00:58:00]

No, I actually think the old OnBeginNavigation name was better, because it
matches our OnFoo naming convention for IPC message handlers.  Please change it
back.

A comment should be sufficient to call attention to that fact.

[carlosk, 2015/02/19 10:45:18]

I reverted the rename and added the comment.

But please help me understand your reasoning. Why the old name when this name is
clearer in stating what this method is actually meant for (now, after the
refactor)? I have nothing against comments but having an better suited method
name is an order of magnitude better.

We could prefix this (or a similar) name with "On". As I stated I was also
willing to change the IPC/handler method names to make them match.

[Charlie Reis, 2015/02/19 18:42:12]

Sure, I'm happy to explain my reasoning:

Names are indeed important, and they should be clear, concise, and consistent
with other names when possible.  I would argue that OnBeginNavigation accurately
describes a request from the renderer process to begin a navigation.

I would say that the name was misleading in the past, because it was also used
for browser-initiated navigations.  That's no longer the case, so I don't think
we need the more verbose "RendererInitiatedNavigationRequest" (which is not
consise nor consistent with our other method names).  Instead, the comment seems
sufficient to me for verifying that the name means what it sounds like.

Hope that helps.

[carlosk, 2015/02/20 09:57:10]

Acknowledged. Thanks.

       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());
+
+  // The renderer navigation request is ignored iff a) there is an ongoing

[Charlie Reis, 2015/02/17 23:48:29]

nit: Please keep the term "renderer-initiated navigation" to be consistent with
other code.

[carlosk, 2015/02/18 15:28:25]

Done.

+  // request b) which is browser or user-initiated and c) the renderer request
+  // is not user-initiated.
+  if (ongoing_navigation_request &&
+      (ongoing_navigation_request->browser_initiated() ||
+       ongoing_navigation_request->begin_params().has_user_gesture) &&
+      !begin_params.has_user_gesture) {
+    return;
+  }
+
+  // In all other cases the current navigation, if any, is canceled and a new
+  // NavigationRequest is created and stored in the map.

[Charlie Reis, 2015/02/17 23:48:29]

Can you mention where it gets canceled?

(I don't see cancelation mentioned in the comment of CreateRendererInitiated. 
Is there code somewhere that tells the network stack, etc, that the old
navigation should be stopped?)

[clamy, 2015/02/18 12:45:33]

This is done upon destruction of the old NavigationRequest. It owns a
NavigationURLLoader, and the destruction of the NavigationURLLoader triggers a
call to the cancelation logic of the network stack.

[carlosk, 2015/02/18 15:28:25]

I changed the comment to clarify that.

[Charlie Reis, 2015/02/19 00:58:00]

Acknowledged.

   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