PlzNavigate: Invoke didStartProvisionalLoad() when the renderer initiates a navigation in startLoad(

content/renderer/render_frame_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/renderer/render_frame_impl.h"
 
 #include <map>
 #include <string>
 #include <utility>
 #include <vector>
@@ skipping 3430 matching lines @@
           DocumentState::FromDataSource(datasource)))
     return;
 
   ServiceWorkerNetworkProvider::AttachToDocumentState(
       DocumentState::FromDataSource(datasource),
       ServiceWorkerNetworkProvider::CreateForNavigation(
           routing_id_, navigation_state->request_params(), frame,
           content_initiated));
 }
 
-void RenderFrameImpl::didStartProvisionalLoad(blink::WebLocalFrame* frame) {
+void RenderFrameImpl::didStartProvisionalLoad(
+    blink::WebLocalFrame* frame,
+    const blink::WebURLRequest& request) {
   DCHECK_EQ(frame_, frame);
   WebDataSource* ds = frame->provisionalDataSource();
 
   // In fast/loader/stop-provisional-loads.html, we abort the load before this
   // callback is invoked.
   if (!ds)

[Nate Chapin, 2017/01/31 22:13:32]

I think this if() statement is dead code, but this patch will make it alive
again here? Ideally, we would pass all the parameters that are currently being
read off the provisionalDataSource() (see comments below).

[ananta, 2017/02/02 01:29:23]

Based on our discussion, I moved the call to send the didStartProvisionalLoad()
notification early in FrameLoader::startLoad(). We need to pass the loader in as
RenderFrameImpl sets the navigation state in there. The other changes in this
patch are mostly boiler plate with a couple removing usage of
provisionalLoader() in the didStartProvisionalLoad notification.

     return;
 
   TRACE_EVENT2("navigation,benchmark,rail",
                "RenderFrameImpl::didStartProvisionalLoad", "id", routing_id_,
                "url", ds->getRequest().url().string().utf8());

[Nate Chapin, 2017/01/31 22:13:32]

ds->getRequest() -> request ?

[ananta, 2017/02/02 01:29:23]

Not needed with the latest patch

   DocumentState* document_state = DocumentState::FromDataSource(ds);
   NavigationStateImpl* navigation_state = static_cast<NavigationStateImpl*>(
       document_state->navigation_state());
   bool is_top_most = !frame->parent();
   if (is_top_most) {
     render_view_->set_navigation_gesture(
         WebUserGestureIndicator::isProcessingUserGesture() ?
             NavigationGestureUser : NavigationGestureAuto);
   } else if (ds->replacesCurrentHistoryItem()) {

[Nate Chapin, 2017/01/31 22:13:32]

As mentioned above, could we plumb this bit?

[ananta, 2017/02/02 01:29:23]

Not needed with the latest patch

     // Subframe navigations that don't add session history items must be
     // marked with AUTO_SUBFRAME. See also didFailProvisionalLoad for how we
     // handle loading of error pages.
     navigation_state->set_transition_type(ui::PAGE_TRANSITION_AUTO_SUBFRAME);
   }
 
   base::TimeTicks navigation_start =
       navigation_state->common_params().navigation_start;
   DCHECK(!navigation_start.is_null());
 
   for (auto& observer : render_view_->observers())
     observer.DidStartProvisionalLoad(frame);
   for (auto& observer : observers_)
     observer.DidStartProvisionalLoad();
 
   Send(new FrameHostMsg_DidStartProvisionalLoad(
       routing_id_, ds->getRequest().url(), navigation_start));

[Nate Chapin, 2017/01/31 22:13:32]

ds->getRequest() -> request ?

[ananta, 2017/02/02 01:29:23]

Not needed with the latest patch

 }
 
 void RenderFrameImpl::didReceiveServerRedirectForProvisionalLoad(
     blink::WebLocalFrame* frame) {
   DCHECK_EQ(frame_, frame);
 
   // TODO(creis): Determine if this can be removed or if we need to clear any
   // local state here to fix https://crbug.com/671276.
 }
 
@@ skipping 2684 matching lines @@
   if (request_params.should_clear_history_list) {
     CHECK_EQ(-1, render_view_->history_list_offset_);
     CHECK_EQ(0, render_view_->history_list_length_);
   }
 }
 
 void RenderFrameImpl::BeginNavigation(const NavigationPolicyInfo& info) {
   CHECK(IsBrowserSideNavigationEnabled());
   browser_side_navigation_pending_ = true;
 
+  didStartProvisionalLoad(frame_, info.urlRequest);

[Nate Chapin, 2017/01/31 22:13:32]

Instead of adding a new path for calling didStartProvisionalLoad(), could we
move it earlier in the navigation start pipeline, so the PlzNavigate and
non-PlzNavigate paths use the same call? I think moving it to the start of
FrameLoader::startLoad() would work.

[ananta, 2017/02/02 01:29:23]

Done.

+
   // Note: At this stage, the goal is to apply all the modifications the
   // renderer wants to make to the request, and then send it to the browser, so
   // that the actual network request can be started. Ideally, all such
   // modifications should take place in willSendRequest, and in the
   // implementation of willSendRequest for the various InspectorAgents
   // (devtools).
   //
   // TODO(clamy): Apply devtools override.
   // TODO(clamy): Make sure that navigation requests are not modified somewhere
   // else in blink.
@@ skipping 626 matching lines @@
   // event target. Potentially a Pepper plugin will receive the event.
   // In order to tell whether a plugin gets the last mouse event and which it
   // is, we set |pepper_last_mouse_event_target_| to null here. If a plugin gets
   // the event, it will notify us via DidReceiveMouseEvent() and set itself as
   // |pepper_last_mouse_event_target_|.
   pepper_last_mouse_event_target_ = nullptr;
 #endif
 }
 
 }  // namespace content