Let RenderFrameImpl set navigationStart based on CommonNavigationParams

This change originally was reverted here:
https://codereview.chromium.org/1423453007/
Due to a flawed test. The test in question tries to set Blink's navigationStart timestamp based on the browsers navigation_start, but blink essentially ignores that for it's reporting to consumers, only using the TimeTicks value passed in for relative measures (i.e. all the other metrics).

Original description:
This entails setting the timestamp before we set it in blink, so DocumentLoadTiming had to be modified to allow for that. We set the value in didCreateDataSource.

didCreateDataSource was also refactored slightly so that same-page navigations don't need to call it anymore.

This is 2/3 of the effort to add a navigation_start timestamp to DidStartProvisional load. The full CL we're breaking up is here:
https://codereview.chromium.org/1425823002/

BUG=548335

Review URL: https://codereview.chromium.org/1409883008

Cr-Commit-Position: refs/heads/master@{#358532}

4 files changed, 81 insertions, 67 deletions

diff --git a/content/renderer/render_frame_impl.cc b/content/renderer/render_frame_impl.cc
index d1ac605..5a9da31 100644
--- a/content/renderer/render_frame_impl.cc
+++ b/content/renderer/render_frame_impl.cc
@@ -455,28 +455,28 @@ WebURLRequest CreateURLRequestForNavigation(
   return request;
 }
 
-void UpdateFrameNavigationTiming(WebFrame* frame,
-                                 base::TimeTicks browser_navigation_start,
-                                 base::TimeTicks renderer_navigation_start) {
-  // 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.
+// Sanitizes the navigation_start timestamp for browser-initiated navigations,
+// where the browser possibly has a better notion of start time than the
+// renderer. In the case of cross-process navigations, this carries over the
+// time of finishing the onbeforeunload handler of the previous page.
+// TimeTicks is sometimes not monotonic across processes, and because
+// |browser_navigation_start| is likely before this process existed,
+// InterProcessTimeTicksConverter won't help. The timestamp is sanitized by
+// clamping it to renderer_navigation_start, initialized earlier in the call
+// stack.
+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();
   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) Investigate how big a problem the cross process
+  // monotonicity really is and on what platforms. Log UMA for:
+  // |renderer_navigation_start - browser_navigation_start|
+  return navigation_start;
 }
 
 // PlzNavigate
@@ -2605,13 +2605,7 @@ void RenderFrameImpl::didCreateDataSource(blink::WebLocalFrame* frame,
 
   // 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
@@ -2661,6 +2655,17 @@ 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());
+  // TODO(clamy) We need to provide additional timing values for the Navigation
+  // Timing API to work with browser-side navigations.
+
   // Create the serviceworker's per-document network observing object if it
   // does not exist (When navigation happens within a page, the provider already
   // exists).
@@ -2668,9 +2673,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(
@@ -3155,11 +3157,10 @@ 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());
+  UpdateNavigationState(document_state);
   static_cast<NavigationStateImpl*>(document_state->navigation_state())
       ->set_was_within_same_page(true);
 
@@ -4657,6 +4658,9 @@ void RenderFrameImpl::NavigateInternal(
   bool browser_side_navigation =
       base::CommandLine::ForCurrentProcess()->HasSwitch(
           switches::kEnableBrowserSideNavigation);
+
+  // Lower 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 =
@@ -4687,6 +4691,14 @@ void RenderFrameImpl::NavigateInternal(
   pending_navigation_params_.reset(
       new NavigationParams(common_params, start_params, request_params));
 
+  // Unless the load is a WebFrameLoadType::Standard, this should remain
+  // uninitialized. It will be updated when the load type is determined to be
+  // Standard, or after the previous document's unload handler has been
+  // triggered. This occurs in UpdateNavigationState.
+  // TODO(csharrison) See if we can always use the browser timestamp.
+  pending_navigation_params_->common_params.navigation_start =
+      base::TimeTicks();
+
   // Create parameters for a standard navigation.
   blink::WebFrameLoadType load_type = blink::WebFrameLoadType::Standard;
   bool should_load_request = false;
@@ -4811,10 +4823,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 &&
@@ -4825,12 +4837,6 @@ void RenderFrameImpl::NavigateInternal(
       frame_->toWebLocalFrame()->load(request, load_type,
                                       item_for_history_navigation);
     }
-
-    if (load_type == blink::WebFrameLoadType::Standard) {
-      UpdateFrameNavigationTiming(frame_,
-                                  common_params.navigation_start,
-                                  renderer_navigation_start);
-    }
   }
 
   // In case LoadRequest failed before didCreateDataSource was called.
@@ -5200,6 +5206,22 @@ NavigationState* RenderFrameImpl::CreateNavigationStateFromPending() {
   return NavigationStateImpl::CreateContentInitiated();
 }
 
+void RenderFrameImpl::UpdateNavigationState(DocumentState* document_state) {
+  if (pending_navigation_params_) {
+    // If this is a browser-initiated load that doesn't override
+    // navigation_start, set it here.
+    if (pending_navigation_params_->common_params.navigation_start.is_null()) {
+      pending_navigation_params_->common_params.navigation_start =
+          base::TimeTicks::Now();
+    }
+    document_state->set_navigation_state(CreateNavigationStateFromPending());
+    pending_navigation_params_.reset();
+  } else {
+    document_state->set_navigation_state(
+        NavigationStateImpl::CreateContentInitiated());
+  }
+}
+
 #if defined(OS_ANDROID)
 WebMediaPlayer* RenderFrameImpl::CreateAndroidWebMediaPlayer(
     WebMediaPlayerClient* client,
diff --git a/content/renderer/render_frame_impl.h b/content/renderer/render_frame_impl.h
index 7474362..e29cc90 100644
--- a/content/renderer/render_frame_impl.h
+++ b/content/renderer/render_frame_impl.h
@@ -870,6 +870,10 @@ class CONTENT_EXPORT RenderFrameImpl
   // saved in OnNavigate().
   NavigationState* CreateNavigationStateFromPending();
 
+  // Sets the NavigationState on the DocumentState based on
+  // the value of |pending_navigation_params_|.
+  void UpdateNavigationState(DocumentState* document_state);
+
 #if defined(OS_ANDROID)
   blink::WebMediaPlayer* CreateAndroidWebMediaPlayer(
       blink::WebMediaPlayerClient* client,
diff --git a/content/renderer/render_view_browsertest.cc b/content/renderer/render_view_browsertest.cc
index 3f1250f..3783e905 100644
--- a/content/renderer/render_view_browsertest.cc
+++ b/content/renderer/render_view_browsertest.cc
@@ -2257,30 +2257,10 @@ TEST_F(RenderViewImplTest, ScreenMetricsEmulation) {
   // Don't disable here to test that emulation is being shutdown properly.
 }
 
-// Sanity checks for the Navigation Timing API |navigationStart| override. We
+// Sanity check for the Navigation Timing API |navigationStart| override. We
 // are asserting only most basic constraints, as TimeTicks (passed as the
 // override) are not comparable with the wall time (returned by the Blink API).
 TEST_F(RenderViewImplTest, NavigationStartOverride) {
-  // Verify that a navigation that claims to have started at the earliest
-  // possible TimeTicks is indeed reported as one that started before
-  // OnNavigate() is called.
-  base::Time before_navigation = base::Time::Now();
-  CommonNavigationParams early_common_params;
-  StartNavigationParams early_start_params;
-  early_common_params.url = GURL("data:text/html,<div>Page</div>");
-  early_common_params.navigation_type = FrameMsg_Navigate_Type::NORMAL;
-  early_common_params.transition = ui::PAGE_TRANSITION_TYPED;
-  early_common_params.navigation_start = base::TimeTicks::FromInternalValue(1);
-  early_start_params.is_post = true;
-
-  frame()->Navigate(early_common_params, early_start_params,
-                    RequestNavigationParams());
-  ProcessPendingMessages();
-
-  base::Time early_nav_reported_start =
-      base::Time::FromDoubleT(GetMainFrame()->performance().navigationStart());
-  EXPECT_LT(early_nav_reported_start, before_navigation);
-
   // Verify that a navigation that claims to have started in the future - 42
   // days from now is *not* reported as one that starts in the future; as we
   // sanitize the override allowing a maximum of ::Now().
diff --git a/third_party/WebKit/Source/core/loader/DocumentLoadTiming.cpp b/third_party/WebKit/Source/core/loader/DocumentLoadTiming.cpp
index 33f8535..0113356 100644
--- a/third_party/WebKit/Source/core/loader/DocumentLoadTiming.cpp
+++ b/third_party/WebKit/Source/core/loader/DocumentLoadTiming.cpp
@@ -87,6 +87,12 @@ double DocumentLoadTiming::pseudoWallTimeToMonotonicTime(double pseudoWallTime)
 
 void DocumentLoadTiming::markNavigationStart()
 {
+    // Allow the embedder to override navigationStart before we record it if
+    // they have a more accurate timestamp.
+    if (m_navigationStart) {
+        ASSERT(m_referenceMonotonicTime && m_referenceWallTime);
+        return;
+    }
     TRACE_EVENT_MARK("blink.user_timing", "navigationStart");
     ASSERT(!m_navigationStart && !m_referenceMonotonicTime && !m_referenceWallTime);
 
@@ -98,14 +104,16 @@ void DocumentLoadTiming::markNavigationStart()
 void DocumentLoadTiming::setNavigationStart(double navigationStart)
 {
     TRACE_EVENT_MARK_WITH_TIMESTAMP("blink.user_timing", "navigationStart", navigationStart);
-    ASSERT(m_referenceMonotonicTime && m_referenceWallTime);
     m_navigationStart = navigationStart;
 
     // |m_referenceMonotonicTime| and |m_referenceWallTime| represent
     // navigationStart. When the embedder sets navigationStart (because the
     // navigation started earlied on the browser side), we need to adjust these
     // as well.
-    m_referenceWallTime = monotonicTimeToPseudoWallTime(navigationStart);
+    if (!m_referenceWallTime)
+        m_referenceWallTime = currentTime();
+    else
+        m_referenceWallTime = monotonicTimeToPseudoWallTime(navigationStart);
     m_referenceMonotonicTime = navigationStart;
     notifyDocumentTimingChanged();
 }