[webkit-reviews] review denied: [Bug 174816] [GTK][WPE] Need a function to convert internal URI to display ("pretty") URI : [Attachment 331339] Patch

[bugzilla-daemon at webkit.org]

Michael Catanzaro <mcatanzaro at igalia.com> has denied Gabriel Ivașcu
<givascu at igalia.com>'s request for review:
Bug 174816: [GTK][WPE] Need a function to convert internal URI to display
("pretty") URI
https://bugs.webkit.org/show_bug.cgi?id=174816

Attachment 331339: Patch

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




--- Comment #39 from Michael Catanzaro <mcatanzaro at igalia.com> ---
Comment on attachment 331339
  --> https://bugs.webkit.org/attachment.cgi?id=331339
Patch

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

r- for the failing tests.

> Source/WebCore/platform/URLParser.cpp:3265
> +String URLParser::ICUConvertHostName(const String& hostName, bool encode,
const uint32_t (&IDNScriptBlackList)[(USCRIPT_CODE_LIMIT + 31) / 32])

Hm, I didn't  know you could declare array arguments like this, except in
templates. Does the size specification actually do anything? I bet it's
ignored. A better declaration would be:

const uint32_t* IDNScriptBlackList

which everyone will understand.

> Source/WebKit/UIProcess/API/glib/WebKitURIUtilities.cpp:46
> + * Return value: (transfer full): @uri suitable for display

Returns:

("Return value" is an older syntax.)

> Source/WebKit/UIProcess/API/glib/WebKitURIUtilities.cpp:52
> +    auto URI = WebCore::URL(WebCore::ParsedURLStringTag::ParsedURLString,
WTF::String(uri));

WTF::String is hoisted into the global namespace with a using declaration in
its header file, so you don't have to qualify it with WTF::.

You do need to use String::fromUTF8 to create the string, though, since WebKit
unfortunately does not use UTF-8 internally, like GNOME code does.

Also, URI in all caps is not a great name. How about "coreURI"?

> Source/WebKit/UIProcess/API/glib/WebKitURIUtilities.cpp:55
> +    URI.setPass(WTF::emptyString());

Ditto, you don't need WTF:: here. (See the bottom of WTFString.h.)

> Source/WebKit/UIProcess/API/glib/WebKitURIUtilities.cpp:58
> +   
URI.setHost(WebCore::URLParser::ICUConvertHostName(String(percentDecodedHost.ge
t()), false));

Use String::fromUTF8