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

[bugzilla-daemon at webkit.org]

Alex Christensen <achristensen at apple.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 331840: Patch

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




--- Comment #49 from Alex Christensen <achristensen at apple.com> ---
Comment on attachment 331840
  --> https://bugs.webkit.org/attachment.cgi?id=331840
Patch

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

> Source/WebCore/platform/URLParser.h:56
> +    template<typename CharacterType> static bool
isLookalikeCharacter(CharacterType, CharacterType anotherCharacter);
> +    static String ICUConvertHostName(const String& hostName, bool encode,
const uint32_t (&IDNScriptBlackList)[(USCRIPT_CODE_LIMIT + 31) / 32] = { 0 });

I'm not against moving this from a .mm to a sharable location, but the
URLParser is an implementation of https://url.spec.whatwg.org so let's make a
new class for this.

> Source/WebCore/platform/mac/WebCoreNSURLExtras.mm:48
> -static uint32_t IDNScriptWhiteList[(USCRIPT_CODE_LIMIT + 31) / 32];
> +static uint32_t IDNScriptBlackList[(USCRIPT_CODE_LIMIT + 31) / 32];

Why are you making this change?  Let's not change the functionality of the ObjC
code.

> Source/WebCore/platform/mac/WebCoreNSURLExtras.mm:639
> -    std::optional<UChar32> previousCodePoint;
> +    UChar32 previousCodePoint = INT32_MAX;

This doesn't look good.  We should use std::optional instead of having a
sentinel value.  Also, there's a signed/unsigned mismatch, but we shouldn't do
this anyway.