PlzNavigate: transmit redirect info to the renderer side

PlzNavigate: transmit redirect info to the renderer side

This CL ensures that the net::RedirectInfo received during redirects are
transmitted to the renderer and that the renderer is made aware of
those. This fixes an issue with DevTools not being aware of the redirect
info on the renderer side.

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

Review-Url: https://codereview.chromium.org/2653953005
Cr-Commit-Position: refs/heads/master@{#451645}
Committed: https://chromium.googlesource.com/chromium/src/+/c6b06865571bf675f2d5b692af3bf25cbceb7732

[clamy, 2017-01-25 17:45:37]

Description was changed from

==========
PlzNavigate: transmit redirect info to the renderer side

This CL ensures that the net::RedirectInfo received during redirects are
transmitted to the renderer and that the renderer is made aware of
those. This fixes an issue with DevTools not being aware of the redirect
info on the renderer side.

BUG=575210
==========

to

==========
PlzNavigate: transmit redirect info to the renderer side

This CL ensures that the net::RedirectInfo received during redirects are
transmitted to the renderer and that the renderer is made aware of
those. This fixes an issue with DevTools not being aware of the redirect
info on the renderer side.

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

[clamy, 2017-01-26 16:29:46]

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

[commit-bot: I haz the power, 2017-01-26 16:30: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-01-26 17:35:47]

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

[commit-bot: I haz the power, 2017-01-26 17:35:48]

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-01-27 14:30:04]

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

[commit-bot: I haz the power, 2017-01-27 14:30:22]

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-27 15:25:08]

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

[commit-bot: I haz the power, 2017-01-27 15:25:09]

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-02-07 15:15:35]

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

[commit-bot: I haz the power, 2017-02-07 15:15:51]

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 16:22:14]

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

[commit-bot: I haz the power, 2017-02-07 16:22:15]

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

[clamy, 2017-02-09 13:04:12]

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

[commit-bot: I haz the power, 2017-02-09 13:04:29]

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-09 15:29:43]

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

[commit-bot: I haz the power, 2017-02-09 15:29:44]

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

[clamy, 2017-02-09 16:59:36]

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

[clamy, 2017-02-09 17:00:01]

Description was changed from

==========
PlzNavigate: transmit redirect info to the renderer side

This CL ensures that the net::RedirectInfo received during redirects are
transmitted to the renderer and that the renderer is made aware of
those. This fixes an issue with DevTools not being aware of the redirect
info on the renderer side.

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

to

==========
PlzNavigate: transmit redirect info to the renderer side

This CL ensures that the net::RedirectInfo received during redirects are
transmitted to the renderer and that the renderer is made aware of
those. This fixes an issue with DevTools not being aware of the redirect
info on the renderer side.

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

[clamy, 2017-02-09 17:00:01]

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

[commit-bot: I haz the power, 2017-02-09 17:00:14]

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

[clamy, 2017-02-09 17:02:07]

@nasko: PTAL. This CL fixes the issue where DevTools needs to be notified about
redirects that happened during navigation. Basically what this CL does is fake
on the renderer side everything that happened in PlzNavigate in the browser
process during navigation. One question in particular: should we drop this
information when we get redirected cross-site?

[commit-bot: I haz the power, 2017-02-09 19:39:12]

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

[commit-bot: I haz the power, 2017-02-09 19:39:13]

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

[nasko, 2017-02-10 23:53:09]

Looks good overall, I just have one question about reusing existing data.

https://codereview.chromium.org/2653953005/diff/100001/content/browser/frame_...
File content/browser/frame_host/navigation_request.cc (right):

https://codereview.chromium.org/2653953005/diff/100001/content/browser/frame_...
content/browser/frame_host/navigation_request.cc:277: common_params.url,
common_params.method,
nit: Put each argument on a separate line to be consistent with the rest.

https://codereview.chromium.org/2653953005/diff/100001/content/browser/frame_...
File content/browser/frame_host/navigator_impl.cc (right):

https://codereview.chromium.org/2653953005/diff/100001/content/browser/frame_...
content/browser/frame_host/navigator_impl.cc:464: frame_entry, GURL(),
std::string(),
Shouldn't this be dest_url and the actual method?

https://codereview.chromium.org/2653953005/diff/100001/content/child/web_url_...
File content/child/web_url_loader_impl.cc (left):

https://codereview.chromium.org/2653953005/diff/100001/content/child/web_url_...
content/child/web_url_loader_impl.cc:739: }
Just to make sure I understand, the reason we don't need this block anymore is
the new replaying of receiving redirects will properly populate the response
object?

https://codereview.chromium.org/2653953005/diff/100001/content/common/navigat...
File content/common/navigation_params.h (right):

https://codereview.chromium.org/2653953005/diff/100001/content/common/navigat...
content/common/navigation_params.h:267: std::string original_method;
Can the method change between redirects? If not, why not reuse the one from
CommonNavigationParams?

[clamy, 2017-02-13 14:29:27]

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

[clamy, 2017-02-13 14:29:37]

Thanks!

https://codereview.chromium.org/2653953005/diff/100001/content/browser/frame_...
File content/browser/frame_host/navigation_request.cc (right):

https://codereview.chromium.org/2653953005/diff/100001/content/browser/frame_...
content/browser/frame_host/navigation_request.cc:277: common_params.url,
common_params.method,
On 2017/02/10 23:53:09, nasko wrote:
> nit: Put each argument on a separate line to be consistent with the rest.

Done.

https://codereview.chromium.org/2653953005/diff/100001/content/browser/frame_...
File content/browser/frame_host/navigator_impl.cc (right):

https://codereview.chromium.org/2653953005/diff/100001/content/browser/frame_...
content/browser/frame_host/navigator_impl.cc:464: frame_entry, GURL(),
std::string(),
On 2017/02/10 23:53:09, nasko wrote:
> Shouldn't this be dest_url and the actual method?

I marked this parameter as PlzNavigate only, hence why I'm not initializing it
in the non-PlzNavigate case. In particular, I was not sure what to put in the
transfer navigation case: should this be the original url, or the url the
transferred renderer needs to navigate to? Since this is not meant to be used
outside of PlzNavigate, I though it was safer not to initialize it.

https://codereview.chromium.org/2653953005/diff/100001/content/child/web_url_...
File content/child/web_url_loader_impl.cc (left):

https://codereview.chromium.org/2653953005/diff/100001/content/child/web_url_...
content/child/web_url_loader_impl.cc:739: }
On 2017/02/10 23:53:09, nasko wrote:
> Just to make sure I understand, the reason we don't need this block anymore is
> the new replaying of receiving redirects will properly populate the response
> object?

Precisely.

https://codereview.chromium.org/2653953005/diff/100001/content/common/navigat...
File content/common/navigation_params.h (right):

https://codereview.chromium.org/2653953005/diff/100001/content/common/navigat...
content/common/navigation_params.h:267: std::string original_method;
On 2017/02/10 23:53:09, nasko wrote:
> Can the method change between redirects? If not, why not reuse the one from
> CommonNavigationParams?

Yes. It's possible for a POST (like a form submission for example) to be
redirected as a GET. See
https://cs.chromium.org/chromium/src/content/browser/frame_host/navigation_re...
(NavigationRequest::OnRequestRedirected) for where we update the method in
CommonNavigationParams.

[commit-bot: I haz the power, 2017-02-13 14:29:47]

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-13 16:50:14]

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

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

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

[nasko, 2017-02-14 17:25:02]

LGTM

https://codereview.chromium.org/2653953005/diff/100001/content/browser/frame_...
File content/browser/frame_host/navigator_impl.cc (right):

https://codereview.chromium.org/2653953005/diff/100001/content/browser/frame_...
content/browser/frame_host/navigator_impl.cc:464: frame_entry, GURL(),
std::string(),
On 2017/02/13 14:29:37, clamy wrote:
> On 2017/02/10 23:53:09, nasko wrote:
> > Shouldn't this be dest_url and the actual method?
> 
> I marked this parameter as PlzNavigate only, hence why I'm not initializing it
> in the non-PlzNavigate case. In particular, I was not sure what to put in the
> transfer navigation case: should this be the original url, or the url the
> transferred renderer needs to navigate to? Since this is not meant to be used
> outside of PlzNavigate, I though it was safer not to initialize it.

Acknowledged.

https://codereview.chromium.org/2653953005/diff/100001/content/common/navigat...
File content/common/navigation_params.h (right):

https://codereview.chromium.org/2653953005/diff/100001/content/common/navigat...
content/common/navigation_params.h:267: std::string original_method;
On 2017/02/13 14:29:37, clamy wrote:
> On 2017/02/10 23:53:09, nasko wrote:
> > Can the method change between redirects? If not, why not reuse the one from
> > CommonNavigationParams?
> 
> Yes. It's possible for a POST (like a form submission for example) to be
> redirected as a GET. See
>
https://cs.chromium.org/chromium/src/content/browser/frame_host/navigation_re...
> (NavigationRequest::OnRequestRedirected) for where we update the method in
> CommonNavigationParams.

Acknowledged.

[nasko, 2017-02-14 17:26:19]

The CQ bit was checked by nasko@chromium.org

[commit-bot: I haz the power, 2017-02-14 17:26:45]

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-14 17:34:22]

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

[commit-bot: I haz the power, 2017-02-14 17:34:23]

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

[clamy, 2017-02-15 12:06:07]

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

[clamy, 2017-02-15 12:06:23]

@japhet: PTAL

[clamy, 2017-02-17 13:44:28]

@japhet: friendly ping :).

[Nate Chapin, 2017-02-17 18:25:28]

LGTM with a small cleanup request

https://codereview.chromium.org/2653953005/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebDataSourceImpl.cpp (left):

https://codereview.chromium.org/2653953005/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebDataSourceImpl.cpp:86:
didRedirect(redirectChain[i], redirectChain[i + 1]);
I think didRedirect now just has a single caller, in
DocumentLoader::redirectReceived. Could we merge it into redirectReceived?

[clamy, 2017-02-20 15:42:01]

Thanks!

https://codereview.chromium.org/2653953005/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebDataSourceImpl.cpp (left):

https://codereview.chromium.org/2653953005/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebDataSourceImpl.cpp:86:
didRedirect(redirectChain[i], redirectChain[i + 1]);
On 2017/02/17 18:25:28, Nate Chapin wrote:
> I think didRedirect now just has a single caller, in
> DocumentLoader::redirectReceived. Could we merge it into redirectReceived?

Done.

[clamy, 2017-02-20 15:42:07]

The CQ bit was checked by clamy@chromium.org

[clamy, 2017-02-20 15:42:07]

The patchset sent to the CQ was uploaded after l-g-t-m from japhet@chromium.org,
nasko@chromium.org
Link to the patchset: https://codereview.chromium.org/2653953005/#ps160001
(title: "Rebase + addressed comments")

[commit-bot: I haz the power, 2017-02-20 15:42: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-20 16:40:10]

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

[commit-bot: I haz the power, 2017-02-20 16:40:13]

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

[clamy, 2017-02-20 17:03:39]

The CQ bit was checked by clamy@chromium.org

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

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-20 17:49:04]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1487610219406460,
"parent_rev": "56ea5e27823b0583bef03c6e5d9d746317a35cda", "commit_rev":
"c6b06865571bf675f2d5b692af3bf25cbceb7732"}

[commit-bot: I haz the power, 2017-02-20 17:49:46]

Description was changed from

==========
PlzNavigate: transmit redirect info to the renderer side

This CL ensures that the net::RedirectInfo received during redirects are
transmitted to the renderer and that the renderer is made aware of
those. This fixes an issue with DevTools not being aware of the redirect
info on the renderer side.

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

to

==========
PlzNavigate: transmit redirect info to the renderer side

This CL ensures that the net::RedirectInfo received during redirects are
transmitted to the renderer and that the renderer is made aware of
those. This fixes an issue with DevTools not being aware of the redirect
info on the renderer side.

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

Review-Url: https://codereview.chromium.org/2653953005
Cr-Commit-Position: refs/heads/master@{#451645}
Committed:
https://chromium.googlesource.com/chromium/src/+/c6b06865571bf675f2d5b692af3b...
==========

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

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