Bug 29666 – ClipboardWin::files() isn't implemented in Windows

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Marshall Culpepper]

When a user drags a list of files into a Win32 WebKit app, the dataTransfer.files list is always null (because it hasn't been implemented)

[Marshall Culpepper]

Created attachment 39974 [details]
ClipboardWin::files() patch

[Eric Seidel (no email)]

Comment on attachment 39974 [details]
ClipboardWin::files() patch

We don't need a manual test, this is already tested by layout tests.  We need someone to fix windows DumpRenderTree to have eventSender.beginDragWithFiles() support. :)

Your ChangeLog might confuse svn-apply, thus making this impossible to automatically land but someone could land this by hand.

[Eric Seidel (no email)]

*** Bug 26711 has been marked as a duplicate of this bug. ***

[Marshall Culpepper]

Created attachment 40077 [details]
patch #2

Here's an updated patch that removes the manual test. I can take a look at https://bugs.webkit.org/show_bug.cgi?id=25924 after this one.

[Eric Seidel (no email)]

I believe Adam Roben and Oliver Hunt have the most experience with Windows D&D patches.  They're probably better reviewers than I am, although I'm happy to look.

Dan Bates has also recently worked in windows drag and drop code and might be of some help in solving  bug 25924 too.

[Adam Roben (:aroben)]

Comment on attachment 40077 [details]
patch #2

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

[Eric Seidel (no email)]

For better or worse .files always returns an empty file list.  That's how I implemented it on mac, and there is a comment in the Mac code:
http://trac.webkit.org/browser/trunk/WebCore/platform/mac/ClipboardMac.mm#L291

I don't know if the spec comments on this edgecase (what to return when access is disallowed, or what to return when there are not files on the clipboard, etc.).  However this patch as implemented currently matches Mac.

Until bug 25924 is fixed, this change will not affect any tests.  I agree with Adam, it would be better to fix bug 25924 first before fixing this one though. :)

[Marshall Culpepper]

Created attachment 40274 [details]
patch #3

changes to match some of aroben's suggestions.

[Adam Roben (:aroben)]

Comment on attachment 40274 [details]
patch #3

>  PassRefPtr<FileList> ClipboardWin::files() const
>  {
> -    notImplemented();
> -    return 0;
> +    if (policy() != ClipboardReadable && policy() != ClipboardTypesReadable)
> +      return FileList::create();
> +
> +    if (!m_dataObject)
> +        return FileList::create();
> +
> +    RefPtr<FileList> files = FileList::create();
> +    STGMEDIUM medium;
> +    if (FAILED(m_dataObject->GetData(cfHDropFormat(), &medium)))
> +        return files.release();
> +
> +    HDROP hdrop = reinterpret_cast<HDROP>(GlobalLock(medium.hGlobal));
> +    if (!hdrop)
> +        return files.release();

I think it would be more consistent to move the declaration of files to the top of the function and return files.release() in all the bail-out cases.

> +    WCHAR filename[MAX_PATH];
> +    UINT fileCount = DragQueryFileW(hdrop, (UINT)0xFFFFFFFF, 0, 0);

I don't think you need the (UINT) cast here. If you do, please use static_cast instead of a C-style cast.

> +    for (UINT i = 0; i < fileCount; i++) {
> +        if (!DragQueryFileW(hdrop, i, filename, ARRAYSIZE(filename)))
> +            continue;
> +        files->append(File::create(reinterpret_cast<UChar*>(filename)));
> +    }
> +
> +    DragFinish(hdrop);
> +    GlobalUnlock(medium.hGlobal);
> +    return files.release();
>  }

http://msdn.microsoft.com/en-us/library/bb776904(VS.85).aspx#extracting_filenames says that you should be calling ReleaseStgMedium instead of DragFinish. Other documentation on MSDN leads me to believe that DragFinish should only be called when handling the WM_DROPFILES message. Sorry for not realizing this sooner!

[Marshall Culpepper]

Created attachment 41405 [details]
patch #4

Following more of Adam's suggestions.. hopefully we can land this one now :)

[Adam Roben (:aroben)]

Comment on attachment 41405 [details]
patch #4

> +    STGMEDIUM medium;
> +    if (FAILED(m_dataObject->GetData(cfHDropFormat(), &medium)))
> +        return files.release();
> +
> +    HDROP hdrop = reinterpret_cast<HDROP>(GlobalLock(medium.hGlobal));
> +    if (!hdrop)
> +        return files.release();
> +
> +    WCHAR filename[MAX_PATH];
> +    UINT fileCount = DragQueryFileW(hdrop, 0xFFFFFFFF, 0, 0);
> +    for (UINT i = 0; i < fileCount; i++) {
> +        if (!DragQueryFileW(hdrop, i, filename, ARRAYSIZE(filename)))
> +            continue;
> +        files->append(File::create(reinterpret_cast<UChar*>(filename)));
> +    }
> +
> +    ReleaseStgMedium(&medium);
> +    GlobalUnlock(medium.hGlobal);

I think you should reverse the calls to ReleaseStgMedium and GlobalUnlock here. Otherwise you're using the medium after releasing it, which, while it may happen to work, doesn't seem like a good thing to do.

Once that's fixed, I think we're good to go!

[Marshall Culpepper]

Created attachment 41446 [details]
patch #5

oops -- oversight on my part. here's the updated patch

[Adam Roben (:aroben)]

Comment on attachment 41446 [details]
patch #5

r=me

[Eric Seidel (no email)]

So sad.  We still really need bug 25924 before its easy to tell if this code is correct or not. :(

[Eric Seidel (no email)]

(I'm glad that windows will have this ability soon.  Just very sad it's not tested.)

[WebKit Commit Bot]

Comment on attachment 41446 [details]
patch #5

Clearing flags on attachment: 41446

Committed r49810: <http://trac.webkit.org/changeset/49810>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.