Bug 22008 - Add windows support for selecting multiple files in a file upload control
Summary: Add windows support for selecting multiple files in a file upload control
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-10-31 13:04 PDT by Adele Peterson
Modified: 2008-10-31 16:26 PDT (History)
0 users

See Also:


Attachments
patch (5.54 KB, patch)
2008-10-31 13:06 PDT, Adele Peterson
darin: review+
Details | Formatted Diff | Diff
updated patch (13.92 KB, patch)
2008-10-31 15:07 PDT, Adele Peterson
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adele Peterson 2008-10-31 13:04:45 PDT
Add windows support for selecting multiple files in a file upload control
Comment 1 Adele Peterson 2008-10-31 13:06:19 PDT
Created attachment 24808 [details]
patch
Comment 2 Darin Adler 2008-10-31 13:54:32 PDT
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?
Comment 3 Adele Peterson 2008-10-31 15:07:27 PDT
Created attachment 24818 [details]
updated patch

uploading an updated patch to make sure this correctly addresses Darin's concerns.
Comment 4 Darin Adler 2008-10-31 16:17:17 PDT
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
Comment 5 Adele Peterson 2008-10-31 16:26:27 PDT
Committed revision 38057.