[webkit-reviews] review denied: [Bug 29666] ClipboardWin::files() isn't implemented in Windows : [Attachment 40077] patch #2

[bugzilla-daemon at webkit.org]

Adam Roben (aroben) <aroben at apple.com> has denied Marshall Culpepper
<mculpepper at appcelerator.org>'s request for review:
Bug 29666: ClipboardWin::files() isn't implemented in Windows
https://bugs.webkit.org/show_bug.cgi?id=29666

Attachment 40077: patch #2
https://bugs.webkit.org/attachment.cgi?id=40077&action=review

------- Additional Comments from Adam Roben (aroben) <aroben at apple.com>
>  PassRefPtr<FileList> ClipboardWin::files() const
>  {
> -    notImplemented();
> -    return 0;
> +    if (policy() != ClipboardReadable && policy() != ClipboardTypesReadable)

> +	   return FileList::create();
> +
> +    if (!m_dataObject)
> +	   return FileList::create();

Is returning an empty FileList preferred to returning 0?

> +
> +    RefPtr<FileList> files = FileList::create();
> +    STGMEDIUM medium;
> +    if (FAILED(m_dataObject->GetData(cfHDropFormat(), &medium)))
> +	   return files.release();
> +
> +    HDROP hdrop = (HDROP) GlobalLock(medium.hGlobal);
> +    if (!hdrop)
> +	   return files.release();

I think you should wait to declare files until after this point, since you
haven't used it yet.

reinterpret_cast would be better than a C-style case.

> +
> +    WCHAR filename[MAX_PATH];
> +    UINT fileCount = DragQueryFile(hdrop, (UINT)-1, 0, 0);

I think 0xFFFFFFFF would be better than (UINT)-1, since the former is what MSDN
actually says (though I know they're equivalent).

> +    for (UINT i = 0; i < fileCount; i++) {
> +	   if (!DragQueryFileW(hdrop, i, filename, ARRAYSIZE(filename)))

It seems a little strange to call DragQueryFile above and then DragQueryFileW
here. Let's just call DragQueryFileW in both places.

> +	   files->append(File::create((UChar*)filename));

Again, reinterpret_cast would be better.

Are there regression tests that will now pass because of this?

This looks really nice! Thanks for working on it! r- so we can get these little
issues taken care of.