Make URL::path() return a StringView
Created attachment 393737 [details] Patch
Created attachment 393739 [details] Patch
Created attachment 393741 [details] Patch
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
Created attachment 393743 [details] Patch
Created attachment 393751 [details] Patch
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.
Created attachment 393850 [details] Patch
(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
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?
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.
> >> 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.
https://bugs.webkit.org/show_bug.cgi?id=209273
Committed r258685: <https://trac.webkit.org/changeset/258685> All reviewed patches have been landed. Closing bug and clearing flags on attachment 393850 [details].
<rdar://problem/60624394>