[webkit-reviews] review granted: [Bug 237068] Add a URL constructor that takes a String : [Attachment 452919] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Chris Dumez <cdumez at apple.com>'s
request for review:
Bug 237068: Add a URL constructor that takes a String
https://bugs.webkit.org/show_bug.cgi?id=237068

Attachment 452919: Patch

https://bugs.webkit.org/attachment.cgi?id=452919&action=review




--- Comment #2 from Darin Adler <darin at apple.com> ---
Comment on attachment 452919
  --> https://bugs.webkit.org/attachment.cgi?id=452919
Patch

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

> Source/WTF/wtf/URL.h:71
> +    explicit URL(const String& urlString)
> +	   : URL(URL(), urlString)
> +    {
> +    }

Is the purpose of this so obvious that it doesn’t even need to be stated in a
comment? The constructor above is quite explicit.

Do we also want to point out that sometimes we would not want to just parse a
string as a URL with no base URL without some additional processing?

Should we also have an overload that takes a String&& so we can save reference
count churn in common cases where we end up just keeping the string unmodified?

> Source/WTF/wtf/URL.h:321
> +    return URL(WTFMove(*string));

This code is written as if we had a version that takes an rvalue reference.

> Source/WebCore/Modules/websockets/WebSocket.cpp:226
> +    m_url = URL { url };

Don’t think we need to write URL here, just { url } will do. You of course are
welcome to keep the URL if you think it helps readability. Or it’s possible I
am wrong because of the use of "explicit"?

> Source/WebCore/bindings/js/ScriptModuleLoader.cpp:226
> +    return URL { asString(moduleKeyValue)->value(&jsGlobalObject) };

No need for URL here, just braces will do.

> Source/WebCore/bindings/js/ScriptModuleLoader.cpp:331
> +	   baseURL = URL { sourceOrigin.string() };

No need for URL here, just braces will do.

> Source/WebCore/bindings/js/SerializedScriptValue.cpp:2567
> +	   file = File::deserialize(executionContext(m_lexicalGlobalObject),
filePath, URL { url->string() }, type->string(), name->string(),
optionalLastModified);

No need for URL here, just braces will do.

> Source/WebCore/bindings/js/SerializedScriptValue.cpp:3652
> +	       return
getJSValue(Blob::deserialize(executionContext(m_lexicalGlobalObject), URL {
url->string() }, type->string(), size,
blobFilePathForBlobURL(url->string())).get());

No need for URL here, just braces will do.

> Source/WebCore/contentextensions/ContentExtensionActions.cpp:385
> +	   request.setURL(URL { action.url });

No need for URL here, just braces will do.

> Source/WebCore/dom/Document.cpp:5342
> +		   m_referrerOverride =
URL(referrerURL.protocolHostAndPort()).string();

I am unclear what this code does. I am pretty sure that converting to a URL and
back to a string is a round trip no-op that does nothing. Sometimes makes a
valid URL, other times makes an invalid URL, but never changes the string. Am I
wrong about this?

> Source/WebCore/loader/DocumentLoader.cpp:852
> +    return URL {
"https://www.microsoft.com/en-us/microsoft-365/microsoft-teams/"_s };

No need for URL here, just braces will do.

> Source/WebCore/loader/archive/cf/LegacyWebArchive.cpp:228
> +    return ArchiveResource::create(SharedBuffer::create(resourceData), URL {
url }, mimeType, textEncoding, frameName, response);

No need for URL here, just braces will do.

> Source/WebCore/page/DOMWindow.cpp:2586
> +	       auto radioPlayerDomain = RegistrableDomain(URL {
Quirks::staticRadioPlayerURLString() });
> +	       auto BBCDomain = RegistrableDomain(URL {
Quirks::BBCRadioPlayerURLString() });

Not sure we need URL here, likely just braces will do.

> Source/WebCore/page/Quirks.cpp:1101
> +    static NeverDestroyed<RegistrableDomain> BBCDomain =
RegistrableDomain(URL { Quirks::BBCRadioPlayerURLString() });

Ditto.

> Source/WebCore/page/Quirks.cpp:1175
> +    static NeverDestroyed<URL> kinjaURL = URL { "https://kinja.com" };

Could just write NeverDestroyed here instead of NeverDestroyed<URL>.

> Source/WebCore/page/SecurityOrigin.cpp:593
> +    return SecurityOrigin::create(URL { originString });

Not sure we need URL here, likely just braces will do.

> Source/WebCore/page/SecurityOrigin.cpp:599
> +    auto origin = create(URL { protocol + "://" + host + "/" });

Not sure we need URL here, likely just braces will do.

> Source/WebCore/page/SecurityOriginData.cpp:54
> +    return URL { toString() };

Don’t need URL here, just braces will do.

> Source/WebCore/page/SecurityPolicy.cpp:95
> +    ASSERT(referrer == URL(referrer).strippedForUseAsReferrer()
> +	   || referrer == SecurityOrigin::create(URL { referrer
})->toString());

Should write this the same way on these two successive lines?

>
Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:925
> +    m_url = URL { cleanURLString };

No need for URL here, just braces will do.

> Source/WebCore/platform/network/cf/ResourceErrorCF.cpp:123
> +	       m_failingURL = URL { failingURLString };

No need for URL here, just braces will do.

> Source/WebCore/platform/network/curl/CurlRequest.cpp:369
> +	   m_response.proxyUrl = URL { *proxyUrl };

No need for URL here, just braces will do. Also surprised they named it
proxyUrl and not proxyURL.

> Source/WebCore/platform/network/mac/ResourceErrorMac.mm:163
> +	   m_failingURL = URL { failingURLString };

No need for URL here, just braces will do.

> Source/WebCore/platform/win/PasteboardWin.cpp:455
> +	   WebCore::writeURL(m_writableDataObject.get(), URL { data },
String(), false, true);

Might not need URL here, maybe just braces will do.

> Source/WebCore/testing/Internals.cpp:5691
> +    frame->loader().client().sendH2Ping(URL { url }, [promise =
WTFMove(promise)] (Expected<Seconds, ResourceError>&& result) mutable {

Ditto.