(DEPRECATED) Send navigation_start to browser process in DidStartProvisionalLoad

content/renderer/render_frame_impl.cc

Index: content/renderer/render_frame_impl.cc
diff --git a/content/renderer/render_frame_impl.cc b/content/renderer/render_frame_impl.cc
index a7492bb2d872e90fc04cf365e47c633da40d5260..adc6e710a3b4670328075285eecc89ac2a91e7c9 100644
--- a/content/renderer/render_frame_impl.cc
+++ b/content/renderer/render_frame_impl.cc
@@ -457,34 +457,33 @@ WebURLRequest CreateURLRequestForNavigation(
   return request;
 }
 
-void UpdateFrameNavigationTiming(WebFrame* frame,
-                                 base::TimeTicks browser_navigation_start,
-                                 base::TimeTicks renderer_navigation_start) {
+base::TimeTicks SanitizeNavigationTiming(
+    blink::WebFrameLoadType load_type,
+    const base::TimeTicks& browser_navigation_start,
+    const base::TimeTicks& renderer_navigation_start) {
+  if (load_type != blink::WebFrameLoadType::Standard)
+    return base::TimeTicks();
   // The browser provides the navigation_start time to bootstrap the
   // Navigation Timing information for the browser-initiated navigations. In
   // case of cross-process navigations, this carries over the time of
   // finishing the onbeforeunload handler of the previous page.
+  // |browser_navigation_start| is likely before this process existed, so we
+  // can't use InterProcessTimeTicksConverter. We need at least to ensure
+  // that the browser-side navigation start we set is not later than the one
+  // on the renderer side.
   DCHECK(!browser_navigation_start.is_null());
-  if (frame->provisionalDataSource()) {
-    // |browser_navigation_start| is likely before this process existed, so we
-    // can't use InterProcessTimeTicksConverter. We need at least to ensure
-    // that the browser-side navigation start we set is not later than the one
-    // on the renderer side.
-    base::TimeTicks navigation_start = std::min(
-        browser_navigation_start, renderer_navigation_start);
-    double navigation_start_seconds =
-        (navigation_start - base::TimeTicks()).InSecondsF();
-    frame->provisionalDataSource()->setNavigationStartTime(
-        navigation_start_seconds);
-    // TODO(clamy): We need to provide additional timing values for the
-    // Navigation Timing API to work with browser-side navigations.
-  }
+  base::TimeTicks navigation_start =
+      std::min(browser_navigation_start, renderer_navigation_start);
+  // TODO(csharrison) UMA log:
+  // |renderer_navigation_start - browser_navigation_start|
+  return navigation_start;
 }
 
 // PlzNavigate
 CommonNavigationParams MakeCommonNavigationParams(
     blink::WebURLRequest* request,
-    bool should_replace_current_entry) {
+    bool should_replace_current_entry,
+    const base::TimeTicks& navigation_start) {
   const RequestExtraData kEmptyData;
   const RequestExtraData* extra_data =
       static_cast<RequestExtraData*>(request->extraData());
@@ -508,7 +507,8 @@ CommonNavigationParams MakeCommonNavigationParams(
   return CommonNavigationParams(
       request->url(), referrer, extra_data->transition_type(),
       FrameMsg_Navigate_Type::NORMAL, true, should_replace_current_entry,
-      ui_timestamp, report_type, GURL(), GURL(), LOFI_UNSPECIFIED);
+      ui_timestamp, report_type, GURL(), GURL(), LOFI_UNSPECIFIED,
+      navigation_start);
 }
 
 #if !defined(OS_ANDROID) || defined(ENABLE_MEDIA_PIPELINE_ON_ANDROID)
@@ -2548,6 +2548,24 @@ void RenderFrameImpl::willSubmitForm(blink::WebLocalFrame* frame,
   FOR_EACH_OBSERVER(RenderFrameObserver, observers_, WillSubmitForm(form));
 }
 
+void RenderFrameImpl::UpdateNavigationState(DocumentState* document_state) {
+  bool content_initiated = !pending_navigation_params_.get();
+  // Note for some browser initiated navigations (i.e. not
+  // WebFrameLoadType::Standard), we want to record their start time now. They
+  // will have had common_params.navigation_start set to null. Other
+  // content-initiated loads will have start time set when their NavigationState
+  // is constructed.
+  if (!content_initiated &&
+      pending_navigation_params_->common_params.navigation_start.is_null()) {
+    pending_navigation_params_->common_params.navigation_start =
+        base::TimeTicks::Now();
+  }
+  document_state->set_navigation_state(
+      content_initiated ? NavigationStateImpl::CreateContentInitiated()
+                        : CreateNavigationStateFromPending());
+  pending_navigation_params_.reset();
+}
+
 void RenderFrameImpl::didCreateDataSource(blink::WebLocalFrame* frame,
                                           blink::WebDataSource* datasource) {
   DCHECK(!frame_ || frame_ == frame);
@@ -2586,16 +2604,9 @@ void RenderFrameImpl::didCreateDataSource(blink::WebLocalFrame* frame,
           old_internal_data->is_overriding_user_agent());
     }
   }
-
   // The rest of RenderView assumes that a WebDataSource will always have a
   // non-null NavigationState.
-  if (content_initiated) {
-    document_state->set_navigation_state(
-        NavigationStateImpl::CreateContentInitiated());
-  } else {
-    document_state->set_navigation_state(CreateNavigationStateFromPending());
-    pending_navigation_params_.reset();
-  }
+  UpdateNavigationState(document_state);
 
   // DocumentState::referred_by_prefetcher_ is true if we are
   // navigating from a page that used prefetching using a link on that
@@ -2645,6 +2656,15 @@ void RenderFrameImpl::didCreateDataSource(blink::WebLocalFrame* frame,
     }
   }
 
+  NavigationStateImpl* navigation_state = static_cast<NavigationStateImpl*>(
+      document_state->navigation_state());
+
+  // Set the navigation start time in blink.
+  base::TimeTicks navigation_start =
+      navigation_state->common_params().navigation_start;
+  datasource->setNavigationStartTime(
+      (navigation_start - base::TimeTicks()).InSecondsF());
+
   // Create the serviceworker's per-document network observing object if it
   // does not exist (When navigation happens within a page, the provider already
   // exists).
@@ -2652,9 +2672,6 @@ void RenderFrameImpl::didCreateDataSource(blink::WebLocalFrame* frame,
           DocumentState::FromDataSource(datasource)))
     return;
 
-  NavigationStateImpl* navigation_state = static_cast<NavigationStateImpl*>(
-      DocumentState::FromDataSource(datasource)->navigation_state());
-
   ServiceWorkerNetworkProvider::AttachToDocumentState(
       DocumentState::FromDataSource(datasource),
       ServiceWorkerNetworkProvider::CreateForNavigation(
@@ -2703,12 +2720,20 @@ void RenderFrameImpl::didStartProvisionalLoad(blink::WebLocalFrame* frame,
         ->set_transition_type(ui::PAGE_TRANSITION_AUTO_SUBFRAME);
   }
 
+  // Get the navigation start in TimeTicks format. This will be
+  // lossy because blink converts TimeTicks internal int64 representation to a
+  // double. TODO(csharrison) Do we want to override this with the TimeTicks
+  // timestamp in document_state->navigation_params for browser loads?
+  DCHECK(ds->getNavigationStartTime());
+  base::TimeTicks navigation_start = base::TimeTicks::FromInternalValue(

[clamy, 2015/11/03 13:33:00]

Since we always set the start time of the DataSource using the value from the
NavigationState, why not use the timestamp from the NavigationState here?

+      ds->getNavigationStartTime() * base::Time::kMicrosecondsPerSecond);
+
   FOR_EACH_OBSERVER(RenderViewObserver, render_view_->observers(),
                     DidStartProvisionalLoad(frame));
   FOR_EACH_OBSERVER(RenderFrameObserver, observers_, DidStartProvisionalLoad());
 
   Send(new FrameHostMsg_DidStartProvisionalLoadForFrame(
-       routing_id_, ds->request().url()));
+      routing_id_, ds->request().url(), navigation_start));
 }
 
 void RenderFrameImpl::didReceiveServerRedirectForProvisionalLoad(
@@ -3130,14 +3155,13 @@ void RenderFrameImpl::didNavigateWithinPage(blink::WebLocalFrame* frame,
   // ExtraData will get the new NavigationState.  Similarly, if we did not
   // initiate this navigation, then we need to take care to reset any pre-
   // existing navigation state to a content-initiated navigation state.
-  // didCreateDataSource conveniently takes care of this for us.
-  didCreateDataSource(frame, frame->dataSource());
-
+  // UpdateNavigationState conveniently takes care of this for us.
   DocumentState* document_state =
       DocumentState::FromDataSource(frame->dataSource());
+  DCHECK(document_state);
+  UpdateNavigationState(document_state);
   static_cast<NavigationStateImpl*>(document_state->navigation_state())
       ->set_was_within_same_page(true);
-
   didCommitProvisionalLoad(frame, item, commit_type);
 }
 
@@ -4317,7 +4341,7 @@ void RenderFrameImpl::OnFailedNavigation(
   // Inform the browser of the start of the provisional load. This is needed so
   // that the load is properly tracked by the WebNavigation API.
   Send(new FrameHostMsg_DidStartProvisionalLoadForFrame(
-      routing_id_, common_params.url));
+      routing_id_, common_params.url, common_params.navigation_start));
 
   // Send the provisional load failure.
   blink::WebURLError error =
@@ -4621,6 +4645,8 @@ void RenderFrameImpl::NavigateInternal(
   bool browser_side_navigation =
       base::CommandLine::ForCurrentProcess()->HasSwitch(
           switches::kEnableBrowserSideNavigation);
+  // Upper bound for browser initiated navigation start time.
+  base::TimeTicks renderer_navigation_start = base::TimeTicks::Now();
   bool is_reload = IsReload(common_params.navigation_type);
   bool is_history_navigation = request_params.page_state.IsValid();
   WebURLRequest::CachePolicy cache_policy =
@@ -4646,6 +4672,11 @@ void RenderFrameImpl::NavigateInternal(
 
   pending_navigation_params_.reset(
       new NavigationParams(common_params, start_params, request_params));
+  // We don't know for sure that this load type is WebFrameLoadType::Standard.
+  // For now, set this to null, and update it when we are sure of the right
+  // value.

[clamy, 2015/11/03 13:33:00]

Could we check with Navigation Timing API people whether we should also use the
browser-provided timestamp for reloads and history loads? I think it would make
sense to do so, and the code would be simpler.

+  pending_navigation_params_->common_params.navigation_start =
+      base::TimeTicks();
 
   // Create parameters for a standard navigation.
   blink::WebFrameLoadType load_type = blink::WebFrameLoadType::Standard;
@@ -4777,10 +4808,10 @@ void RenderFrameImpl::NavigateInternal(
   }
 
   if (should_load_request) {
-    // Record this before starting the load. We need a lower bound of this
-    // time to sanitize the navigationStart override set below.
-    base::TimeTicks renderer_navigation_start = base::TimeTicks::Now();
-
+    // Sanitize navigation start now that we know the load_type.
+    pending_navigation_params_->common_params.navigation_start =
+        SanitizeNavigationTiming(load_type, common_params.navigation_start,
+                                 renderer_navigation_start);
     // Perform a navigation to a data url if needed.
     if (!common_params.base_url_for_data_url.is_empty() ||
         (browser_side_navigation &&
@@ -4791,12 +4822,6 @@ void RenderFrameImpl::NavigateInternal(
       frame_->toWebLocalFrame()->load(request, load_type,
                                       item_for_history_navigation);
     }
-
-    if (load_type == blink::WebFrameLoadType::Standard) {
-      UpdateFrameNavigationTiming(frame_,
-                                  request_params.browser_navigation_start,
-                                  renderer_navigation_start);
-    }
   }
 
   // In case LoadRequest failed before didCreateDataSource was called.
@@ -4987,6 +5012,9 @@ void RenderFrameImpl::BeginNavigation(blink::WebURLRequest* request) {
   // TODO(clamy): Apply devtools override.
   // TODO(clamy): Make sure that navigation requests are not modified somewhere
   // else in blink.
+
+  base::TimeTicks navigation_start = base::TimeTicks::Now();

[clamy, 2015/11/03 13:33:00]

I don't think this is needed. None of the functions below take time to execute,
so we can postpone getting the navigation_start time until we construct the
CommonNavigationParams.

+
   willSendRequest(frame_, 0, *request, blink::WebURLResponse());
 
   // TODO(clamy): Same-document navigations should not be sent back to the
@@ -5020,8 +5048,8 @@ void RenderFrameImpl::BeginNavigation(blink::WebURLRequest* request) {
              REQUEST_CONTEXT_FRAME_TYPE_NESTED);
 
   Send(new FrameHostMsg_BeginNavigation(
-      routing_id_,
-      MakeCommonNavigationParams(request, should_replace_current_entry),
+      routing_id_, MakeCommonNavigationParams(
+                       request, should_replace_current_entry, navigation_start),
       BeginNavigationParams(
           request->httpMethod().latin1(), GetWebURLRequestHeaders(*request),
           GetLoadFlagsForWebURLRequest(*request), request->hasUserGesture(),