CSSParserString::equalIgnoringCase should check string length.

CSSParserString::equalIgnoringCase should check string length.

The current equalIgnoringCase just checks whether the given CSSParserString is
the given str's prefix or not.

Suppose that we have the following two strings:
   char* str1; // not nul-terminated.
   int str1len; // str1's length.
   char* str2; // nul-terminated.

strncasecmp(str1, str2, str1len) == 0 doesn't mean str1 and str2 are the same string.
(Using strncasecmp instead of WTF::equalIgnoringCase.)

For example,

   str1: "abcdefg" (str1len = 7)
   str2: "abcdefghijkl\0"

so strncmp(str1, str2, str1len) returns 0.

BUG=348891
TEST=CSSParserValuesTest.EqualIgnoringCase8BitsString, CSSParserValuesTest.EqualIgnoringCase16BitsString

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

[esprehn, 2014-03-07 07:05:52]

It's weird we can't just use the equalIgnoringCase that StringImpl uses to
compare to LChar* and UChar*. Maybe abarth@ has a better way to do this.

lgtm to me though.

https://codereview.chromium.org/189843005/diff/1/Source/core/css/CSSParserVal...
File Source/core/css/CSSParserValues.h (right):

https://codereview.chromium.org/189843005/diff/1/Source/core/css/CSSParserVal...
Source/core/css/CSSParserValues.h:102: return match && str[length()] == '\0';
Can you add an ASSERT(strlen(str) < length()) here? The index into the string is
really scary looking.

[abarth-chromium, 2014-03-07 07:22:10]

On 2014/03/07 07:05:52, esprehn wrote:
> It's weird we can't just use the equalIgnoringCase that StringImpl uses to
> compare to LChar* and UChar*. Maybe abarth@ has a better way to do this.

Crazy bug.

This is basically what StringImpl is doing internally, although StringImpl would
call strlen because of how its structured.

[abarth-chromium, 2014-03-07 07:22:21]

Why no test?

[tasak, 2014-03-07 09:55:49]

Thank you for reviewing.

I have just added unit tests for this.

https://codereview.chromium.org/189843005/diff/1/Source/core/css/CSSParserVal...
File Source/core/css/CSSParserValues.h (right):

https://codereview.chromium.org/189843005/diff/1/Source/core/css/CSSParserVal...
Source/core/css/CSSParserValues.h:102: return match && str[length()] == '\0';
On 2014/03/07 07:05:53, esprehn wrote:
> Can you add an ASSERT(strlen(str) < length()) here? The index into the string
is
> really scary looking.

Sure. I added ASSERT.

[tasak, 2014-03-10 02:08:03]

On 2014/03/07 09:55:49, tasak wrote:
> Thank you for reviewing.
> 
> I have just added unit tests for this.
> 
>
https://codereview.chromium.org/189843005/diff/1/Source/core/css/CSSParserVal...
> File Source/core/css/CSSParserValues.h (right):
> 
>
https://codereview.chromium.org/189843005/diff/1/Source/core/css/CSSParserVal...
> Source/core/css/CSSParserValues.h:102: return match && str[length()] == '\0';
> On 2014/03/07 07:05:53, esprehn wrote:
> > Can you add an ASSERT(strlen(str) < length()) here? The index into the
string
> is
> > really scary looking.
> 
> Sure. I added ASSERT.

I have just uploaded the newest patch.

[tasak, 2014-03-10 08:23:22]

On 2014/03/10 02:08:03, tasak wrote:
> On 2014/03/07 09:55:49, tasak wrote:
> > Thank you for reviewing.
> > 
> > I have just added unit tests for this.
> > 
> >
>
https://codereview.chromium.org/189843005/diff/1/Source/core/css/CSSParserVal...
> > File Source/core/css/CSSParserValues.h (right):
> > 
> >
>
https://codereview.chromium.org/189843005/diff/1/Source/core/css/CSSParserVal...
> > Source/core/css/CSSParserValues.h:102: return match && str[length()] ==
'\0';
> > On 2014/03/07 07:05:53, esprehn wrote:
> > > Can you add an ASSERT(strlen(str) < length()) here? The index into the
> string
> > is
> > > really scary looking.
> > 
> > Sure. I added ASSERT.
> 
> I have just uploaded the newest patch.

I found the following error. I will fix ASAP.

[ RUN      ] CSSParserValuesTest.EqualIgnoringCase8BitsString
../../third_party/WebKit/Source/core/css/CSSParserValuesTest.cpp:64: Failure
Value of: cssParserString.equalIgnoringCase("shadow")
  Actual: false
Expected: true
[  FAILED  ] CSSParserValuesTest.EqualIgnoringCase8BitsString (0 ms)

[tasak, 2014-03-10 09:28:47]

> [ RUN      ] CSSParserValuesTest.EqualIgnoringCase8BitsString
> ../../third_party/WebKit/Source/core/css/CSSParserValuesTest.cpp:64: Failure
> Value of: cssParserString.equalIgnoringCase("shadow")
>   Actual: false
> Expected: true
> [  FAILED  ] CSSParserValuesTest.EqualIgnoringCase8BitsString (0 ms)

Fixed.

[----------] 4 tests from CSSParserValuesTest
[ RUN      ] CSSParserValuesTest.InitWithEmpty8BitsString
[       OK ] CSSParserValuesTest.InitWithEmpty8BitsString (0 ms)
[ RUN      ] CSSParserValuesTest.InitWithEmpty16BitsString
[       OK ] CSSParserValuesTest.InitWithEmpty16BitsString (0 ms)
[ RUN      ] CSSParserValuesTest.EqualIgnoringCase8BitsString
[       OK ] CSSParserValuesTest.EqualIgnoringCase8BitsString (0 ms)
[ RUN      ] CSSParserValuesTest.EqualIgnoringCase16BitsString
[       OK ] CSSParserValuesTest.EqualIgnoringCase16BitsString (0 ms)
[----------] 4 tests from CSSParserValuesTest (0 ms total)

[dglazkov, 2014-03-10 19:16:49]

lgtm

[dglazkov, 2014-03-10 19:16:53]

The CQ bit was checked by dglazkov@chromium.org

[commit-bot: I haz the power, 2014-03-10 19:17:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tasak@google.com/189843005/60001

[commit-bot: I haz the power, 2014-03-10 19:17:24]

Change committed as 168853