Implement transfer navigation with mojo

Implement TransferNavigation with mojo

This CL implements transfer navigation which transfer a content::ResourceLoader
from one renderer to another.

BUG=603396
Committed: https://crrev.com/bbc904d0f1d2c8db77e9344aaa3779c27d04a055
Cr-Commit-Position: refs/heads/master@{#434569}

[yhirano, 2016-11-14 12:06:42]

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

[commit-bot: I haz the power, 2016-11-14 12:06:50]

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

[yhirano, 2016-11-14 12:09:12]

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

[commit-bot: I haz the power, 2016-11-14 12:09:20]

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

[yhirano, 2016-11-14 13:16:30]

Patchset #1 (id:1) has been deleted

[commit-bot: I haz the power, 2016-11-14 13:49:06]

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

[commit-bot: I haz the power, 2016-11-14 13:49:06]

Dry run: This issue passed the CQ dry run.

[yhirano, 2016-11-15 02:31:42]

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

[commit-bot: I haz the power, 2016-11-15 02:34:07]

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, 2016-11-15 04:33:31]

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

[commit-bot: I haz the power, 2016-11-15 04:33:32]

Dry run: This issue passed the CQ dry run.

[yhirano, 2016-11-15 09:29:07]

Description was changed from

==========
Implement TransferNavigation with mojo

BUG=603396
==========

to

==========
Implement TransferNavigation with mojo

This CL implements transfer navigation which transfer a resource loading
operation
from one renderer to another.

BUG=603396
==========

[yhirano, 2016-11-15 09:29:07]

yhirano@chromium.org changed reviewers:
+ mmenke@chromium.org, tzik@chromium.org

[yhirano, 2016-11-15 09:32:48]

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

[commit-bot: I haz the power, 2016-11-15 09:32:59]

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

[yhirano, 2016-11-15 09:33:32]

Patchset #2 (id:40001) has been deleted

[yhirano, 2016-11-15 09:33:36]

Patchset #1 (id:20001) has been deleted

[yhirano, 2016-11-15 09:34:30]

Description was changed from

==========
Implement TransferNavigation with mojo

This CL implements transfer navigation which transfer a resource loading
operation
from one renderer to another.

BUG=603396
==========

to

==========
Implement TransferNavigation with mojo

This CL implements transfer navigation which transfer a content::ResourceLoader
from one renderer to another.

BUG=603396
==========

[commit-bot: I haz the power, 2016-11-15 09:35:02]

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

[commit-bot: I haz the power, 2016-11-15 09:35:03]

Dry run: Try jobs failed on following builders:
  blimp_linux_dbg on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/blimp_linux_dbg...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[yhirano, 2016-11-15 09:36:50]

yhirano@chromium.org changed reviewers:
- mmenke@chromium.org, tzik@chromium.org

[yhirano, 2016-11-15 09:44:15]

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

[commit-bot: I haz the power, 2016-11-15 09:44:24]

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

[yhirano, 2016-11-15 09:45:51]

Patchset #1 (id:60001) has been deleted

[commit-bot: I haz the power, 2016-11-15 11:07:26]

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

[commit-bot: I haz the power, 2016-11-15 11:07:27]

Dry run: This issue passed the CQ dry run.

[yhirano, 2016-11-15 12:55:15]

yhirano@chromium.org changed reviewers:
+ mmenke@chromium.org, tzik@chromium.org

[mmenke, 2016-11-15 15:53:21]

Question:  Do we really want to do this?

Regardless, I'm not a good reviewer for how transfers work, and don't want to
spend time learning the code, given PlzNavigate is eventually happening.  Maybe
use clamy as a reviewer instead?  Happy to defer to her, she's also a content/
owner.

[yhirano, 2016-11-15 17:19:40]

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

[yhirano, 2016-11-15 17:22:59]

On 2016/11/15 15:53:21, mmenke wrote:
> Question:  Do we really want to do this?
> 
> Regardless, I'm not a good reviewer for how transfers work, and don't want to
> spend time learning the code, given PlzNavigate is eventually happening. 
Maybe
> use clamy as a reviewer instead?  Happy to defer to her, she's also a content/
> owner.

+clamy@

[mmenke, 2016-11-15 21:47:33]

mmenke@chromium.org changed reviewers:
- mmenke@chromium.org

[clamy, 2016-11-16 16:38:54]

Thanks! A few comments below.

Apart from that, do you have a timeline of when the switch to Mojo in loading is
happening? It may not be worth the effort to convert transfers to Mojo if we
make them go away when PlzNavigate launches.

https://codereview.chromium.org/2496193002/diff/80001/content/browser/cross_s...
File content/browser/cross_site_transfer_browsertest.cc (right):

https://codereview.chromium.org/2496193002/diff/80001/content/browser/cross_s...
content/browser/cross_site_transfer_browsertest.cc:219:
command_line->AppendSwitchASCII(switches::kEnableBlinkFeatures,
If this is used in several places, it could be abstracted behind a function in
browser_tests helpers, named for example EnableLoadingWithMojo.

https://codereview.chromium.org/2496193002/diff/80001/content/browser/loader/...
File content/browser/loader/resource_request_info_impl.h (right):

https://codereview.chromium.org/2496193002/diff/80001/content/browser/loader/...
content/browser/loader/resource_request_info_impl.h:133:
mojom::URLLoaderAssociatedRequest url_loader_request,
I don't think ResourceRequestInfo is the right place to do the mojo-related
work, as it's meant to store parameters related to the request and not execute a
lot of logic. I would suggest using content::ResourceLoader instead, if it still
exist in the Mojo world. content::ResourceLoader contains several functions
related to transfers, so I would use this instead.

[tzik, 2016-11-17 06:22:03]

lgtm

[yhirano, 2016-11-17 11:25:33]

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

[commit-bot: I haz the power, 2016-11-17 11:25:51]

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

[yhirano, 2016-11-17 14:11:17]

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

[commit-bot: I haz the power, 2016-11-17 14:11:42]

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, 2016-11-17 15:17:10]

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

[commit-bot: I haz the power, 2016-11-17 15:17:10]

Dry run: This issue passed the CQ dry run.

[yhirano, 2016-11-17 15:23:19]

Thanks for your comments.

> Apart from that, do you have a timeline of when the switch to Mojo in loading
is
> happening? It may not be worth the effort to convert transfers to Mojo if we
> make them go away when PlzNavigate launches.

We are making efforts to make functional tests pass with mojo-loading in this
quarter.  Some tests are failing because of this missing feature and I would
like to implement it.

https://codereview.chromium.org/2496193002/diff/80001/content/browser/cross_s...
File content/browser/cross_site_transfer_browsertest.cc (right):

https://codereview.chromium.org/2496193002/diff/80001/content/browser/cross_s...
content/browser/cross_site_transfer_browsertest.cc:219:
command_line->AppendSwitchASCII(switches::kEnableBlinkFeatures,
On 2016/11/16 16:38:53, clamy wrote:
> If this is used in several places, it could be abstracted behind a function in
> browser_tests helpers, named for example EnableLoadingWithMojo.

This is the first time we are doing this.

https://codereview.chromium.org/2496193002/diff/80001/content/browser/loader/...
File content/browser/loader/resource_request_info_impl.h (right):

https://codereview.chromium.org/2496193002/diff/80001/content/browser/loader/...
content/browser/loader/resource_request_info_impl.h:133:
mojom::URLLoaderAssociatedRequest url_loader_request,
On 2016/11/16 16:38:53, clamy wrote:
> I don't think ResourceRequestInfo is the right place to do the mojo-related
> work, as it's meant to store parameters related to the request and not execute
a
> lot of logic. I would suggest using content::ResourceLoader instead, if it
still
> exist in the Mojo world. content::ResourceLoader contains several functions
> related to transfers, so I would use this instead.

Thanks. How about having a callback in NavigatorResourceThrottle? ResourceLoader
already has a callback and introducing another one looks a bit confusing to me.

[clamy, 2016-11-21 17:55:54]

Thanks! A few more comments below.

https://codereview.chromium.org/2496193002/diff/80001/content/browser/loader/...
File content/browser/loader/resource_request_info_impl.h (right):

https://codereview.chromium.org/2496193002/diff/80001/content/browser/loader/...
content/browser/loader/resource_request_info_impl.h:133:
mojom::URLLoaderAssociatedRequest url_loader_request,
On 2016/11/17 15:23:19, yhirano wrote:
> On 2016/11/16 16:38:53, clamy wrote:
> > I don't think ResourceRequestInfo is the right place to do the mojo-related
> > work, as it's meant to store parameters related to the request and not
execute
> a
> > lot of logic. I would suggest using content::ResourceLoader instead, if it
> still
> > exist in the Mojo world. content::ResourceLoader contains several functions
> > related to transfers, so I would use this instead.
> 
> Thanks. How about having a callback in NavigatorResourceThrottle?
ResourceLoader
> already has a callback and introducing another one looks a bit confusing to
me.

Ok after seeing the other patchset, I think the version in ResourceRequestInfo
was better. Can you add more comments about what the callback is used for, and a
DCHECK similar to what I mentioned in NavigationResourceThrottle? Thanks!

https://codereview.chromium.org/2496193002/diff/120001/content/browser/loader...
File content/browser/loader/navigation_resource_throttle.cc (right):

https://codereview.chromium.org/2496193002/diff/120001/content/browser/loader...
content/browser/loader/navigation_resource_throttle.cc:375: if (on_transfer_) {
Could you dcheck this only happens when loading with mojo is activated?

https://codereview.chromium.org/2496193002/diff/120001/content/browser/loader...
File content/browser/loader/navigation_resource_throttle.h (right):

https://codereview.chromium.org/2496193002/diff/120001/content/browser/loader...
content/browser/loader/navigation_resource_throttle.h:61:
mojom::URLLoaderAssociatedRequest mojo_request,
Should mojo_request/url_load_client be passed as const ref or pointers? It seems
this copy them, unless they're aliases for something else.

https://codereview.chromium.org/2496193002/diff/120001/content/browser/loader...
File content/browser/loader/resource_loader.h (right):

https://codereview.chromium.org/2496193002/diff/120001/content/browser/loader...
content/browser/loader/resource_loader.h:140: // Called when a navigation has
finished transfer.
Can you mention that this will update the NavigationThrottle? Now that we have
two TransferCallbacks, the code becomes less clear.

[yhirano, 2016-11-22 03:54:21]

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

[commit-bot: I haz the power, 2016-11-22 03:54:42]

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, 2016-11-22 05:13:43]

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

[commit-bot: I haz the power, 2016-11-22 05:13:44]

Dry run: This issue passed the CQ dry run.

[yhirano, 2016-11-22 05:52:39]

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

[commit-bot: I haz the power, 2016-11-22 05:52:56]

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

[yhirano, 2016-11-22 06:04:17]

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

[commit-bot: I haz the power, 2016-11-22 06:04: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, 2016-11-22 07:47:45]

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

[commit-bot: I haz the power, 2016-11-22 07:47:46]

Dry run: This issue passed the CQ dry run.

[yhirano, 2016-11-22 08:12:55]

https://codereview.chromium.org/2496193002/diff/80001/content/browser/loader/...
File content/browser/loader/resource_request_info_impl.h (right):

https://codereview.chromium.org/2496193002/diff/80001/content/browser/loader/...
content/browser/loader/resource_request_info_impl.h:133:
mojom::URLLoaderAssociatedRequest url_loader_request,
On 2016/11/21 17:55:54, clamy (slow) wrote:
> On 2016/11/17 15:23:19, yhirano wrote:
> > On 2016/11/16 16:38:53, clamy wrote:
> > > I don't think ResourceRequestInfo is the right place to do the
mojo-related
> > > work, as it's meant to store parameters related to the request and not
> execute
> > a
> > > lot of logic. I would suggest using content::ResourceLoader instead, if it
> > still
> > > exist in the Mojo world. content::ResourceLoader contains several
functions
> > > related to transfers, so I would use this instead.
> > 
> > Thanks. How about having a callback in NavigatorResourceThrottle?
> ResourceLoader
> > already has a callback and introducing another one looks a bit confusing to
> me.
> 
> Ok after seeing the other patchset, I think the version in ResourceRequestInfo
> was better. Can you add more comments about what the callback is used for, and
a
> DCHECK similar to what I mentioned in NavigationResourceThrottle? Thanks! 

Done. Unfortunately, ResourceRequestInfoImpl doesn't know if mojo-loading is
enabled or not. It is controlled by a runtime flag in the renderer.

https://codereview.chromium.org/2496193002/diff/120001/content/browser/loader...
File content/browser/loader/navigation_resource_throttle.h (right):

https://codereview.chromium.org/2496193002/diff/120001/content/browser/loader...
content/browser/loader/navigation_resource_throttle.h:61:
mojom::URLLoaderAssociatedRequest mojo_request,
On 2016/11/21 17:55:54, clamy (slow) wrote:
> Should mojo_request/url_load_client be passed as const ref or pointers? It
seems
> this copy them, unless they're aliases for something else.

These are movable-but-not-copyable types, so I think it's OK.

[clamy, 2016-11-24 10:08:36]

Thanks! Lgtm.

[yhirano, 2016-11-25 14:04:20]

The CQ bit was checked by yhirano@chromium.org

[yhirano, 2016-11-25 14:04:20]

The patchset sent to the CQ was uploaded after l-g-t-m from tzik@chromium.org
Link to the patchset: https://codereview.chromium.org/2496193002/#ps180001
(title: "fix")

[commit-bot: I haz the power, 2016-11-25 14:04:37]

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, 2016-11-25 16:11:33]

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

[commit-bot: I haz the power, 2016-11-25 16:11:34]

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

[yhirano, 2016-11-26 04:53:11]

The CQ bit was checked by yhirano@chromium.org

[commit-bot: I haz the power, 2016-11-26 04:53:23]

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, 2016-11-26 05:59:33]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1480135991397470,
"parent_rev": "bf9c865b1eeaee6e2f30814efc00dd905c34b2f0", "commit_rev":
"ba6829b80de7de1d3d5790c9468ea257a355d434"}

[commit-bot: I haz the power, 2016-11-26 06:00:09]

Description was changed from

==========
Implement TransferNavigation with mojo

This CL implements transfer navigation which transfer a content::ResourceLoader
from one renderer to another.

BUG=603396
==========

to

==========
Implement TransferNavigation with mojo

This CL implements transfer navigation which transfer a content::ResourceLoader
from one renderer to another.

BUG=603396
==========

[commit-bot: I haz the power, 2016-11-26 06:00:10]

Committed patchset #6 (id:180001)

[commit-bot: I haz the power, 2016-11-26 06:02:37]

Description was changed from

==========
Implement TransferNavigation with mojo

This CL implements transfer navigation which transfer a content::ResourceLoader
from one renderer to another.

BUG=603396
==========

to

==========
Implement TransferNavigation with mojo

This CL implements transfer navigation which transfer a content::ResourceLoader
from one renderer to another.

BUG=603396

Committed: https://crrev.com/bbc904d0f1d2c8db77e9344aaa3779c27d04a055
Cr-Commit-Position: refs/heads/master@{#434569}
==========

[commit-bot: I haz the power, 2016-11-26 06:02:39]

Patchset 6 (id:??) landed as
https://crrev.com/bbc904d0f1d2c8db77e9344aaa3779c27d04a055
Cr-Commit-Position: refs/heads/master@{#434569}