[Webkit-unassigned] [Bug 174816] [GTK][WPE] Need a function to convert internal URI to display ("pretty") URI

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Sat Jan 27 09:50:02 PST 2018


https://bugs.webkit.org/show_bug.cgi?id=174816

--- Comment #52 from Michael Catanzaro <mcatanzaro at igalia.com> ---
(In reply to Gabriel Ivașcu from comment #50)
> Hmm OK but that will make a class with only two static methods.

In that case, I would use a namespace instead of a class. It's fine to have global functions outside of any class in C++. The namespace is even optional, but I think it's helpful to group related functions together in cases like this.

> > > Source/WebCore/platform/mac/WebCoreNSURLExtras.mm:48
> I changed this so that callers that don't use script white/black lists
> wouldn't have to create explicitly a whitelist with all bits set to one to
> pass to the function. This way, they would leave the function's default
> blacklist's value of all zeros, i.e. all scripts are allowed. I can change
> it back however.

I think the change is good (as long as it's not what's causing the tests to break ;)

> > > 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.
> 
> OK. But UChar32 is defined as int32_t in WTF/icu/unicode/umachine.h.

The U means "Unicode", not "unsigned"... but yes, much better to use std::optional here instead. Also in hasCharactersInIDNScriptBlackList as well.

(In reply to Alex Christensen from comment #49)
> 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.

Gabriel, the tricky part here is that you cannot create new cross-platform files without breaking the Mac build. You can add the new file in WebCore/CMakeLists.txt, but you'll need help from Alex to modify your patch in order to add the new file to the XCode build system, or the Mac EWS is going to be red, which will not be helpful in trying to figure out if your patch passes the Mac tests or not.

So for now, my recommendation is to move the code to the URL class instead of URLParser, and make sure it works there. Then, only once it passes tests on Mac, we can move it into a new file if Alex wants to help with that. I kinda think that URL might be a good permanent home for this code, since that keeps it out of URLParser, and being able to call url.displayURL() would be nice, but we'll do whatever Alex prefers.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20180127/a879788c/attachment-0001.html>


More information about the webkit-unassigned mailing list