Bug 47620 – [WTFURL] Implement URLFragmentCanonicalizer

• Status: RESOLVED
• Resolution: WONTFIX
• Component: New Bugs

[Adam Barth]

[WTFURL] Implement URLFragmentCanonicalizer

[Adam Barth]

Created attachment 70657 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 70657 [details]
Patch

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

I think we should see a patch with a couple more comments.  r- for the nits.

> JavaScriptCore/wtf/url/src/URLCanonicalizer.h:44
> +    static void canonicalize(const InChar* spec, const URLSegments& segments, URLBuffer<OutChar>& buffer, URLSegments& resultSegments)
> +    {
> +        // FIXME: Implement this function.
> +    }
> +};

Should this be ASSERT_NOT_REACHED?

> JavaScriptCore/wtf/url/src/URLFragmentCanonicalizer.h:43
> +        if (fragment.length() < 0) {

isEmpty?  Or isNull?  or isValid?  length() < 0 seems very opaque.

> JavaScriptCore/wtf/url/src/URLFragmentCanonicalizer.h:50
> +        convertFragment(spec, fragment, buffer);

Why doesn't convertFragment return a length?  Maybe by refernce?

> JavaScriptCore/wtf/url/src/URLFragmentCanonicalizer.h:59
> +            if (spec[i] == '\0')

Does this have sign-extension problems?  I guess not since \0 is 0, and thus could never error due to sign extension...

You mentioned in person that this line needs a FIXME.  Please add.

> JavaScriptCore/wtf/url/src/URLFragmentCanonicalizer.h:61
> +            if (static_cast<unsigned>(spec[i]) < 0x20) {

Do we want a compareTo helper here to avoid anyone doing this wrong?

This probably needs some comments to explain what this is doing.  These are fast-paths, but they're not obvious.

> JavaScriptCore/wtf/url/src/URLFragmentCanonicalizer.h:66
> +                buffer.append(static_cast<char>(spec[i]));

Shouldn't this be OutChar?

> JavaScriptCore/wtf/url/src/URLQueryCanonicalizer.h:97
> +        Unicode::decodeToBuffer(spec, query.begin(), query.end(), convertedQuery);

Unicode:: seems more like a platform-style dependency instead of one needing templates.  But that's something which can be changed in a later patch. In either case, I think using Unicode:: is better than convertCharset was.

[Adam Barth]

WTFURL is gone.