Add support for case-insensitive attribute value selectors

Add support for case-insensitive attribute value selectors

Selectors Level 4 adds the syntax '*[foo="bar" i]' for making an attribute
value selector case-insensitive. Add support for that.

Does not alter the actual case-sensitivity match - i.e. matching is
performed using Unicode case-insensitivity rather than ASCII which
the spec expects.

Based on patch by <jl@opera.com>;.

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

[fs, 2014-07-01 08:50:41]

Attempting to reboot https://codereview.chromium.org/252683011/. I've left some
comments in that review, and attempted to address the issues here (PS2+3). I've
not done anything about the 'ASCII case insensitive' bit, since it seemed like
an orthogonal issue (and probably better landed as a separate CL regardless.)

PTAL.

[ojan, 2014-07-19 02:55:13]

Sorry I haven't had time to review this. Been busy with the code yellow. Tim, do
you feel comfortable reviewing this code?

[Timothy Loh, 2014-07-21 00:13:42]

On 2014/07/19 02:55:13, ojan-only-code-yellow-reviews wrote:
> Sorry I haven't had time to review this. Been busy with the code yellow. Tim,
do
> you feel comfortable reviewing this code?

I'm not too familiar with this code, but I'll have a look.

[fs, 2014-07-21 07:31:14]

On 2014/07/19 02:55:13, ojan-only-code-yellow-reviews wrote:
> Sorry I haven't had time to review this. Been busy with the code yellow. Tim,
do
> you feel comfortable reviewing this code?

No worries - I've had a hard time remembering to do anything about this myself
=). Been meaning to ping eseidel about it (since he had some issues raised on
the original one which tried to address here).

[Timothy Loh, 2014-07-22 08:51:03]

https://codereview.chromium.org/330043003/diff/30001/Source/core/css/Selector...
File Source/core/css/SelectorChecker.cpp (right):

https://codereview.chromium.org/330043003/diff/30001/Source/core/css/Selector...
Source/core/css/SelectorChecker.cpp:520: bool caseSensitive =
!element.document().isHTMLDocument() ||
HTMLDocument::isCaseSensitiveAttribute(selectorAttr);
This is super confusing (we have a caseSensitive and a caseInsensitive variable
in the same scope)! Can we rename things and tweak comments to aid comprehension
here?

[fs, 2014-07-22 11:06:17]

https://codereview.chromium.org/330043003/diff/30001/Source/core/css/Selector...
File Source/core/css/SelectorChecker.cpp (right):

https://codereview.chromium.org/330043003/diff/30001/Source/core/css/Selector...
Source/core/css/SelectorChecker.cpp:520: bool caseSensitive =
!element.document().isHTMLDocument() ||
HTMLDocument::isCaseSensitiveAttribute(selectorAttr);
On 2014/07/22 08:51:02, Timothy Loh wrote:
> This is super confusing (we have a caseSensitive and a caseInsensitive
variable
> in the same scope)! Can we rename things and tweak comments to aid
comprehension
> here?

I suppose we could make the new (currently |caseInsensitive|)
|selectorCaseInsensitive| or somesuch. Maybe |matchAttrCaseSensitive(ly)| for
the current |caseSensitive|? Naming is hard though, so I'll gladly take
suggestions...

What kind of comment tweaks were you envisioning?

[Timothy Loh, 2014-07-22 11:50:26]

https://codereview.chromium.org/330043003/diff/30001/Source/core/css/Selector...
File Source/core/css/SelectorChecker.cpp (right):

https://codereview.chromium.org/330043003/diff/30001/Source/core/css/Selector...
Source/core/css/SelectorChecker.cpp:520: bool caseSensitive =
!element.document().isHTMLDocument() ||
HTMLDocument::isCaseSensitiveAttribute(selectorAttr);
On 2014/07/22 11:06:17, fs wrote:
> On 2014/07/22 08:51:02, Timothy Loh wrote:
> > This is super confusing (we have a caseSensitive and a caseInsensitive
> variable
> > in the same scope)! Can we rename things and tweak comments to aid
> comprehension
> > here?
> 
> I suppose we could make the new (currently |caseInsensitive|)
> |selectorCaseInsensitive| or somesuch. Maybe |matchAttrCaseSensitive(ly)| for
> the current |caseSensitive|? Naming is hard though, so I'll gladly take
> suggestions...
> 
> What kind of comment tweaks were you envisioning?

If I'm reading crbug.com/327060 right, the existing |caseSensitive| branch is
legacy behaviour that we want to remove and this patch will allow us to tweak
our UA stylesheets to not hit this case. So perhaps keep |caseInsensitive| as
is, but change |caseSensitive| into |legacyCaseInsensitive|.

I would write a comment like "Legacy behavior is to treat values for some
attributes as case insensitive regardless of whether the case insensitive flag
is set" but feel free to write what makes the most sense to you; the existing
bug link here is probably the most useful part.

[fs, 2014-07-22 14:09:04]

https://codereview.chromium.org/330043003/diff/30001/Source/core/css/Selector...
File Source/core/css/SelectorChecker.cpp (right):

https://codereview.chromium.org/330043003/diff/30001/Source/core/css/Selector...
Source/core/css/SelectorChecker.cpp:520: bool caseSensitive =
!element.document().isHTMLDocument() ||
HTMLDocument::isCaseSensitiveAttribute(selectorAttr);
On 2014/07/22 11:50:26, Timothy Loh wrote:
> On 2014/07/22 11:06:17, fs wrote:
> > On 2014/07/22 08:51:02, Timothy Loh wrote:
> > > This is super confusing (we have a caseSensitive and a caseInsensitive
> > variable
> > > in the same scope)! Can we rename things and tweak comments to aid
> > comprehension
> > > here?
> > 
> > I suppose we could make the new (currently |caseInsensitive|)
> > |selectorCaseInsensitive| or somesuch. Maybe |matchAttrCaseSensitive(ly)|
for
> > the current |caseSensitive|? Naming is hard though, so I'll gladly take
> > suggestions...
> > 
> > What kind of comment tweaks were you envisioning?
> 
> If I'm reading crbug.com/327060 right, the existing |caseSensitive| branch is
> legacy behaviour that we want to remove and this patch will allow us to tweak
> our UA stylesheets to not hit this case. So perhaps keep |caseInsensitive| as
> is, but change |caseSensitive| into |legacyCaseInsensitive|.
> 
> I would write a comment like "Legacy behavior is to treat values for some
> attributes as case insensitive regardless of whether the case insensitive flag
> is set" but feel free to write what makes the most sense to you; the existing
> bug link here is probably the most useful part.

Thank you for elaborating. I've updated to something like the above.

[Timothy Loh, 2014-07-23 01:46:51]

lgtm, but I wouldn't mind if someone else (eseidel?) had a look over this.

We should also be testing the case-insensitive flag for the other attribute
selectors ([att^=val i], [att~=val i], etc.)

Also maybe worthwhile to add a note in the description that this is unicode case
insensitivity instead of ASCII which the spec expects.

https://codereview.chromium.org/330043003/diff/30001/Source/core/css/Selector...
File Source/core/css/SelectorChecker.cpp (right):

https://codereview.chromium.org/330043003/diff/30001/Source/core/css/Selector...
Source/core/css/SelectorChecker.cpp:520: bool caseSensitive =
!element.document().isHTMLDocument() ||
HTMLDocument::isCaseSensitiveAttribute(selectorAttr);
On 2014/07/22 14:09:04, fs wrote:
> On 2014/07/22 11:50:26, Timothy Loh wrote:
> > On 2014/07/22 11:06:17, fs wrote:
> > > On 2014/07/22 08:51:02, Timothy Loh wrote:
> > > > This is super confusing (we have a caseSensitive and a caseInsensitive
> > > variable
> > > > in the same scope)! Can we rename things and tweak comments to aid
> > > comprehension
> > > > here?
> > > 
> > > I suppose we could make the new (currently |caseInsensitive|)
> > > |selectorCaseInsensitive| or somesuch. Maybe |matchAttrCaseSensitive(ly)|
> for
> > > the current |caseSensitive|? Naming is hard though, so I'll gladly take
> > > suggestions...
> > > 
> > > What kind of comment tweaks were you envisioning?
> > 
> > If I'm reading crbug.com/327060 right, the existing |caseSensitive| branch
is
> > legacy behaviour that we want to remove and this patch will allow us to
tweak
> > our UA stylesheets to not hit this case. So perhaps keep |caseInsensitive|
as
> > is, but change |caseSensitive| into |legacyCaseInsensitive|.
> > 
> > I would write a comment like "Legacy behavior is to treat values for some
> > attributes as case insensitive regardless of whether the case insensitive
flag
> > is set" but feel free to write what makes the most sense to you; the
existing
> > bug link here is probably the most useful part.
> 
> Thank you for elaborating. I've updated to something like the above.

I think |legacyCaseInsensitive| (instead of |legacyCaseSensitive|) makes this
slightly easier to understand

[fs, 2014-07-23 11:24:44]

On 2014/07/23 01:46:51, Timothy Loh wrote:
> lgtm, but I wouldn't mind if someone else (eseidel?) had a look over this.

Absolutely.

@eseidel: Could you PTAL? (Revisiting https://codereview.chromium.org/252683011/
might help you remember what you thought the last time =))
 
> We should also be testing the case-insensitive flag for the other attribute
> selectors ([att^=val i], [att~=val i], etc.)

Extended the TC with more match operators.

> Also maybe worthwhile to add a note in the description that this is unicode
case
> insensitivity instead of ASCII which the spec expects.

Added.
 
>
https://codereview.chromium.org/330043003/diff/30001/Source/core/css/Selector...
> File Source/core/css/SelectorChecker.cpp (right):
> 
>
https://codereview.chromium.org/330043003/diff/30001/Source/core/css/Selector...
> Source/core/css/SelectorChecker.cpp:520: bool caseSensitive =
> !element.document().isHTMLDocument() ||
> HTMLDocument::isCaseSensitiveAttribute(selectorAttr);
> On 2014/07/22 14:09:04, fs wrote:
> > On 2014/07/22 11:50:26, Timothy Loh wrote:
> > > On 2014/07/22 11:06:17, fs wrote:
> > > > On 2014/07/22 08:51:02, Timothy Loh wrote:
> > > > > This is super confusing (we have a caseSensitive and a caseInsensitive
> > > > variable
> > > > > in the same scope)! Can we rename things and tweak comments to aid
> > > > comprehension
> > > > > here?
> > > > 
> > > > I suppose we could make the new (currently |caseInsensitive|)
> > > > |selectorCaseInsensitive| or somesuch. Maybe
|matchAttrCaseSensitive(ly)|
> > for
> > > > the current |caseSensitive|? Naming is hard though, so I'll gladly take
> > > > suggestions...
> > > > 
> > > > What kind of comment tweaks were you envisioning?
> > > 
> > > If I'm reading crbug.com/327060 right, the existing |caseSensitive| branch
> is
> > > legacy behaviour that we want to remove and this patch will allow us to
> tweak
> > > our UA stylesheets to not hit this case. So perhaps keep |caseInsensitive|
> as
> > > is, but change |caseSensitive| into |legacyCaseInsensitive|.
> > > 
> > > I would write a comment like "Legacy behavior is to treat values for some
> > > attributes as case insensitive regardless of whether the case insensitive
> flag
> > > is set" but feel free to write what makes the most sense to you; the
> existing
> > > bug link here is probably the most useful part.
> > 
> > Thank you for elaborating. I've updated to something like the above.
> 
> I think |legacyCaseInsensitive| (instead of |legacyCaseSensitive|) makes this
> slightly easier to understand

Done.

[fs, 2014-07-25 17:00:09]

On 2014/07/23 11:24:44, fs wrote:
> On 2014/07/23 01:46:51, Timothy Loh wrote:
> > lgtm, but I wouldn't mind if someone else (eseidel?) had a look over this.
> 
> Absolutely.
> 
> @eseidel: Could you PTAL? (Revisiting
https://codereview.chromium.org/252683011/
> might help you remember what you thought the last time =))

eseidel, PING?!

[eseidel, 2014-07-29 15:43:23]

The feature seems fine.  The implementation feels a bit tacked on but it may
just be that I'm missing context in this code.

https://codereview.chromium.org/330043003/diff/70001/Source/core/css/CSSParse...
File Source/core/css/CSSParserValues.h (right):

https://codereview.chromium.org/330043003/diff/70001/Source/core/css/CSSParse...
Source/core/css/CSSParserValues.h:224: void setAttribute(const QualifiedName&
value, unsigned flags) { m_selector->setAttribute(value, flags); }
How many flags are there?  Should we just list out the enums instead?

setAttribute(value, CaseSensitive, OtherFlag, FinalFlag)

Or should the flags be wrapped in a named type even if they're stored as an
unsigned in the end?
AttributeFlags flags(CaseSensitive, OtherFlag, FinalFlag);
setAttribute(value, flags);

https://codereview.chromium.org/330043003/diff/70001/Source/core/css/CSSSelec...
File Source/core/css/CSSSelector.cpp (right):

https://codereview.chromium.org/330043003/diff/70001/Source/core/css/CSSSelec...
Source/core/css/CSSSelector.cpp:884: m_bits.m_nth.m_b = nthB;
We could wrap these in private accessors and then not care how they're stored,
now or in the future.

https://codereview.chromium.org/330043003/diff/70001/Source/core/css/CSSSelec...
Source/core/css/CSSSelector.cpp:891: if (!m_bits.m_nth.m_a)
nthAValue() might be shorter?

https://codereview.chromium.org/330043003/diff/70001/Source/core/css/CSSSelec...
File Source/core/css/CSSSelector.h (right):

https://codereview.chromium.org/330043003/diff/70001/Source/core/css/CSSSelec...
Source/core/css/CSSSelector.h:356: union Bits {
I'm confused by these.  It looks like this is a big object, but I thoguht that
we had a ton of CSSSelector objects, so I would expect them to be size
sensitive, yet we haven't had a union before this one.

this feels like a half-hearte attempt to save space in this object, when we
probably could be more agressive. Can m_attribute and m_argument and m_value all
be used at the same time?  can m_selectorList?  Just confused that we are only
introducing a union now.  Feels like either we should continue to avoid the
union or unionize more of these attributes (in a separate patch).

https://codereview.chromium.org/330043003/diff/70001/Source/core/css/CSSSelec...
Source/core/css/CSSSelector.h:357: Bits() : m_nth() { }
This doesn't do anything.

[eseidel, 2014-07-29 15:46:01]

https://codereview.chromium.org/330043003/diff/70001/Source/core/css/CSSParse...
File Source/core/css/CSSParserValues.h (right):

https://codereview.chromium.org/330043003/diff/70001/Source/core/css/CSSParse...
Source/core/css/CSSParserValues.h:224: void setAttribute(const QualifiedName&
value, unsigned flags) { m_selector->setAttribute(value, flags); }
If there aren't any other flags, feels like we coudl just stick with the one
enum for now:

setAttribute(value, CaseSensitive)

[fs, 2014-07-29 17:14:32]

https://codereview.chromium.org/330043003/diff/70001/Source/core/css/CSSParse...
File Source/core/css/CSSParserValues.h (right):

https://codereview.chromium.org/330043003/diff/70001/Source/core/css/CSSParse...
Source/core/css/CSSParserValues.h:224: void setAttribute(const QualifiedName&
value, unsigned flags) { m_selector->setAttribute(value, flags); }
On 2014/07/29 15:46:01, eseidel wrote:
> If there aren't any other flags, feels like we coudl just stick with the one
> enum for now:
> 
> setAttribute(value, CaseSensitive)

Dropped the notion of 'flags' and made it a plain enum. (Maybe the rename was a
bit unnecessary...)

https://codereview.chromium.org/330043003/diff/70001/Source/core/css/CSSSelec...
File Source/core/css/CSSSelector.cpp (right):

https://codereview.chromium.org/330043003/diff/70001/Source/core/css/CSSSelec...
Source/core/css/CSSSelector.cpp:884: m_bits.m_nth.m_b = nthB;
On 2014/07/29 15:43:22, eseidel wrote:
> We could wrap these in private accessors and then not care how they're stored,
> now or in the future.

Done.

https://codereview.chromium.org/330043003/diff/70001/Source/core/css/CSSSelec...
Source/core/css/CSSSelector.cpp:891: if (!m_bits.m_nth.m_a)
On 2014/07/29 15:43:22, eseidel wrote:
> nthAValue() might be shorter?

Yepp, and looks nicer as a bonus. Added.

https://codereview.chromium.org/330043003/diff/70001/Source/core/css/CSSSelec...
File Source/core/css/CSSSelector.h (right):

https://codereview.chromium.org/330043003/diff/70001/Source/core/css/CSSSelec...
Source/core/css/CSSSelector.h:356: union Bits {
On 2014/07/29 15:43:22, eseidel wrote:
> I'm confused by these.  It looks like this is a big object, but I thoguht that
> we had a ton of CSSSelector objects, so I would expect them to be size
> sensitive, yet we haven't had a union before this one.
> 
> this feels like a half-hearte attempt to save space in this object, when we
> probably could be more agressive. Can m_attribute and m_argument and m_value
all
> be used at the same time?  can m_selectorList?  Just confused that we are only
> introducing a union now.  Feels like either we should continue to avoid the
> union or unionize more of these attributes (in a separate patch).

The union is "reaction" to comments in
https://codereview.chromium.org/252683011/ (the origin of this code). I don't
really have any data on with the frequencies of the different selector-types, so
this was mostly intended as an illustration of how the new field could be added
without affecting the "status quo" (memory-wise). I can revert the union-bit
again if requested (the fact that there's DataUnion in outer-class suggests to
me that this indeed rare data.)

(IIRC, the "groups" are (excluding the new field):

m_value+m_attribute
m_value+m_selectorList
m_value+m_argument+m_nth{A,B})

https://codereview.chromium.org/330043003/diff/70001/Source/core/css/CSSSelec...
Source/core/css/CSSSelector.h:357: Bits() : m_nth() { }
On 2014/07/29 15:43:22, eseidel wrote:
> This doesn't do anything.

It initializes m_nth to zeros.

[eseidel, 2014-07-30 20:12:41]

I'm still unclear on teh zeros, otherwise still looks fine.

https://codereview.chromium.org/330043003/diff/90001/Source/core/css/CSSSelec...
File Source/core/css/CSSSelector.h (right):

https://codereview.chromium.org/330043003/diff/90001/Source/core/css/CSSSelec...
Source/core/css/CSSSelector.h:360: Bits() : m_nth() { }
How does this initailize to zeros?  Do structs have a default constructor which
fills with zeros??

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

https://codereview.chromium.org/330043003/diff/90001/Source/core/css/parser/B...
Source/core/css/parser/BisonCSSParser-in.cpp:1117: if (string == "i") {
Why are we going through a string type here?

[fs, 2014-07-31 09:27:15]

Last round?

https://codereview.chromium.org/330043003/diff/90001/Source/core/css/CSSSelec...
File Source/core/css/CSSSelector.h (right):

https://codereview.chromium.org/330043003/diff/90001/Source/core/css/CSSSelec...
Source/core/css/CSSSelector.h:360: Bits() : m_nth() { }
On 2014/07/30 20:12:41, eseidel wrote:
> How does this initailize to zeros?  Do structs have a default constructor
which
> fills with zeros??

This boils down to "value-initialize" vs. "default-initialize". With this we get
the former - which ends up as "zero-initialize" in this case - the latter would
mean "no initialization". So this is "C++-magic" [1].

Anyway, I moved this to the RareData-constructor instead - hopefully that is
less confusing.

[1] The relevant parts are 8.5 [dcl.init] (clauses 6-8 for the above) and 12.6.2
[class.base.init] (clauses 7 and 8) in the version I found.

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

https://codereview.chromium.org/330043003/diff/90001/Source/core/css/parser/B...
Source/core/css/parser/BisonCSSParser-in.cpp:1117: if (string == "i") {
On 2014/07/30 20:12:41, eseidel wrote:
> Why are we going through a string type here?

Because of one of the following I suppose:
1) there's no operator==(const String&, char)
2) it's more readable than the open-coded version (string.length() == 1 &&
string[0] == 'i')

If you meant s/string type/WTF::String type/, then the answer is "we don't",
because there is an operator== overload for this case (so this compiles to a
call to WTF::equal(const StringImpl*, const LChar*).)

[eseidel, 2014-07-31 16:53:14]

I guess I still don't understand "value initialized".  I was always trained that
c++ default constructors did nothing for you :)

https://codereview.chromium.org/330043003/diff/110001/Source/core/css/CSSSele...
File Source/core/css/CSSSelector.cpp (right):

https://codereview.chromium.org/330043003/diff/110001/Source/core/css/CSSSele...
Source/core/css/CSSSelector.cpp:832: , m_bits()
I still don't understand how this auto-initializes to 0.  C++ magic?

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

https://codereview.chromium.org/330043003/diff/110001/Source/core/css/parser/...
Source/core/css/parser/BisonCSSParser-in.cpp:1117: if (string == "i") {
In one of the places you use " i", is that a concern?

[eseidel, 2014-07-31 16:53:33]

lgtm

[ojan, 2014-07-31 17:17:15]

https://codereview.chromium.org/330043003/diff/110001/Source/core/css/CSSSele...
File Source/core/css/CSSSelector.cpp (right):

https://codereview.chromium.org/330043003/diff/110001/Source/core/css/CSSSele...
Source/core/css/CSSSelector.cpp:832: , m_bits()
On 2014/07/31 16:53:14, eseidel wrote:
> I still don't understand how this auto-initializes to 0.  C++ magic?

Yes.

[fs, 2014-08-01 06:41:29]

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

https://codereview.chromium.org/330043003/diff/110001/Source/core/css/parser/...
Source/core/css/parser/BisonCSSParser-in.cpp:1117: if (string == "i") {
On 2014/07/31 16:53:14, eseidel wrote:
> In one of the places you use " i", is that a concern?

That other place is the serialization of the selector, so not really a concern
as long there's only one modifier. The whitespace is not strictly necessary I
think, but it probably makes it more obvious ('[foo="bar"i]' vs. '[foo="bar"
i]').

[fs, 2014-08-01 06:41:40]

The CQ bit was checked by fs@opera.com

[commit-bot: I haz the power, 2014-08-01 06:42:59]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/fs@opera.com/330043003/110001

[commit-bot: I haz the power, 2014-08-01 06:46:50]

Change committed as 179370