Move URL to use StringView when returning substrings of the URL
Created attachment 396373 [details] Patch
Created attachment 396375 [details] Patch
Created attachment 396377 [details] Patch
Created attachment 396430 [details] Patch
Created attachment 396772 [details] Patch
Created attachment 397004 [details] Patch
Created attachment 397163 [details] Patch
Created attachment 397297 [details] Patch
Created attachment 397316 [details] Patch
Created attachment 397413 [details] Patch
Created attachment 397462 [details] Patch
Created attachment 397474 [details] Patch
Comment on attachment 397474 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=397474&action=review > Source/WTF/wtf/URL.cpp:169 > + Vector<LChar, 512> percentDecoded; 512 seems kinda arbitrary and a little high. > Source/WTF/wtf/text/StringView.cpp:315 > +// FIXME: Should this be named parseNumber<uint16_t> instead? I like this idea. > Source/WebCore/loader/appcache/ManifestParser.cpp:38 > +static StringView manifestPath(const URL& manifestURL) Need to be careful about passing temporaries to this function! > Source/WebCore/platform/network/curl/CurlProxySettings.cpp:40 > +constexpr uint16_t SocksProxyPort = 1080; I'd keep this static. > Source/WebCore/platform/win/PasteboardWin.cpp:-582 > - String lastComponent = kurl.lastPathComponent(); kurl! > Source/WebKit/NetworkProcess/soup/NetworkDataTaskSoup.cpp:1127 > + || isPublicSuffix(m_currentRequest.url().host().toStringWithoutCopying()); Surprised isPublicSuffic doesn't take a StringView. > Source/WebKitLegacy/win/Plugins/PluginDatabase.cpp:2 > + * Copyright (C) 2006, 2007 Apple Inc. All rights reserved. Maybe change the copyright years here too.
Comment on attachment 397474 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=397474&action=review >> Source/WTF/wtf/URL.cpp:169 >> + Vector<LChar, 512> percentDecoded; > > 512 seems kinda arbitrary and a little high. Was matching the other 512 numbers elsewhere in URL code, but those are for lengths of entire URLs. I would be happy to choose a different arbitrary number here. Maybe I should do a histogram of how this is actually used when doing some web browsing. For now, I’ll put 100 instead of 512 and I’ll even add a FIXME about setting the number better. >> Source/WTF/wtf/text/StringView.cpp:315 >> +// FIXME: Should this be named parseNumber<uint16_t> instead? > > I like this idea. Just for the moment, I will leave it like this with this FIXME. But I will follow up. >> Source/WebCore/loader/appcache/ManifestParser.cpp:38 >> +static StringView manifestPath(const URL& manifestURL) > > Need to be careful about passing temporaries to this function! Good point. Also true about all the URL functions; I did make them more dangerous with temporaries too. Any thoughts on what I should do to lower the risk? >> Source/WebCore/platform/network/curl/CurlProxySettings.cpp:40 >> +constexpr uint16_t SocksProxyPort = 1080; > > I'd keep this static. Alexey told me in the past that I should not write static in front of numeric constants in .cpp files. I will change this one back, but I am a bit confused about the tradeoffs. I looked it up and it seems that constexpr does not affect external vs. internal linkage, although it does make both "inline" and "const" redundant and unnecessary. >> Source/WebCore/platform/win/PasteboardWin.cpp:-582 >> - String lastComponent = kurl.lastPathComponent(); > > kurl! I will leave this as a warning to future explorers, a concise way to say: "Abandon all hope, ye who enter here." >> Source/WebKit/NetworkProcess/soup/NetworkDataTaskSoup.cpp:1127 >> + || isPublicSuffix(m_currentRequest.url().host().toStringWithoutCopying()); > > Surprised isPublicSuffic doesn't take a StringView. The Mac implementation easily could: it just creates a CF/NSString. It would take a little more work to wean the curl and soup implementations off of the convertToLowercaseWithoutLocale (incorrect in the curl one), convertToASCIILowercase (correct in the soup one), and utf8 functions. And this is soup-specific code. I won’t do anything about this at this time.
Committed r260707: <https://trac.webkit.org/changeset/260707>
<rdar://problem/62374069>
Comment on attachment 397474 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=397474&action=review > Source/WebCore/workers/service/context/ServiceWorkerThreadProxy.cpp:54 > + url.setPort(origin.port()); We should probably reintroduce the nullopt check here. > Source/WebCore/workers/service/server/SWServer.cpp:430 > + url.setPort(*origin.port()); Ditto here.
(In reply to youenn fablet from comment #17) > Comment on attachment 397474 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=397474&action=review > > > Source/WebCore/workers/service/context/ServiceWorkerThreadProxy.cpp:54 > > + url.setPort(origin.port()); > > We should probably reintroduce the nullopt check here. > > > Source/WebCore/workers/service/server/SWServer.cpp:430 > > + url.setPort(*origin.port()); > > Ditto here. Actually no, just remove * from SWServer.cpp:430