PlzNavigate: Fix the http/tests/loading/307-after-303-after-post.html and the http/tests/loading/re…

PlzNavigate: Fix the http/tests/loading/307-after-303-after-post.html and the http/tests/loading/redirect-methods.html layout tests.

These tests fail because willSendRequest() occurs before didStartProvisionalLoad(). In PlzNavigate willSendRequest is called when
we send a navigation to the browser via BeginNavigation().

Proposed fix is to remember the navigation state in decidePolicyForNavigation() and invoke BeginNavigation() when we
receive the didStartProvisionalLoad() notification.

We pass the WebURLRequest in WebFrameClient::didStartProvisionalLoad()

The other change is to remember the navigation state in a PendingNavigationInfo structure in
RenderFrameImpl. This is populated in decidePolicyForNavigation() and released in didStartProvisionalLoad().

BUG=647698, 625765
Review-Url: https://codereview.chromium.org/2734633002
Cr-Commit-Position: refs/heads/master@{#456510}
Committed: https://chromium.googlesource.com/chromium/src/+/878b2e67ac952b00b2fcafc54867f61b25507923

[ananta, 2017-03-04 01:38:27]

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

[commit-bot: I haz the power, 2017-03-04 01:38:39]

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

[ananta, 2017-03-04 01:38:59]

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

[commit-bot: I haz the power, 2017-03-04 04:24:46]

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

[commit-bot: I haz the power, 2017-03-04 04:24:47]

Dry run: This issue passed the CQ dry run.

[Nate Chapin, 2017-03-06 19:16:03]

I don't love stashing this information here, but it would be  a fair bit of
PlzNavigate-specific plumbing to get the data through didStartProvisionalLoad().
Still, I think it's worth it for the writeable request, rather than exposing it
on WebDataSource.

LGTM otherwise.

https://codereview.chromium.org/2734633002/diff/1/third_party/WebKit/public/w...
File third_party/WebKit/public/web/WebDataSource.h (right):

https://codereview.chromium.org/2734633002/diff/1/third_party/WebKit/public/w...
third_party/WebKit/public/web/WebDataSource.h:65: virtual WebURLRequest&
getRequestWritable() = 0;
Rather than exposing this as a one-off, let's pass the WebURLRequest as a
parameter on didStartProvisionalLoad.

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

Description was changed from

==========
PlzNavigate: Fix the http/tests/loading/307-after-303-after-post.html and the
http/tests/loading/redirect-methods.html layout tests.

These tests fail because willSendRequest() occurs before
didStartProvisionalLoad(). In PlzNavigate willSendRequest is called when
we send a navigation to the browser via BeginNavigation().

Proposed fix is to remember the navigation state in decidePolicyForNavigation()
and invoke BeginNavigation() when we
receive the didStartProvisionalLoad() notification.

I added a getter getRequestWritable to the blink WebDataSouce class as the
BeginNavigation() code path expects to
write to the request.

The other change is to remember the navigation state in a PendingNavigationInfo
structure in
RenderFrameImpl. This is populated in decidePolicyForNavigation() and released
in didStartProvisionalLoad().

BUG=647698,625765
==========

to

==========
PlzNavigate: Fix the http/tests/loading/307-after-303-after-post.html and the
http/tests/loading/redirect-methods.html layout tests.

These tests fail because willSendRequest() occurs before
didStartProvisionalLoad(). In PlzNavigate willSendRequest is called when
we send a navigation to the browser via BeginNavigation().

Proposed fix is to remember the navigation state in decidePolicyForNavigation()
and invoke BeginNavigation() when we
receive the didStartProvisionalLoad() notification.

We pass the WebURLRequest in WebFrameClient::didStartProvisionalLoad()

The other change is to remember the navigation state in a PendingNavigationInfo
structure in
RenderFrameImpl. This is populated in decidePolicyForNavigation() and released
in didStartProvisionalLoad().

BUG=647698,625765
==========

[ananta, 2017-03-06 22:42:05]

https://codereview.chromium.org/2734633002/diff/1/third_party/WebKit/public/w...
File third_party/WebKit/public/web/WebDataSource.h (right):

https://codereview.chromium.org/2734633002/diff/1/third_party/WebKit/public/w...
third_party/WebKit/public/web/WebDataSource.h:65: virtual WebURLRequest&
getRequestWritable() = 0;
On 2017/03/06 19:16:03, Nate Chapin wrote:
> Rather than exposing this as a one-off, let's pass the WebURLRequest as a
> parameter on didStartProvisionalLoad.

Done.

[Nate Chapin, 2017-03-06 22:46:38]

Still LGTM, just one more nit:

https://codereview.chromium.org/2734633002/diff/20001/third_party/WebKit/publ...
File third_party/WebKit/public/web/WebFrameClient.h (right):

https://codereview.chromium.org/2734633002/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/web/WebFrameClient.h:350: WebURLRequest* request) {}
Nit: WebURLRequest/ResourceRequest are usually passed as a reference&, not a
pointer*.

[ananta, 2017-03-06 23:14:21]

https://codereview.chromium.org/2734633002/diff/20001/third_party/WebKit/publ...
File third_party/WebKit/public/web/WebFrameClient.h (right):

https://codereview.chromium.org/2734633002/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/web/WebFrameClient.h:350: WebURLRequest* request) {}
On 2017/03/06 22:46:38, Nate Chapin wrote:
> Nit: WebURLRequest/ResourceRequest are usually passed as a reference&, not a
> pointer*.

Done.

[ananta, 2017-03-06 23:43:25]

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

[commit-bot: I haz the power, 2017-03-06 23:43:57]

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-03-06 23:47:28]

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

[commit-bot: I haz the power, 2017-03-06 23:47:29]

Dry run: Try jobs failed on following builders:
  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-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-03-07 00:19:04]

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

[commit-bot: I haz the power, 2017-03-07 00:20:06]

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-03-07 00:31:26]

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

[commit-bot: I haz the power, 2017-03-07 00:31:27]

Dry run: Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  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-03-07 00:36:58]

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

[commit-bot: I haz the power, 2017-03-07 00:37:58]

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-03-07 00:53:53]

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

[commit-bot: I haz the power, 2017-03-07 00:53:54]

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

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

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

[commit-bot: I haz the power, 2017-03-07 01:03:43]

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

[jam, 2017-03-07 01:13:49]

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

[jam, 2017-03-07 01:13:49]

->clamy as she'd be a better reviewer

[ananta, 2017-03-07 01:14:52]

+clamy

[commit-bot: I haz the power, 2017-03-07 01:22:02]

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

[commit-bot: I haz the power, 2017-03-07 01:22:03]

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

[ananta, 2017-03-07 01:35:32]

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

[commit-bot: I haz the power, 2017-03-07 01:36:38]

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-03-07 04:40:57]

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

[commit-bot: I haz the power, 2017-03-07 04:40:58]

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_...)

[clamy, 2017-03-09 14:12:10]

Thanks! I'm mostly ok with the patch, one comment below.

https://codereview.chromium.org/2734633002/diff/120001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2734633002/diff/120001/content/renderer/rende...
content/renderer/render_frame_impl.cc:3439: if (pending_navigation_info_.get())
{
Could we reset the pending_navigation_info_ at the beginning of
NavigateInternal? I'm worried we might have edge cases where we don't actually
create the dummy provisional DocumentLoader and didStartProvisionalLoad only
gets called when we try to commit a navigation.

[ananta, 2017-03-09 21:54:36]

https://codereview.chromium.org/2734633002/diff/120001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2734633002/diff/120001/content/renderer/rende...
content/renderer/render_frame_impl.cc:3439: if (pending_navigation_info_.get())
{
On 2017/03/09 14:12:09, clamy wrote:
> Could we reset the pending_navigation_info_ at the beginning of
> NavigateInternal? I'm worried we might have edge cases where we don't actually
> create the dummy provisional DocumentLoader and didStartProvisionalLoad only
> gets called when we try to commit a navigation.

Done.

[clamy, 2017-03-13 16:03:33]

Thanks! content/ lgtm % comments.

https://codereview.chromium.org/2734633002/diff/140001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2734633002/diff/140001/content/renderer/rende...
content/renderer/render_frame_impl.cc:3438: // If we have pending navigation to
be sent to the browser, do it here.
nit: s/pending navigation/a pending navigation

https://codereview.chromium.org/2734633002/diff/140001/content/renderer/rende...
content/renderer/render_frame_impl.cc:5886: // Lower bound for browser initiated
navigation start timepending_navigation_info_
I think this is an involuntary change :). Remove it?

[ananta, 2017-03-13 19:57:07]

https://codereview.chromium.org/2734633002/diff/140001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2734633002/diff/140001/content/renderer/rende...
content/renderer/render_frame_impl.cc:3438: // If we have pending navigation to
be sent to the browser, do it here.
On 2017/03/13 16:03:33, clamy wrote:
> nit: s/pending navigation/a pending navigation

Thanks Done.

https://codereview.chromium.org/2734633002/diff/140001/content/renderer/rende...
content/renderer/render_frame_impl.cc:5886: // Lower bound for browser initiated
navigation start timepending_navigation_info_
On 2017/03/13 16:03:33, clamy wrote:
> I think this is an involuntary change :). Remove it?

Done.

[ananta, 2017-03-13 19:57:43]

The CQ bit was checked by ananta@chromium.org

[ananta, 2017-03-13 19:57:43]

The patchset sent to the CQ was uploaded after l-g-t-m from japhet@chromium.org,
clamy@chromium.org
Link to the patchset: https://codereview.chromium.org/2734633002/#ps160001
(title: "Fix comment")

[commit-bot: I haz the power, 2017-03-13 19:58:27]

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-03-13 21:28:56]

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

[commit-bot: I haz the power, 2017-03-13 21:28:58]

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

[ananta, 2017-03-13 21:40:03]

The CQ bit was checked by ananta@chromium.org

[commit-bot: I haz the power, 2017-03-13 21:40:41]

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-03-13 22:40:30]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1489441203678490,
"parent_rev": "48a95392c57e24be127436d4e318a573467591df", "commit_rev":
"878b2e67ac952b00b2fcafc54867f61b25507923"}

[commit-bot: I haz the power, 2017-03-13 22:41:13]

Description was changed from

==========
PlzNavigate: Fix the http/tests/loading/307-after-303-after-post.html and the
http/tests/loading/redirect-methods.html layout tests.

These tests fail because willSendRequest() occurs before
didStartProvisionalLoad(). In PlzNavigate willSendRequest is called when
we send a navigation to the browser via BeginNavigation().

Proposed fix is to remember the navigation state in decidePolicyForNavigation()
and invoke BeginNavigation() when we
receive the didStartProvisionalLoad() notification.

We pass the WebURLRequest in WebFrameClient::didStartProvisionalLoad()

The other change is to remember the navigation state in a PendingNavigationInfo
structure in
RenderFrameImpl. This is populated in decidePolicyForNavigation() and released
in didStartProvisionalLoad().

BUG=647698,625765
==========

to

==========
PlzNavigate: Fix the http/tests/loading/307-after-303-after-post.html and the
http/tests/loading/redirect-methods.html layout tests.

These tests fail because willSendRequest() occurs before
didStartProvisionalLoad(). In PlzNavigate willSendRequest is called when
we send a navigation to the browser via BeginNavigation().

Proposed fix is to remember the navigation state in decidePolicyForNavigation()
and invoke BeginNavigation() when we
receive the didStartProvisionalLoad() notification.

We pass the WebURLRequest in WebFrameClient::didStartProvisionalLoad()

The other change is to remember the navigation state in a PendingNavigationInfo
structure in
RenderFrameImpl. This is populated in decidePolicyForNavigation() and released
in didStartProvisionalLoad().

BUG=647698,625765

Review-Url: https://codereview.chromium.org/2734633002
Cr-Commit-Position: refs/heads/master@{#456510}
Committed:
https://chromium.googlesource.com/chromium/src/+/878b2e67ac952b00b2fcafc54867...
==========

[commit-bot: I haz the power, 2017-03-13 22:41:14]

Committed patchset #9 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/878b2e67ac952b00b2fcafc54867...