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

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

A number of layouttests fail because the didStartProvisionalLoad() notification comes in for PlzNavigate after
didFinishDocumentLoad() notification is called. In the non PlzNavigate case the load is handled in blink and hence the
notifications happen in the correct order.

Approach being tried is to invoke didStartProvisionaLoad() in FrameLoader::startLoad() when we find that a navigation is being
handled by the client. Ideally we would want to invoke this notification early regardless of whether PlzNavigate is on. However
a number of tests and codepaths assume that the current document would be unloaded before we receive the provisional load notification
for the non PlzNavigate case. We create a temporary loader just for the purpose of firing the didStartProvisionalLoad()
notification.

Most of the changes in this patch are boilerplate changes to the didStartProvisionalLoad() function.

BUG=638900
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2661743002
Cr-Commit-Position: refs/heads/master@{#448695}
Committed: https://chromium.googlesource.com/chromium/src/+/1a023a38bddf1b2d4e64a1f31175b72e1776713d

[ananta, 2017-01-28 00:16:32]

The CQ bit was checked by ananta@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-28 00:17:04]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-28 03:30:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-28 03:30:41]

Dry run: This issue passed the CQ dry run.

[ananta, 2017-01-28 05:05:35]

ananta@chromium.org changed reviewers:
+ jam@chromium.org

[jam, 2017-01-30 15:31:10]

jam@chromium.org changed reviewers:
+ clamy@chromium.org

[jam, 2017-01-30 15:31:11]

Camille would be a better reviewer

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

Thanks! A few comments below.

https://codereview.chromium.org/2661743002/diff/20001/components/test_runner/...
File components/test_runner/web_frame_test_client.cc (right):

https://codereview.chromium.org/2661743002/diff/20001/components/test_runner/...
components/test_runner/web_frame_test_client.cc:420: // Please see the
RenderFrameImpl::BeginNavigation() function.
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:34:40]

The CQ bit was checked by ananta@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-31 03:35:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

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

https://codereview.chromium.org/2661743002/diff/20001/components/test_runner/...
File components/test_runner/web_frame_test_client.cc (right):

https://codereview.chromium.org/2661743002/diff/20001/components/test_runner/...
components/test_runner/web_frame_test_client.cc:420: // Please see the
RenderFrameImpl::BeginNavigation() function.
On 2017/01/31 00:20:01, clamy (very slow - traveling) wrote:
> 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.

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

[commit-bot: I haz the power, 2017-01-31 04:10:11]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-31 04:10:12]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[ananta, 2017-01-31 04:11:20]

The CQ bit was checked by ananta@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-31 04:11:37]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-31 05:54:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-31 05:54:41]

Dry run: This issue passed the CQ dry run.

[ananta, 2017-01-31 20:28:41]

ananta@chromium.org changed reviewers:
+ japhet@chromium.org

[ananta, 2017-01-31 20:28:42]

+japhet for Webkit

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

https://codereview.chromium.org/2661743002/diff/60001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2661743002/diff/60001/content/renderer/render...
content/renderer/render_frame_impl.cc:3459: if (!ds)
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).

https://codereview.chromium.org/2661743002/diff/60001/content/renderer/render...
content/renderer/render_frame_impl.cc:3464: "url",
ds->getRequest().url().string().utf8());
ds->getRequest() -> request ?

https://codereview.chromium.org/2661743002/diff/60001/content/renderer/render...
content/renderer/render_frame_impl.cc:3473: } else if
(ds->replacesCurrentHistoryItem()) {
As mentioned above, could we plumb this bit?

https://codereview.chromium.org/2661743002/diff/60001/content/renderer/render...
content/renderer/render_frame_impl.cc:3490: routing_id_, ds->getRequest().url(),
navigation_start));
ds->getRequest() -> request ?

https://codereview.chromium.org/2661743002/diff/60001/content/renderer/render...
content/renderer/render_frame_impl.cc:6195: didStartProvisionalLoad(frame_,
info.urlRequest);
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 00:47:49]

Description was changed from

==========
PlzNavigate: Invoke didStartProvisionalLoad() when the renderer initiates a
navigation via BeginNavigation()

A number of layouttests fail because the didStartProvisionalLoad() notification
comes in for PlzNavigate after
WebFrameClient::willSendRequest() is called. In the non plznavigate case
willSendRequest() is called when a navigation
is committed in blink i.e. it is loaded by a frame.  In the PlzNavigate case
willSendRequest is called when a frame navigation
is sent to the browser via RenderFrameImpl::BeginNavigation().

Approach being tried is to call didStartProvisionalLoad() in
RenderFrameImpl::BeginNavigation() before willSendRequest.

BUG=638900
==========

to

==========
PlzNavigate: Invoke didStartProvisionalLoad() when the renderer initiates a
navigation via BeginNavigation()

A number of layouttests fail because the didStartProvisionalLoad() notification
comes in for PlzNavigate after
WebFrameClient::willSendRequest() is called. In the non plznavigate case
willSendRequest() is called when a navigation
is committed in blink i.e. it is loaded by a frame.  In the PlzNavigate case
willSendRequest is called when a frame navigation
is sent to the browser via RenderFrameImpl::BeginNavigation().

Approach being tried is to invoke didStartProvisionalLoad() before we check the
navigation policy in FrameLoader::startLoad()
We need a working DocumentLoader instance because the
RenderFrameImpl::didStartProvisionalLoad() function saves the navigationState
in the provisional document loader instance. Longer term we want to get rid of
provisional document loader accessor usage. To achieve
this we create a temporary document loader instance and pass it in the
didStartProvisionalLoad() notification. If the navigation policy
wants the navigation to be continued in blink, we promote the temporary loader
to provisional loader.

Most of the changes in this patch are boilerplate changes to the
didStartProvisionalLoad() function.

BUG=638900
==========

[ananta, 2017-02-02 01:16:09]

Description was changed from

==========
PlzNavigate: Invoke didStartProvisionalLoad() when the renderer initiates a
navigation via BeginNavigation()

A number of layouttests fail because the didStartProvisionalLoad() notification
comes in for PlzNavigate after
WebFrameClient::willSendRequest() is called. In the non plznavigate case
willSendRequest() is called when a navigation
is committed in blink i.e. it is loaded by a frame.  In the PlzNavigate case
willSendRequest is called when a frame navigation
is sent to the browser via RenderFrameImpl::BeginNavigation().

Approach being tried is to invoke didStartProvisionalLoad() before we check the
navigation policy in FrameLoader::startLoad()
We need a working DocumentLoader instance because the
RenderFrameImpl::didStartProvisionalLoad() function saves the navigationState
in the provisional document loader instance. Longer term we want to get rid of
provisional document loader accessor usage. To achieve
this we create a temporary document loader instance and pass it in the
didStartProvisionalLoad() notification. If the navigation policy
wants the navigation to be continued in blink, we promote the temporary loader
to provisional loader.

Most of the changes in this patch are boilerplate changes to the
didStartProvisionalLoad() function.

BUG=638900
==========

to

==========
PlzNavigate: Invoke didStartProvisionalLoad() when the renderer initiates a
navigation early on in FrameLoader::startLoad()

A number of layouttests fail because the didStartProvisionalLoad() notification
comes in for PlzNavigate after
WebFrameClient::willSendRequest() is called. In the non plznavigate case
willSendRequest() is called when a navigation
is committed in blink i.e. it is loaded by a frame.  In the PlzNavigate case
willSendRequest is called when a frame navigation
is sent to the browser via RenderFrameImpl::BeginNavigation().

Approach being tried is to invoke didStartProvisionalLoad() before we check the
navigation policy in FrameLoader::startLoad()
We need a working DocumentLoader instance because the
RenderFrameImpl::didStartProvisionalLoad() function saves the navigationState
in the provisional document loader instance. Longer term we want to get rid of
provisional document loader accessor usage. To achieve
this we create a temporary document loader instance and pass it in the
didStartProvisionalLoad() notification. If the navigation policy
wants the navigation to be continued in blink, we promote the temporary loader
to provisional loader.

Most of the changes in this patch are boilerplate changes to the
didStartProvisionalLoad() function.

BUG=638900
==========

[ananta, 2017-02-02 01:16:23]

The CQ bit was checked by ananta@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-02 01:16:50]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[ananta, 2017-02-02 01:20:52]

The CQ bit was checked by ananta@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-02 01:21:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

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

https://codereview.chromium.org/2661743002/diff/60001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2661743002/diff/60001/content/renderer/render...
content/renderer/render_frame_impl.cc:3459: if (!ds)
On 2017/01/31 22:13:32, Nate Chapin wrote:
> 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).

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.

https://codereview.chromium.org/2661743002/diff/60001/content/renderer/render...
content/renderer/render_frame_impl.cc:3464: "url",
ds->getRequest().url().string().utf8());
On 2017/01/31 22:13:32, Nate Chapin wrote:
> ds->getRequest() -> request ?

Not needed with the latest patch

https://codereview.chromium.org/2661743002/diff/60001/content/renderer/render...
content/renderer/render_frame_impl.cc:3473: } else if
(ds->replacesCurrentHistoryItem()) {
On 2017/01/31 22:13:32, Nate Chapin wrote:
> As mentioned above, could we plumb this bit?

Not needed with the latest patch

https://codereview.chromium.org/2661743002/diff/60001/content/renderer/render...
content/renderer/render_frame_impl.cc:3490: routing_id_, ds->getRequest().url(),
navigation_start));
On 2017/01/31 22:13:32, Nate Chapin wrote:
> ds->getRequest() -> request ?

Not needed with the latest patch

https://codereview.chromium.org/2661743002/diff/60001/content/renderer/render...
content/renderer/render_frame_impl.cc:6195: didStartProvisionalLoad(frame_,
info.urlRequest);
On 2017/01/31 22:13:32, Nate Chapin wrote:
> 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.

Done.

[commit-bot: I haz the power, 2017-02-02 01:37:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-02 01:37:57]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[ananta, 2017-02-02 02:17:51]

The CQ bit was checked by ananta@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-02 02:18:10]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-02 02:20:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-02 02:20:48]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[ananta, 2017-02-02 02:39:09]

The CQ bit was checked by ananta@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-02 02:39:30]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-02 03:49:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-02 03:49:56]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Nate Chapin, 2017-02-02 19:16:48]

I'm liking where this is going :)

Just a couple more nitpicks, sorry :/

https://codereview.chromium.org/2661743002/diff/140001/components/test_runner...
File components/test_runner/web_frame_test_client.cc (right):

https://codereview.chromium.org/2661743002/diff/140001/components/test_runner...
components/test_runner/web_frame_test_client.cc:417: blink::WebLocalFrame*
frame,
Could we use web_frame_test_proxy_base_->web_frame() below and get rid of the
Frame* parameter? RenderFrameImpl::didStartProvisionalLoad should just be able
to use RenderFrameImpl::frame_

https://codereview.chromium.org/2661743002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/FrameLoader.cpp (left):

https://codereview.chromium.org/2661743002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:1503: DCHECK_EQ(loader,
m_documentLoader);
I assume this is because tempLoader->detachFromFrame() calls in to
FrameLoader::loadFailed()?

https://codereview.chromium.org/2661743002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/2661743002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:1743: DocumentLoader*
tempLoader(client()->createDocumentLoader(
I don't love the name tempLoader. newLoader? Just plain 'loader'? policyLoader
(i.e., what it was called in the WebKit era)?

If you think all of these are worse than tempLoader, feel free to leave it
as-is.

[ananta, 2017-02-02 23:48:08]

Description was changed from

==========
PlzNavigate: Invoke didStartProvisionalLoad() when the renderer initiates a
navigation early on in FrameLoader::startLoad()

A number of layouttests fail because the didStartProvisionalLoad() notification
comes in for PlzNavigate after
WebFrameClient::willSendRequest() is called. In the non plznavigate case
willSendRequest() is called when a navigation
is committed in blink i.e. it is loaded by a frame.  In the PlzNavigate case
willSendRequest is called when a frame navigation
is sent to the browser via RenderFrameImpl::BeginNavigation().

Approach being tried is to invoke didStartProvisionalLoad() before we check the
navigation policy in FrameLoader::startLoad()
We need a working DocumentLoader instance because the
RenderFrameImpl::didStartProvisionalLoad() function saves the navigationState
in the provisional document loader instance. Longer term we want to get rid of
provisional document loader accessor usage. To achieve
this we create a temporary document loader instance and pass it in the
didStartProvisionalLoad() notification. If the navigation policy
wants the navigation to be continued in blink, we promote the temporary loader
to provisional loader.

Most of the changes in this patch are boilerplate changes to the
didStartProvisionalLoad() function.

BUG=638900
==========

to

==========
PlzNavigate: Invoke didStartProvisionalLoad() when the renderer initiates a
navigation early on in FrameLoader::startLoad()

A number of layouttests fail because the didStartProvisionalLoad() notification
comes in for PlzNavigate after
didFinishDocumentLoad() notification is called. In the non PlzNavigate case the
load is handled in blink and hence the
notifications happen in the correct order.

Approach being tried is to invoke didStartProvisionaLoad() in
FrameLoader::startLoad() when we find that a navigation is being
handled by the client. Ideally we would want to invoke this notification early
regardless of whether PlzNavigate is on. However
a number of tests and codepaths assume that the current document would be
unloaded before we receive the provisional load notification
for the non PlzNavigate case. We create a temporary loader just for the purpose
of firing the didStartProvisionalLoad()
notification.

Most of the changes in this patch are boilerplate changes to the
didStartProvisionalLoad() function.

BUG=638900
==========

[ananta, 2017-02-02 23:54:03]

https://codereview.chromium.org/2661743002/diff/140001/components/test_runner...
File components/test_runner/web_frame_test_client.cc (right):

https://codereview.chromium.org/2661743002/diff/140001/components/test_runner...
components/test_runner/web_frame_test_client.cc:417: blink::WebLocalFrame*
frame,
On 2017/02/02 19:16:48, Nate Chapin wrote:
> Could we use web_frame_test_proxy_base_->web_frame() below and get rid of the
> Frame* parameter? RenderFrameImpl::didStartProvisionalLoad should just be able
> to use RenderFrameImpl::frame_

Done.

https://codereview.chromium.org/2661743002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/FrameLoader.cpp (left):

https://codereview.chromium.org/2661743002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:1503: DCHECK_EQ(loader,
m_documentLoader);
On 2017/02/02 19:16:48, Nate Chapin wrote:
> I assume this is because tempLoader->detachFromFrame() calls in to
> FrameLoader::loadFailed()?

Reverted this change

https://codereview.chromium.org/2661743002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/2661743002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:1743: DocumentLoader*
tempLoader(client()->createDocumentLoader(
On 2017/02/02 19:16:48, Nate Chapin wrote:
> I don't love the name tempLoader. newLoader? Just plain 'loader'? policyLoader
> (i.e., what it was called in the WebKit era)?
> 
> If you think all of these are worse than tempLoader, feel free to leave it
> as-is.

Moved this code to a helper function FrameLoader::createDocumentLoader()

[ananta, 2017-02-03 00:01:52]

The CQ bit was checked by ananta@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-03 00:03:19]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Nate Chapin, 2017-02-03 00:17:38]

lgtm

https://codereview.chromium.org/2661743002/diff/220001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/navigation/back-to-dynamic-iframe.html
(right):

https://codereview.chromium.org/2661743002/diff/220001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/navigation/back-to-dynamic-iframe.html:11:
setTimeout(function () {
The changes in this file look accidental?

[commit-bot: I haz the power, 2017-02-03 00:33:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-03 00:33:25]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[ananta, 2017-02-03 01:35:14]

https://codereview.chromium.org/2661743002/diff/220001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/navigation/back-to-dynamic-iframe.html
(right):

https://codereview.chromium.org/2661743002/diff/220001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/navigation/back-to-dynamic-iframe.html:11:
setTimeout(function () {
On 2017/02/03 00:17:38, Nate Chapin wrote:
> The changes in this file look accidental?

Yes. Reverted

[ananta, 2017-02-03 01:37:28]

The CQ bit was checked by ananta@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-03 01:38:10]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-03 02:36:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-03 02:36:54]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[ananta, 2017-02-03 03:52:01]

The CQ bit was checked by ananta@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-03 03:52:59]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-03 06:35:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-03 06:35:14]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Nate Chapin, 2017-02-03 19:35:22]

lgtm

https://codereview.chromium.org/2661743002/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/2661743002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:1736: if
(m_isNavigationHandledByClient &&
Nit: Can we make this if() block part of the new if
(m_isNavigationHandledByClient) {) block?

https://codereview.chromium.org/2661743002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:1793:
client()->dispatchDidStartProvisionalLoad(m_provisionalDocumentLoader);
Why did this move back here? No objections, just curious :)

[ananta, 2017-02-03 20:42:01]

https://codereview.chromium.org/2661743002/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/2661743002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:1736: if
(m_isNavigationHandledByClient &&
On 2017/02/03 19:35:22, Nate Chapin wrote:
> Nit: Can we make this if() block part of the new if
> (m_isNavigationHandledByClient) {) block?

Done.

https://codereview.chromium.org/2661743002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:1793:
client()->dispatchDidStartProvisionalLoad(m_provisionalDocumentLoader);
On 2017/02/03 19:35:22, Nate Chapin wrote:
> Why did this move back here? No objections, just curious :)

Sending the provisional load notification early causes a number of tests to
break in the non plznavigate case. Turns out that there are assumptions that the
current page unload handlers would be called before subsequent load
notifications etc.

[ananta, 2017-02-03 20:44:47]

Description was changed from

==========
PlzNavigate: Invoke didStartProvisionalLoad() when the renderer initiates a
navigation early on in FrameLoader::startLoad()

A number of layouttests fail because the didStartProvisionalLoad() notification
comes in for PlzNavigate after
didFinishDocumentLoad() notification is called. In the non PlzNavigate case the
load is handled in blink and hence the
notifications happen in the correct order.

Approach being tried is to invoke didStartProvisionaLoad() in
FrameLoader::startLoad() when we find that a navigation is being
handled by the client. Ideally we would want to invoke this notification early
regardless of whether PlzNavigate is on. However
a number of tests and codepaths assume that the current document would be
unloaded before we receive the provisional load notification
for the non PlzNavigate case. We create a temporary loader just for the purpose
of firing the didStartProvisionalLoad()
notification.

Most of the changes in this patch are boilerplate changes to the
didStartProvisionalLoad() function.

BUG=638900
==========

to

==========
PlzNavigate: Invoke didStartProvisionalLoad() when the renderer initiates a
navigation early on in FrameLoader::startLoad()

A number of layouttests fail because the didStartProvisionalLoad() notification
comes in for PlzNavigate after
didFinishDocumentLoad() notification is called. In the non PlzNavigate case the
load is handled in blink and hence the
notifications happen in the correct order.

Approach being tried is to invoke didStartProvisionaLoad() in
FrameLoader::startLoad() when we find that a navigation is being
handled by the client. Ideally we would want to invoke this notification early
regardless of whether PlzNavigate is on. However
a number of tests and codepaths assume that the current document would be
unloaded before we receive the provisional load notification
for the non PlzNavigate case. We create a temporary loader just for the purpose
of firing the didStartProvisionalLoad()
notification.

Most of the changes in this patch are boilerplate changes to the
didStartProvisionalLoad() function.

BUG=638900
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[ananta, 2017-02-03 20:45:18]

The CQ bit was checked by ananta@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-03 20:45:46]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[ananta, 2017-02-03 21:40:32]

ananta@chromium.org changed reviewers:
+ lazyboy@chromium.org

[ananta, 2017-02-03 21:40:33]

+lazyboy for extensions owners

[commit-bot: I haz the power, 2017-02-03 23:40:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-03 23:40:45]

Dry run: This issue passed the CQ dry run.

[lazyboy, 2017-02-06 22:30:55]

extension_frame_helper.* lgtm

[ananta, 2017-02-06 22:40:03]

The CQ bit was checked by ananta@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-06 22:41:15]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-07 00:38:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-07 00:38:25]

Dry run: Try jobs failed on following builders:
  linux_site_isolation on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_site_isol...)

[ananta, 2017-02-07 00:57:36]

The CQ bit was checked by ananta@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-07 00:58:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-07 01:18:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-07 01:18:17]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[ananta, 2017-02-07 01:24:32]

The CQ bit was checked by ananta@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-07 01:25:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-07 02:20:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-07 02:20:32]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[ananta, 2017-02-07 03:06:28]

The CQ bit was checked by ananta@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-07 03:06:46]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-07 06:25:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-07 06:25:56]

Dry run: This issue passed the CQ dry run.

[clamy, 2017-02-07 12:40:50]

Thanks! content/ lgtm.

[jam, 2017-02-07 19:23:00]

files outside webkit/content lgtm

[ananta, 2017-02-07 19:26:44]

The CQ bit was checked by ananta@chromium.org

[ananta, 2017-02-07 19:26:44]

The patchset sent to the CQ was uploaded after l-g-t-m from japhet@chromium.org,
lazyboy@chromium.org
Link to the patchset: https://codereview.chromium.org/2661743002/#ps360001
(title: "Remove CHECK for redirect chain as the redirects may not be populated
for provisional loads for ren…")

[commit-bot: I haz the power, 2017-02-07 19:27:28]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-07 19:34:05]

CQ is committing da patch.

Bot data: {"patchset_id": 360001, "attempt_start_ts": 1486495604041260,
"parent_rev": "ce2dd23c6d23adae7ceec51a2b5d7ec4f9a6c941", "commit_rev":
"1a023a38bddf1b2d4e64a1f31175b72e1776713d"}

[commit-bot: I haz the power, 2017-02-07 19:35:32]

Description was changed from

==========
PlzNavigate: Invoke didStartProvisionalLoad() when the renderer initiates a
navigation early on in FrameLoader::startLoad()

A number of layouttests fail because the didStartProvisionalLoad() notification
comes in for PlzNavigate after
didFinishDocumentLoad() notification is called. In the non PlzNavigate case the
load is handled in blink and hence the
notifications happen in the correct order.

Approach being tried is to invoke didStartProvisionaLoad() in
FrameLoader::startLoad() when we find that a navigation is being
handled by the client. Ideally we would want to invoke this notification early
regardless of whether PlzNavigate is on. However
a number of tests and codepaths assume that the current document would be
unloaded before we receive the provisional load notification
for the non PlzNavigate case. We create a temporary loader just for the purpose
of firing the didStartProvisionalLoad()
notification.

Most of the changes in this patch are boilerplate changes to the
didStartProvisionalLoad() function.

BUG=638900
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
PlzNavigate: Invoke didStartProvisionalLoad() when the renderer initiates a
navigation early on in FrameLoader::startLoad()

A number of layouttests fail because the didStartProvisionalLoad() notification
comes in for PlzNavigate after
didFinishDocumentLoad() notification is called. In the non PlzNavigate case the
load is handled in blink and hence the
notifications happen in the correct order.

Approach being tried is to invoke didStartProvisionaLoad() in
FrameLoader::startLoad() when we find that a navigation is being
handled by the client. Ideally we would want to invoke this notification early
regardless of whether PlzNavigate is on. However
a number of tests and codepaths assume that the current document would be
unloaded before we receive the provisional load notification
for the non PlzNavigate case. We create a temporary loader just for the purpose
of firing the didStartProvisionalLoad()
notification.

Most of the changes in this patch are boilerplate changes to the
didStartProvisionalLoad() function.

BUG=638900
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2661743002
Cr-Commit-Position: refs/heads/master@{#448695}
Committed:
https://chromium.googlesource.com/chromium/src/+/1a023a38bddf1b2d4e64a1f31175...
==========

[commit-bot: I haz the power, 2017-02-07 19:35:34]

Committed patchset #19 (id:360001) as
https://chromium.googlesource.com/chromium/src/+/1a023a38bddf1b2d4e64a1f31175...