Send navigation_start to the browser in DidStartProvisionalLoad IPC

content/renderer/render_view_browsertest.cc

Index: content/renderer/render_view_browsertest.cc
diff --git a/content/renderer/render_view_browsertest.cc b/content/renderer/render_view_browsertest.cc
index 3f1250f1827d91ab1f29353b54af4902bd316be6..8a7cd75a693ce8e0e1f575d57f77e8ef7550b528 100644
--- a/content/renderer/render_view_browsertest.cc
+++ b/content/renderer/render_view_browsertest.cc
@@ -168,6 +168,29 @@ class RenderViewImplTest : public RenderViewTest {
     return static_cast<TestRenderFrame*>(view()->GetMainRenderFrame());
   }
 
+  void GoToOffsetWithParams(int offset,
+                            const PageState& state,
+                            const CommonNavigationParams common_params,
+                            const StartNavigationParams start_params,
+                            RequestNavigationParams request_params) {
+    EXPECT_TRUE(common_params.transition & ui::PAGE_TRANSITION_FORWARD_BACK);
+    int history_list_length = view()->historyBackListCount() +
+                              view()->historyForwardListCount() + 1;
+    int pending_offset = offset + view()->history_list_offset_;

[nasko, 2015/11/13 21:52:54]

Hmmm, not sure if asking the view() is the right thing long term, as we are
moving history navigation around. Will add avi@ to take a poke at this one.

[Charlie Harrison, 2015/11/14 00:25:00]

For context, most of this logic is copied from render_view_test's GoToOffset. I
just needed to finely tune the parameters.

+
+    request_params.page_state = state;
+    request_params.page_id = view()->page_id_ + offset;
+    request_params.nav_entry_id = pending_offset + 1;
+    request_params.pending_history_list_offset = pending_offset;
+    request_params.current_history_list_offset = view()->history_list_offset_;
+    request_params.current_history_list_length = history_list_length;
+    frame()->Navigate(common_params, start_params, request_params);
+
+    // The load actually happens asynchronously, so we pump messages to process
+    // the pending continuation.
+    FrameLoadWaiter(frame()).Wait();
+  }
+
   // Sends IPC messages that emulates a key-press event.
   int SendKeyEvent(MockKeyboard::Layout layout,
                    int key_code,
@@ -2304,6 +2327,108 @@ TEST_F(RenderViewImplTest, NavigationStartOverride) {
   EXPECT_LE(late_nav_reported_start, after_navigation);
 }
 
+TEST_F(RenderViewImplTest, RendererNavigationStartTransmittedToBrowser) {
+  base::TimeTicks lower_bound_navigation_start;
+  frame()->GetWebFrame()->loadHTMLString(
+      "hello world", blink::WebURL(GURL("data:text/html,")));
+  ProcessPendingMessages();
+  const IPC::Message* frame_navigate_msg =
+      render_thread_->sink().GetUniqueMessageMatching(
+          FrameHostMsg_DidStartProvisionalLoadForFrame::ID);
+  FrameHostMsg_DidStartProvisionalLoadForFrame::Param host_nav_params;
+  FrameHostMsg_DidStartProvisionalLoadForFrame::Read(frame_navigate_msg,
+                                                     &host_nav_params);
+  base::TimeTicks transmitted_start = base::get<1>(host_nav_params);
+  EXPECT_FALSE(transmitted_start.is_null());
+  EXPECT_LT(lower_bound_navigation_start, transmitted_start);
+}
+
+TEST_F(RenderViewImplTest, BrowserNavigationStartNotUsedForReload) {
+  const char url_string[] = "data:text/html,<div>Page</div>";
+  // Navigate once, then reload.
+  LoadHTML(url_string);
+  ProcessPendingMessages();
+  render_thread_->sink().ClearMessages();
+
+  CommonNavigationParams common_params;
+  common_params.url = GURL(url_string);
+  common_params.navigation_type =
+      FrameMsg_Navigate_Type::RELOAD_ORIGINAL_REQUEST_URL;
+  common_params.transition = ui::PAGE_TRANSITION_LINK;

[nasko, 2015/11/13 21:52:54]

Why not PAGE_TRANSITION_RELOAD?

[Charlie Harrison, 2015/11/14 00:25:00]

No good reason. Fixed.

+
+  frame()->Navigate(common_params, StartNavigationParams(),
+                    RequestNavigationParams());
+  ProcessPendingMessages();
+
+  const IPC::Message* frame_navigate_msg =
+      render_thread_->sink().GetUniqueMessageMatching(
+          FrameHostMsg_DidStartProvisionalLoadForFrame::ID);
+  FrameHostMsg_DidStartProvisionalLoadForFrame::Param host_nav_params;
+  FrameHostMsg_DidStartProvisionalLoadForFrame::Read(frame_navigate_msg,
+                                                     &host_nav_params);
+  // The true timestamp is later than the browser initiated one.
+  EXPECT_GT(base::get<1>(host_nav_params), common_params.navigation_start);

[nasko, 2015/11/13 21:52:54]

Do we have enough granularity to ensure we are not going to collide if the test
runs too fast? It can lead to flakiness if not. Maybe check if the ticks object
is high resolution or not?
https://code.google.com/p/chromium/codesearch#chromium/src/base/time/time.h&l...

[Charlie Harrison, 2015/11/14 00:25:00]

Good idea. It should be monotonic either way, so I'll make the check GTE if
we're using the low resolution timer, because we could possibly mark the
navigationStart in the renderer within 15ms (assuming comment in time.h is
correct).

+}
+
+TEST_F(RenderViewImplTest, BrowserNavigationStartNotUsedForHistoryNavigation) {
+  LoadHTML("<div id=pagename>Page A</div>");
+  LoadHTML("<div id=pagename>Page B</div>");
+  PageState back_state =
+      HistoryEntryToPageState(view()->history_controller()->GetCurrentEntry());
+  LoadHTML("<div id=pagename>Page C</div>");
+  PageState forward_state =
+      HistoryEntryToPageState(view()->history_controller()->GetCurrentEntry());
+  ProcessPendingMessages();
+  render_thread_->sink().ClearMessages();
+
+  CommonNavigationParams common_params;
+  common_params.transition = ui::PAGE_TRANSITION_FORWARD_BACK;
+  // Go back.
+  GoToOffsetWithParams(-1, back_state, common_params, StartNavigationParams(),
+                       RequestNavigationParams());
+  ProcessPendingMessages();
+  const IPC::Message* frame_navigate_msg =
+      render_thread_->sink().GetUniqueMessageMatching(
+          FrameHostMsg_DidStartProvisionalLoadForFrame::ID);
+  FrameHostMsg_DidStartProvisionalLoadForFrame::Param host_nav_params;
+  FrameHostMsg_DidStartProvisionalLoadForFrame::Read(frame_navigate_msg,
+                                                     &host_nav_params);

[nasko, 2015/11/13 21:52:54]

This code for reading the IPC is done enough times that it might be good to
extract it into a helper method called by all the tests using it.

[Charlie Harrison, 2015/11/14 00:25:00]

Sounds good to me. This might not end up being a win for readability unless we
can templatize it. Uploading a draft now to apply to just my tests. I can
probably make a separate CL later cleaning up other tests.

[nasko, 2015/11/14 00:33:24]

This looks good.

+  EXPECT_GT(base::get<1>(host_nav_params), common_params.navigation_start);
+  render_thread_->sink().ClearMessages();
+
+  // Go forward.
+  GoToOffsetWithParams(1, forward_state, common_params,
+                             StartNavigationParams(),
+                             RequestNavigationParams());
+  ProcessPendingMessages();
+  const IPC::Message* frame_navigate_msg2 =
+      render_thread_->sink().GetUniqueMessageMatching(
+          FrameHostMsg_DidStartProvisionalLoadForFrame::ID);
+  FrameHostMsg_DidStartProvisionalLoadForFrame::Param host_nav_params2;
+  FrameHostMsg_DidStartProvisionalLoadForFrame::Read(frame_navigate_msg2,
+                                                     &host_nav_params2);
+  EXPECT_GT(base::get<1>(host_nav_params2), common_params.navigation_start);
+}
+
+TEST_F(RenderViewImplTest, BrowserNavigationStartSuccessfullyTransmitted) {
+  CommonNavigationParams common_params;
+  common_params.url = GURL("data:text/html,<div>Page</div>");
+  common_params.navigation_type = FrameMsg_Navigate_Type::NORMAL;
+  common_params.transition = ui::PAGE_TRANSITION_TYPED;
+
+  frame()->Navigate(common_params, StartNavigationParams(),
+                    RequestNavigationParams());
+  ProcessPendingMessages();
+
+  const IPC::Message* frame_navigate_msg =
+      render_thread_->sink().GetUniqueMessageMatching(
+          FrameHostMsg_DidStartProvisionalLoadForFrame::ID);
+  FrameHostMsg_DidStartProvisionalLoadForFrame::Param host_nav_params;
+  FrameHostMsg_DidStartProvisionalLoadForFrame::Read(frame_navigate_msg,
+                                                     &host_nav_params);
+  EXPECT_EQ(base::get<1>(host_nav_params), common_params.navigation_start);
+}
+
 TEST_F(RenderViewImplTest, PreferredSizeZoomed) {
   LoadHTML("<body style='margin:0;'><div style='display:inline-block; "
            "width:400px; height:400px;'/></body>");