Add UseCounter for web components related CSS selectors

Add UseCounter for web components related CSS selectors

This CL adds histogram statistics for the following
web components-related CSS selectors.

- :unresolved pseudo-class (for custom element)
- ::shadow pseudo element
- ::content pseudo element
- :host pseudo class
- :host-context() pseudo class
- /deep/ combinator

BUG=383749
TEST=open http://jsbin.com/bopezaso/2/edit, reload several times and see if FeatureObserver in chrome://histograms reflects it (number 465-470)

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

[kochi, 2014-06-18 06:00:31]

Hi Takashi,

This adds use counter stats as requested in:
https://code.google.com/p/chromium/issues/detail?id=383749#c10

Please take a look if stats collected by this CL are
valid for the purpose.

The point where we collect the stats may look weird,
but as useCounter (associated with a CSSParserContext)
is onwed by BisonCSSParser and CSS property stats are
collected this way, I followed it.

Dimitri, once tasak@ gives LGTM, could you take an
OWNERS look?

[esprehn, 2014-06-18 06:24:36]

https://codereview.chromium.org/339563004/diff/100001/Source/core/css/parser/...
File Source/core/css/parser/BisonCSSParser-in.cpp (right):

https://codereview.chromium.org/339563004/diff/100001/Source/core/css/parser/...
Source/core/css/parser/BisonCSSParser-in.cpp:1764: for (size_t i = 0; i <
selectors->size(); ++i) {
Lets put this in another function and early return from m_context.useCounter().
If that's false you'll still loop through everything below and just do no
reporting so all the looping is wasted.

I also don't think you handle nested rules properly like :not(:host(...)).

[kochi, 2014-06-18 09:03:32]

Thanks for the review.

PTAL.

https://codereview.chromium.org/339563004/diff/100001/Source/core/css/parser/...
File Source/core/css/parser/BisonCSSParser-in.cpp (right):

https://codereview.chromium.org/339563004/diff/100001/Source/core/css/parser/...
Source/core/css/parser/BisonCSSParser-in.cpp:1764: for (size_t i = 0; i <
selectors->size(); ++i) {
On 2014/06/18 06:24:35, esprehn wrote:
> Lets put this in another function and early return from
m_context.useCounter().
> If that's false you'll still loop through everything below and just do no
> reporting so all the looping is wasted.
> 
> I also don't think you handle nested rules properly like :not(:host(...)).


Done.

Split the part into a separate function, and I noticed
that I do not have to use CSSParserSelector, instead
I should use CSSSelectorList.

Also addressed that :not(:host()) was not counted.

https://codereview.chromium.org/339563004/diff/140001/Source/core/css/parser/...
File Source/core/css/parser/BisonCSSParser-in.cpp (right):

https://codereview.chromium.org/339563004/diff/140001/Source/core/css/parser/...
Source/core/css/parser/BisonCSSParser-in.cpp:1789: if (selector->relation() ==
CSSSelector::ShadowDeep)
I noticed that some pseudo element can be combined with /deep/, separated the
recording point of /deep/ from
other pseudo selectors.

[esprehn, 2014-06-18 11:13:25]

lgtm, but please remove that break case before landing.

https://codereview.chromium.org/339563004/diff/140001/Source/core/css/parser/...
File Source/core/css/parser/BisonCSSParser-in.cpp (right):

https://codereview.chromium.org/339563004/diff/140001/Source/core/css/parser/...
Source/core/css/parser/BisonCSSParser-in.cpp:1793: if
(selector->isLastInTagHistory())
I don't think you should need this check and break, shouldn't tagHistory() just
return null next and then your loop stops?

[kochi, 2014-06-18 16:30:24]

Thanks for the review!

I'll test the code again tomorrow morning before submitting to CQ.

https://codereview.chromium.org/339563004/diff/140001/Source/core/css/parser/...
File Source/core/css/parser/BisonCSSParser-in.cpp (right):

https://codereview.chromium.org/339563004/diff/140001/Source/core/css/parser/...
Source/core/css/parser/BisonCSSParser-in.cpp:1793: if
(selector->isLastInTagHistory())
On 2014/06/18 11:13:25, esprehn wrote:
> I don't think you should need this check and break, shouldn't tagHistory()
just
> return null next and then your loop stops?

This was unintentionally tricky...
The |selector| was used for both inner and outer loop.

Once |selector| gets 0 at the end of tagHistory and exits the inner loop,
the outer loop still tries to continue to the next in CSSSelectorList,
but CSSSelectorList::next(*selector) steps on a null pointer.
As long as I read CSSSelectorList::next() correctly, at LastInTagHistory
the selector's next is handled correctly - so breaking here at LastInTagHistory
should make sense.

The logic should be correct, but I admit it is very confusing.
Fixed the code for readability.

[kochi, 2014-06-19 01:17:23]

The CQ bit was checked by kochi@chromium.org

[kochi, 2014-06-19 01:17:42]

The CQ bit was unchecked by kochi@chromium.org

[kochi, 2014-06-19 01:18:15]

The CQ bit was checked by kochi@chromium.org

[commit-bot: I haz the power, 2014-06-19 01:18:19]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/kochi@chromium.org/339563004/180001

[commit-bot: I haz the power, 2014-06-19 01:24:07]

Change committed as 176473