[webkit-reviews] review denied: [Bug 29666] ClipboardWin::files() isn't implemented in Windows : [Attachment 40077] patch #2
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Fri Sep 25 14:41:49 PDT 2009
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.
More information about the webkit-reviews
mailing list