Bug 210431 – Move URL to use StringView when returning substrings of the URL

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Template Framework

[Darin Adler]

Move URL to use StringView when returning substrings of the URL

[Darin Adler]

Created attachment 396373 [details]
Patch

[Darin Adler]

Created attachment 396375 [details]
Patch

[Darin Adler]

Created attachment 396377 [details]
Patch

[Darin Adler]

Created attachment 396430 [details]
Patch

[Darin Adler]

Created attachment 396772 [details]
Patch

[Darin Adler]

Created attachment 397004 [details]
Patch

[Darin Adler]

Created attachment 397163 [details]
Patch

[Darin Adler]

Created attachment 397297 [details]
Patch

[Darin Adler]

Created attachment 397316 [details]
Patch

[Darin Adler]

Created attachment 397413 [details]
Patch

[Darin Adler]

Created attachment 397462 [details]
Patch

[Darin Adler]

Created attachment 397474 [details]
Patch

[Anders Carlsson]

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.

[Darin Adler]

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.

[Darin Adler]

Committed r260707: <https://trac.webkit.org/changeset/260707>

[Radar WebKit Bug Importer]

<rdar://problem/62374069>

[youenn fablet]

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.

[youenn fablet]

(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