[webkit-reviews] review granted: [Bug 22008] Add windows support for selecting multiple files in a file upload control : [Attachment 24808] patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Adele Peterson <adele at apple.com>'s
request for review:
Bug 22008: Add windows support for selecting multiple files in a file upload
control
https://bugs.webkit.org/show_bug.cgi?id=22008

Attachment 24808: patch
https://bugs.webkit.org/attachment.cgi?id=24808&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
>  PassRefPtr<Icon> Icon::newIconForFiles(const Vector<String>& filenames)

Since this function ignores the argument, you should leave out the argument
name in the function definition. That will make it easier if we later turn on
the warning for unused parameters.

Since these functions return PassRefPtr, I think they should use the word
"create" in their names instead of "new"; but there's no rush to fix that now.

> +    UINT length = ::GetSystemDirectory(buffer, MAX_PATH);

Is MAX_PATH right here, or should it be MAX_PATH - 1?

> +    _tcsncpy_s(buffer + length, MAX_PATH - length, shell32, shell32Length);

I think this needs to be MAX_PATH - 1 - length to avoid overwriting the end of
the buffer. How can we test this edge case?

> +    buffer[length + shell32Length] = 0;

You could _TRUNCATE to take care of this in the tcsncpy_s call.

After considering these last two issues, I think you should use use _tcscat_s,
and let it automatically pick up the buffer size. The only downside is that it
will have to scan for the terminating null even though you already know exactly
where it is, but I think that overall it's simpler and clearer and safer if you
use _tcscat_s in a simpler way.

> +    bool multiFile = fileChooser->allowsMultipleFiles();
> +    Vector<TCHAR> fileBuf(multiFile ? USHRT_MAX : MAX_PATH);

This code says that you're actually allocating a vector with 65536 characters
in it. Is that really what we want to do? Seems OK, but not great. Also seems
like a magic number. I don't think that using the named constant USHRT_MAX
makes this less of a magic number. I think there should be a named constant
somewhere above with a comment explaining why it's big enough. Is there any way
to use GetOpenFileName that finds out how big the buffer needs to be?

> +    } // FIXME: Show some sort of error if too many files are selected and
the buffer is too small.  For now, this will fail silently.

I think you should give this comment its own line instead of putting it on the
end of a line with a brace on it. I also think this issue is so minor we're
unlikely to ever fix it.

r=me as-is, I guess, but how about that tscsat_s thing?