Replace CSSParserString with StringView.

Replace CSSParserString with StringView.

This removes the special string type the parser was using in favor of
StringView. In doing so it also makes all of the string allocations
inside the parser more explicit.

Doing this requires adding equalIgnoringASCIICase for StringView, and
also operator[], tests are included. :)

One major change is now StringView::toString() will attempt to create
an 8bit string from a 16bit StringView whenever possible. This does
mean potentially wasted work inside StringImpl::create8BitIfPossible
if callers are frequently making strings with unicode characters in
them. We should see how often this happens in practice, and probably
optimize the code in create8BitIfPossible to first scan the string
to avoid the wasted malloc.

BUG=615174
Committed: https://crrev.com/e3c7c11bf601fc6ffbea29ac7fd02b42c283840a
Cr-Commit-Position: refs/heads/master@{#397094}

[esprehn, 2016-06-01 04:43:30]

The CQ bit was checked by esprehn@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-01 04:43:43]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2024373002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2024373002/1

[esprehn, 2016-06-01 05:18:47]

Description was changed from

==========
Replace CSSParserString with StringView.

This removes the special string type the parser was using in favor of
StringView. In doing so it also makes all of the string allocations
inside the parser more explicit.

BUG=615174
==========

to

==========
Replace CSSParserString with StringView.

This removes the special string type the parser was using in favor of
StringView. In doing so it also makes all of the string allocations
inside the parser more explicit.

Doing this requires adding equalIgnoringASCIICase for StringView, and
also the operator[], tests are included. :)

BUG=615174
==========

[esprehn, 2016-06-01 05:19:19]

esprehn@chromium.org changed reviewers:
+ haraken@chromium.org, timloh@chromium.org

[esprehn, 2016-06-01 05:19:36]

Description was changed from

==========
Replace CSSParserString with StringView.

This removes the special string type the parser was using in favor of
StringView. In doing so it also makes all of the string allocations
inside the parser more explicit.

Doing this requires adding equalIgnoringASCIICase for StringView, and
also the operator[], tests are included. :)

BUG=615174
==========

to

==========
Replace CSSParserString with StringView.

This removes the special string type the parser was using in favor of
StringView. In doing so it also makes all of the string allocations
inside the parser more explicit.

Doing this requires adding equalIgnoringASCIICase for StringView, and
also operator[], tests are included. :)

BUG=615174
==========

[esprehn, 2016-06-01 05:47:03]

The CQ bit was checked by esprehn@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-01 05:47:20]

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

[haraken, 2016-06-01 05:48:28]

LGTM on my side.

[esprehn, 2016-06-01 05:50:27]

Description was changed from

==========
Replace CSSParserString with StringView.

This removes the special string type the parser was using in favor of
StringView. In doing so it also makes all of the string allocations
inside the parser more explicit.

Doing this requires adding equalIgnoringASCIICase for StringView, and
also operator[], tests are included. :)

BUG=615174
==========

to

==========
Replace CSSParserString with StringView.

This removes the special string type the parser was using in favor of
StringView. In doing so it also makes all of the string allocations
inside the parser more explicit.

Doing this requires adding equalIgnoringASCIICase for StringView, and
also operator[], tests are included. :)

One major change is now StringView::toString() will attempt to create
an 8bit string from a 16bit StringView whenever possible. This does
mean potentially wasted work inside StringImpl::create8BitIfPossible
if callers are frequently making strings with unicode characters in
them. We should see how often this happens in practice, and probably
optimize the code in create8BitIfPossible to first scan the string
to avoid the wasted malloc.

BUG=615174
==========

[esprehn, 2016-06-01 05:50:27]

esprehn@chromium.org changed reviewers:
+ yutak@chromium.org

[Timothy Loh, 2016-06-01 05:51:31]

timloh@chromium.org changed reviewers:
- yutak@chromium.org

[Timothy Loh, 2016-06-01 05:51:32]

lgtm!

https://codereview.chromium.org/2024373002/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/wtf/text/StringView.cpp (right):

https://codereview.chromium.org/2024373002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/wtf/text/StringView.cpp:58: if (a.isEmpty() ||
b.isEmpty())
How about:

if (a.isEmpty())
    return true;

Or just remove the branch, I can't imagine this early-out being particularly
common.

https://codereview.chromium.org/2024373002/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/wtf/text/StringView.h (right):

https://codereview.chromium.org/2024373002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/wtf/text/StringView.h:190: WTF_EXPORT bool
equalIgnoringASCIICase(StringView a, StringView b);
Maybe const StringView& for consistency with the below functions?

[esprehn, 2016-06-01 05:57:44]

On 2016/06/01 at 05:48:28, haraken wrote:
> LGTM on my side.

Just to double check, see the change to StringView::toString which now does
create8BitIfPossible. That matches CSSParserString, but does mean if people are
making unicode StringViews and calling toString() we'll waste some work. In
practice I don't think we use toString() where the cost will add up today
(StringView usage is pretty limited in general). We should probably add some
code to create8BitIfPossible that scans the string looking for non-latin1 chars
before doing the malloc too. It means looping twice, but skipping malloc. Not
sure how those compare to the memcpy that StringImpl::create(const UChar*,
unsigned) does.

In any case, are you okay with the switch to create8BitIfPossible? :)

[haraken, 2016-06-01 06:18:34]

On 2016/06/01 05:57:44, esprehn wrote:
> On 2016/06/01 at 05:48:28, haraken wrote:
> > LGTM on my side.
> 
> Just to double check, see the change to StringView::toString which now does
> create8BitIfPossible. That matches CSSParserString, but does mean if people
are
> making unicode StringViews and calling toString() we'll waste some work. In
> practice I don't think we use toString() where the cost will add up today
> (StringView usage is pretty limited in general). We should probably add some
> code to create8BitIfPossible that scans the string looking for non-latin1
chars
> before doing the malloc too. It means looping twice, but skipping malloc. Not
> sure how those compare to the memcpy that StringImpl::create(const UChar*,
> unsigned) does.
> 
> In any case, are you okay with the switch to create8BitIfPossible? :)

Thanks for raising the point. Yeah, the concern is valid but I'm okay with
landing this and seeing how it goes in perf bots :)

[esprehn, 2016-06-01 06:19:04]

https://codereview.chromium.org/2024373002/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/wtf/text/StringView.cpp (right):

https://codereview.chromium.org/2024373002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/wtf/text/StringView.cpp:58: if (a.isEmpty() ||
b.isEmpty())
On 2016/06/01 at 05:51:32, Timothy Loh wrote:
> How about:
> 
> if (a.isEmpty())
>     return true;
> 
> Or just remove the branch, I can't imagine this early-out being particularly
common.

I see that the method in StringImpl.h does accept strings, the libc things like
memcmp don't even if the length is 0.

I was actually trying to match the semantics of String and StringImpl. I see now
that they make it so null strings are only ever equal to other null strings. I
changed this to do that too and added tests. I want to make StringView a drop in
replacement wherever possible without having subtle differences.

https://codereview.chromium.org/2024373002/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/wtf/text/StringView.h (right):

https://codereview.chromium.org/2024373002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/wtf/text/StringView.h:190: WTF_EXPORT bool
equalIgnoringASCIICase(StringView a, StringView b);
On 2016/06/01 at 05:51:32, Timothy Loh wrote:
> Maybe const StringView& for consistency with the below functions?

Interestingly base thinks we should pass them by value:
go/stringpiecebyvalue

I made the other one take a const StringView& since that's what operator== does,
this one since we call it directly seemed like we should do by value. I guess
lets use const StringView& for now, I'll come clean these up later.

[esprehn, 2016-06-01 06:20:33]

On 2016/06/01 at 06:19:04, esprehn wrote:
>
https://codereview.chromium.org/2024373002/diff/1/third_party/WebKit/Source/w...
> File third_party/WebKit/Source/wtf/text/StringView.cpp (right):
> 
>
https://codereview.chromium.org/2024373002/diff/1/third_party/WebKit/Source/w...
> third_party/WebKit/Source/wtf/text/StringView.cpp:58: if (a.isEmpty() ||
b.isEmpty())
> On 2016/06/01 at 05:51:32, Timothy Loh wrote:
> > How about:
> > 
> > if (a.isEmpty())
> >     return true;
> > 
> > Or just remove the branch, I can't imagine this early-out being particularly
common.
> 
> I see that the method in StringImpl.h does accept strings, the libc things
like memcmp don't even if the length is 0.
> 

Sorry that's does accept *null strings*. libc never allows them even if lengths
are 0, our functions do allow them usually. Anyway, I changed both
equalStringView and equalIgnoringASCIICase to special case null to have the
exact semantics as String to avoid any conversion problems as we migrate the
code.

[esprehn, 2016-06-01 06:20:41]

The CQ bit was checked by esprehn@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-01 06:20:52]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2024373002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2024373002/60001

[esprehn, 2016-06-01 07:55:52]

The CQ bit was unchecked by esprehn@chromium.org

[esprehn, 2016-06-01 07:56:12]

The CQ bit was checked by esprehn@chromium.org

[esprehn, 2016-06-01 07:56:12]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, timloh@chromium.org
Link to the patchset: https://codereview.chromium.org/2024373002/#ps60001
(title: "Match arguments to equalStringView.")

[commit-bot: I haz the power, 2016-06-01 07:56:20]

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

[commit-bot: I haz the power, 2016-06-01 10:12:44]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-06-01 10:14:24]

Description was changed from

==========
Replace CSSParserString with StringView.

This removes the special string type the parser was using in favor of
StringView. In doing so it also makes all of the string allocations
inside the parser more explicit.

Doing this requires adding equalIgnoringASCIICase for StringView, and
also operator[], tests are included. :)

One major change is now StringView::toString() will attempt to create
an 8bit string from a 16bit StringView whenever possible. This does
mean potentially wasted work inside StringImpl::create8BitIfPossible
if callers are frequently making strings with unicode characters in
them. We should see how often this happens in practice, and probably
optimize the code in create8BitIfPossible to first scan the string
to avoid the wasted malloc.

BUG=615174
==========

to

==========
Replace CSSParserString with StringView.

This removes the special string type the parser was using in favor of
StringView. In doing so it also makes all of the string allocations
inside the parser more explicit.

Doing this requires adding equalIgnoringASCIICase for StringView, and
also operator[], tests are included. :)

One major change is now StringView::toString() will attempt to create
an 8bit string from a 16bit StringView whenever possible. This does
mean potentially wasted work inside StringImpl::create8BitIfPossible
if callers are frequently making strings with unicode characters in
them. We should see how often this happens in practice, and probably
optimize the code in create8BitIfPossible to first scan the string
to avoid the wasted malloc.

BUG=615174

Committed: https://crrev.com/e3c7c11bf601fc6ffbea29ac7fd02b42c283840a
Cr-Commit-Position: refs/heads/master@{#397094}
==========

[commit-bot: I haz the power, 2016-06-01 10:14:26]

Patchset 4 (id:??) landed as
https://crrev.com/e3c7c11bf601fc6ffbea29ac7fd02b42c283840a
Cr-Commit-Position: refs/heads/master@{#397094}