[webkit-reviews] review denied: [Bug 113233] filenameFromHTTPContentDisposition() doesn't comply with RFC 6266 : [Attachment 195895] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Christophe Dumez <dchris at gmail.com>'s
request for review:
Bug 113233: filenameFromHTTPContentDisposition() doesn't comply with RFC 6266
https://bugs.webkit.org/show_bug.cgi?id=113233

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

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=195895&action=review


Looks generally quite good.

review- because of the unnecessarily inefficient loop to change slashes into
underscores.

I’d like to see at least one test case with trailing backslash without a
following character.

> Source/WebCore/platform/network/HTTPParsers.cpp:236
> +    return (c == '/' || c == '\\');

Extra parentheses not needed here.

> Source/WebCore/platform/network/HTTPParsers.cpp:246
> +    // Replace path separators with '_'.
> +    size_t index = 0;
> +    while ((index = unsafeFileName.find(isPathSeparator, index)) !=
notFound) {
> +	   unsafeFileName.replace(index, 1, "_");
> +	   ++index;
> +    }

This is an unnecessarily inefficient way to replace path separators with "_"
characters. For every path separator in the string, it creates a constant
string with the "_" character in it to pass to the replace function, and then
goes on to make a new copy of the entire string each time. That can have
pathologically slow behavior. This simpler code is much more efficient:

    unsafeFileName = unsafeFileName.replace('/', '_').replace('\\', '_');

This still makes two copies of the string if it contains both forward and
reverse slashes, but that’s OK; it won’t ever make more than two copies.

> Source/WebCore/platform/network/HTTPParsers.cpp:255
> +    if (index > 0)
> +	   unsafeFileName = unsafeFileName.substring(index);

The if statement here is not needed. The substring function already optimizes
this case, so we can simply call substring unconditionally.

> Source/WebCore/platform/network/HTTPParsers.cpp:259
> +static String parseQuotedString(const String& str, unsigned pos)

In the WebKit project we try to avoid abbreviations like “str“ and “pos”. I
know the existing functions in this file use them, but it’s not the preferred
style. I think the name quotedString and position would be good alternatives.

> Source/WebCore/platform/network/HTTPParsers.cpp:264
> +    bool wasEscaped = false;

There’s a cleaner way of writing this without a boolean.

> Source/WebCore/platform/network/HTTPParsers.cpp:265
> +    while (pos < length) {

I think this would read much better as a for loop rather than a while.

> Source/WebCore/platform/network/HTTPParsers.cpp:276
> +	   } else
> +	       wasEscaped = true;

Here you can just write the following code:

    else {
	if (position <= length)
	    parsedString.append(quotedString[++position]);
    }

Then you don’t need the wasEscaped boolean at all.

But also, this function strips a trailing backslash if it’s the last character
of the string. Is that correct behavior? It also will return the string even if
there is no double quote mark in it. Is that also correct behavior? Is that
covered by test cases below?

> Source/WebCore/platform/network/HTTPParsers.cpp:284
> +// Note that we deliberately don't comply RFC 6266 here to match the

Typo: "comply with RFC 6266".

> Source/WebCore/platform/network/HTTPParsers.cpp:286
> +// but we are way more tolerant. Our layout tests even rely on this.

Instead of “Our layout tests even rely on this” please say “WebKit regression
tests cover this”.

> Source/WebCore/platform/network/HTTPParsers.cpp:287
> +static String parseUnquotedFileName(const String& str, unsigned pos)

Again, please don’t use the abbreviations str and pos here.

> Source/WebCore/platform/network/HTTPParsers.cpp:297
> +    while (endIndex < length) {
> +	   UChar c = str[endIndex];
> +	   if (c == ';')
> +	       break;
> +	   ++endIndex;
> +    }

This is a copy of the String::find function. Please use that instead.

> Source/WebCore/platform/network/HTTPParsers.cpp:303
> +static bool skipToNextSeparator(const String& value, UChar separator,
unsigned& pos)

Please use a word, position, instead of an abbreviation, pos.

> Source/WebCore/platform/network/HTTPParsers.cpp:307
> +    bool insideQuotes = false;
> +    bool escaped = false;

This function can be written better without the escaped boolean. The escape
logic can simply consume an extra character like the one above.

> Source/WebCore/platform/network/HTTPParsers.cpp:308
> +    while (pos < length) {

This would read better as a for loop; it can be one without an initializer.

> Source/WebCore/platform/network/HTTPParsers.cpp:312
> +	   if (c == '\\') {
> +	       escaped = !escaped;
> +	   } else if (c == '"') {

In WebKit coding style we don’t use braces around a single line if body like
this.

> Source/WebCore/platform/network/HTTPParsers.cpp:367
> +    return fileName.isEmpty() ? String() : fileName;

Why is it important to return the null string instead of an empty string? That
seems a bit peculiar.