Bug 29666 - ClipboardWin::files() isn't implemented in Windows
Summary: ClipboardWin::files() isn't implemented in Windows
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Windows 7
: P2 Normal
Assignee: Nobody
URL:
Keywords:
: 26711 (view as bug list)
Depends on: 25924
Blocks:
  Show dependency treegraph
 
Reported: 2009-09-22 19:08 PDT by Marshall Culpepper
Modified: 2009-10-19 14:31 PDT (History)
6 users (show)

See Also:


Attachments
ClipboardWin::files() patch (4.11 KB, patch)
2009-09-22 19:12 PDT, Marshall Culpepper
eric: review-
eric: commit-queue-
Details | Formatted Diff | Diff
patch #2 (2.30 KB, patch)
2009-09-24 11:31 PDT, Marshall Culpepper
aroben: review-
Details | Formatted Diff | Diff
patch #3 (2.23 KB, patch)
2009-09-28 17:47 PDT, Marshall Culpepper
aroben: review-
Details | Formatted Diff | Diff
patch #4 (2.27 KB, patch)
2009-10-19 02:50 PDT, Marshall Culpepper
aroben: review-
Details | Formatted Diff | Diff
patch #5 (2.27 KB, patch)
2009-10-19 13:49 PDT, Marshall Culpepper
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Marshall Culpepper 2009-09-22 19:08:41 PDT
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)
Comment 1 Marshall Culpepper 2009-09-22 19:12:28 PDT
Created attachment 39974 [details]
ClipboardWin::files() patch
Comment 2 Eric Seidel (no email) 2009-09-23 17:45:17 PDT
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.
Comment 3 Eric Seidel (no email) 2009-09-23 17:46:38 PDT
*** Bug 26711 has been marked as a duplicate of this bug. ***
Comment 4 Marshall Culpepper 2009-09-24 11:31:49 PDT
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.
Comment 5 Eric Seidel (no email) 2009-09-25 13:56:11 PDT
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.
Comment 6 Adam Roben (:aroben) 2009-09-25 14:41:49 PDT
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.
Comment 7 Eric Seidel (no email) 2009-09-25 14:48:32 PDT
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. :)
Comment 8 Marshall Culpepper 2009-09-28 17:47:19 PDT
Created attachment 40274 [details]
patch #3

changes to match some of aroben's suggestions.
Comment 9 Adam Roben (:aroben) 2009-09-29 08:09:38 PDT
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!
Comment 10 Marshall Culpepper 2009-10-19 02:50:43 PDT
Created attachment 41405 [details]
patch #4

Following more of Adam's suggestions.. hopefully we can land this one now :)
Comment 11 Adam Roben (:aroben) 2009-10-19 08:33:35 PDT
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!
Comment 12 Marshall Culpepper 2009-10-19 13:49:47 PDT
Created attachment 41446 [details]
patch #5

oops -- oversight on my part. here's the updated patch
Comment 13 Adam Roben (:aroben) 2009-10-19 13:56:13 PDT
Comment on attachment 41446 [details]
patch #5

r=me
Comment 14 Eric Seidel (no email) 2009-10-19 13:57:10 PDT
So sad.  We still really need bug 25924 before its easy to tell if this code is correct or not. :(
Comment 15 Eric Seidel (no email) 2009-10-19 13:57:41 PDT
(I'm glad that windows will have this ability soon.  Just very sad it's not tested.)
Comment 16 WebKit Commit Bot 2009-10-19 14:31:42 PDT
Comment on attachment 41446 [details]
patch #5

Clearing flags on attachment: 41446

Committed r49810: <http://trac.webkit.org/changeset/49810>
Comment 17 WebKit Commit Bot 2009-10-19 14:31:59 PDT
All reviewed patches have been landed.  Closing bug.