Bug 22008 – Add windows support for selecting multiple files in a file upload control

• Status: RESOLVED
• Resolution: FIXED
• Component: Forms

[Adele Peterson]

Add windows support for selecting multiple files in a file upload control

[Adele Peterson]

Created attachment 24808 [details]
patch

[Darin Adler]

Comment on attachment 24808 [details]
patch

>  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?

[Adele Peterson]

Created attachment 24818 [details]
updated patch

uploading an updated patch to make sure this correctly addresses Darin's concerns.

[Darin Adler]

Comment on attachment 24818 [details]
updated patch

> +    if (_tcscat_s(buffer, ARRAYSIZE(buffer), TEXT("\\shell32.dll")))

As we discussed in person, you can leave out the ARRAYSIZE here and it will get the size automatically, and you should. Same in the other _tcscat_s call site.

> +// When you call GetOpenFileName, if the size of the buffer is too small,
> +// MSDN says that the first two bytes of the buffer contain the required size for the file selection, in bytes or characters
> +// So we can assume the required size can't be more than the maximum value for a short.
> +static const unsigned short maxFilePathsListSize = USHRT_MAX;

This should be size_t.

r=me

[Adele Peterson]

Committed revision 38057.