Bug 209173 – Make URL::path() return a StringView

• Component: Page Loading
• Status: RESOLVED FIXED

[youenn fablet]

Make URL::path() return a StringView

[youenn fablet]

Created attachment 393737 [details]
Patch

[youenn fablet]

Created attachment 393739 [details]
Patch

[youenn fablet]

Created attachment 393741 [details]
Patch

[EWS Watchlist]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[youenn fablet]

Created attachment 393743 [details]
Patch

[youenn fablet]

Created attachment 393751 [details]
Patch

[Darin Adler]

Comment on attachment 393751 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=393751&action=review

> Source/WebCore/Modules/plugins/YouTubePluginReplacement.cpp:262
> -    } else if (startsWithLettersIgnoringASCIICase(path, "/v/") || startsWithLettersIgnoringASCIICase(path, "/e/")) {
> +    } else if (path.startsWithIgnoringASCIICase("/v/") || path.startsWithIgnoringASCIICase("/e/")) {

We should add an overload of startsWithLettersIgnoringASCIICase for StringView rather than changing this code.

> Source/WebCore/loader/FormSubmission.cpp:73
> -        body = decodeURLEscapeSequences(body.replaceWithLiteral('&', "\r\n").replace('+', ' ') + "\r\n");
> +        auto value = makeString(body.replaceWithLiteral('&', "\r\n").replace('+', ' '), "\r\n");
> +        body = decodeURLEscapeSequences(value);

I understand why we had to use makeString, but not why we have to break this into two lines.

> Source/WebCore/page/Quirks.cpp:203
> -        if (path.contains("notes") || url.fragmentIdentifier().contains("notes"))
> +        if (path.find("notes", 0) != notFound || url.fragmentIdentifier().contains("notes"))

We should add a StringView::contains overload rather than changing this.

> Source/WebCore/platform/text/TextEncoding.cpp:193
>      if (string.isEmpty())
> -        return string;
> +        return string.toString();

Seems a waste to call the general purpose toString function just to return either a null string or empty string. But I guess I don’t have a better idea.

> Source/WebCore/platform/text/TextEncoding.h:31
>  #include <wtf/URL.h>
> +#include <wtf/text/StringView.h>
>  #include <wtf/text/WTFString.h>

I am pretty sure some of these headers include others. We definitely don’t need to include all three.

[youenn fablet]

Created attachment 393850 [details]
Patch

[youenn fablet]

(In reply to Darin Adler from comment #7)
> Comment on attachment 393751 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=393751&action=review
> 
> > Source/WebCore/Modules/plugins/YouTubePluginReplacement.cpp:262
> > -    } else if (startsWithLettersIgnoringASCIICase(path, "/v/") || startsWithLettersIgnoringASCIICase(path, "/e/")) {
> > +    } else if (path.startsWithIgnoringASCIICase("/v/") || path.startsWithIgnoringASCIICase("/e/")) {
> 
> We should add an overload of startsWithLettersIgnoringASCIICase for
> StringView rather than changing this code.

Done.

> 
> > Source/WebCore/loader/FormSubmission.cpp:73
> > -        body = decodeURLEscapeSequences(body.replaceWithLiteral('&', "\r\n").replace('+', ' ') + "\r\n");
> > +        auto value = makeString(body.replaceWithLiteral('&', "\r\n").replace('+', ' '), "\r\n");
> > +        body = decodeURLEscapeSequences(value);
> 
> I understand why we had to use makeString, but not why we have to break this
> into two lines.

Reverted.

> > Source/WebCore/page/Quirks.cpp:203
> > -        if (path.contains("notes") || url.fragmentIdentifier().contains("notes"))
> > +        if (path.find("notes", 0) != notFound || url.fragmentIdentifier().contains("notes"))
> 
> We should add a StringView::contains overload rather than changing this.

Added.

> > Source/WebCore/platform/text/TextEncoding.cpp:193
> >      if (string.isEmpty())
> > -        return string;
> > +        return string.toString();
> 
> Seems a waste to call the general purpose toString function just to return
> either a null string or empty string. But I guess I don’t have a better idea.
> 
> > Source/WebCore/platform/text/TextEncoding.h:31
> >  #include <wtf/URL.h>
> > +#include <wtf/text/StringView.h>
> >  #include <wtf/text/WTFString.h>
> 
> I am pretty sure some of these headers include others. We definitely don’t
> need to include all three.

Removed WTFString.h

[Alex Christensen]

Comment on attachment 393850 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=393850&action=review

> Source/WebCore/platform/text/TextEncoding.cpp:193
> +        return string.toString();

Does an empty StringView's .toString return "" or { }?  I think we should return that here instead.

> Source/WebCore/workers/service/ServiceWorkerJob.cpp:135
> +    if (maxScopeString.isNull() || !scopeString.startsWith(maxScopeString))

Why did we add a null check here?

[Darin Adler]

Comment on attachment 393850 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=393850&action=review

>> Source/WebCore/platform/text/TextEncoding.cpp:193
>> +        return string.toString();
> 
> Does an empty StringView's .toString return "" or { }?  I think we should return that here instead.

A StringView that returns true for isEmpty can be either empty or null; like String, AtomString, and const char*, StringView has distinct empty and null states. The toString function returns an empty or null String for those two different states. That pass through is accomplished by calling toString here. In this particular function it’s possible no one depends on this behavior, but this change does preserve it.

>> Source/WebCore/workers/service/ServiceWorkerJob.cpp:135
>> +    if (maxScopeString.isNull() || !scopeString.startsWith(maxScopeString))
> 
> Why did we add a null check here?

That’s a good question. StringView and String startsWith functions should handle null strings in the same way and the change should not be necessary.

[youenn fablet]

> >> Source/WebCore/workers/service/ServiceWorkerJob.cpp:135
> >> +    if (maxScopeString.isNull() || !scopeString.startsWith(maxScopeString))
> > 
> > Why did we add a null check here?
> 
> That’s a good question. StringView and String startsWith functions should
> handle null strings in the same way and the change should not be necessary.

String::startsWith(null) returns false.
StringView::startsWith(null) returns true.

Also, this additional check is nicely matching the wording of step 17 of https://w3c.github.io/ServiceWorker/#update.

This subtlety is not great to me but I do not want to change it here.
I would go with StringView behaviour.

[youenn fablet]

https://bugs.webkit.org/show_bug.cgi?id=209273

[EWS]

Committed r258685: <https://trac.webkit.org/changeset/258685>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 393850 [details].

[Radar WebKit Bug Importer]

<rdar://problem/60624394>