Convert Background Fetch' input to a WebServiceWorkerRequest vector

Convert Background Fetch' input to a WebServiceWorkerRequest vector

Developers can pass in either an individual USVString or Request object,
or a sequence of either. Convert the input provided by the developer to
a sequence of WebServiceWorkerRequest objects, which can (soon) be send
over Mojo to the browser process.

BUG=692534, 692535
Review-Url: https://codereview.chromium.org/2762243002
Cr-Commit-Position: refs/heads/master@{#459465}
Committed: https://chromium.googlesource.com/chromium/src/+/9b34b21e479145817c93e1e4870ed2e87376eac0

[Peter Beverloo, 2017-03-21 15:52:27]

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

[commit-bot: I haz the power, 2017-03-21 15:52:40]

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

[Peter Beverloo, 2017-03-21 15:54:03]

peter@chromium.org changed reviewers:
+ haraken@chromium.org, harkness@chromium.org, yhirano@chromium.org

[Peter Beverloo, 2017-03-21 15:54:04]

+harkness for background fetch
+yhirano for Request object usage
+haraken, especially for BackgroundFetchManagerTest::getDictionaryForMethod()
and BUILD.gn addition.

[Peter Beverloo, 2017-03-21 16:12:09]

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

[commit-bot: I haz the power, 2017-03-21 16:12:39]

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

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

[commit-bot: I haz the power, 2017-03-21 16:48:15]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Peter Beverloo, 2017-03-22 14:29:09]

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

[commit-bot: I haz the power, 2017-03-22 14:29:26]

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-22 15:58:19]

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

[commit-bot: I haz the power, 2017-03-22 15:58:20]

Dry run: This issue passed the CQ dry run.

[harkness, 2017-03-22 17:47:31]

background fetch lgtm

https://codereview.chromium.org/2762243002/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/background_fetch/BackgroundFetchManager.cpp
(right):

https://codereview.chromium.org/2762243002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/background_fetch/BackgroundFetchManager.cpp:151:
} else if (requests.isRequest() || requests.isUSVString()) {
optional: personally, this would read better for me as 

} else if (requests.isRequest()) {
...
} else if (requests.isUSVString()) {
...
} else

I think it would actually be shorter, and it would avoid the extra if statement
on 155. Up to you though.

[Peter Beverloo, 2017-03-23 19:57:43]

Thank you! PTAL

https://codereview.chromium.org/2762243002/diff/60001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/background_fetch/BackgroundFetchManager.cpp
(right):

https://codereview.chromium.org/2762243002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/background_fetch/BackgroundFetchManager.cpp:151:
} else if (requests.isRequest() || requests.isUSVString()) {
On 2017/03/22 17:47:30, harkness wrote:
> optional: personally, this would read better for me as 
> 
> } else if (requests.isRequest()) {
> ...
> } else if (requests.isUSVString()) {
> ...
> } else
> 
> I think it would actually be shorter, and it would avoid the extra if
statement
> on 155. Up to you though.

Done.

[Peter Beverloo, 2017-03-23 19:57:48]

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

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

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-23 22:05:54]

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

[commit-bot: I haz the power, 2017-03-23 22:05:55]

Dry run: This issue passed the CQ dry run.

[haraken, 2017-03-24 01:29:38]

LGTM

https://codereview.chromium.org/2762243002/diff/80001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/background_fetch/BackgroundFetchManager.cpp
(right):

https://codereview.chromium.org/2762243002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/background_fetch/BackgroundFetchManager.cpp:142:
return Vector<WebServiceWorkerRequest>();

Don't we need to throw a type error?

https://codereview.chromium.org/2762243002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/background_fetch/BackgroundFetchManager.cpp:159:
return Vector<WebServiceWorkerRequest>();

Ditto.

[horo, 2017-03-24 02:01:50]

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

[horo, 2017-03-24 02:01:51]

https://codereview.chromium.org/2762243002/diff/80001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/background_fetch/BackgroundFetchManager.cpp
(right):

https://codereview.chromium.org/2762243002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/background_fetch/BackgroundFetchManager.cpp:154:
requests.getAsRequest()->populateWebServiceWorkerRequest(webRequests[0]);
populateWebServiceWorkerRequest() doesn't support the body of the request.
This method is used for CacheStorage API which doesn't support the request body.

Don't we need to support the request body in Background Fetch?

[Peter Beverloo, 2017-03-24 14:41:20]

Thank you for the comments!

https://codereview.chromium.org/2762243002/diff/80001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/modules/background_fetch/BackgroundFetchManager.cpp
(right):

https://codereview.chromium.org/2762243002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/background_fetch/BackgroundFetchManager.cpp:142:
return Vector<WebServiceWorkerRequest>();
On 2017/03/24 01:29:38, haraken wrote:
> 
> Don't we need to throw a type error?

No, because an exception already was thrown (through the `exceptionState`) when
creating the Request. That'll contain much more detailed information than
anything a TypeError would have.

https://codereview.chromium.org/2762243002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/background_fetch/BackgroundFetchManager.cpp:154:
requests.getAsRequest()->populateWebServiceWorkerRequest(webRequests[0]);
On 2017/03/24 02:01:51, horo wrote:
> populateWebServiceWorkerRequest() doesn't support the body of the request.
> This method is used for CacheStorage API which doesn't support the request
body.
> 
> Don't we need to support the request body in Background Fetch?

Right now we're focusing on `URL` to get end-to-end working, after which we'll
ramp up on the other additions necessary to fully support the Request options.

The reason for this is that our fetches will be routed through the downloads
system, which is rather different from making an immediate request in the
browser process. It's not yet clear which of the Request options are completely
supported, because much of the plumbing seems to be unused. Some options will
also have UI/UX impact that we'll have to assess.

In the ramp-up we'll fix "bugs" like this and ensure proper test coverage for
them.

https://codereview.chromium.org/2762243002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/background_fetch/BackgroundFetchManager.cpp:159:
return Vector<WebServiceWorkerRequest>();
On 2017/03/24 01:29:38, haraken wrote:
> 
> Ditto.

Acknowledged.

[Peter Beverloo, 2017-03-24 17:06:03]

The CQ bit was checked by peter@chromium.org

[Peter Beverloo, 2017-03-24 17:06:03]

The patchset sent to the CQ was uploaded after l-g-t-m from
harkness@chromium.org
Link to the patchset: https://codereview.chromium.org/2762243002/#ps80001
(title: "Convert Background Fetch' input to a WebServiceWorkerRequest vector")

[commit-bot: I haz the power, 2017-03-24 17:06:51]

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-24 17:12:21]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1490375163279260,
"parent_rev": "73041822bfec1b9d8f58a4a4605dcfe9d96a1d6d", "commit_rev":
"9b34b21e479145817c93e1e4870ed2e87376eac0"}

[commit-bot: I haz the power, 2017-03-24 17:13:08]

Description was changed from

==========
Convert Background Fetch' input to a WebServiceWorkerRequest vector

Developers can pass in either an individual USVString or Request object,
or a sequence of either. Convert the input provided by the developer to
a sequence of WebServiceWorkerRequest objects, which can (soon) be send
over Mojo to the browser process.

BUG=692534, 692535
==========

to

==========
Convert Background Fetch' input to a WebServiceWorkerRequest vector

Developers can pass in either an individual USVString or Request object,
or a sequence of either. Convert the input provided by the developer to
a sequence of WebServiceWorkerRequest objects, which can (soon) be send
over Mojo to the browser process.

BUG=692534, 692535

Review-Url: https://codereview.chromium.org/2762243002
Cr-Commit-Position: refs/heads/master@{#459465}
Committed:
https://chromium.googlesource.com/chromium/src/+/9b34b21e479145817c93e1e4870e...
==========

[commit-bot: I haz the power, 2017-03-24 17:13:09]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/9b34b21e479145817c93e1e4870e...