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

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted 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 197979: Patch
https://bugs.webkit.org/attachment.cgi?id=197979&action=review

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


I am OK with this patch as is, but I still have many suggestions for
improvement.

> Source/WebCore/ChangeLog:19
> +	   (WebCore):

Please remove bogus lines like this from the change log.

> Source/WebCore/platform/network/HTTPParsers.cpp:234
> +static void replaceUnsafeCharactersInFileName(String& unsafeFileName)

We should figure out whether it’s “filename” or “file name”. If it’s
“filename”, then we should not capitalize the “N”. Elsewhere in the file we use
that unsafeFilename, so lets do that consistently.

> Source/WebCore/platform/network/HTTPParsers.cpp:243
> +    unsigned length = unsafeFileName.length();
> +    unsigned index = 0;
> +    while (index < length && unsafeFileName[index] == '.')
> +	   ++index;

While we prefer words over single letters, the word “index” for a loop index is
an exceptoin.

Since String does a range check on indices passed in, this can be written more
simply like this:

    unsigned i;
    for (i = 0; unsafeFileName[i] == '.'; ++i)
	;

Or as a while loop if you prefer, but without the length variable and length
check.

> Source/WebCore/platform/network/HTTPParsers.cpp:258
> +	   if (c == '\\' && position < (length - 1)) {

The parentheses around length - 1 are not idiomatic for our coding style.

> Source/WebCore/platform/network/HTTPParsers.cpp:275
> +    return unquotedFilename.substring(position, endIndex == notFound ? -1 :
endIndex - position);

The use of -1 here is wrong since the argument is unsigned; while it works it’s
not suggested style. In fact, the value for notFound was chosen to make
expressions like this work without special cases. This can be written like
this:

    unquotedFilename.substring(position, unquotedFilename.find(';', position) -
position);

Or you can use the local variable endIndex, but no special case is needed.

> Source/WebCore/platform/network/HTTPParsers.cpp:285
> +	   if (c == '\\' && position < (length -1)) {

We write this as "length - 1" not "(length -1)", please leave off the
parentheses.

> Source/WebCore/platform/network/HTTPParsers.cpp:290
> +	   else if ((c == separator) && !insideQuotes)

We don’t parenthesize expressions like "(c == separator)" in contexts like
this.

> Source/WebCore/platform/network/HTTPParsers.cpp:291
> +	       break; // Found separator.

Comment doesn’t seem helpful since previous line says "c == separator".

> Source/WebCore/platform/network/HTTPParsers.cpp:297
> +// Returns true if Filename start was found and there is still more.

The word “Filename” should not be capitalized here.

> Source/WebCore/platform/network/HTTPParsers.cpp:298
> +static bool skipToFileNameStart(const String& value, unsigned& position)

The word “FileName” should be “Filename” here.

> Source/WebCore/platform/network/HTTPParsers.cpp:321
> +    unsigned pos = 0;

How about position instead of “pos”? Or maybe filenameStartPosition?

> Source/WebCore/platform/network/HTTPParsers.cpp:325
> +    String fileName;

Lets use “filename” instead of “fileName”, unless you want to argue we should
go the other way.

> Tools/TestWebKitAPI/Tests/WebCore/HTTPParsers.cpp:43
> +    EXPECT_EQ(String("foo.html"),
filenameFromHTTPContentDisposition("inline; filename=\"foo.html\""));

I think it would be nice to expose this on the internals object so we can test
it from JavaScript. Doesn’t seem necessary to test it with a C++ unit test.