[webkit-reviews] review denied: [Bug 47620] [WTFURL] Implement URLFragmentCanonicalizer : [Attachment 70657] Patch

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied Adam Barth <abarth at webkit.org>'s
request for review:
Bug 47620: [WTFURL] Implement URLFragmentCanonicalizer
https://bugs.webkit.org/show_bug.cgi?id=47620

Attachment 70657: Patch
https://bugs.webkit.org/attachment.cgi?id=70657&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
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.