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

[bugzilla-daemon at webkit.org]

Carlos Garcia Campos <cgarcia at igalia.com> has denied Ms2ger
<Ms2ger 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 344291: Patch

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




--- Comment #90 from Carlos Garcia Campos <cgarcia at igalia.com> ---
Comment on attachment 344291
  --> https://bugs.webkit.org/attachment.cgi?id=344291
Patch

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

> Source/WebKit/SourcesGTK.txt:169
> +UIProcess/API/glib/WebKitURIUtilities.cpp @no-unify

I think this could be shared, you might want to use it in a web extension. I
would move it to Shared/API/glib.

> Source/WebKit/UIProcess/API/glib/WebKitURIUtilities.cpp:36
> + **/

**/ -> */

> Source/WebKit/UIProcess/API/glib/WebKitURIUtilities.cpp:40
> +static void readIDNScriptWhiteListFile()

As I commented before I think this could also be refactored to be done by
WebCore in cross-platform code. We only need to had a function to read the data
that would be platform specific.

> Source/WebKit/UIProcess/API/glib/WebKitURIUtilities.cpp:105
> + **/

Since: 2.22
**/ -> */

> Source/WebKit/UIProcess/API/glib/WebKitURIUtilities.cpp:121
> +    if (!coreURI.isValid())
> +	   return nullptr;
> +
> +    // Remove password and percent-decode host name.
> +    coreURI.setPass(emptyString());
> +    auto soupURI = coreURI.createSoupURI();
> +    if (!soupURI.get()->host)
> +	   return nullptr;
> +
> +    GUniquePtr<gchar>
percentDecodedHostChars(soup_uri_decode(soupURI.get()->host));
> +    auto percentDecodedHost =
String::fromUTF8(percentDecodedHostChars.get());

I wonder if we could use WebCore::URLParser for this instead of libsoup to
avoid the utf16 -> utf8 -> utf16 conversions

> Source/WebKit/UIProcess/API/glib/WebKitURIUtilities.cpp:128
> +    auto convertedHostName =
WebCore::URLHelpers::decodePunycode(percentDecodedHost, IDNScriptWhiteList,
error);
> +    if (error)
> +	   return nullptr;

Maybe we could add a GError** parameter to provide more information about what
failed.

> Source/WebKit/UIProcess/API/glib/WebKitURIUtilities.cpp:137
> +    // Now, decode any percent-encoded characters in the URI.
> +    GUniquePtr<char> percentEncodedURI(soup_uri_to_string(soupURI.get(),
FALSE));

I don't understand this. The comment says we are decoding the uri, but the
variable name says it's encoded. Does soup_uri_to_string() decode percentage
encoded parts or not? Why do we need to do the hostname part separately? Can't
we simply call userVisibleString() and decode the resulting string?

> Source/WebKit/UIProcess/API/gtk/WebKitURIUtilities.h:20
> +#if !defined(__WEBKIT2_H_INSIDE__) && !defined(WEBKIT2_COMPILATION)

&& !defined(__WEBKIT_WEB_EXTENSION_H_INSIDE__) if we make this shared.

> Source/WebKit/UIProcess/API/wpe/WebKitURIUtilities.h:20
> +#if !defined(__WEBKIT_H_INSIDE__) && !defined(WEBKIT2_COMPILATION)

Ditto.

> Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebKitURIUtilities.cpp:53
> +	   { "http://www.%7Bexample%7D.com/", "http://www.{example}.com/" },
> +	   { "http://example.com/a%2Fb", nullptr }, // '/' in path needs to
remain encoded.

I would like to see more test cases about the percent-decoding thing which is
what we make different compared to WebCore:userVisibleString().