[webkit-reviews] review denied: [Bug 174816] [GTK][WPE] Need a function to convert internal URI to display ("pretty") URI : [Attachment 356804] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Fri Dec 7 20:27:39 PST 2018
Michael Catanzaro <mcatanzaro at igalia.com> has denied Ms2ger (he/him; ⌚
UTC+1/+2) <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 356804: Patch
https://bugs.webkit.org/attachment.cgi?id=356804&action=review
--- Comment #97 from Michael Catanzaro <mcatanzaro at igalia.com> ---
Comment on attachment 356804
--> https://bugs.webkit.org/attachment.cgi?id=356804
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=356804&action=review
Amazing monster patch!
I have a lot of comments, but don't worry: all are for quick trivial stuff. I
sense the finish line is very near. My only important complaint is to fix the
build failures related to loadIDNScriptWhiteList, which I explain how to do
below. The red ios-sim EWS looks unrelated and can be ignored.
> LayoutTests/TestExpectations:-71
> # These tests don't have to be platform-specific, but they are only
implemented on Mac now.
> -fast/url/user-visible [ Skip ]
Very nice! Good that you saw the opportunity to implement the TestController
function.
> Source/WTF/wtf/PlatformGTK.cmake:29
> + glib/URLHelpersGlib.cpp
URLHelpersGLib.cpp (capital L)
We should probably rename other files ending in Glib.cpp at some point since I
know there's a lot of inconsistency currently.
> Source/WTF/wtf/URLHelpers.cpp:41
> +// Needs to be big enough to hold an IDN-encoded name.
> +// For host names bigger than this, we won't do IDN encoding, which is
almost certainly OK.
> +#define HOST_NAME_BUFFER_LENGTH 2048
> +#define URL_BYTES_BUFFER_LENGTH 2048
Since this is C++ now you should use const unsigned or const int instead of
#define, and move them into the WTF namespace.
> Source/WTF/wtf/URLHelpers.cpp:43
> +static uint32_t IDNScriptWhiteList[(USCRIPT_CODE_LIMIT + 31) / 32];
This should also go inside the WTF namespace.
> Source/WTF/wtf/URLHelpers.cpp:67
> +}
> +
> +
> +template<typename CharacterType> inline bool
isASCIIDigitOrValidHostCharacter(CharacterType charCode)
One blank line between functions.
> Source/WTF/wtf/URLHelpers.cpp:92
> +}
> +
> +
> +
> +static bool isLookalikeCharacter(std::optional<UChar32> previousCodePoint,
UChar32 charCode)
> +{
Ditto regarding too many blank lines.
Also, careful not to copy optionals around unnecessarily. This should be const
std::optional<UChar32>& previousCodePoint, or std::optional<UChar32>&&
previousCodePoint if you add the WTFMove() at its callsite. Probably the later,
but whichever you prefer is fine.
> Source/WTF/wtf/URLHelpers.cpp:284
> +static bool allCharactersInIDNScriptWhiteList(const UChar *buffer, int32_t
length)
const UChar* buffer
> Source/WTF/wtf/URLHelpers.cpp:341
> +#define CHECK_RULES_IF_SUFFIX_MATCHES(suffix, function) \
> + { \
> + static const int32_t suffixLength = sizeof(suffix) /
sizeof(suffix[0]); \
> + if (length > suffixLength && 0 == memcmp(buffer + length -
suffixLength, suffix, sizeof(suffix))) \
> + return isSecondLevelDomainNameAllowedByTLDRules(buffer, length -
suffixLength, function); \
> + }
Eh, couldn't this become a function instead of a macro?
> Source/WTF/wtf/URLHelpers.cpp:507
> +String mapHostName(String string, std::optional<DecodeFunction>
decodeFunction, bool *error)
bool* error (the * hangs on the type).
Or better: bool& error, unless the error parameter is really supposed to be
optional, which I doubt.
Then I think we need to not copy the String or std::optional by value here. So
const String& string and const std::optional<DecodeFunction>& decodeFunction
would be good safe declarations. But if the values are being "sunk" then
String&& string and std::optional<DecodeFunction>>&& decodeFunction would be
better. Looks like they are being sunk throughout this file, but I see that
they are APIs accessible to other files, so that swings the balance in favor of
const-ref. So that leaves us with:
String mapHostName(const String& string, const std::optional<DecodeFunction>
decodeFunction, bool& error)
> Source/WTF/wtf/URLHelpers.cpp:511
> + return String();
This is old C++. Modern C++:
return { };
> Source/WTF/wtf/URLHelpers.cpp:514
> + return String();
Ditto.
> Source/WTF/wtf/URLHelpers.cpp:528
> + return String();
Ditto.
> Source/WTF/wtf/URLHelpers.cpp:532
> + return String();
Ditto.
> Source/WTF/wtf/URLHelpers.cpp:535
> + return String();
Ditto.
> Source/WTF/wtf/URLHelpers.cpp:542
> +static void collectRangesThatNeedMapping(String string, unsigned location,
unsigned length, MappingRangesVector *array, std::optional<DecodeFunction>
decodeFunction)
const String& string
MappingRangesVector& array -- the * hangs on the type, and then since you
dereference it unconditionally, it's not optional/nullable and should use &
instead of *
const std::optional<DecodeFunction>& decodeFunction
> Source/WTF/wtf/URLHelpers.cpp:545
> + // Therefore, we use nil to indicate no mapping here and an empty array
to indicate error.
It's not Objective C++ anymore so "nil" is no longer right.
> Source/WTF/wtf/URLHelpers.cpp:561
> +static void applyHostNameFunctionToMailToURLString(String string,
std::optional<DecodeFunction> decodeFunction, MappingRangesVector *array)
Ditto.
> Source/WTF/wtf/URLHelpers.cpp:633
> +static void applyHostNameFunctionToURLString(String string,
std::optional<DecodeFunction> decodeFunction, MappingRangesVector *array)
Ditto.
> Source/WTF/wtf/URLHelpers.cpp:692
> +String mapHostNames(String string, std::optional<DecodeFunction>
decodeFunction)
Ditto.
> Source/WTF/wtf/URLHelpers.cpp:706
> + return String();
return { };
> Source/WTF/wtf/URLHelpers.cpp:759
> + return emptyString();
return { };
> Source/WTF/wtf/URLHelpers.cpp:763
> + Vector<char, URL_BYTES_BUFFER_LENGTH> after(bufferLength); // large
enough to %-escape every character
Comment should be a complete sentence.
> Source/WTF/wtf/URLHelpers.cpp:765
> + char *q = after.data();
char* q
> Source/WTF/wtf/URLHelpers.cpp:767
> + const unsigned char *p = before;
const unsigned char* p
> Source/WTF/wtf/URLHelpers.cpp:803
> + char *p = after.data() + bufferLength - afterlength - 1;
char* p
> Source/WTF/wtf/URLHelpers.cpp:805
> + char *q = after.data();
char* q
> Source/WTF/wtf/URLHelpers.cpp:846
> + return String();
return { };
> Source/WTF/wtf/URLHelpers.h:37
> +using DecodeFunction = String(&)(String);
WTF is a big namespace and DecodeFunction could mean a lot of things. I'd call
it URLDecodeFunction.
> Source/WTF/wtf/URLHelpers.h:39
> +WTF_EXPORT_PRIVATE String userVisibleString(CString URL);
const CString& URL?
> Source/WTF/wtf/cocoa/NSURLExtras.h:29
> +#import <wtf/text/WTFString.h>
Are you sure this is needed here? Can't it go in NSURLExtras.mm?
> Source/WTF/wtf/cocoa/NSURLExtras.mm:34
> +#import <wtf/URLHelpers.h>
This isn't sorted properly?
> Source/WTF/wtf/cocoa/NSURLExtras.mm:42
> #define URL_BYTES_BUFFER_LENGTH 2048
This is unused and can be deleted.
> Source/WTF/wtf/cocoa/NSURLExtras.mm:109
> + return !host ? string : (NSString*)host;
Above you used (NSString *)host, which I suspect is the right style here. Not
sure. Any Apple developer should know, or you could poke around in the *.mm
files more looking for the answer.
> Source/WTF/wtf/cocoa/NSURLExtras.mm:118
> + return !host ? string : (NSString*)host;
(NSString *)?
> Source/WTF/wtf/glib/URLHelpersGlib.cpp:39
> +void loadIDNScriptWhiteList()
> +{
> + static std::once_flag flag;
> + std::call_once(flag, initializeDefaultIDNScriptWhiteList);
> +}
The Windows build failure is because there's no cross-platform implementation.
And the WPE build failure is because you forgot to add this file to
PlatformWPE.cmake.
Well we need a cross-platform implementation because we have to care for
JSCOnly, PlayStation, and Windows. And there's nothing GLib-specific here, so
this can be that implementation, right? Just rename the file to URLHelpers.cpp,
build it on all platforms in the cross-platform CMakeLists.txt (you can even
add it to XCode build in case we want to add more functions here in the
future), and slap an #if !PLATFORM(COCOA) around the definition here.
> Source/WebKit/PlatformGTK.cmake:106
> + ${WEBKIT_DIR}/UIProcess/API/gtk/WebKitURIUtilities.h
You remembered to install it AND add docs... nice.
> Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebKitURIUtilities.cpp:112
> +void beforeAll()
> +{
> + Test::add("WebKitURIUtilities", "uri-for-display-unaffected",
testURIForDisplayUnaffected);
> + Test::add("WebKitURIUtilities", "uri-for-display-affected",
testURIForDisplayAffected);
> +}
Although having a test for webkit_uri_for_display() is important, copying the
entire thing from the cross-platform test is not valuable. The tests will get
out of sync, anyway. Better to test the implementation details in the
cross-platform test, since that will be run for all developers. Then the GLib
test can become way simpler. E.g. just pass in a %-encoded URL and verify that
the result is decoded. The GLib test can be simpler because you have a good
cross-platform test.
More information about the webkit-reviews
mailing list