Dispatch a bare Service Worker event for a finished Background Fetch

Dispatch a bare Service Worker event for a finished Background Fetch

Following this CL, we'll invoke the `backgroundfetched` Service Worker
event when a Background Fetch has completed. The responses will be
filled with the tiniest bits of information we have right now, so we
can't actually distinguish between success and failure yet, but from
this point on we'll be able to iterate on the missing fields.

BUG=692579
Review-Url: https://codereview.chromium.org/2786783002
Cr-Commit-Position: refs/heads/master@{#460780}
Committed: https://chromium.googlesource.com/chromium/src/+/f1cfaa03b05e225971f220bed2451cb2dc4b497d

[Peter Beverloo, 2017-03-29 20:36:00]

peter@chromium.org changed reviewers:
+ harkness@chromium.org

[Peter Beverloo, 2017-03-29 20:36:00]

+harkness, PTAL

The good: this gives us end-to-end.
The fair: this still needs tests, so is not ready to land.
The bad: redness :( -- but it matches what we discussed.

[harkness, 2017-03-29 22:12:33]

Looks good % testing.

The good: yey!
The fair: yup.
The bad: *sigh*

https://codereview.chromium.org/2786783002/diff/1/content/browser/background_...
File content/browser/background_fetch/background_fetch_context.cc (right):

https://codereview.chromium.org/2786783002/diff/1/content/browser/background_...
content/browser/background_fetch/background_fetch_context.cc:174: if
(controller->state() != BackgroundFetchJobController::State::COMPLETED) {
We need to call the aborted event if that's the state on the Controller, too.

https://codereview.chromium.org/2786783002/diff/1/content/browser/background_...
File content/browser/background_fetch/background_fetch_data_manager.cc (right):

https://codereview.chromium.org/2786783002/diff/1/content/browser/background_...
content/browser/background_fetch/background_fetch_data_manager.cc:72: void
MarkRequestAsComplete(const BackgroundFetchRequestInfo& request) {
Maybe add the file_path and received_bytes to this call?

[Peter Beverloo, 2017-03-29 22:20:28]

https://codereview.chromium.org/2786783002/diff/1/content/browser/background_...
File content/browser/background_fetch/background_fetch_context.cc (right):

https://codereview.chromium.org/2786783002/diff/1/content/browser/background_...
content/browser/background_fetch/background_fetch_context.cc:174: if
(controller->state() != BackgroundFetchJobController::State::COMPLETED) {
On 2017/03/29 22:12:33, harkness wrote:
> We need to call the aborted event if that's the state on the Controller, too.

As I understand it, `backgroundfetchabort` is for when the user aborts, not the
developer. I'll send a quick note to Jake.

https://codereview.chromium.org/2786783002/diff/1/content/browser/background_...
File content/browser/background_fetch/background_fetch_data_manager.cc (right):

https://codereview.chromium.org/2786783002/diff/1/content/browser/background_...
content/browser/background_fetch/background_fetch_data_manager.cc:72: void
MarkRequestAsComplete(const BackgroundFetchRequestInfo& request) {
On 2017/03/29 22:12:33, harkness wrote:
> Maybe add the file_path and received_bytes to this call?

We'll need much more information, so I'd like those to be set on the |request|.
(And presumably retire |download_guid| in MarkRequestAsStarted() in favor of
that too.) That requires some more fiddling with ownership, but I'll get that
done in the near future. E2E first.

[Peter Beverloo, 2017-03-30 10:19:37]

https://codereview.chromium.org/2786783002/diff/1/content/browser/background_...
File content/browser/background_fetch/background_fetch_context.cc (right):

https://codereview.chromium.org/2786783002/diff/1/content/browser/background_...
content/browser/background_fetch/background_fetch_context.cc:174: if
(controller->state() != BackgroundFetchJobController::State::COMPLETED) {
On 2017/03/29 22:20:28, Peter Beverloo wrote:
> On 2017/03/29 22:12:33, harkness wrote:
> > We need to call the aborted event if that's the state on the Controller,
too.
> 
> As I understand it, `backgroundfetchabort` is for when the user aborts, not
the
> developer. I'll send a quick note to Jake.

https://github.com/WICG/background-fetch#reacting-to-abort agrees with you. Will
fix the comment here, but firing the actual event in a future CL. (Timing will
be interesting.)

[Peter Beverloo, 2017-03-30 13:41:06]

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

[Peter Beverloo, 2017-03-30 13:41:06]

+harkness, PTAL
+avi for BUILD.gn

I'll add even more in-depth integration tests in a subsequent CL if that's OK,
given that this already is pretty big and the request/response testing can be
tricky.

[Peter Beverloo, 2017-03-30 13:42:28]

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

[commit-bot: I haz the power, 2017-03-30 13:42:54]

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

[harkness, 2017-03-30 14:15:28]

lgtm

https://codereview.chromium.org/2786783002/diff/1/content/browser/background_...
File content/browser/background_fetch/background_fetch_context.cc (right):

https://codereview.chromium.org/2786783002/diff/1/content/browser/background_...
content/browser/background_fetch/background_fetch_context.cc:174: if
(controller->state() != BackgroundFetchJobController::State::COMPLETED) {
On 2017/03/30 10:19:37, Peter Beverloo wrote:
> On 2017/03/29 22:20:28, Peter Beverloo wrote:
> > On 2017/03/29 22:12:33, harkness wrote:
> > > We need to call the aborted event if that's the state on the Controller,
> too.
> > 
> > As I understand it, `backgroundfetchabort` is for when the user aborts, not
> the
> > developer. I'll send a quick note to Jake.
> 
> https://github.com/WICG/background-fetch#reacting-to-abort agrees with you.
Will
> fix the comment here, but firing the actual event in a future CL. (Timing will
> be interesting.)

Acknowledged.

https://codereview.chromium.org/2786783002/diff/20001/content/browser/backgro...
File content/browser/background_fetch/background_fetch_service_impl.h (right):

https://codereview.chromium.org/2786783002/diff/20001/content/browser/backgro...
content/browser/background_fetch/background_fetch_service_impl.h:79: //
micro-nit: remove

https://codereview.chromium.org/2786783002/diff/20001/content/browser/backgro...
File content/browser/background_fetch/background_fetch_service_unittest.cc
(right):

https://codereview.chromium.org/2786783002/diff/20001/content/browser/backgro...
content/browser/background_fetch/background_fetch_service_unittest.cc:363: 
nit: For a future patch you should test Abort() for a valid but not started
fetch.

[Peter Beverloo, 2017-03-30 14:50:39]

Thanks!

https://codereview.chromium.org/2786783002/diff/20001/content/browser/backgro...
File content/browser/background_fetch/background_fetch_service_impl.h (right):

https://codereview.chromium.org/2786783002/diff/20001/content/browser/backgro...
content/browser/background_fetch/background_fetch_service_impl.h:79: //
On 2017/03/30 14:15:28, harkness wrote:
> micro-nit: remove

Done.

https://codereview.chromium.org/2786783002/diff/20001/content/browser/backgro...
File content/browser/background_fetch/background_fetch_service_unittest.cc
(right):

https://codereview.chromium.org/2786783002/diff/20001/content/browser/backgro...
content/browser/background_fetch/background_fetch_service_unittest.cc:363: 
On 2017/03/30 14:15:28, harkness wrote:
> nit: For a future patch you should test Abort() for a valid but not started
> fetch.

Acknowledged.

[Peter Beverloo, 2017-03-30 14:50:47]

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

[commit-bot: I haz the power, 2017-03-30 14:51:17]

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

[Avi (use Gerrit), 2017-03-30 14:54:24]

build file lgtm

[Peter Beverloo, 2017-03-30 15:56:31]

The CQ bit was unchecked by peter@chromium.org

[Peter Beverloo, 2017-03-30 15:56:43]

The CQ bit was checked by peter@chromium.org

[Peter Beverloo, 2017-03-30 15:56:43]

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/2786783002/#ps40001
(title: "Dispatch a bare Service Worker event for a finished Background Fetch")

[commit-bot: I haz the power, 2017-03-30 15:57:26]

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-30 16:04:12]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1490889403037500,
"parent_rev": "e376f4717780e3248c359ea4fd5e47976130cc0d", "commit_rev":
"f1cfaa03b05e225971f220bed2451cb2dc4b497d"}

[commit-bot: I haz the power, 2017-03-30 16:05:21]

Description was changed from

==========
Dispatch a bare Service Worker event for a finished Background Fetch

Following this CL, we'll invoke the `backgroundfetched` Service Worker
event when a Background Fetch has completed. The responses will be
filled with the tiniest bits of information we have right now, so we
can't actually distinguish between success and failure yet, but from
this point on we'll be able to iterate on the missing fields.

BUG=692579
==========

to

==========
Dispatch a bare Service Worker event for a finished Background Fetch

Following this CL, we'll invoke the `backgroundfetched` Service Worker
event when a Background Fetch has completed. The responses will be
filled with the tiniest bits of information we have right now, so we
can't actually distinguish between success and failure yet, but from
this point on we'll be able to iterate on the missing fields.

BUG=692579

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

[commit-bot: I haz the power, 2017-03-30 16:05:22]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/f1cfaa03b05e225971f220bed245...