Match 'i' attribute selector modifier case-insensitively

Match 'i' attribute selector modifier case-insensitively

The 'i' should be treated as an identifier, so matching should be ASCII
case-insensitive.

BUG=580446
Committed: https://crrev.com/8c4aee69ad7391e4afc02276987958091038e308
Cr-Commit-Position: refs/heads/master@{#371228}

[fs, 2016-01-22 18:53:18]

fs@opera.com changed reviewers:
+ timloh@chromium.org

[rune, 2016-01-22 21:48:01]

rune@opera.com changed reviewers:
+ rune@opera.com

[rune, 2016-01-22 21:48:02]

https://codereview.chromium.org/1626563002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/css/parser/CSSSelectorParser.cpp (right):

https://codereview.chromium.org/1626563002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/parser/CSSSelectorParser.cpp:611: if
(flag.valueEqualsIgnoringASCIICase("i"))
Briefly looking through the relevant specs, it seems every keyword is supposed
to be matched ascii-case-insensitively. Looking at the use of
valueEqualsIgnoringCase/equalsIgnoringCase, it seems it should always have been
valueEqualsIgnoringASCIICase/equalsIgnoringASCIICase.

If so, could we just get rid of the unicode-case-insensitive matching functions?

[fs, 2016-01-22 23:53:17]

https://codereview.chromium.org/1626563002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/css/parser/CSSSelectorParser.cpp (right):

https://codereview.chromium.org/1626563002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/parser/CSSSelectorParser.cpp:611: if
(flag.valueEqualsIgnoringASCIICase("i"))
On 2016/01/22 at 21:48:01, rune wrote:
> Briefly looking through the relevant specs, it seems every keyword is supposed
to be matched ascii-case-insensitively. Looking at the use of
valueEqualsIgnoringCase/equalsIgnoringCase, it seems it should always have been
valueEqualsIgnoringASCIICase/equalsIgnoringASCIICase.
> 
> If so, could we just get rid of the unicode-case-insensitive matching
functions?

I was hoping we could. I was planning on doing that in follow-up CLs (want this
as pristine as possible since I want to try to merge it to M49) - not looking
forward to chasing testcases for all uses though, so hopefully we can punt on
those a bit... (else I'll need to read the case-folding tables very
thoroughly...)

[rune, 2016-01-24 10:46:26]

https://codereview.chromium.org/1626563002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/css/parser/CSSSelectorParser.cpp (right):

https://codereview.chromium.org/1626563002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/parser/CSSSelectorParser.cpp:611: if
(flag.valueEqualsIgnoringASCIICase("i"))
On 2016/01/22 23:53:17, fs wrote:
> On 2016/01/22 at 21:48:01, rune wrote:
> > Briefly looking through the relevant specs, it seems every keyword is
supposed
> to be matched ascii-case-insensitively. Looking at the use of
> valueEqualsIgnoringCase/equalsIgnoringCase, it seems it should always have
been
> valueEqualsIgnoringASCIICase/equalsIgnoringASCIICase.
> > 
> > If so, could we just get rid of the unicode-case-insensitive matching
> functions?
> 
> I was hoping we could. I was planning on doing that in follow-up CLs (want
this
> as pristine as possible since I want to try to merge it to M49) - not looking
> forward to chasing testcases for all uses though, so hopefully we can punt on
> those a bit... (else I'll need to read the case-folding tables very
> thoroughly...)

In another context I was trying to find out if we case folding were we'd make
'i' 'I' case-equal to some other character, and couldn't. Maybe you could just
use valueEqualsIgnoringCase for this CL instead?

[fs, 2016-01-25 09:47:19]

https://codereview.chromium.org/1626563002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/css/parser/CSSSelectorParser.cpp (right):

https://codereview.chromium.org/1626563002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/parser/CSSSelectorParser.cpp:611: if
(flag.valueEqualsIgnoringASCIICase("i"))
On 2016/01/24 at 10:46:26, rune wrote:
> On 2016/01/22 23:53:17, fs wrote:
> > On 2016/01/22 at 21:48:01, rune wrote:
> > > Briefly looking through the relevant specs, it seems every keyword is
supposed
> > to be matched ascii-case-insensitively. Looking at the use of
> > valueEqualsIgnoringCase/equalsIgnoringCase, it seems it should always have
been
> > valueEqualsIgnoringASCIICase/equalsIgnoringASCIICase.
> > > 
> > > If so, could we just get rid of the unicode-case-insensitive matching
> > functions?
> > 
> > I was hoping we could. I was planning on doing that in follow-up CLs (want
this
> > as pristine as possible since I want to try to merge it to M49) - not
looking
> > forward to chasing testcases for all uses though, so hopefully we can punt
on
> > those a bit... (else I'll need to read the case-folding tables very
> > thoroughly...)
> 
> In another context I was trying to find out if we case folding were we'd make
'i' 'I' case-equal to some other character, and couldn't. Maybe you could just
use valueEqualsIgnoringCase for this CL instead?

If you look at the TC I added, you'll see one example of a non-ascii char that
maps to lower-case ('i') and one that maps to upper-case ('I'):

http://www.fileformat.info/info/unicode/char/0130/index.htm
http://www.fileformat.info/info/unicode/char/0131/index.htm

I did consider just using valueEqualsIgnoringCase "for now" though since that
would mean a trivial backport (merge). This is not that much larger though, so
it felt reasonable to "set the example" =). (Also, you newer know what level of
picky reviewers will choose ;-))

[rune, 2016-01-25 10:15:29]

https://codereview.chromium.org/1626563002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/css/parser/CSSSelectorParser.cpp (right):

https://codereview.chromium.org/1626563002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/parser/CSSSelectorParser.cpp:611: if
(flag.valueEqualsIgnoringASCIICase("i"))
On 2016/01/25 09:47:19, fs wrote:
> On 2016/01/24 at 10:46:26, rune wrote:
> > On 2016/01/22 23:53:17, fs wrote:
> > > On 2016/01/22 at 21:48:01, rune wrote:
> > > > Briefly looking through the relevant specs, it seems every keyword is
> supposed
> > > to be matched ascii-case-insensitively. Looking at the use of
> > > valueEqualsIgnoringCase/equalsIgnoringCase, it seems it should always have
> been
> > > valueEqualsIgnoringASCIICase/equalsIgnoringASCIICase.
> > > > 
> > > > If so, could we just get rid of the unicode-case-insensitive matching
> > > functions?
> > > 
> > > I was hoping we could. I was planning on doing that in follow-up CLs (want
> this
> > > as pristine as possible since I want to try to merge it to M49) - not
> looking
> > > forward to chasing testcases for all uses though, so hopefully we can punt
> on
> > > those a bit... (else I'll need to read the case-folding tables very
> > > thoroughly...)
> > 
> > In another context I was trying to find out if we case folding were we'd
make
> 'i' 'I' case-equal to some other character, and couldn't. Maybe you could just
> use valueEqualsIgnoringCase for this CL instead?
> 
> If you look at the TC I added, you'll see one example of a non-ascii char that
> maps to lower-case ('i') and one that maps to upper-case ('I'):
> 
> http://www.fileformat.info/info/unicode/char/0130/index.htm
> http://www.fileformat.info/info/unicode/char/0131/index.htm

I ran your new cases locally, and those test pass with valueEqualsIgnoringCase.
We do a simpler for of folding than what's required to make those fail, I think.

> I did consider just using valueEqualsIgnoringCase "for now" though since that
> would mean a trivial backport (merge). This is not that much larger though, so
> it felt reasonable to "set the example" =). (Also, you newer know what level
of
> picky reviewers will choose ;-))

[rune, 2016-01-25 10:27:32]

lgtm

you can choose if you do the ascii comparison or not for "i". equalsIgnoringCase
should work for this specific case.

Could you report a crbug for the ascii-case-insensitivity for the CSS parser in
general?

[fs, 2016-01-25 11:30:31]

On 2016/01/25 at 10:27:32, rune wrote:
...
> you can choose if you do the ascii comparison or not for "i".
equalsIgnoringCase should work for this specific case.

Went with the simpler alternative for now.

> Could you report a crbug for the ascii-case-insensitivity for the CSS parser
in general?

crbug.com/581001

https://codereview.chromium.org/1626563002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/css/parser/CSSSelectorParser.cpp (right):

https://codereview.chromium.org/1626563002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/parser/CSSSelectorParser.cpp:611: if
(flag.valueEqualsIgnoringASCIICase("i"))
On 2016/01/25 at 10:15:29, rune wrote:
> On 2016/01/25 09:47:19, fs wrote:
> > On 2016/01/24 at 10:46:26, rune wrote:
> > > On 2016/01/22 23:53:17, fs wrote:
> > > > On 2016/01/22 at 21:48:01, rune wrote:
> > > > > Briefly looking through the relevant specs, it seems every keyword is
> > supposed
> > > > to be matched ascii-case-insensitively. Looking at the use of
> > > > valueEqualsIgnoringCase/equalsIgnoringCase, it seems it should always
have
> > been
> > > > valueEqualsIgnoringASCIICase/equalsIgnoringASCIICase.
> > > > > 
> > > > > If so, could we just get rid of the unicode-case-insensitive matching
> > > > functions?
> > > > 
> > > > I was hoping we could. I was planning on doing that in follow-up CLs
(want
> > this
> > > > as pristine as possible since I want to try to merge it to M49) - not
> > looking
> > > > forward to chasing testcases for all uses though, so hopefully we can
punt
> > on
> > > > those a bit... (else I'll need to read the case-folding tables very
> > > > thoroughly...)
> > > 
> > > In another context I was trying to find out if we case folding were we'd
make
> > 'i' 'I' case-equal to some other character, and couldn't. Maybe you could
just
> > use valueEqualsIgnoringCase for this CL instead?
> > 
> > If you look at the TC I added, you'll see one example of a non-ascii char
that
> > maps to lower-case ('i') and one that maps to upper-case ('I'):
> > 
> > http://www.fileformat.info/info/unicode/char/0130/index.htm
> > http://www.fileformat.info/info/unicode/char/0131/index.htm
> 
> I ran your new cases locally, and those test pass with
valueEqualsIgnoringCase. We do a simpler for of folding than what's required to
make those fail, I think.

Ok, so looks like it pans out fine in these cases, so I guess we can run with
scissors for a while longer.

[fs, 2016-01-25 11:43:33]

The CQ bit was checked by fs@opera.com

[commit-bot: I haz the power, 2016-01-25 11:43:41]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1626563002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1626563002/20001

[commit-bot: I haz the power, 2016-01-25 11:48:39]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-01-25 11:51:07]

Description was changed from

==========
Match 'i' attribute selector modifier case-insensitively

The 'i' should be treated as an identifier, so matching should be ASCII
case-insensitive.

BUG=580446
==========

to

==========
Match 'i' attribute selector modifier case-insensitively

The 'i' should be treated as an identifier, so matching should be ASCII
case-insensitive.

BUG=580446

Committed: https://crrev.com/8c4aee69ad7391e4afc02276987958091038e308
Cr-Commit-Position: refs/heads/master@{#371228}
==========

[commit-bot: I haz the power, 2016-01-25 11:51:08]

Patchset 2 (id:??) landed as
https://crrev.com/8c4aee69ad7391e4afc02276987958091038e308
Cr-Commit-Position: refs/heads/master@{#371228}