Service Worker event dispatcher for Background Fetch

Service Worker event dispatcher for Background Fetch

This CL implements the BackgroundFetchEventDispatcher and teaches it to
dispatch abort and click events. It then plumbs all the way down to
Blink to do so, after which it feeds back the result to the caller.

The `backgroundfetched` and `backgroundfetchfail` events rely on the
Request and Response objects to be available through Mojo; they will
follow in a subsequent CL.

TBR=avi (new file listed in BUILD.gn)
BUG=692534, 692581
Review-Url: https://codereview.chromium.org/2748213003
Cr-Commit-Position: refs/heads/master@{#457989}
Committed: https://chromium.googlesource.com/chromium/src/+/ef079770a54afefdb2248ad47143a216f8f6ed85

[Peter Beverloo, 2017-03-15 16:20:53]

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

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

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-15 16:21:27]

Description was changed from

==========
Service Worker event dispatcher for Background Fetch

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

to

==========
Service Worker event dispatcher for Background Fetch

This CL implements the BackgroundFetchEventDispatcher and teaches it to
dispatch abort and click events. It then plumbs all the way down to
Blink to do so, after which it feeds back the result to the caller.

The `backgroundfetched` and `backgroundfetchfail` events rely on the
Request and Response objects to be available through Mojo; they will
follow in a subsequent CL.

BUG=692534, 692581
==========

[Peter Beverloo, 2017-03-15 16:24:58]

peter@chromium.org changed reviewers:
+ harkness@chromium.org, horo@chromium.org, isherman@chromium.org,
tsepez@chromium.org

[Peter Beverloo, 2017-03-15 16:24:59]

+harkness for background fetch bits
+horo for Service Workers plumbing
+isherman for UMA

    Notably, see background_fetch_event_dispatcher.cc for recording
    (w/ the unit test), as well as service_worker_metrics.cc.

+tsepez for Mojo

    Notably, service_worker_event_dispatcher.mojom following existing
    users. Browser-side caller in background_fetch_event_dispatcher.cc
    and renderer-side receiver in service_worker_context_client.cc.

[Tom Sepez, 2017-03-15 17:39:17]

mojom LGTM.

[commit-bot: I haz the power, 2017-03-15 18:03:11]

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

[commit-bot: I haz the power, 2017-03-15 18:03:13]

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

[Ilya Sherman, 2017-03-16 03:58:50]

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

https://codereview.chromium.org/2748213003/diff/20001/content/browser/backgro...
content/browser/background_fetch/background_fetch_event_dispatcher.cc:38:
base::StringPrintf("BackgroundFetch.EventDispatchResult.%s",
nit: I'm pretty sure that returning an std::string from
HistogramSuffixForEventType and writing this as

    "BackgroundFetch.EventDispatchResult." + HistogramSuffixForEventType(event)

would be both easier to read and more efficient (or at least comparably
efficient).

https://codereview.chromium.org/2748213003/diff/20001/content/browser/backgro...
content/browser/background_fetch/background_fetch_event_dispatcher.cc:42:
base::LinearHistogram::FactoryGet(
Could you use a function from histogram_functions.h instead?

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

https://codereview.chromium.org/2748213003/diff/20001/content/browser/backgro...
content/browser/background_fetch/background_fetch_event_dispatcher.h:35:
DISPATCH_RESULT_MAX = DISPATCH_RESULT_CANNOT_DISPATCH_EVENT
Optional: I find that it's easier to work with DISPATCH_RESULT_COUNT = 4 than
MAX = 3, but either is okay as long as you make the C++ code match correctly.

https://codereview.chromium.org/2748213003/diff/20001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2748213003/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:4149: +<!-- Name completed by
histogram_suffixes name="BackgroundFetchEvents" -->
This comment makes me think that you probably want to add base="true" to the
<histogram> element.

https://codereview.chromium.org/2748213003/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:4153: +    Records the Service Worker
status code that caused an Background Fetch API
nit: s/an/a

https://codereview.chromium.org/2748213003/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:81997: +  <int value="3" label="Cannot
dispatch the event on the Service Worker"/>
Optional: I'd recommend using shorter labels for a more readable dashboard
display, perhaps with hovertext to provide more detail if need be
, e.g.

<int value="3" label="Cannot dispatch event">Cannot dispatch the event on the
Service Worker</int>

https://codereview.chromium.org/2748213003/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:114707: +  <suffix name="ClickEvent"/>
nit: Please add a brief label for each suffix.

[horo, 2017-03-16 07:08:22]

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

https://codereview.chromium.org/2748213003/diff/20001/content/browser/backgro...
content/browser/background_fetch/background_fetch_event_dispatcher.cc:128:
service_worker_context_->FindReadyRegistrationForIdOnly(
Can you use FindReadyRegistrationForId instead?
FindReadyRegistrationForId is more efficient than
FindReadyRegistrationForIdOnly.


Please make sure that the database of Background Fetch must be cleared when
ServiceWorkerContextObserver::OnStorageWiped() is called.
Once the database of Service Worker is wiped out, the same
service_worker_registration_id will be reused.

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

https://codereview.chromium.org/2748213003/diff/20001/content/browser/backgro...
content/browser/background_fetch/background_fetch_event_dispatcher_unittest.cc:111:
{
nit:You don't need the brackets.


Or 

    {
      base::RunLoop run_loop;
      embedded_worker_test_helper_.context()->storage()->FindRegistrationForId(
          service_worker_registration_id, origin,
          base::Bind(&BackgroundFetchEventDispatcherTest::
                         DidFindServiceWorkerRegistration,
                     base::Unretained(this), &service_worker_registration,
                     run_loop.QuitClosure()));
      run_loop.Run();
    }

    // Wait for the worker to be activated.
    base::RunLoop().RunUntilIdle();

https://codereview.chromium.org/2748213003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/ServiceWorkerGlobalScopeProxy.cpp (right):

https://codereview.chromium.org/2748213003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/ServiceWorkerGlobalScopeProxy.cpp:124:
DEFINE_STATIC_LOCAL(const AtomicString, pending, ("pending"));
We should not use DEFINE_STATIC_LOCAL for AtomicStrings which are used at
multiple worker threads.

See: https://codereview.chromium.org/2638623004/

[Peter Beverloo, 2017-03-16 16:05:59]

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

https://codereview.chromium.org/2748213003/diff/20001/content/browser/backgro...
content/browser/background_fetch/background_fetch_event_dispatcher.cc:38:
base::StringPrintf("BackgroundFetch.EventDispatchResult.%s",
On 2017/03/16 03:58:50, Ilya Sherman wrote:
> nit: I'm pretty sure that returning an std::string from
> HistogramSuffixForEventType and writing this as
> 
>     "BackgroundFetch.EventDispatchResult." +
HistogramSuffixForEventType(event)
> 
> would be both easier to read and more efficient (or at least comparably
> efficient).

Done.

https://codereview.chromium.org/2748213003/diff/20001/content/browser/backgro...
content/browser/background_fetch/background_fetch_event_dispatcher.cc:42:
base::LinearHistogram::FactoryGet(
On 2017/03/16 03:58:50, Ilya Sherman wrote:
> Could you use a function from histogram_functions.h instead?

Aaaaah. With pleasure - done :)

https://codereview.chromium.org/2748213003/diff/20001/content/browser/backgro...
content/browser/background_fetch/background_fetch_event_dispatcher.cc:128:
service_worker_context_->FindReadyRegistrationForIdOnly(
On 2017/03/16 07:08:22, horo wrote:
> Can you use FindReadyRegistrationForId instead?
> FindReadyRegistrationForId is more efficient than
> FindReadyRegistrationForIdOnly.
> 
> 
> Please make sure that the database of Background Fetch must be cleared when
> ServiceWorkerContextObserver::OnStorageWiped() is called.
> Once the database of Service Worker is wiped out, the same
> service_worker_registration_id will be reused.

We'll use the Service Worker database's user data system for storage, and will
have an observer for deleted registrations and wiped databases. This will likely
have to be async in some manner (I mentioned it to Matt a few weeks ago).

https://codereview.chromium.org/2748213003/diff/20001/content/browser/backgro...
content/browser/background_fetch/background_fetch_event_dispatcher.cc:128:
service_worker_context_->FindReadyRegistrationForIdOnly(
On 2017/03/16 07:08:22, horo wrote:
> Can you use FindReadyRegistrationForId instead?
> FindReadyRegistrationForId is more efficient than
> FindReadyRegistrationForIdOnly.
> 
> 
> Please make sure that the database of Background Fetch must be cleared when
> ServiceWorkerContextObserver::OnStorageWiped() is called.
> Once the database of Service Worker is wiped out, the same
> service_worker_registration_id will be reused.

Done.

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

https://codereview.chromium.org/2748213003/diff/20001/content/browser/backgro...
content/browser/background_fetch/background_fetch_event_dispatcher.h:35:
DISPATCH_RESULT_MAX = DISPATCH_RESULT_CANNOT_DISPATCH_EVENT
On 2017/03/16 03:58:50, Ilya Sherman wrote:
> Optional: I find that it's easier to work with DISPATCH_RESULT_COUNT = 4 than
> MAX = 3, but either is okay as long as you make the C++ code match correctly.

We need MAX now that I've updated the .cc to use UmaHistogramEnumeration(), so
while this indeed is more tedious to update, it avoids needing to subtract one
at usage. Happy to flip it around if you still prefer that, too.

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

https://codereview.chromium.org/2748213003/diff/20001/content/browser/backgro...
content/browser/background_fetch/background_fetch_event_dispatcher_unittest.cc:111:
{
On 2017/03/16 07:08:22, horo wrote:
> nit:You don't need the brackets.
> 
> 
> Or 
> 
>     {
>       base::RunLoop run_loop;
>      
embedded_worker_test_helper_.context()->storage()->FindRegistrationForId(
>           service_worker_registration_id, origin,
>           base::Bind(&BackgroundFetchEventDispatcherTest::
>                          DidFindServiceWorkerRegistration,
>                      base::Unretained(this), &service_worker_registration,
>                      run_loop.QuitClosure()));
>       run_loop.Run();
>     }
> 
>     // Wait for the worker to be activated.
>     base::RunLoop().RunUntilIdle();

Amended. You found the comment I was looking for :)

https://codereview.chromium.org/2748213003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/ServiceWorkerGlobalScopeProxy.cpp (right):

https://codereview.chromium.org/2748213003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/ServiceWorkerGlobalScopeProxy.cpp:124:
DEFINE_STATIC_LOCAL(const AtomicString, pending, ("pending"));
On 2017/03/16 07:08:22, horo wrote:
> We should not use DEFINE_STATIC_LOCAL for AtomicStrings which are used at
> multiple worker threads.
> 
> See: https://codereview.chromium.org/2638623004/

Removed, thanks

https://codereview.chromium.org/2748213003/diff/20001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2748213003/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:4149: +<!-- Name completed by
histogram_suffixes name="BackgroundFetchEvents" -->
On 2017/03/16 03:58:50, Ilya Sherman wrote:
> This comment makes me think that you probably want to add base="true" to the
> <histogram> element.

Done.

https://codereview.chromium.org/2748213003/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:4153: +    Records the Service Worker
status code that caused an Background Fetch API
On 2017/03/16 03:58:50, Ilya Sherman wrote:
> nit: s/an/a

Done. Elsewhere too.

https://codereview.chromium.org/2748213003/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:81997: +  <int value="3" label="Cannot
dispatch the event on the Service Worker"/>
On 2017/03/16 03:58:50, Ilya Sherman wrote:
> Optional: I'd recommend using shorter labels for a more readable dashboard
> display, perhaps with hovertext to provide more detail if need be
> , e.g.
> 
> <int value="3" label="Cannot dispatch event">Cannot dispatch the event on the
> Service Worker</int>

Ack. Removed additional text, it's still clear enough to not need the hover
text. That's a cool feature.

https://codereview.chromium.org/2748213003/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:114707: +  <suffix name="ClickEvent"/>
On 2017/03/16 03:58:50, Ilya Sherman wrote:
> nit: Please add a brief label for each suffix.

Done.

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

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

[commit-bot: I haz the power, 2017-03-16 16:06:55]

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-16 16:44:27]

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

[commit-bot: I haz the power, 2017-03-16 16:44:28]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[Peter Beverloo, 2017-03-16 17:20:35]

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

[commit-bot: I haz the power, 2017-03-16 17:21:11]

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-16 17:42:18]

LGTM

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

https://codereview.chromium.org/2748213003/diff/20001/content/browser/backgro...
content/browser/background_fetch/background_fetch_event_dispatcher.h:71:
base::Closure finished_closure,
nit: error_closure? "finished" is harder to distinguish from "loaded".

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

https://codereview.chromium.org/2748213003/diff/20001/content/browser/backgro...
content/browser/background_fetch/background_fetch_event_dispatcher_unittest.cc:172:
{
micro-nit: don't need the {}

https://codereview.chromium.org/2748213003/diff/20001/content/browser/backgro...
content/browser/background_fetch/background_fetch_event_dispatcher_unittest.cc:242:
BackgroundFetchEventDispatcher::DISPATCH_RESULT_SUCCESS, 1);
Can you save the state in BackgroundFetchEmbeddedWorkerTestHelper and EXPECT it
here?

[commit-bot: I haz the power, 2017-03-16 18:13:36]

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

[commit-bot: I haz the power, 2017-03-16 18:13:37]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Ilya Sherman, 2017-03-16 22:36:02]

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

https://codereview.chromium.org/2748213003/diff/20001/content/browser/backgro...
content/browser/background_fetch/background_fetch_event_dispatcher.h:35:
DISPATCH_RESULT_MAX = DISPATCH_RESULT_CANNOT_DISPATCH_EVENT
On 2017/03/16 16:05:58, Peter Beverloo wrote:
> On 2017/03/16 03:58:50, Ilya Sherman wrote:
> > Optional: I find that it's easier to work with DISPATCH_RESULT_COUNT = 4
than
> > MAX = 3, but either is okay as long as you make the C++ code match
correctly.
> 
> We need MAX now that I've updated the .cc to use UmaHistogramEnumeration(), so
> while this indeed is more tedious to update, it avoids needing to subtract one
> at usage. Happy to flip it around if you still prefer that, too.

You should still use COUNT or MAX+1, even with UmaHistogramEnumeration().  I was
just looking at the docs today, and agree that it's pretty confusing.  The idea
of the histogram_functions code is to exactly match the macros.  If you have
some time to update the documentation, I'd be happy to review the changes!  (If
not, I'll try to get to it sometime soon, if I don't get sidetracked.)

https://codereview.chromium.org/2748213003/diff/60001/content/browser/backgro...
File content/browser/background_fetch/background_fetch_event_dispatcher.cc
(right):

https://codereview.chromium.org/2748213003/diff/60001/content/browser/backgro...
content/browser/background_fetch/background_fetch_event_dispatcher.cc:56:
SERVICE_WORKER_ERROR_MAX_VALUE);
Both here and above, you'll want to add "+1" to the final param.

[Ilya Sherman, 2017-03-16 22:53:21]

Metrics LGTM with the below comments resolved.

On 2017/03/16 22:36:02, Ilya Sherman wrote:
>
https://codereview.chromium.org/2748213003/diff/20001/content/browser/backgro...
> File content/browser/background_fetch/background_fetch_event_dispatcher.h
> (right):
> 
>
https://codereview.chromium.org/2748213003/diff/20001/content/browser/backgro...
> content/browser/background_fetch/background_fetch_event_dispatcher.h:35:
> DISPATCH_RESULT_MAX = DISPATCH_RESULT_CANNOT_DISPATCH_EVENT
> On 2017/03/16 16:05:58, Peter Beverloo wrote:
> > On 2017/03/16 03:58:50, Ilya Sherman wrote:
> > > Optional: I find that it's easier to work with DISPATCH_RESULT_COUNT = 4
> than
> > > MAX = 3, but either is okay as long as you make the C++ code match
> correctly.
> > 
> > We need MAX now that I've updated the .cc to use UmaHistogramEnumeration(),
so
> > while this indeed is more tedious to update, it avoids needing to subtract
one
> > at usage. Happy to flip it around if you still prefer that, too.
> 
> You should still use COUNT or MAX+1, even with UmaHistogramEnumeration().  I
was
> just looking at the docs today, and agree that it's pretty confusing.  The
idea
> of the histogram_functions code is to exactly match the macros.  If you have
> some time to update the documentation, I'd be happy to review the changes! 
(If
> not, I'll try to get to it sometime soon, if I don't get sidetracked.)
> 
>
https://codereview.chromium.org/2748213003/diff/60001/content/browser/backgro...
> File content/browser/background_fetch/background_fetch_event_dispatcher.cc
> (right):
> 
>
https://codereview.chromium.org/2748213003/diff/60001/content/browser/backgro...
> content/browser/background_fetch/background_fetch_event_dispatcher.cc:56:
> SERVICE_WORKER_ERROR_MAX_VALUE);
> Both here and above, you'll want to add "+1" to the final param.

[horo, 2017-03-17 02:22:07]

lgtm

[haraken, 2017-03-17 11:44:51]

WebKit LGTM

[Peter Beverloo, 2017-03-17 13:42:20]

Description was changed from

==========
Service Worker event dispatcher for Background Fetch

This CL implements the BackgroundFetchEventDispatcher and teaches it to
dispatch abort and click events. It then plumbs all the way down to
Blink to do so, after which it feeds back the result to the caller.

The `backgroundfetched` and `backgroundfetchfail` events rely on the
Request and Response objects to be available through Mojo; they will
follow in a subsequent CL.

BUG=692534, 692581
==========

to

==========
Service Worker event dispatcher for Background Fetch

This CL implements the BackgroundFetchEventDispatcher and teaches it to
dispatch abort and click events. It then plumbs all the way down to
Blink to do so, after which it feeds back the result to the caller.

The `backgroundfetched` and `backgroundfetchfail` events rely on the
Request and Response objects to be available through Mojo; they will
follow in a subsequent CL.

TBR=avi (new file listed in BUILD.gn)
BUG=692534, 692581
==========

[Peter Beverloo, 2017-03-17 13:43:27]

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

[Peter Beverloo, 2017-03-17 13:43:28]

Thanks all!

TBR=avi for a new file in the background fetch directory. (I'll separately send
a CL because that shouldn't need your stamp.)

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

https://codereview.chromium.org/2748213003/diff/20001/content/browser/backgro...
content/browser/background_fetch/background_fetch_event_dispatcher.h:35:
DISPATCH_RESULT_MAX = DISPATCH_RESULT_CANNOT_DISPATCH_EVENT
On 2017/03/16 22:36:02, Ilya Sherman wrote:
> On 2017/03/16 16:05:58, Peter Beverloo wrote:
> > On 2017/03/16 03:58:50, Ilya Sherman wrote:
> > > Optional: I find that it's easier to work with DISPATCH_RESULT_COUNT = 4
> than
> > > MAX = 3, but either is okay as long as you make the C++ code match
> correctly.
> > 
> > We need MAX now that I've updated the .cc to use UmaHistogramEnumeration(),
so
> > while this indeed is more tedious to update, it avoids needing to subtract
one
> > at usage. Happy to flip it around if you still prefer that, too.
> 
> You should still use COUNT or MAX+1, even with UmaHistogramEnumeration().  I
was
> just looking at the docs today, and agree that it's pretty confusing.  The
idea
> of the histogram_functions code is to exactly match the macros.  If you have
> some time to update the documentation, I'd be happy to review the changes! 
(If
> not, I'll try to get to it sometime soon, if I don't get sidetracked.)

Ack as discussed - changed to DISPATCH_RESULT_COUNT.

https://codereview.chromium.org/2748213003/diff/20001/content/browser/backgro...
content/browser/background_fetch/background_fetch_event_dispatcher.h:71:
base::Closure finished_closure,
On 2017/03/16 17:42:18, harkness wrote:
> nit: error_closure? "finished" is harder to distinguish from "loaded".

We always invoke the |finished_closure|, so that's really what it is. Happy to
rename later if we come up with a better name.

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

https://codereview.chromium.org/2748213003/diff/20001/content/browser/backgro...
content/browser/background_fetch/background_fetch_event_dispatcher_unittest.cc:172:
{
On 2017/03/16 17:42:18, harkness wrote:
> micro-nit: don't need the {}

Done.

https://codereview.chromium.org/2748213003/diff/20001/content/browser/backgro...
content/browser/background_fetch/background_fetch_event_dispatcher_unittest.cc:242:
BackgroundFetchEventDispatcher::DISPATCH_RESULT_SUCCESS, 1);
On 2017/03/16 17:42:18, harkness wrote:
> Can you save the state in BackgroundFetchEmbeddedWorkerTestHelper and EXPECT
it
> here?

Done, also for the `tag`.

https://codereview.chromium.org/2748213003/diff/60001/content/browser/backgro...
File content/browser/background_fetch/background_fetch_event_dispatcher.cc
(right):

https://codereview.chromium.org/2748213003/diff/60001/content/browser/backgro...
content/browser/background_fetch/background_fetch_event_dispatcher.cc:56:
SERVICE_WORKER_ERROR_MAX_VALUE);
On 2017/03/16 22:36:02, Ilya Sherman wrote:
> Both here and above, you'll want to add "+1" to the final param.

Done.

[Peter Beverloo, 2017-03-17 13:43:40]

The CQ bit was checked by peter@chromium.org

[Peter Beverloo, 2017-03-17 13:43:40]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org,
haraken@chromium.org, harkness@chromium.org, isherman@chromium.org,
horo@chromium.org
Link to the patchset: https://codereview.chromium.org/2748213003/#ps80001
(title: "Service Worker event dispatcher for Background Fetch")

[commit-bot: I haz the power, 2017-03-17 13:43:54]

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-17 13:54:41]

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

[commit-bot: I haz the power, 2017-03-17 13:54:43]

Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Peter Beverloo, 2017-03-17 14:17:01]

Ilya - mind taking another look?

https://codereview.chromium.org/2748213003/diff/100001/content/browser/backgr...
File content/browser/background_fetch/background_fetch_event_dispatcher.cc
(right):

https://codereview.chromium.org/2748213003/diff/100001/content/browser/backgr...
content/browser/background_fetch/background_fetch_event_dispatcher.cc:56:
SERVICE_WORKER_ERROR_MAX_VALUE);
isherman@ - looking at this more closely, we don't need + 1 here.

SERVICE_WORKER_ERROR_MAX_VALUE is identical to _COUNT. The only reason that it's
got a value assigned is because the otherwise highest value got reverted in this
CL:

https://chromium.googlesource.com/chromium/src/+/cb65da13abc71994dbad796aaef9...

The call to LinearHistogram::FactoryGet() ends up being identical:

https://cs.chromium.org/chromium/src/base/metrics/histogram_macros_internal.h...
https://cs.chromium.org/chromium/src/base/metrics/histogram_functions.cc?dr=C...

(The +1 is done internally.)

I find 14 other instances of UMA_HISTOGRAM_ENUMERATION that use it like this.

[Peter Beverloo, 2017-03-17 14:17:05]

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

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

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-17 15:59:40]

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

[commit-bot: I haz the power, 2017-03-17 15:59:41]

Dry run: This issue passed the CQ dry run.

[Avi (use Gerrit), 2017-03-17 16:37:52]

lgtm

build stamp

[Ilya Sherman, 2017-03-17 19:52:08]

Metrics LGTM, thanks for double-checking and clarifying.

https://codereview.chromium.org/2748213003/diff/100001/content/browser/backgr...
File content/browser/background_fetch/background_fetch_event_dispatcher.cc
(right):

https://codereview.chromium.org/2748213003/diff/100001/content/browser/backgr...
content/browser/background_fetch/background_fetch_event_dispatcher.cc:56:
SERVICE_WORKER_ERROR_MAX_VALUE);
On 2017/03/17 14:17:01, Peter Beverloo wrote:
> isherman@ - looking at this more closely, we don't need + 1 here.
> 
> SERVICE_WORKER_ERROR_MAX_VALUE is identical to _COUNT.

Yes, you're right -- I just assumed based on the name that it was conceptually a
"max" and not a "count".  But, whoever named that probably took the nomenclature
from the histograms documentation, which uses the term "max" in a mysterious way
=P

> The only reason that it's
> got a value assigned is because the otherwise highest value got reverted in
this
> CL:
> 
>
https://chromium.googlesource.com/chromium/src/+/cb65da13abc71994dbad796aaef9...
> 
> The call to LinearHistogram::FactoryGet() ends up being identical:
> 
>
https://cs.chromium.org/chromium/src/base/metrics/histogram_macros_internal.h...
>
https://cs.chromium.org/chromium/src/base/metrics/histogram_functions.cc?dr=C...
> 
> (The +1 is done internally.)

The +1 that's done internally is for the number of buckets.  It's a very
confusing API in terms of what exactly those values are meant to be.  For the
wrapper functions/macro, it's a bit simpler to remember: the "boundary" value
should be one past the max that's ever recorded.

> I find 14 other instances of UMA_HISTOGRAM_ENUMERATION that use it like this.

[Peter Beverloo, 2017-03-19 01:58:47]

The CQ bit was checked by peter@chromium.org

[Peter Beverloo, 2017-03-19 01:58:48]

The patchset sent to the CQ was uploaded after l-g-t-m from
harkness@chromium.org, tsepez@chromium.org, horo@chromium.org,
haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/2748213003/#ps100001
(title: "uma fix")

[commit-bot: I haz the power, 2017-03-19 01:58:53]

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-19 04:06:45]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1489888727929070,
"parent_rev": "e9aea4d208c896bd53497d7694801cf76fd26593", "commit_rev":
"ef079770a54afefdb2248ad47143a216f8f6ed85"}

[commit-bot: I haz the power, 2017-03-19 04:08:08]

Description was changed from

==========
Service Worker event dispatcher for Background Fetch

This CL implements the BackgroundFetchEventDispatcher and teaches it to
dispatch abort and click events. It then plumbs all the way down to
Blink to do so, after which it feeds back the result to the caller.

The `backgroundfetched` and `backgroundfetchfail` events rely on the
Request and Response objects to be available through Mojo; they will
follow in a subsequent CL.

TBR=avi (new file listed in BUILD.gn)
BUG=692534, 692581
==========

to

==========
Service Worker event dispatcher for Background Fetch

This CL implements the BackgroundFetchEventDispatcher and teaches it to
dispatch abort and click events. It then plumbs all the way down to
Blink to do so, after which it feeds back the result to the caller.

The `backgroundfetched` and `backgroundfetchfail` events rely on the
Request and Response objects to be available through Mojo; they will
follow in a subsequent CL.

TBR=avi (new file listed in BUILD.gn)
BUG=692534, 692581

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

[commit-bot: I haz the power, 2017-03-19 04:08:09]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/ef079770a54afefdb2248ad47143...