Add RAPPOR metrics of ServiceWorker.ControlledPage

Add RAPPOR metrics of ServiceWorker.ControlledPage

This cl introduce "ServiceWorker.ControlledPage" RAPPOR metrics to understand which origin are driving Service Worker usage.

BUG=460855
Committed: https://crrev.com/48d4d3eede5fae249de9417931ac2b0fed1d41dd
Cr-Commit-Position: refs/heads/master@{#318387}

[horo, 2015-02-24 08:19:33]

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

[horo, 2015-02-24 08:19:34]

falken@
Could you please review this?

[Steven Holte, 2015-02-24 19:42:28]

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

[Steven Holte, 2015-02-24 19:42:28]

Remember to update tools/metrics/rappor/rappor.xml

[horo, 2015-02-25 00:29:26]

On 2015/02/24 19:42:28, Steven Holte wrote:
> Remember to update tools/metrics/rappor/rappor.xml

Done.
Thank you.

[falken, 2015-02-25 01:43:59]

Wow fast! The CL description would benefit from a short explanation of what
metric we're recording and why.

https://codereview.chromium.org/948293002/diff/20001/content/browser/service_...
File content/browser/service_worker/service_worker_metrics.cc (right):

https://codereview.chromium.org/948293002/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_metrics.cc:18: void
SampleDomainAndRegistryFromGURLOnUI(const GURL&url) {
missing space before url (weird, does that compile?)

https://codereview.chromium.org/948293002/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_metrics.cc:21:
"ServiceWorker.ControlledPageLoad",
I might not understand RAPPOR enough, but I feel we shouldn't use the same
metric name for the UMA count and the RAPPOR one? Or are they supposed to be
linked together somehow?

https://codereview.chromium.org/948293002/diff/20001/content/public/browser/c...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/948293002/diff/20001/content/public/browser/c...
content/public/browser/content_browser_client.h:630: virtual void
SampleDomainAndRegistryFromGURL(const std::string& metric,
What's the reason we're recording domain and registry vs origin?

https://codereview.chromium.org/948293002/diff/20001/tools/metrics/rappor/rap...
File tools/metrics/rappor/rappor.xml (right):

https://codereview.chromium.org/948293002/diff/20001/tools/metrics/rappor/rap...
tools/metrics/rappor/rappor.xml:151: The domain and registry of the top level
URL of a page which is controlled
I'm not sure top level URL makes sense here, if SW controls an iframe, we'll
record the iframe's url. What's top level mean anyhow?

[horo, 2015-02-25 02:27:00]

https://codereview.chromium.org/948293002/diff/20001/content/browser/service_...
File content/browser/service_worker/service_worker_metrics.cc (right):

https://codereview.chromium.org/948293002/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_metrics.cc:18: void
SampleDomainAndRegistryFromGURLOnUI(const GURL&url) {
On 2015/02/25 01:43:59, falken wrote:
> missing space before url (weird, does that compile?)

Done.

https://codereview.chromium.org/948293002/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_metrics.cc:21:
"ServiceWorker.ControlledPageLoad",
On 2015/02/25 01:43:59, falken wrote:
> I might not understand RAPPOR enough, but I feel we shouldn't use the same
> metric name for the UMA count and the RAPPOR one? Or are they supposed to be
> linked together somehow?

Done.

https://codereview.chromium.org/948293002/diff/20001/content/public/browser/c...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/948293002/diff/20001/content/public/browser/c...
content/public/browser/content_browser_client.h:630: virtual void
SampleDomainAndRegistryFromGURL(const std::string& metric,
On 2015/02/25 01:43:59, falken wrote:
> What's the reason we're recording domain and registry vs origin?

Rappor is designed to use "the eTLD+1 of the user's homepage".
http://www.chromium.org/developers/design-documents/rappor

https://codereview.chromium.org/948293002/diff/20001/tools/metrics/rappor/rap...
File tools/metrics/rappor/rappor.xml (right):

https://codereview.chromium.org/948293002/diff/20001/tools/metrics/rappor/rap...
tools/metrics/rappor/rappor.xml:151: The domain and registry of the top level
URL of a page which is controlled
On 2015/02/25 01:43:59, falken wrote:
> I'm not sure top level URL makes sense here, if SW controls an iframe, we'll
> record the iframe's url. What's top level mean anyhow?

I thought that "the top level URL" means the origin of URL.
But it was my misunderstanding.
I removed "top level".

[falken, 2015-02-25 03:36:58]

lgtm

https://codereview.chromium.org/948293002/diff/20001/content/public/browser/c...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/948293002/diff/20001/content/public/browser/c...
content/public/browser/content_browser_client.h:630: virtual void
SampleDomainAndRegistryFromGURL(const std::string& metric,
On 2015/02/25 02:27:00, horo wrote:
> On 2015/02/25 01:43:59, falken wrote:
> > What's the reason we're recording domain and registry vs origin?
> 
> Rappor is designed to use "the eTLD+1 of the user's homepage".
> http://www.chromium.org/developers/design-documents/rappor

I think that's just saying "Settings.HomePage2" uses eTLD+1 but it's not a
design restriction of Rappor itself. Anyway, domain/registry seems sufficient. I
was just curious because SW are per-origin. Maybe Steven Holte can chime in.

https://codereview.chromium.org/948293002/diff/40001/content/browser/service_...
File content/browser/service_worker/service_worker_metrics.cc (right):

https://codereview.chromium.org/948293002/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_metrics.cc:21:
"ServiceWorker.ControlledPage",
just bikeshedding now... "ControlledPageUrl" would be clearer for me to
understand the diff between ControlledPageLoad.

[horo, 2015-02-25 04:30:04]

https://codereview.chromium.org/948293002/diff/40001/content/browser/service_...
File content/browser/service_worker/service_worker_metrics.cc (right):

https://codereview.chromium.org/948293002/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_metrics.cc:21:
"ServiceWorker.ControlledPage",
On 2015/02/25 03:36:58, falken wrote:
> just bikeshedding now... "ControlledPageUrl" would be clearer for me to
> understand the diff between ControlledPageLoad.

Done.

[Steven Holte, 2015-02-25 18:30:12]

https://codereview.chromium.org/948293002/diff/20001/content/public/browser/c...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/948293002/diff/20001/content/public/browser/c...
content/public/browser/content_browser_client.h:630: virtual void
SampleDomainAndRegistryFromGURL(const std::string& metric,
On 2015/02/25 03:36:57, falken wrote:
> On 2015/02/25 02:27:00, horo wrote:
> > On 2015/02/25 01:43:59, falken wrote:
> > > What's the reason we're recording domain and registry vs origin?
> > 
> > Rappor is designed to use "the eTLD+1 of the user's homepage".
> > http://www.chromium.org/developers/design-documents/rappor
> 
> I think that's just saying "Settings.HomePage2" uses eTLD+1 but it's not a
> design restriction of Rappor itself. Anyway, domain/registry seems sufficient.
I
> was just curious because SW are per-origin. Maybe Steven Holte can chime in.

It's possible to record things other that eTLD+1, but I would generally
recommend that approach.  In order to detect a result, we need to see the same
value reported from a significant proportion of sources.  The more you subdivide
the smaller the percentage of what your report will actually be estimable.

[Steven Holte, 2015-02-25 18:30:35]

lgtm

[Steven Holte, 2015-02-25 18:30:36]

lgtm

[horo, 2015-02-26 00:56:20]

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

[horo, 2015-02-26 00:56:20]

nasko@
Could you please review content/public/browser/content_browser_client.h?

Thank you!

[nasko, 2015-02-26 01:03:20]

https://codereview.chromium.org/948293002/diff/60001/content/public/browser/c...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/948293002/diff/60001/content/public/browser/c...
content/public/browser/content_browser_client.h:630: virtual void
SampleDomainAndRegistryFromGURL(const std::string& metric,
This client interface should be towards a generic embedder, not specifically
chrome. As such, I find the name to be overly specific to the implementation
that chrome/ provides. Let's rename this to a bit more generic method and
comment it properly.

[horo, 2015-02-26 07:07:45]

https://codereview.chromium.org/948293002/diff/60001/content/public/browser/c...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/948293002/diff/60001/content/public/browser/c...
content/public/browser/content_browser_client.h:630: virtual void
SampleDomainAndRegistryFromGURL(const std::string& metric,
On 2015/02/26 01:03:20, nasko wrote:
> This client interface should be towards a generic embedder, not specifically
> chrome. As such, I find the name to be overly specific to the implementation
> that chrome/ provides. Let's rename this to a bit more generic method and
> comment it properly.

Done.

[nasko, 2015-02-26 14:52:45]

LGTM with couple of nits.

https://codereview.chromium.org/948293002/diff/80001/content/public/browser/c...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/948293002/diff/80001/content/public/browser/c...
content/public/browser/content_browser_client.h:629: // Records the url to
collect the metric.
Let's follow the style of comments already present in this file. Something like
"Allows the embedder to record |metric| for a specific |url|".

https://codereview.chromium.org/948293002/diff/80001/content/public/browser/c...
content/public/browser/content_browser_client.h:630: virtual void
RecordURLMetric(const std::string& metric, const GURL& gurl) {}
nit: s/gurl/url/

[horo, 2015-02-27 00:59:14]

Thank you!

https://codereview.chromium.org/948293002/diff/80001/content/public/browser/c...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/948293002/diff/80001/content/public/browser/c...
content/public/browser/content_browser_client.h:629: // Records the url to
collect the metric.
On 2015/02/26 14:52:44, nasko wrote:
> Let's follow the style of comments already present in this file. Something
like
> "Allows the embedder to record |metric| for a specific |url|".

Done.

https://codereview.chromium.org/948293002/diff/80001/content/public/browser/c...
content/public/browser/content_browser_client.h:630: virtual void
RecordURLMetric(const std::string& metric, const GURL& gurl) {}
On 2015/02/26 14:52:44, nasko wrote:
> nit: s/gurl/url/

Done.

[horo, 2015-02-27 00:59:22]

The CQ bit was checked by horo@chromium.org

[horo, 2015-02-27 00:59:22]

The patchset sent to the CQ was uploaded after l-g-t-m from falken@chromium.org,
holte@chromium.org, nasko@chromium.org
Link to the patchset: https://codereview.chromium.org/948293002/#ps100001
(title: "incorporated nasko's comment")

[commit-bot: I haz the power, 2015-02-27 01:00:14]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/948293002/100001

[commit-bot: I haz the power, 2015-02-27 03:04:17]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-02-27 03:05:01]

Patchset 6 (id:??) landed as
https://crrev.com/48d4d3eede5fae249de9417931ac2b0fed1d41dd
Cr-Commit-Position: refs/heads/master@{#318387}