Implement Mojo SyncLoad

Implement Mojo SyncLoad

This CL add a Mojo IPC for synchronous resource loading, and plumps it
from content::ResourceDispatcher to content::ResourceDispatcherHost.

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

[tzik, 2016-10-04 16:50:41]

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

[commit-bot: I haz the power, 2016-10-04 16:50:59]

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

[tzik, 2016-10-04 16:51:13]

Description was changed from

==========
Implement Mojo SyncLoad

BUG=
==========

to

==========
Implement Mojo SyncLoad

WIP

BUG=
==========

[tzik, 2016-10-04 16:51:13]

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

[tzik, 2016-10-04 16:55:55]

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

[commit-bot: I haz the power, 2016-10-04 16:56:16]

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-10-04 17:28:53]

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

[commit-bot: I haz the power, 2016-10-04 17:28:53]

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

[tzik, 2016-10-06 14:37:33]

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

[commit-bot: I haz the power, 2016-10-06 14:37:57]

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

[tzik, 2016-10-06 14:44:14]

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

[commit-bot: I haz the power, 2016-10-06 14:44:25]

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-10-06 15:55:49]

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

[commit-bot: I haz the power, 2016-10-06 15:55:50]

Dry run: This issue passed the CQ dry run.

[tzik, 2016-10-07 12:31:33]

Description was changed from

==========
Implement Mojo SyncLoad

WIP

BUG=
==========

to

==========
Implement Mojo SyncLoad

TODO: description

BUG=
==========

[tzik, 2016-10-12 05:45:57]

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

[commit-bot: I haz the power, 2016-10-12 05:46:10]

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

[tzik, 2016-10-12 05:49:38]

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

[commit-bot: I haz the power, 2016-10-12 05:49:52]

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

[tzik, 2016-10-12 05:52:26]

Description was changed from

==========
Implement Mojo SyncLoad

TODO: description

BUG=
==========

to

==========
Implement Mojo SyncLoad

BUG=
==========

[tzik, 2016-10-12 05:52:39]

PTAL

[yhirano, 2016-10-12 10:55:30]

https://codereview.chromium.org/2390983003/diff/160001/content/browser/loader...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/2390983003/diff/160001/content/browser/loader...
content/browser/loader/resource_dispatcher_host_impl.cc:2275: int32_t
routing_id,
int?

https://codereview.chromium.org/2390983003/diff/160001/content/browser/loader...
content/browser/loader/resource_dispatcher_host_impl.cc:2279: const
SyncLoadResultCallback& cb) {
"result_handler" might be better.

https://codereview.chromium.org/2390983003/diff/160001/content/browser/loader...
File content/browser/loader/url_loader_factory_impl.cc (right):

https://codereview.chromium.org/2390983003/diff/160001/content/browser/loader...
content/browser/loader/url_loader_factory_impl.cc:63: // TODO(yhirano): Provide
the right routing ID.
Please provide the routing ID.

https://codereview.chromium.org/2390983003/diff/160001/content/common/url_syn...
File content/common/url_sync_load_result.typemap (right):

https://codereview.chromium.org/2390983003/diff/160001/content/common/url_syn...
content/common/url_sync_load_result.typemap:6: public_headers = [
"//content/common/resource_request.h" ]
Should this be content/public/common/resource_response.h?

[tzik, 2016-10-12 12:05:41]

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

[commit-bot: I haz the power, 2016-10-12 12:05:47]

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

[tzik, 2016-10-12 12:06:01]

https://codereview.chromium.org/2390983003/diff/160001/content/browser/loader...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/2390983003/diff/160001/content/browser/loader...
content/browser/loader/resource_dispatcher_host_impl.cc:2275: int32_t
routing_id,
On 2016/10/12 10:55:30, yhirano wrote:
> int?

Done.

https://codereview.chromium.org/2390983003/diff/160001/content/browser/loader...
content/browser/loader/resource_dispatcher_host_impl.cc:2279: const
SyncLoadResultCallback& cb) {
On 2016/10/12 10:55:30, yhirano wrote:
> "result_handler" might be better.

Done.

https://codereview.chromium.org/2390983003/diff/160001/content/browser/loader...
File content/browser/loader/url_loader_factory_impl.cc (right):

https://codereview.chromium.org/2390983003/diff/160001/content/browser/loader...
content/browser/loader/url_loader_factory_impl.cc:63: // TODO(yhirano): Provide
the right routing ID.
On 2016/10/12 10:55:30, yhirano wrote:
> Please provide the routing ID.

Done.

https://codereview.chromium.org/2390983003/diff/160001/content/common/url_syn...
File content/common/url_sync_load_result.typemap (right):

https://codereview.chromium.org/2390983003/diff/160001/content/common/url_syn...
content/common/url_sync_load_result.typemap:6: public_headers = [
"//content/common/resource_request.h" ]
On 2016/10/12 10:55:30, yhirano wrote:
> Should this be content/public/common/resource_response.h?

Done.

[commit-bot: I haz the power, 2016-10-12 13:43:56]

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

[commit-bot: I haz the power, 2016-10-12 13:43:56]

Dry run: This issue passed the CQ dry run.

[yhirano, 2016-10-13 04:40:05]

lgtm

Please add some issue description.

[tzik, 2016-10-13 04:58:22]

Description was changed from

==========
Implement Mojo SyncLoad

BUG=
==========

to

==========
Implement Mojo SyncLoad

This CL add a Mojo IPC for synchronous resource loading, and plumps it
from content::ResourceDispatcher to content::ResourceDispatcherHost.

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

[tzik, 2016-10-13 04:59:51]

tzik@chromium.org changed reviewers:
+ dcheng@chromium.org

[tzik, 2016-10-13 05:03:25]

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

[tzik, 2016-10-13 05:04:08]

tzik@chromium.org changed reviewers:
+ avi@chromium.org

[tzik, 2016-10-13 05:13:05]

Adding OWNERS. Could you PTAL to this?

dcheng: *.mojom and *.typemap
mmenke: //content/browser/loader and overall
avi: //content/common and //content/child

[kinuko, 2016-10-13 05:35:59]

kinuko@chromium.org changed reviewers:
+ kinuko@chromium.org

[kinuko, 2016-10-13 05:36:00]

drive-by questions...

https://codereview.chromium.org/2390983003/diff/180001/content/browser/loader...
File content/browser/loader/url_loader_factory_impl.cc (right):

https://codereview.chromium.org/2390983003/diff/180001/content/browser/loader...
content/browser/loader/url_loader_factory_impl.cc:21: if (!result) {
sorry, could you teach me when this could happen?

https://codereview.chromium.org/2390983003/diff/180001/content/browser/loader...
content/browser/loader/url_loader_factory_impl.cc:64: SyncLoadResult result;
Do we need this line?

[tzik, 2016-10-13 06:31:26]

https://codereview.chromium.org/2390983003/diff/180001/content/browser/loader...
File content/browser/loader/url_loader_factory_impl.cc (right):

https://codereview.chromium.org/2390983003/diff/180001/content/browser/loader...
content/browser/loader/url_loader_factory_impl.cc:21: if (!result) {
On 2016/10/13 05:36:00, kinuko (slow) wrote:
> sorry, could you teach me when this could happen?

This can be called from the dtor of SyncResourceHandler. It's probably on a
channel error or a network error?
https://chromium.googlesource.com/chromium/src/+/d41ce0ce55bb756c85f63c94ddac...

https://codereview.chromium.org/2390983003/diff/180001/content/browser/loader...
content/browser/loader/url_loader_factory_impl.cc:64: SyncLoadResult result;
On 2016/10/13 05:36:00, kinuko (slow) wrote:
> Do we need this line?

Oops. No, it's needed. Removed.

[tzik, 2016-10-13 06:31:40]

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

[commit-bot: I haz the power, 2016-10-13 06:31:53]

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-10-13 07:18:53]

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

[commit-bot: I haz the power, 2016-10-13 07:18:55]

Dry run: This issue passed the CQ dry run.

[kinuko, 2016-10-13 11:39:01]

content/child lg % one more question for the error handling

https://codereview.chromium.org/2390983003/diff/180001/content/browser/loader...
File content/browser/loader/url_loader_factory_impl.cc (right):

https://codereview.chromium.org/2390983003/diff/180001/content/browser/loader...
content/browser/loader/url_loader_factory_impl.cc:21: if (!result) {
On 2016/10/13 06:31:26, tzik wrote:
> On 2016/10/13 05:36:00, kinuko (slow) wrote:
> > sorry, could you teach me when this could happen?
> 
> This can be called from the dtor of SyncResourceHandler. It's probably on a
> channel error or a network error?
>
https://chromium.googlesource.com/chromium/src/+/d41ce0ce55bb756c85f63c94ddac...

I see thanks.  Maybe worth commenting that result could be null when
SyncResourceHandler gets destructed?

https://codereview.chromium.org/2390983003/diff/200001/content/child/resource...
File content/child/resource_dispatcher.cc (right):

https://codereview.chromium.org/2390983003/diff/200001/content/child/resource...
content/child/resource_dispatcher.cc:626: result.error_code == net::ERR_FAILED)
{
It's a bit unclear if specifically handling ERR_FAILED here makes a lot sense,
do you expect it is returned (only) for SyncResourceHandler dtor case above?  
(In any error_code != OK cases we seem to be only referring
response->{error_code,url})

[mmenke, 2016-10-13 15:07:04]

Should we have some tests for this?  Simple cases would be:

1)  Success.
2)  Network error.
3)  Cancellation (Where a NULL result is passed along).

Also, I agree with Kinuko's comment on ERR_FAILED.

[mmenke, 2016-10-13 15:07:54]

On 2016/10/13 15:07:04, mmenke wrote:
> Should we have some tests for this?  Simple cases would be:
> 
> 1)  Success.
> 2)  Network error.
> 3)  Cancellation (Where a NULL result is passed along).
> 
> Also, I agree with Kinuko's comment on ERR_FAILED.

Otherwise, looks good.

[Avi (use Gerrit), 2016-10-13 15:23:52]

I'm not super up-to-speed on mojo. Stamp LGTM with other mojo people are happy.

[dcheng, 2016-10-14 05:23:27]

https://codereview.chromium.org/2390983003/diff/200001/content/browser/loader...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/2390983003/diff/200001/content/browser/loader...
content/browser/loader/resource_dispatcher_host_impl.cc:2281:
BeginRequest(request_id, request_data, result_handler, routing_id,
Sorry, I don't understand this code very well, but the comments on
https://cs.chromium.org/chromium/src/content/browser/loader/resource_dispatch...
say route id is only valid for async, but here it's set? Is the comment out of
date?

https://codereview.chromium.org/2390983003/diff/200001/content/common/url_syn...
File content/common/url_sync_load_result.typemap (right):

https://codereview.chromium.org/2390983003/diff/200001/content/common/url_syn...
content/common/url_sync_load_result.typemap:12: "//ui/base",
Are all these deps required?

[tzik, 2016-10-14 07:25:54]

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

[commit-bot: I haz the power, 2016-10-14 07:26:09]

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

[tzik, 2016-10-14 08:01:48]

On 2016/10/13 15:07:04, mmenke wrote:
> Should we have some tests for this?  Simple cases would be:
> 
> 1)  Success.
> 2)  Network error.
> 3)  Cancellation (Where a NULL result is passed along).
> 
> Also, I agree with Kinuko's comment on ERR_FAILED.

I added virtual layout tests config, that starts passing after this CL.
They covers 1) success cases and 2) network error cases.
For (3), can we leave a TODO comment here to do it in the next CL?

[tzik, 2016-10-14 08:02:00]

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

[commit-bot: I haz the power, 2016-10-14 08:02:15]

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

[tzik, 2016-10-14 08:02:31]

https://codereview.chromium.org/2390983003/diff/180001/content/browser/loader...
File content/browser/loader/url_loader_factory_impl.cc (right):

https://codereview.chromium.org/2390983003/diff/180001/content/browser/loader...
content/browser/loader/url_loader_factory_impl.cc:21: if (!result) {
On 2016/10/13 11:39:01, kinuko (slowish) wrote:
> On 2016/10/13 06:31:26, tzik wrote:
> > On 2016/10/13 05:36:00, kinuko (slow) wrote:
> > > sorry, could you teach me when this could happen?
> > 
> > This can be called from the dtor of SyncResourceHandler. It's probably on a
> > channel error or a network error?
> >
>
https://chromium.googlesource.com/chromium/src/+/d41ce0ce55bb756c85f63c94ddac...
> 
> I see thanks.  Maybe worth commenting that result could be null when
> SyncResourceHandler gets destructed?

Done.

https://codereview.chromium.org/2390983003/diff/200001/content/browser/loader...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/2390983003/diff/200001/content/browser/loader...
content/browser/loader/resource_dispatcher_host_impl.cc:2281:
BeginRequest(request_id, request_data, result_handler, routing_id,
On 2016/10/14 05:23:27, dcheng wrote:
> Sorry, I don't understand this code very well, but the comments on
>
https://cs.chromium.org/chromium/src/content/browser/loader/resource_dispatch...
> say route id is only valid for async, but here it's set? Is the comment out of
> date?

This was to align to OnSyncLoad impl.
I think the comment is obsolete, since existing OnSyncLoad sets the value and
there are some hook that uses the route id to identify the renderer. E.g.
web_request_api.cc and chrome_signin_helper.cc.
mmenke: Could you double check this?

Let me remove the comment on the header.

https://codereview.chromium.org/2390983003/diff/200001/content/child/resource...
File content/child/resource_dispatcher.cc (right):

https://codereview.chromium.org/2390983003/diff/200001/content/child/resource...
content/child/resource_dispatcher.cc:626: result.error_code == net::ERR_FAILED)
{
On 2016/10/13 11:39:01, kinuko (slowish) wrote:
> It's a bit unclear if specifically handling ERR_FAILED here makes a lot sense,
> do you expect it is returned (only) for SyncResourceHandler dtor case above?  
> (In any error_code != OK cases we seem to be only referring
> response->{error_code,url})

OK, let me remove the special handling.

https://codereview.chromium.org/2390983003/diff/200001/content/common/url_syn...
File content/common/url_sync_load_result.typemap (right):

https://codereview.chromium.org/2390983003/diff/200001/content/common/url_syn...
content/common/url_sync_load_result.typemap:12: "//ui/base",
On 2016/10/14 05:23:27, dcheng wrote:
> Are all these deps required?

Hmm? Looks like none of them are needed.
Done. Removed.

[commit-bot: I haz the power, 2016-10-14 09:49:35]

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

[commit-bot: I haz the power, 2016-10-14 09:49:36]

Dry run: This issue passed the CQ dry run.

[mmenke, 2016-10-14 14:48:56]

On 2016/10/14 08:01:48, tzik wrote:
> On 2016/10/13 15:07:04, mmenke wrote:
> > Should we have some tests for this?  Simple cases would be:
> > 
> > 1)  Success.
> > 2)  Network error.
> > 3)  Cancellation (Where a NULL result is passed along).
> > 
> > Also, I agree with Kinuko's comment on ERR_FAILED.
> 
> I added virtual layout tests config, that starts passing after this CL.
> They covers 1) success cases and 2) network error cases.
> For (3), can we leave a TODO comment here to do it in the next CL?

Just to ensure the initial implementation works, I'd rather land the
cancellation test in this CL, or a CL landed before this (Looks like you're
re-using an existing test suite, which should handle cancellation without mojo,
too, so seems like the test can be landed before this one).

[dcheng, 2016-10-14 17:24:07]

mojo lgtm

[tzik, 2016-10-18 11:55:36]

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

[commit-bot: I haz the power, 2016-10-18 11:55:50]

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

[tzik, 2016-10-18 13:04:40]

On 2016/10/14 14:48:56, mmenke wrote:
> On 2016/10/14 08:01:48, tzik wrote:
> > On 2016/10/13 15:07:04, mmenke wrote:
> > > Should we have some tests for this?  Simple cases would be:
> > > 
> > > 1)  Success.
> > > 2)  Network error.
> > > 3)  Cancellation (Where a NULL result is passed along).
> > > 
> > > Also, I agree with Kinuko's comment on ERR_FAILED.
> > 
> > I added virtual layout tests config, that starts passing after this CL.
> > They covers 1) success cases and 2) network error cases.
> > For (3), can we leave a TODO comment here to do it in the next CL?
> 
> Just to ensure the initial implementation works, I'd rather land the
> cancellation test in this CL, or a CL landed before this (Looks like you're
> re-using an existing test suite, which should handle cancellation without
mojo,
> too, so seems like the test can be landed before this one).

OK, added a test in this CL, and I sent another CL to add tests for SyncLoad
without mojo. PTAL.

[commit-bot: I haz the power, 2016-10-18 13:41:01]

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

[commit-bot: I haz the power, 2016-10-18 13:41:03]

Dry run: This issue passed the CQ dry run.

[mmenke, 2016-10-20 15:38:35]

On 2016/10/18 13:04:40, tzik wrote:
> On 2016/10/14 14:48:56, mmenke wrote:
> > On 2016/10/14 08:01:48, tzik wrote:
> > > On 2016/10/13 15:07:04, mmenke wrote:
> > > > Should we have some tests for this?  Simple cases would be:
> > > > 
> > > > 1)  Success.
> > > > 2)  Network error.
> > > > 3)  Cancellation (Where a NULL result is passed along).
> > > > 
> > > > Also, I agree with Kinuko's comment on ERR_FAILED.
> > > 
> > > I added virtual layout tests config, that starts passing after this CL.
> > > They covers 1) success cases and 2) network error cases.
> > > For (3), can we leave a TODO comment here to do it in the next CL?
> > 
> > Just to ensure the initial implementation works, I'd rather land the
> > cancellation test in this CL, or a CL landed before this (Looks like you're
> > re-using an existing test suite, which should handle cancellation without
> mojo,
> > too, so seems like the test can be landed before this one).
> 
> OK, added a test in this CL, and I sent another CL to add tests for SyncLoad
> without mojo. PTAL.

LGTM, thanks again for adding the test!

[kinuko, 2016-10-20 16:17:39]

lgtm/2 just in case it matters-- great to see all those additional tests!

[tzik, 2016-10-24 01:39:44]

The CQ bit was checked by tzik@chromium.org

[tzik, 2016-10-24 01:39:45]

The patchset sent to the CQ was uploaded after l-g-t-m from
yhirano@chromium.org, avi@chromium.org, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2390983003/#ps260001
(title: "+tests")

[commit-bot: I haz the power, 2016-10-24 01:39:55]

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-10-24 03:18:06]

Description was changed from

==========
Implement Mojo SyncLoad

This CL add a Mojo IPC for synchronous resource loading, and plumps it
from content::ResourceDispatcher to content::ResourceDispatcherHost.

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

to

==========
Implement Mojo SyncLoad

This CL add a Mojo IPC for synchronous resource loading, and plumps it
from content::ResourceDispatcher to content::ResourceDispatcherHost.

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

[commit-bot: I haz the power, 2016-10-24 03:18:07]

Committed patchset #14 (id:260001)

[commit-bot: I haz the power, 2016-10-24 03:21:05]

Description was changed from

==========
Implement Mojo SyncLoad

This CL add a Mojo IPC for synchronous resource loading, and plumps it
from content::ResourceDispatcher to content::ResourceDispatcherHost.

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

to

==========
Implement Mojo SyncLoad

This CL add a Mojo IPC for synchronous resource loading, and plumps it
from content::ResourceDispatcher to content::ResourceDispatcherHost.

BUG=603396

Committed: https://crrev.com/4020282e126730319280b79d29c13464be2d6be5
Cr-Commit-Position: refs/heads/master@{#427011}
==========

[commit-bot: I haz the power, 2016-10-24 03:21:06]

Patchset 14 (id:??) landed as
https://crrev.com/4020282e126730319280b79d29c13464be2d6be5
Cr-Commit-Position: refs/heads/master@{#427011}