[Mojo-Loading] Implement URLLoader::SetPriority

[Mojo-Loading] Implement URLLoader::SetPriority

This CL implements URLLoader::SetPriority corresponding to
ResourceHostMsg_DidChangePriority IPC message.

BUG=603396
Review-Url: https://codereview.chromium.org/2646343007
Cr-Commit-Position: refs/heads/master@{#446657}
Committed: https://chromium.googlesource.com/chromium/src/+/933ee58984d38b13be78bb9859b5e9109ad565a5

[yhirano, 2017-01-24 06:57:08]

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

[commit-bot: I haz the power, 2017-01-24 06:57: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-24 08:25:49]

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

[commit-bot: I haz the power, 2017-01-24 08:25:50]

Dry run: This issue passed the CQ dry run.

[yhirano, 2017-01-24 08:32:39]

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

[yhirano, 2017-01-24 08:32:42]

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

[commit-bot: I haz the power, 2017-01-24 08:32:47]

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

[tzik, 2017-01-24 10:49:46]

lgtm

[commit-bot: I haz the power, 2017-01-24 10:53:15]

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

[commit-bot: I haz the power, 2017-01-24 10:53:16]

Dry run: This issue passed the CQ dry run.

[yhirano, 2017-01-24 11:00:10]

yhirano@chromium.org changed reviewers:
+ dcheng@chromium.org, horo@chromium.org, mmenke@chromium.org

[yhirano, 2017-01-24 11:00:11]

+mmenke@ for content/browser/loader
+horo@ for content/browser/service_worker
+dcheng@ for url_loader.mojom

[mmenke, 2017-01-24 16:57:52]

LGTM

https://codereview.chromium.org/2646343007/diff/20001/content/browser/loader/...
File content/browser/loader/mojo_async_resource_handler_unittest.cc (right):

https://codereview.chromium.org/2646343007/diff/20001/content/browser/loader/...
content/browser/loader/mojo_async_resource_handler_unittest.cc:808: 4, 2, false,
request_.get());
Should label the 4 and 2 with comments.

https://codereview.chromium.org/2646343007/diff/20001/content/browser/loader/...
content/browser/loader/mojo_async_resource_handler_unittest.cc:812:
handler_->SetPriority(mojom::RequestPriority::kHighest, 4);
Please label this 4 as well.

[dcheng, 2017-01-24 19:07:18]

https://codereview.chromium.org/2646343007/diff/20001/content/browser/loader/...
File content/browser/loader/mojo_async_resource_handler.cc (right):

https://codereview.chromium.org/2646343007/diff/20001/content/browser/loader/...
content/browser/loader/mojo_async_resource_handler.cc:82: return net::HIGHEST;
Can we add an EnumTraits and typemap this instead?

https://codereview.chromium.org/2646343007/diff/20001/content/common/url_load...
File content/common/url_loader.mojom (right):

https://codereview.chromium.org/2646343007/diff/20001/content/common/url_load...
content/common/url_loader.mojom:37: SetPriority(RequestPriority priority, int32
intra_priority_value);
Nit: maybe document what intra_priority_value is? I assume it's used to
prioritize within a given priority level.

[horo, 2017-01-25 00:57:22]

service_worker_fetch_dispatcher.cc lgtm

[yhirano, 2017-01-25 03:02:54]

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

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

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

[kinuko, 2017-01-25 03:48:30]

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

[kinuko, 2017-01-25 03:48:31]

https://codereview.chromium.org/2646343007/diff/40001/content/child/resource_...
File content/child/resource_dispatcher.cc (right):

https://codereview.chromium.org/2646343007/diff/40001/content/child/resource_...
content/child/resource_dispatcher.cc:109: }
Could these use enum traits?

[commit-bot: I haz the power, 2017-01-25 04:41:07]

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

[commit-bot: I haz the power, 2017-01-25 04:41:08]

Dry run: This issue passed the CQ dry run.

[yhirano, 2017-01-27 06:50:59]

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

[commit-bot: I haz the power, 2017-01-27 06:51:15]

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

[yhirano, 2017-01-27 06:53:47]

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

[commit-bot: I haz the power, 2017-01-27 06:54:08]

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 06:57:41]

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

[commit-bot: I haz the power, 2017-01-27 06:57:42]

Dry run: 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...)

[yhirano, 2017-01-27 06:58:28]

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

[commit-bot: I haz the power, 2017-01-27 06:58:48]

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

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

[commit-bot: I haz the power, 2017-01-27 07:07:11]

Dry run: 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...)

[yhirano, 2017-01-27 07:47:08]

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

[commit-bot: I haz the power, 2017-01-27 07:47:21]

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

[yhirano, 2017-01-27 07:52:23]

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

[commit-bot: I haz the power, 2017-01-27 07:52:45]

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

[yhirano, 2017-01-27 07:53:55]

https://codereview.chromium.org/2646343007/diff/20001/content/browser/loader/...
File content/browser/loader/mojo_async_resource_handler.cc (right):

https://codereview.chromium.org/2646343007/diff/20001/content/browser/loader/...
content/browser/loader/mojo_async_resource_handler.cc:82: return net::HIGHEST;
On 2017/01/24 19:07:18, dcheng wrote:
> Can we add an EnumTraits and typemap this instead?

Done.

https://codereview.chromium.org/2646343007/diff/20001/content/browser/loader/...
File content/browser/loader/mojo_async_resource_handler_unittest.cc (right):

https://codereview.chromium.org/2646343007/diff/20001/content/browser/loader/...
content/browser/loader/mojo_async_resource_handler_unittest.cc:808: 4, 2, false,
request_.get());
On 2017/01/24 16:57:52, mmenke wrote:
> Should label the 4 and 2 with comments.

I gave names to these values.

https://codereview.chromium.org/2646343007/diff/20001/content/browser/loader/...
content/browser/loader/mojo_async_resource_handler_unittest.cc:812:
handler_->SetPriority(mojom::RequestPriority::kHighest, 4);
On 2017/01/24 16:57:52, mmenke wrote:
> Please label this 4 as well.

I gave a name to the value.

https://codereview.chromium.org/2646343007/diff/20001/content/common/url_load...
File content/common/url_loader.mojom (right):

https://codereview.chromium.org/2646343007/diff/20001/content/common/url_load...
content/common/url_loader.mojom:37: SetPriority(RequestPriority priority, int32
intra_priority_value);
On 2017/01/24 19:07:18, dcheng wrote:
> Nit: maybe document what intra_priority_value is? I assume it's used to
> prioritize within a given priority level.

I found no document but it looks so according to the implementation.

https://codereview.chromium.org/2646343007/diff/40001/content/child/resource_...
File content/child/resource_dispatcher.cc (right):

https://codereview.chromium.org/2646343007/diff/40001/content/child/resource_...
content/child/resource_dispatcher.cc:109: }
On 2017/01/25 03:48:31, kinuko wrote:
> Could these use enum traits?

Done.

[commit-bot: I haz the power, 2017-01-27 07:58:56]

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

[commit-bot: I haz the power, 2017-01-27 07:58:56]

Dry run: 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...)

[kinuko, 2017-01-27 09:03:21]

content/child lgtm

[yhirano, 2017-01-27 09:05:17]

On 2017/01/27 09:03:21, kinuko wrote:
> content/child lgtm

Thanks. Could you please have a look at content/common changes other than mojom
and typemap?

[dcheng, 2017-01-27 09:09:34]

mojo lgtm

https://codereview.chromium.org/2646343007/diff/140001/content/common/url_req...
File content/common/url_request_struct_traits.cc (right):

https://codereview.chromium.org/2646343007/diff/140001/content/common/url_req...
content/common/url_request_struct_traits.cc:54: 
Nit: NOTREACHED()?

[yhirano, 2017-01-27 09:13:55]

https://codereview.chromium.org/2646343007/diff/140001/content/common/url_req...
File content/common/url_request_struct_traits.cc (right):

https://codereview.chromium.org/2646343007/diff/140001/content/common/url_req...
content/common/url_request_struct_traits.cc:54: 
On 2017/01/27 09:09:33, dcheng wrote:
> Nit: NOTREACHED()?

Can we have NOTREACHED() here? I think we need to handle any possible (including
malformed) values here. Is there another protection layer for malformed
requests?

[kinuko, 2017-01-27 09:18:48]

On 2017/01/27 09:05:17, yhirano wrote:
> On 2017/01/27 09:03:21, kinuko wrote:
> > content/child lgtm
> 
> Thanks. Could you please have a look at content/common changes other than
mojom
> and typemap?

The change lgtm overall!

[kinuko, 2017-01-27 09:25:28]

On 2017/01/27 09:13:55, yhirano wrote:
>
https://codereview.chromium.org/2646343007/diff/140001/content/common/url_req...
> File content/common/url_request_struct_traits.cc (right):
> 
>
https://codereview.chromium.org/2646343007/diff/140001/content/common/url_req...
> content/common/url_request_struct_traits.cc:54: 
> On 2017/01/27 09:09:33, dcheng wrote:
> > Nit: NOTREACHED()?
> 
> Can we have NOTREACHED() here? I think we need to handle any possible
(including
> malformed) values here. Is there another protection layer for malformed
> requests?

Mojo generates a validator and we seem to have error code for enum
(VALIDATION_ERROR_UNKNOWN_ENUM_VALUE).... I guess probably malformed requests
are validated there?

[dcheng, 2017-01-27 09:32:33]

On 2017/01/27 09:25:28, kinuko wrote:
> On 2017/01/27 09:13:55, yhirano wrote:
> >
>
https://codereview.chromium.org/2646343007/diff/140001/content/common/url_req...
> > File content/common/url_request_struct_traits.cc (right):
> > 
> >
>
https://codereview.chromium.org/2646343007/diff/140001/content/common/url_req...
> > content/common/url_request_struct_traits.cc:54: 
> > On 2017/01/27 09:09:33, dcheng wrote:
> > > Nit: NOTREACHED()?
> > 
> > Can we have NOTREACHED() here? I think we need to handle any possible
> (including
> > malformed) values here. Is there another protection layer for malformed
> > requests?
> 
> Mojo generates a validator and we seem to have error code for enum
> (VALIDATION_ERROR_UNKNOWN_ENUM_VALUE).... I guess probably malformed requests
> are validated there?

Yes, I think Mojo enums are validated unless they are annotated with
[Extensible] (even when typemapped)

[yhirano, 2017-01-27 09:42:11]

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

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

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

[yhirano, 2017-01-27 09:46:36]

Added

+per-file *.typemap=set noparent
+per-file *.typemap=file://ipc/SECURITY_OWNERS
+

to content/common/OWNERS to suppress a presubmit warning.

https://codereview.chromium.org/2646343007/diff/140001/content/common/url_req...
File content/common/url_request_struct_traits.cc (right):

https://codereview.chromium.org/2646343007/diff/140001/content/common/url_req...
content/common/url_request_struct_traits.cc:54: 
On 2017/01/27 09:13:55, yhirano wrote:
> On 2017/01/27 09:09:33, dcheng wrote:
> > Nit: NOTREACHED()?
> 
> Can we have NOTREACHED() here? I think we need to handle any possible
(including
> malformed) values here. Is there another protection layer for malformed
> requests?

Done (It returns true to let the compiler optimize the function. In any way, we
don't care about this section).

[yhirano, 2017-01-27 09:48:02]

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

[commit-bot: I haz the power, 2017-01-27 09:48: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-01-27 11:38:54]

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

[commit-bot: I haz the power, 2017-01-27 11:38:55]

Dry run: This issue passed the CQ dry run.

[yhirano, 2017-01-27 13:24:42]

The CQ bit was checked by yhirano@chromium.org

[yhirano, 2017-01-27 13:24:42]

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

[commit-bot: I haz the power, 2017-01-27 13:24:57]

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 13:28:44]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1485523482431880,
"parent_rev": "74c7e4b3a075d01cb9f04b8c746fa8bc888ed768", "commit_rev":
"933ee58984d38b13be78bb9859b5e9109ad565a5"}

[commit-bot: I haz the power, 2017-01-27 13:29:24]

Description was changed from

==========
[Mojo-Loading] Implement URLLoader::SetPriority

This CL implements URLLoader::SetPriority corresponding to
ResourceHostMsg_DidChangePriority IPC message.

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

to

==========
[Mojo-Loading] Implement URLLoader::SetPriority

This CL implements URLLoader::SetPriority corresponding to
ResourceHostMsg_DidChangePriority IPC message.

BUG=603396

Review-Url: https://codereview.chromium.org/2646343007
Cr-Commit-Position: refs/heads/master@{#446657}
Committed:
https://chromium.googlesource.com/chromium/src/+/933ee58984d38b13be78bb9859b5...
==========

[commit-bot: I haz the power, 2017-01-27 13:29:26]

Committed patchset #10 (id:180001) as
https://chromium.googlesource.com/chromium/src/+/933ee58984d38b13be78bb9859b5...