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

components/test_runner/web_frame_test_client.cc

Index: components/test_runner/web_frame_test_client.cc
diff --git a/components/test_runner/web_frame_test_client.cc b/components/test_runner/web_frame_test_client.cc
index ca0aa871cf8c7080239684ef9614ce313809b63e..c6652364067778c85f483c2c449cb86e0a60feac 100644
--- a/components/test_runner/web_frame_test_client.cc
+++ b/components/test_runner/web_frame_test_client.cc
@@ -29,6 +29,7 @@
 #include "third_party/WebKit/public/platform/WebURLRequest.h"
 #include "third_party/WebKit/public/platform/WebURLResponse.h"
 #include "third_party/WebKit/public/web/WebConsoleMessage.h"
+#include "third_party/WebKit/public/web/WebDataSource.h"
 #include "third_party/WebKit/public/web/WebElement.h"
 #include "third_party/WebKit/public/web/WebFrame.h"
 #include "third_party/WebKit/public/web/WebLocalFrame.h"
@@ -413,6 +414,14 @@ void WebFrameTestClient::loadErrorPage(int reason) {
 }
 
 void WebFrameTestClient::didStartProvisionalLoad(blink::WebLocalFrame* frame) {
+  // PlzNavigate
+  // A provisional load notification is received when a frame navigation is
+  // sent to the browser. We don't want to log it again during commit.
+  // Please see the RenderFrameImpl::BeginNavigation() function.

[clamy, 2017/01/31 00:20:01]

I'm a bit worried about this. Considering at which time we send the
didStartProvisionalLoad, we haven't set up a provisional DataSource (ie a
provisional DocumentLoader). So what we get here is a nullptr I imagine. The
provisional DataSource will not be created until the navigation is ready to
commit, which IMO is fairly confusing, since you have no way to actually get at
the request for which you called DidStartProvisionalLoad. Maybe it would be
better to add the WebURLRequest explicitly as a parameter to
didStartProvisionalLoad? I understand this works, but I'm worried about other
users of the API getting confused by weird behavior.

Additionally, I'm wondering how this works with renderer-initiated navigations
that transfer: we will end up committing the navigation in a different renderer.
I imagine that for the purpose of outputting DidStartProvisionalLoad only once
that would work though.

[ananta, 2017/01/31 03:37:07]

I added the request as a parameter to WebFrameClient. The
didStartProvisionalLoad() call in BeginNavigate() serves the purpose of the
layout test only. The problem you pointed out with the navigation getting
committed  in a different renderer exists. However in that  case we would end up
calling didStartProvisionalLoad() again anyways in the commit code path

+  blink::WebDataSource* ds = frame->provisionalDataSource();
+  if (ds && delegate_->IsNavigationInitiatedByRenderer(ds->getRequest()))
+    return;
+
   test_runner()->tryToSetTopLoadingFrame(frame);
 
   if (test_runner()->shouldDumpFrameLoadCallbacks()) {