Add UseCounter for case-insensitive attribute value selector matching

Add UseCounter for case-insensitive attribute value selector matching

More specifically, count the cases where the result would be different
if a currently (automatically) case-insensitive attribute value match
was made case-sensitive instead, as is the current status of the spec.

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

[Jens Widell, 2014-01-20 15:12:51]

Please take a look.

This change inevitably slows down some selector matching (where we do both a
case-sensitive and a case-insensitive comparison) but the slight restructuring
actually makes it faster in some cases too, according to my testing.

In particular, a selector that references a case-insensitive attribute and still
matches if the match is made case-sensitively is slightly faster now, since we
only do a fast case-sensitive match in that case.  Attribute selectors that
reference case-insensitive attributes and either don't match at all or only
match case-insensitively are of course slightly slower.

[Mike West, 2014-01-23 09:21:13]

https://codereview.chromium.org/140323003/diff/1/Source/core/css/SelectorChec...
File Source/core/css/SelectorChecker.cpp (right):

https://codereview.chromium.org/140323003/diff/1/Source/core/css/SelectorChec...
Source/core/css/SelectorChecker.cpp:452: bool caseSensitive =
!element.document().isHTMLDocument() ||
HTMLDocument::isCaseSensitiveAttribute(selectorAttr);
Why did you move this inside the `for` loop? It appears to be an invariant.

[Mike West, 2014-01-23 09:22:53]

https://codereview.chromium.org/140323003/diff/1/Source/core/frame/UseCounter.h
File Source/core/frame/UseCounter.h (right):

https://codereview.chromium.org/140323003/diff/1/Source/core/frame/UseCounter...
Source/core/frame/UseCounter.h:301: CaseInsensitiveAttrSelectorMatch, //
Case-insensitivity dropped from specification.
I'd suggest adding a counter for attribute selectors in general, so that we have
something to compare against. Right now, it'll be tough to answer the question
"What percentage of calls will this change effect?"

[Jens Widell, 2014-01-23 09:38:49]

https://codereview.chromium.org/140323003/diff/1/Source/core/css/SelectorChec...
File Source/core/css/SelectorChecker.cpp (right):

https://codereview.chromium.org/140323003/diff/1/Source/core/css/SelectorChec...
Source/core/css/SelectorChecker.cpp:452: bool caseSensitive =
!element.document().isHTMLDocument() ||
HTMLDocument::isCaseSensitiveAttribute(selectorAttr);
On 2014/01/23 09:21:14, Mike West wrote:
> Why did you move this inside the `for` loop? It appears to be an invariant.

Moving it is essentially a tiny optimization.

It is an invariant, yes, but it's also something we probably don't need to know.
 By moving it, we only check this when we need to know; i.e. when we've found an
attribute with a matching name whose value didn't match case-sensitively.

Also, in most cases, there will at most be one attribute on each element with
the right name, so moving it here doesn't actually mean it gets executed more
than once anyway in typical cases.

Another possible optimization here is of course to break out of the loop after
finding an attribute with a matching name, unless the selector can match
attributes with different (qualified) names.

https://codereview.chromium.org/140323003/diff/1/Source/core/frame/UseCounter.h
File Source/core/frame/UseCounter.h (right):

https://codereview.chromium.org/140323003/diff/1/Source/core/frame/UseCounter...
Source/core/frame/UseCounter.h:301: CaseInsensitiveAttrSelectorMatch, //
Case-insensitivity dropped from specification.
On 2014/01/23 09:22:54, Mike West wrote:
> I'd suggest adding a counter for attribute selectors in general, so that we
have
> something to compare against. Right now, it'll be tough to answer the question
> "What percentage of calls will this change effect?"

I'm not 100 % sure how use counters work, but won't this counter alone answer
the question "how many page loads had at least one selector whose behavior would
change?"?  Do we need to know how many page loads were not affected because they
had no attribute selectors vs. how many page loads were not affected because
their attribute selectors were not case-insensitive to begin with, or were but
matched even without case-insensitivity?

I don't mind adding additional use-counters, of course, but I guess they (too)
would carry a slight performance cost in matching attribute selectors.

[Mike West, 2014-01-23 10:08:24]

https://codereview.chromium.org/140323003/diff/1/Source/core/css/SelectorChec...
File Source/core/css/SelectorChecker.cpp (right):

https://codereview.chromium.org/140323003/diff/1/Source/core/css/SelectorChec...
Source/core/css/SelectorChecker.cpp:452: bool caseSensitive =
!element.document().isHTMLDocument() ||
HTMLDocument::isCaseSensitiveAttribute(selectorAttr);
On 2014/01/23 09:38:49, Jens Lindström wrote:
> On 2014/01/23 09:21:14, Mike West wrote:
>
> It is an invariant, yes, but it's also something we probably don't need to
know.
>  By moving it, we only check this when we need to know; i.e. when we've found
an
> attribute with a matching name whose value didn't match case-sensitively.

Hrm. Ok, sounds reasonable. Thanks for the explanation.

https://codereview.chromium.org/140323003/diff/1/Source/core/frame/UseCounter.h
File Source/core/frame/UseCounter.h (right):

https://codereview.chromium.org/140323003/diff/1/Source/core/frame/UseCounter...
Source/core/frame/UseCounter.h:301: CaseInsensitiveAttrSelectorMatch, //
Case-insensitivity dropped from specification.
On 2014/01/23 09:38:49, Jens Lindström wrote:
> I'm not 100 % sure how use counters work, but won't this counter alone answer
> the question "how many page loads had at least one selector whose behavior
would
> change?"?  Do we need to know how many page loads were not affected because
they
> had no attribute selectors vs. how many page loads were not affected because
> their attribute selectors were not case-insensitive to begin with, or were but
> matched even without case-insensitivity?

I'd suggest that there are two relevant numbers:

1. The percentage of total page loads that this change would affect: that's the
number the counter you're adding gives us.
2. The percentage of page loads that contain selectors which would be affected.

If, for example, #1 was 0.002%, but #2 was 100% (low penetration of a feature,
but every use is broken), we might approach it differently than if #2 was 0.02%
(high penetration of a feature, low impact change).

Does that make sense?

[Jens Widell, 2014-01-23 10:27:21]

https://codereview.chromium.org/140323003/diff/1/Source/core/css/SelectorChec...
File Source/core/css/SelectorChecker.cpp (right):

https://codereview.chromium.org/140323003/diff/1/Source/core/css/SelectorChec...
Source/core/css/SelectorChecker.cpp:452: bool caseSensitive =
!element.document().isHTMLDocument() ||
HTMLDocument::isCaseSensitiveAttribute(selectorAttr);
On 2014/01/23 10:08:24, Mike West wrote:
> Hrm. Ok, sounds reasonable. Thanks for the explanation.

Sorry about the unexplained micro-optimizations I included in the patch.  I was
worried it would be difficult to apply if it regressed performance tests too
much.

https://codereview.chromium.org/140323003/diff/1/Source/core/frame/UseCounter.h
File Source/core/frame/UseCounter.h (right):

https://codereview.chromium.org/140323003/diff/1/Source/core/frame/UseCounter...
Source/core/frame/UseCounter.h:301: CaseInsensitiveAttrSelectorMatch, //
Case-insensitivity dropped from specification.
On 2014/01/23 10:08:24, Mike West wrote:
> On 2014/01/23 09:38:49, Jens Lindström wrote:
> > I'm not 100 % sure how use counters work, but won't this counter alone
answer
> > the question "how many page loads had at least one selector whose behavior
> would
> > change?"?  Do we need to know how many page loads were not affected because
> they
> > had no attribute selectors vs. how many page loads were not affected because
> > their attribute selectors were not case-insensitive to begin with, or were
but
> > matched even without case-insensitivity?
> 
> I'd suggest that there are two relevant numbers:
> 
> 1. The percentage of total page loads that this change would affect: that's
the
> number the counter you're adding gives us.
> 2. The percentage of page loads that contain selectors which would be
affected.

How would you define 2?  A case-insensitive attribute selector is

 A) parsed in a stylesheet
 B) checked against an element
 C) checked against an attribute value
 D) something else?

Option A would probably be performance neutral, while B/C would increase the
performance impact of this patch somewhat.

[Mike West, 2014-01-24 09:24:52]

I thought about this a little overnight, and I think I'm overanalyzing it.

This counter is enough to help you determine whether you can make the change
that's been proposed. Land it. :)

LGTM.

[Jens Widell, 2014-01-24 09:34:54]

On 2014/01/24 09:24:52, Mike West wrote:
> I thought about this a little overnight, and I think I'm overanalyzing it.
> 
> This counter is enough to help you determine whether you can make the change
> that's been proposed. Land it. :)
> 
> LGTM.

Thanks.  :-)

[commit-bot: I haz the power, 2014-01-24 10:06:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jl@opera.com/140323003/100002

[commit-bot: I haz the power, 2014-01-24 13:37:16]

Change committed as 165740