Collect host of Web Component usage to send to RAPPOR

Collect host of Web Component usage to send to RAPPOR

This patch is the Blink side of using RAPPOR to understand Web Components
usage. It collects the host part of the URL where
Element::createShadowRoot() and Document::registerElement() are used.

To send the data to RAPPOR, we need to IPC to the browser process. This
part will be in a separate patch for the Chromium side.

The following conditions apply:
1. Calls from extensions and private scripts are excluded.
2. When IFRAME is used, counts the same hosts just once.
3. Count HTTP and HTTPS only.

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

[kojii, 2015-03-09 16:16:46]

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

[kojii, 2015-03-09 16:16:46]

hayato@, PTAL.

CC'ed bashi@ and haraken@ as told by bashi@ when I consulted him for hooking
these APIs.

[bashi, 2015-03-09 23:42:31]

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

[bashi, 2015-03-09 23:42:32]

https://codereview.chromium.org/986583002/diff/100001/Source/core/frame/UseBy...
File Source/core/frame/UseByOriginCounter.cpp (right):

https://codereview.chromium.org/986583002/diff/100001/Source/core/frame/UseBy...
Source/core/frame/UseByOriginCounter.cpp:1: /*
nit: Use the 3-line license header.

[kojii, 2015-03-10 05:06:16]

Thank you for your comment, license header updated.

https://codereview.chromium.org/986583002/diff/100001/Source/core/frame/UseBy...
File Source/core/frame/UseByOriginCounter.cpp (right):

https://codereview.chromium.org/986583002/diff/100001/Source/core/frame/UseBy...
Source/core/frame/UseByOriginCounter.cpp:1: /*
On 2015/03/09 23:42:31, bashi1 wrote:
> nit: Use the 3-line license header.

Done, thank you, I missed that.

[kojii, 2015-03-13 11:52:51]

PTAL. Chromium side is now working, I'll test and clean up a bit more before
submitting a CL, but this CL needs to land first.

[kojii, 2015-03-13 11:58:09]

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

[kojii, 2015-03-13 12:46:18]

kojii@chromium.org changed reviewers:
+ tkent@chromium.org
- yosin@chromium.org

[kojii, 2015-03-13 12:46:19]

tkent@, could you PTAL? I noticed that public/OWNERS has much fewer owners.

Should I split the change to that file to a separate CL so that you don't have
to review the whole patch? Your advice is appreciated in advance.

[tkent, 2015-03-16 00:30:19]

https://codereview.chromium.org/986583002/diff/160001/Source/core/frame/UseBy...
File Source/core/frame/UseByOriginCounter.cpp (right):

https://codereview.chromium.org/986583002/diff/160001/Source/core/frame/UseBy...
Source/core/frame/UseByOriginCounter.cpp:33: ASSERT((unsigned)feature >= 0 &&
feature < Feature::NumberOfFeatures);
We don't use C-style cast.  (unsigned) -> static_cast<unsigned>.
Anyway, casting to unsigned and checking >=0 makes no sense.  unsigned is always
>=0.

We shouldn't add multiple assertions in one ASSERT:
  ASSERT(A  && B);  should be:
  ASSERT(A);
  ASSERT(B);

https://codereview.chromium.org/986583002/diff/160001/Source/core/frame/UseBy...
Source/core/frame/UseByOriginCounter.cpp:59: for (std::pair<String,
UseByOriginCounter::Value>& originAndValue : m_originAndValues) {
nit: You can use |const auto&| here.

https://codereview.chromium.org/986583002/diff/160001/Source/core/frame/UseBy...
File Source/core/frame/UseByOriginCounter.h (right):

https://codereview.chromium.org/986583002/diff/160001/Source/core/frame/UseBy...
Source/core/frame/UseByOriginCounter.h:17: class UseByOriginCounter {
I feel this is not a counter.  Is this origin-using-features-reporter?

https://codereview.chromium.org/986583002/diff/160001/Source/core/frame/UseBy...
Source/core/frame/UseByOriginCounter.h:42: bool get(Feature feature) const {
return m_countBits & (1 << (unsigned)feature); }
We don't use C-style cast.  (unsigned) -> static_cast<unsigned>()

https://codereview.chromium.org/986583002/diff/160001/Source/core/frame/UseBy...
Source/core/frame/UseByOriginCounter.h:48: unsigned m_countBits :
(unsigned)Feature::NumberOfFeatures;
Ditto.

Also, do you think the number of features won't grow over the size of an
integer?  UseCounter had similar problem, and we switched to BitVector.

https://codereview.chromium.org/986583002/diff/160001/Source/core/frame/UseBy...
Source/core/frame/UseByOriginCounter.h:51: DEFINE_INLINE_TRACE() {
visitor->trace(m_originAndValues); }
Oilpan tracing is not necessary because this class isn't GarbageCollected.

https://codereview.chromium.org/986583002/diff/160001/Source/core/frame/UseBy...
Source/core/frame/UseByOriginCounter.h:54: WillBeHeapVector<std::pair<String,
UseByOriginCounter::Value>, 1> m_originAndValues;
Ditto.
should be Vector<std::pair<String, UseByOriginCounter::Value>, 1>.

[tkent, 2015-03-16 00:31:35]

On 2015/03/13 12:46:19, koji wrote:
> Should I split the change to that file to a separate CL so that you don't have
> to review the whole patch? Your advice is appreciated in advance.

No.  You can ask a reviewer to review a specific part of the CL.

[kojii, 2015-03-16 03:52:38]

On 2015/03/16 00:31:35, tkent wrote:
> On 2015/03/13 12:46:19, koji wrote:
> > Should I split the change to that file to a separate CL so that you don't
have
> > to review the whole patch? Your advice is appreciated in advance.
> 
> No.  You can ask a reviewer to review a specific part of the CL.

Understood, thank you.

[kojii, 2015-03-16 03:52:51]

All fixed, PTAL.

https://codereview.chromium.org/986583002/diff/160001/Source/core/frame/UseBy...
File Source/core/frame/UseByOriginCounter.cpp (right):

https://codereview.chromium.org/986583002/diff/160001/Source/core/frame/UseBy...
Source/core/frame/UseByOriginCounter.cpp:33: ASSERT((unsigned)feature >= 0 &&
feature < Feature::NumberOfFeatures);
On 2015/03/16 00:30:19, tkent wrote:
> We don't use C-style cast.  (unsigned) -> static_cast<unsigned>.
> Anyway, casting to unsigned and checking >=0 makes no sense.  unsigned is
always
> >=0.
> 
> We shouldn't add multiple assertions in one ASSERT:
>   ASSERT(A  && B);  should be:
>   ASSERT(A);
>   ASSERT(B);

Done.

https://codereview.chromium.org/986583002/diff/160001/Source/core/frame/UseBy...
Source/core/frame/UseByOriginCounter.cpp:59: for (std::pair<String,
UseByOriginCounter::Value>& originAndValue : m_originAndValues) {
On 2015/03/16 00:30:19, tkent wrote:
> nit: You can use |const auto&| here.

Done.

https://codereview.chromium.org/986583002/diff/160001/Source/core/frame/UseBy...
File Source/core/frame/UseByOriginCounter.h (right):

https://codereview.chromium.org/986583002/diff/160001/Source/core/frame/UseBy...
Source/core/frame/UseByOriginCounter.h:17: class UseByOriginCounter {
On 2015/03/16 00:30:19, tkent wrote:
> I feel this is not a counter.  Is this origin-using-features-reporter?

Done.

https://codereview.chromium.org/986583002/diff/160001/Source/core/frame/UseBy...
Source/core/frame/UseByOriginCounter.h:42: bool get(Feature feature) const {
return m_countBits & (1 << (unsigned)feature); }
On 2015/03/16 00:30:19, tkent wrote:
> We don't use C-style cast.  (unsigned) -> static_cast<unsigned>()

Done.

https://codereview.chromium.org/986583002/diff/160001/Source/core/frame/UseBy...
Source/core/frame/UseByOriginCounter.h:48: unsigned m_countBits :
(unsigned)Feature::NumberOfFeatures;
On 2015/03/16 00:30:19, tkent wrote:
> Ditto.

Done.

> Also, do you think the number of features won't grow over the size of an
> integer?  UseCounter had similar problem, and we switched to BitVector.

Looks like it's unknown. Heard an idea to turn all features in UseCounter to use
RAPPOR. But then that'd mean we'll upload almost every domain names users visit,
and we weren't sure we have enough justifications to do so.

So my strategy here was to have the minimum code to support the requirements for
now, while making it close to UseCounter by seeing the possibility to move to
BitVector or UseCounter::CountBits, or even merge this class into UseCounter.
Does this look ok?

https://codereview.chromium.org/986583002/diff/160001/Source/core/frame/UseBy...
Source/core/frame/UseByOriginCounter.h:51: DEFINE_INLINE_TRACE() {
visitor->trace(m_originAndValues); }
On 2015/03/16 00:30:19, tkent wrote:
> Oilpan tracing is not necessary because this class isn't GarbageCollected.

Done, I thought we need because it has String, sorry for misunderstanding.

https://codereview.chromium.org/986583002/diff/160001/Source/core/frame/UseBy...
Source/core/frame/UseByOriginCounter.h:54: WillBeHeapVector<std::pair<String,
UseByOriginCounter::Value>, 1> m_originAndValues;
On 2015/03/16 00:30:19, tkent wrote:
> Ditto.
> should be Vector<std::pair<String, UseByOriginCounter::Value>, 1>.

Done.

[tkent, 2015-03-16 04:59:21]

lgtm

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

https://codereview.chromium.org/986583002/diff/180001/public/platform/Platfor...
public/platform/Platform.h:545: // Record to RAPPPOR.
RAPPPOR -> RAPPOR

[bashi, 2015-03-16 05:28:56]

LGTM on ScriptState use

[hayato, 2015-03-16 05:50:40]

lgtm

[kojii, 2015-03-16 06:45:41]

Thank you for the reviews!

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

https://codereview.chromium.org/986583002/diff/180001/public/platform/Platfor...
public/platform/Platform.h:545: // Record to RAPPPOR.
On 2015/03/16 04:59:21, tkent wrote:
> RAPPPOR -> RAPPOR

Done.

[kojii, 2015-03-16 06:45:46]

The CQ bit was checked by kojii@chromium.org

[kojii, 2015-03-16 06:45:47]

The patchset sent to the CQ was uploaded after l-g-t-m from tkent@chromium.org
Link to the patchset: https://codereview.chromium.org/986583002/#ps200001
(title: "Fix typo")

[commit-bot: I haz the power, 2015-03-16 06:45:54]

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

[commit-bot: I haz the power, 2015-03-16 06:49:09]

Committed patchset #11 (id:200001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=191893