Add RAPPOR metric recording for Device{Orientation,Motion} in blink.

Add RAPPOR metric recording for Device{Orientation,Motion} in blink.

Make it possible to record url domains where DeviceOrientation and
DeviceMotion APIs are used. The general idea is to record the url
domain when 'deviceorientation' or 'devicemotion' event listeners
are added for the first time in JavaScript. The actual recording is
done browser-side via RAPPOR which is privacy preserving.

This patch also adds an empty recordRapporURL method to Platform.h.
The content-side implementation has been implemented here:
https://codereview.chromium.org/1014543003/

NOTE:
this blink-side patch should after the content-side patch with
rappor.xml definitions:
https://codereview.chromium.org/1052013004/ (content side)

BUG=467648
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=193957

[timvolodine, 2015-03-23 17:22:41]

timvolodine@chromium.org changed reviewers:
+ mkwst@chromium.org, smus@chromium.org

[timvolodine, 2015-03-23 17:22:42]

mkwst@: for Platform.h and blink bits
smus@: for the actual logging rationale

[Mike West, 2015-03-23 17:27:30]

https://codereview.chromium.org/1017433002/diff/1/public/platform/Platform.h
File public/platform/Platform.h (right):

https://codereview.chromium.org/1017433002/diff/1/public/platform/Platform.h#...
public/platform/Platform.h:609: virtual void
recordRapporMetric(WebPlatformEventType type, const WebURL& url) { }
Don't we already have "recordRappor"? Why do we need another method?

[timvolodine, 2015-03-23 17:33:30]

+cc: holte@

https://codereview.chromium.org/1017433002/diff/1/public/platform/Platform.h
File public/platform/Platform.h (right):

https://codereview.chromium.org/1017433002/diff/1/public/platform/Platform.h#...
public/platform/Platform.h:609: virtual void
recordRapporMetric(WebPlatformEventType type, const WebURL& url) { }
On 2015/03/23 17:27:30, Mike West wrote:
> Don't we already have "recordRappor"? Why do we need another method?

I understood from holte@ that there is currently no way to log from blink, i.e.
we need to send IPC to the browser, since rappor metrics have to be recorded on
the browser UI thread.. There is a crbug.com/429901 for this but it has not been
implemented yet.

[Mike West, 2015-03-23 17:51:11]

On 2015/03/23 at 17:33:30, timvolodine wrote:
> +cc: holte@
> 
> https://codereview.chromium.org/1017433002/diff/1/public/platform/Platform.h
> File public/platform/Platform.h (right):
> 
>
https://codereview.chromium.org/1017433002/diff/1/public/platform/Platform.h#...
> public/platform/Platform.h:609: virtual void
recordRapporMetric(WebPlatformEventType type, const WebURL& url) { }
> On 2015/03/23 17:27:30, Mike West wrote:
> > Don't we already have "recordRappor"? Why do we need another method?
> 
> I understood from holte@ that there is currently no way to log from blink,
i.e. we need to send IPC to the browser, since rappor metrics have to be
recorded on the browser UI thread.. There is a crbug.com/429901 for this but it
has not been implemented yet.

Right. My understanding was that the Service Worker team was piping this
through.
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
exists. It doesn't look like it's wired up on the Chromium side yet.

[Mike West, 2015-03-23 17:52:56]

mkwst@chromium.org changed reviewers:
+ kojii@chromium.org

[Mike West, 2015-03-23 17:52:57]

On 2015/03/23 at 17:51:11, Mike West wrote:
> > I understood from holte@ that there is currently no way to log from blink,
i.e. we need to send IPC to the browser, since rappor metrics have to be
recorded on the browser UI thread.. There is a crbug.com/429901 for this but it
has not been implemented yet.
> 
> Right. My understanding was that the Service Worker team was piping this
through.
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
exists. It doesn't look like it's wired up on the Chromium side yet.

https://codereview.chromium.org/1014543003/ is in-flight. Perhaps you could sync
with kojii@ (CC'd) to see who wants to do what? :)

[kojii, 2015-03-24 01:30:31]

On 2015/03/23 17:52:57, Mike West wrote:
> On 2015/03/23 at 17:51:11, Mike West wrote:
> > > I understood from holte@ that there is currently no way to log from blink,
> i.e. we need to send IPC to the browser, since rappor metrics have to be
> recorded on the browser UI thread.. There is a crbug.com/429901 for this but
it
> has not been implemented yet.
> > 
> > Right. My understanding was that the Service Worker team was piping this
> through.
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> exists. It doesn't look like it's wired up on the Chromium side yet.
> 
> https://codereview.chromium.org/1014543003/ is in-flight. Perhaps you could
sync
> with kojii@ (CC'd) to see who wants to do what? :)

Thank you Mike for connecting us.

The Platform method was added in this CL:
https://codereview.chromium.org/986583002/
and Chromium side is WIP:
https://codereview.chromium.org/1014543003/

In Chromium side, I took a bit different approach, to reach to RapporService
directly through the callback in base. Does this work for you too, or do you
need to dispatch them to your code in Chromium?

[timvolodine, 2015-03-24 12:38:22]

On 2015/03/24 01:30:31, koji wrote:
> On 2015/03/23 17:52:57, Mike West wrote:
> > On 2015/03/23 at 17:51:11, Mike West wrote:
> > > > I understood from holte@ that there is currently no way to log from
blink,
> > i.e. we need to send IPC to the browser, since rappor metrics have to be
> > recorded on the browser UI thread.. There is a crbug.com/429901 for this but
> it
> > has not been implemented yet.
> > > 
> > > Right. My understanding was that the Service Worker team was piping this
> > through.
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> > exists. It doesn't look like it's wired up on the Chromium side yet.
> > 
> > https://codereview.chromium.org/1014543003/ is in-flight. Perhaps you could
> sync
> > with kojii@ (CC'd) to see who wants to do what? :)
> 
> Thank you Mike for connecting us.
> 
> The Platform method was added in this CL:
> https://codereview.chromium.org/986583002/
> and Chromium side is WIP:
> https://codereview.chromium.org/1014543003/
> 
> In Chromium side, I took a bit different approach, to reach to RapporService
> directly through the callback in base. Does this work for you too, or do you
> need to dispatch them to your code in Chromium?

ah ok I was not aware there is a patch in flight for this, great!

Basically I just want to log the domains using the feature from blink, don't
really care how they are dispatched. However I see you are using
COARSE_RAPPOR_TYPE in your chromium-side patch, while I think I need eTLD+1,
i.e. I am simply using GetContentClient()->browser()->RecordURLMetric(metric,
url); to record the metric. So if I can trigger this call (or equivalent) from
blink that would be great.

[kojii, 2015-03-25 06:06:06]

On 2015/03/24 12:38:22, timvolodine wrote:
> However I see you are using
> COARSE_RAPPOR_TYPE in your chromium-side patch, while I think I need eTLD+1,
> i.e. I am simply using GetContentClient()->browser()->RecordURLMetric(metric,
> url); to record the metric. So if I can trigger this call (or equivalent) from
> blink that would be great.

Changed to eTLD+1 in PS3, as I got the same feedback from a reviewer.

[kojii, 2015-04-09 03:54:11]

The Chromium CL is close to land:
https://codereview.chromium.org/1014543003/

All reviews are done, working on the last nit that relies on another CL:
https://codereview.chromium.org/1070863002/

Can I suggest two things to Platform.h?

1. The impl side of Platform for a string and for a URL are in the CL:
https://codereview.chromium.org/1014543003/diff/520001/content/renderer/rende...
but the abstract method for a URL isn't done yet. Can you do that in this CL?

2. asvitkine@ suggested adding more friendly comments to Platform, the same one
as in:
https://codereview.chromium.org/1014543003/diff/520001/content/public/rendere...

Could you do it, for both recordRapporURL() and recordRapporURL()?

His original suggestion is here:
https://codereview.chromium.org/1014543003/#msg52

[timvolodine, 2015-04-09 14:29:37]

On 2015/04/09 03:54:11, koji wrote:
> The Chromium CL is close to land:
> https://codereview.chromium.org/1014543003/
> 
> All reviews are done, working on the last nit that relies on another CL:
> https://codereview.chromium.org/1070863002/
> 
> Can I suggest two things to Platform.h?
> 
> 1. The impl side of Platform for a string and for a URL are in the CL:
>
https://codereview.chromium.org/1014543003/diff/520001/content/renderer/rende...
> but the abstract method for a URL isn't done yet. Can you do that in this CL?
> 
> 2. asvitkine@ suggested adding more friendly comments to Platform, the same
one
> as in:
>
https://codereview.chromium.org/1014543003/diff/520001/content/public/rendere...
> 
> Could you do it, for both recordRapporURL() and recordRapporURL()?
> 
> His original suggestion is here:
> https://codereview.chromium.org/1014543003/#msg52

thanks koji!
sure, will add your suggestions to this patch

[timvolodine, 2015-04-13 15:42:28]

ok, done, mkwst@ could you PTAL please?

[Mike West, 2015-04-14 05:20:39]

LGTM % nit.

https://codereview.chromium.org/1017433002/diff/40001/public/platform/Platform.h
File public/platform/Platform.h (right):

https://codereview.chromium.org/1017433002/diff/40001/public/platform/Platfor...
public/platform/Platform.h:577: // Records a domain and registry of a url to a
Rappor privacy-preserving metric.
Nit: These method-level comments seem somewhat self-explanatory from the method
signature. I'd suggest dropping them. Adding the URL to the top-level comment is
a good idea, though!

[timvolodine, 2015-04-14 10:24:22]

thanks Mike!

https://codereview.chromium.org/1017433002/diff/40001/public/platform/Platform.h
File public/platform/Platform.h (right):

https://codereview.chromium.org/1017433002/diff/40001/public/platform/Platfor...
public/platform/Platform.h:577: // Records a domain and registry of a url to a
Rappor privacy-preserving metric.
On 2015/04/14 05:20:38, Mike West wrote:
> Nit: These method-level comments seem somewhat self-explanatory from the
method
> signature. I'd suggest dropping them. Adding the URL to the top-level comment
is
> a good idea, though!

I've added these comments on suggestion of koji@ and asvitkine@, I think one of
the concerns was that recordRapporURL might give the wrong impression that we
are logging urls..

[kojii, 2015-04-14 14:43:30]

On 2015/04/14 10:24:22, timvolodine wrote:
> thanks Mike!
> 
>
https://codereview.chromium.org/1017433002/diff/40001/public/platform/Platform.h
> File public/platform/Platform.h (right):
> 
>
https://codereview.chromium.org/1017433002/diff/40001/public/platform/Platfor...
> public/platform/Platform.h:577: // Records a domain and registry of a url to a
> Rappor privacy-preserving metric.
> On 2015/04/14 05:20:38, Mike West wrote:
> > Nit: These method-level comments seem somewhat self-explanatory from the
> method
> > signature. I'd suggest dropping them. Adding the URL to the top-level
comment
> is
> > a good idea, though!
> 
> I've added these comments on suggestion of koji@ and asvitkine@, I think one
of
> the concerns was that recordRapporURL might give the wrong impression that we
> are logging urls..

IIUC Mike's suggestion is to have single (top-level) comment for two methods by
merging the two. Doing so shouldn't conflict with what asvitkine@ requested.

recordRapporURL taking only domain and registry of the URL isn't that obvious
from the method signature, so merging the two comments might need a bit of
editorial creativity, but if you could make a good text, merging is fine with
me.

[timvolodine, 2015-04-15 13:53:45]

On 2015/04/14 14:43:30, koji wrote:
> On 2015/04/14 10:24:22, timvolodine wrote:
> > thanks Mike!
> > 
> >
>
https://codereview.chromium.org/1017433002/diff/40001/public/platform/Platform.h
> > File public/platform/Platform.h (right):
> > 
> >
>
https://codereview.chromium.org/1017433002/diff/40001/public/platform/Platfor...
> > public/platform/Platform.h:577: // Records a domain and registry of a url to
a
> > Rappor privacy-preserving metric.
> > On 2015/04/14 05:20:38, Mike West wrote:
> > > Nit: These method-level comments seem somewhat self-explanatory from the
> > method
> > > signature. I'd suggest dropping them. Adding the URL to the top-level
> comment
> > is
> > > a good idea, though!
> > 
> > I've added these comments on suggestion of koji@ and asvitkine@, I think one
> of
> > the concerns was that recordRapporURL might give the wrong impression that
we
> > are logging urls..
> 
> IIUC Mike's suggestion is to have single (top-level) comment for two methods
by
> merging the two. Doing so shouldn't conflict with what asvitkine@ requested.
> 
> recordRapporURL taking only domain and registry of the URL isn't that obvious
> from the method signature, so merging the two comments might need a bit of
> editorial creativity, but if you could make a good text, merging is fine with
> me.

ok, I tried being creative and merged the text into one comment ;)

[timvolodine, 2015-04-17 15:22:44]

The CQ bit was checked by timvolodine@chromium.org

[timvolodine, 2015-04-17 15:22:45]

The patchset sent to the CQ was uploaded after l-g-t-m from mkwst@chromium.org
Link to the patchset: https://codereview.chromium.org/1017433002/#ps60001
(title: "merge comments in Platform.h")

[commit-bot: I haz the power, 2015-04-17 15:23:04]

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

[commit-bot: I haz the power, 2015-04-17 16:50:56]

Committed patchset #4 (id:60001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=193957