service worker: Attribute purpose to start worker attempts for UMA

service worker: Attribute purpose to start worker attempts for UMA

We want to see whether certain start worker purposes are timing out more
than others.

# Skip presubmit due to crbug.com/595198, presubmit passes locally
NOPRESUBMIT=true
BUG=559001
Committed: https://crrev.com/98a214071cb9542f09bf62250c5c7f7637bfa9c5
Cr-Commit-Position: refs/heads/master@{#381391}

[falken, 2016-03-14 08:29:26]

falken@chromium.org changed reviewers:
+ nhiroki@chromium.org

[falken, 2016-03-14 08:29:26]

PTAL

[nhiroki, 2016-03-15 02:18:14]

Looks good. Added some minor comments.

https://codereview.chromium.org/1795863006/diff/1/content/browser/service_wor...
File content/browser/service_worker/service_worker_fetch_dispatcher.cc (right):

https://codereview.chromium.org/1795863006/diff/1/content/browser/service_wor...
content/browser/service_worker/service_worker_fetch_dispatcher.cc:26: case
RESOURCE_TYPE_LAST_TYPE:
Maybe RESOURCE_TYPE_SERVICE_WORKER should not reach here, too.

https://codereview.chromium.org/1795863006/diff/1/content/browser/service_wor...
File content/browser/service_worker/service_worker_version.h (right):

https://codereview.chromium.org/1795863006/diff/1/content/browser/service_wor...
content/browser/service_worker/service_worker_version.h:169:
ServiceWorkerMetrics::EventType::UNKNOWN);
When is this argument omitted? Is it only in unittests?

https://codereview.chromium.org/1795863006/diff/1/content/browser/service_wor...
content/browser/service_worker/service_worker_version.h:191:
ServiceWorkerMetrics::EventType purpose);
This argument order is different from StartRequest()'s order. Can you make them
consistent?

[nhiroki, 2016-03-15 02:27:49]

https://codereview.chromium.org/1795863006/diff/1/content/browser/service_wor...
File content/browser/service_worker/service_worker_version.h (right):

https://codereview.chromium.org/1795863006/diff/1/content/browser/service_wor...
content/browser/service_worker/service_worker_version.h:169:
ServiceWorkerMetrics::EventType::UNKNOWN);
On 2016/03/15 02:18:14, nhiroki wrote:
> When is this argument omitted? Is it only in unittests?

IMHO, if there are no strong needs of the default argument, I'd prefer to
explicitly pass an argument instead of using a default argument because
production code could accidentally use the default argument w/o any compile
failure and UMA results could be mixed up.

[falken, 2016-03-15 05:15:53]

Thank you, PTAL

https://codereview.chromium.org/1795863006/diff/1/content/browser/service_wor...
File content/browser/service_worker/service_worker_fetch_dispatcher.cc (right):

https://codereview.chromium.org/1795863006/diff/1/content/browser/service_wor...
content/browser/service_worker/service_worker_fetch_dispatcher.cc:26: case
RESOURCE_TYPE_LAST_TYPE:
On 2016/03/15 02:18:14, nhiroki wrote:
> Maybe RESOURCE_TYPE_SERVICE_WORKER should not reach here, too.

Done.

https://codereview.chromium.org/1795863006/diff/1/content/browser/service_wor...
File content/browser/service_worker/service_worker_version.h (right):

https://codereview.chromium.org/1795863006/diff/1/content/browser/service_wor...
content/browser/service_worker/service_worker_version.h:169:
ServiceWorkerMetrics::EventType::UNKNOWN);
On 2016/03/15 02:27:48, nhiroki wrote:
> On 2016/03/15 02:18:14, nhiroki wrote:
> > When is this argument omitted? Is it only in unittests?
> 
> IMHO, if there are no strong needs of the default argument, I'd prefer to
> explicitly pass an argument instead of using a default argument because
> production code could accidentally use the default argument w/o any compile
> failure and UMA results could be mixed up.

Yes good point. Actually I was just too lazy to change all the callsites, but
that's dangerous as you say. Removed the default.

https://codereview.chromium.org/1795863006/diff/1/content/browser/service_wor...
content/browser/service_worker/service_worker_version.h:191:
ServiceWorkerMetrics::EventType purpose);
On 2016/03/15 02:18:14, nhiroki wrote:
> This argument order is different from StartRequest()'s order. Can you make
them
> consistent?

Done.

[nhiroki, 2016-03-15 06:27:02]

lgtm

[falken, 2016-03-15 07:12:13]

falken@chromium.org changed reviewers:
+ isherman@chromium.org, jkarlin@chromium.org, johnme@chromium.org,
mek@chromium.org

[falken, 2016-03-15 07:12:14]

Adding owners:
johnme: notifications/ and push_messaging/
mek: geofencing/ and navigator_connect/
jkarlin: background_sync/
isherman: histograms

Thank you!

[jkarlin, 2016-03-15 14:55:52]

background_sync/ lgtm

[Marijn Kruisselbrink, 2016-03-15 18:53:04]

> mek: geofencing/ and navigator_connect/
LGTM

[johnme, 2016-03-15 19:39:02]

lgtm content/browser/notifications/ and content/browser/push_messaging/

[Ilya Sherman, 2016-03-16 00:52:44]

metrics lgtm

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

https://codereview.chromium.org/1795863006/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:80163: +  <int value="14"
label="UNKOWN"/>
Optional nit: Mebbe update the "FETCH" label to something like "FETCH
(deprecated)"

[falken, 2016-03-16 02:07:46]

Thanks all.

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

https://codereview.chromium.org/1795863006/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:80163: +  <int value="14"
label="UNKOWN"/>
On 2016/03/16 00:52:44, Ilya Sherman wrote:
> Optional nit: Mebbe update the "FETCH" label to something like "FETCH
> (deprecated)"

Done.

[falken, 2016-03-16 02:07:55]

The CQ bit was checked by falken@chromium.org

[falken, 2016-03-16 02:07:56]

The patchset sent to the CQ was uploaded after l-g-t-m from johnme@chromium.org,
mek@chromium.org, isherman@chromium.org, nhiroki@chromium.org,
jkarlin@chromium.org
Link to the patchset: https://codereview.chromium.org/1795863006/#ps60001
(title: "review comments and fix")

[commit-bot: I haz the power, 2016-03-16 02:08:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1795863006/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1795863006/60001

[commit-bot: I haz the power, 2016-03-16 02:18:21]

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

[commit-bot: I haz the power, 2016-03-16 02:18:23]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[falken, 2016-03-16 02:27:43]

Description was changed from

==========
service worker: Attribute purpose to start worker attempts for UMA

We want to see whether certain start worker purposes are timing out more
than others.

BUG=559001
==========

to

==========
service worker: Attribute purpose to start worker attempts for UMA

We want to see whether certain start worker purposes are timing out more
than others.

# Skip presubmit due to crbug.com/595198, presubmit passes locally
NOPRESUBMIT=true
BUG=559001
==========

[falken, 2016-03-16 02:29:51]

The CQ bit was checked by falken@chromium.org

[falken, 2016-03-16 02:29:52]

The patchset sent to the CQ was uploaded after l-g-t-m from johnme@chromium.org,
mek@chromium.org, isherman@chromium.org, nhiroki@chromium.org,
jkarlin@chromium.org
Link to the patchset: https://codereview.chromium.org/1795863006/#ps80001
(title: "patch for landing?")

[commit-bot: I haz the power, 2016-03-16 02:30:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1795863006/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1795863006/80001

[commit-bot: I haz the power, 2016-03-16 05:21:46]

Description was changed from

==========
service worker: Attribute purpose to start worker attempts for UMA

We want to see whether certain start worker purposes are timing out more
than others.

# Skip presubmit due to crbug.com/595198, presubmit passes locally
NOPRESUBMIT=true
BUG=559001
==========

to

==========
service worker: Attribute purpose to start worker attempts for UMA

We want to see whether certain start worker purposes are timing out more
than others.

# Skip presubmit due to crbug.com/595198, presubmit passes locally
NOPRESUBMIT=true
BUG=559001
==========

[commit-bot: I haz the power, 2016-03-16 05:21:48]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-03-16 05:24:57]

Description was changed from

==========
service worker: Attribute purpose to start worker attempts for UMA

We want to see whether certain start worker purposes are timing out more
than others.

# Skip presubmit due to crbug.com/595198, presubmit passes locally
NOPRESUBMIT=true
BUG=559001
==========

to

==========
service worker: Attribute purpose to start worker attempts for UMA

We want to see whether certain start worker purposes are timing out more
than others.

# Skip presubmit due to crbug.com/595198, presubmit passes locally
NOPRESUBMIT=true
BUG=559001

Committed: https://crrev.com/98a214071cb9542f09bf62250c5c7f7637bfa9c5
Cr-Commit-Position: refs/heads/master@{#381391}
==========

[commit-bot: I haz the power, 2016-03-16 05:24:58]

Patchset 5 (id:??) landed as
https://crrev.com/98a214071cb9542f09bf62250c5c7f7637bfa9c5
Cr-Commit-Position: refs/heads/master@{#381391}

[Marijn Kruisselbrink, 2016-03-16 23:33:41]

https://codereview.chromium.org/1795863006/diff/80001/content/browser/service...
File content/browser/service_worker/service_worker_fetch_dispatcher.cc (right):

https://codereview.chromium.org/1795863006/diff/80001/content/browser/service...
content/browser/service_worker/service_worker_fetch_dispatcher.cc:98:
version_->StartRequest(ServiceWorkerMetrics::EventType::FETCH,
Bit late of a comment, but it seems weird that you marked EventType::FETCH as
deprecated (at least in comments) while it is still being used here, and in
histograms as the actual event type for ServiceWorkerMetrics::RecordEventTimeout
and RecordEventHandledRatio (which only records the ratio for ::FETCH).

[falken, 2016-03-17 01:37:37]

On 2016/03/16 23:33:41, Marijn Kruisselbrink wrote:
>
https://codereview.chromium.org/1795863006/diff/80001/content/browser/service...
> File content/browser/service_worker/service_worker_fetch_dispatcher.cc
(right):
> 
>
https://codereview.chromium.org/1795863006/diff/80001/content/browser/service...
> content/browser/service_worker/service_worker_fetch_dispatcher.cc:98:
> version_->StartRequest(ServiceWorkerMetrics::EventType::FETCH,
> Bit late of a comment, but it seems weird that you marked EventType::FETCH as
> deprecated (at least in comments) while it is still being used here, and in
> histograms as the actual event type for
ServiceWorkerMetrics::RecordEventTimeout
> and RecordEventHandledRatio (which only records the ratio for ::FETCH).

Yeah I should have made a TODO or comment here about that. I intend to clean
those up in future patches. It's deprecated in the sense that new code shouldn't
use it.