Bug 56193

Summary: Dragging image to desktop gives webloc instead of image file in WebKit2
Product: WebKit Reporter: Darin Adler <darin>
Component: New BugsAssignee: Darin Adler <darin>
Status: RESOLVED FIXED    
Severity: Normal CC: eric, yongjun_zhang
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: OS X 10.5   
Attachments:
Description Flags
Patch
none
Patch sam: review+

Description Darin Adler 2011-03-11 07:51:24 PST
Dragging image to desktop gives webloc instead of image file in WebKit2
Comment 1 Darin Adler 2011-03-11 07:55:01 PST
Created attachment 85477 [details]
Patch
Comment 2 Enrica Casucci 2011-03-11 12:03:19 PST
Comment on attachment 85477 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=85477&action=review

The code looks good to me. It's a very clever way of solving the problem! I'm assuming you've verified that the rest of the image dragging works as before.

> Source/WebKit2/WebProcess/WebCoreSupport/mac/WebDragClientMac.mm:99
> +    // FIXME: Seems this mesage should be named StartDrag, not SetDragImage.

typo message. It was named after the AppKit API, but I agree that this actually starts the dragging.

> Source/WebKit2/WebProcess/WebCoreSupport/mac/WebDragClientMac.mm:173
> +

It is a bit hard to follow, given the amount of changes, but I don't understand why you're writing all the data to the pasteboard without checking the types required. Could you please clarify?
Comment 3 Eric Seidel (no email) 2011-03-11 16:11:21 PST
Attachment 85477 [details] did not build on mac:
Build output: http://queues.webkit.org/results/8144005
Comment 4 Darin Adler 2011-03-12 05:30:11 PST
Created attachment 85589 [details]
Patch
Comment 5 Sam Weinig 2011-03-12 11:03:12 PST
Comment on attachment 85589 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=85589&action=review

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:1519
> +#if PLATFORM(MAC)
> +    platformDragEnded();
> +#endif

In general, I like having calls like these without the #if, and have each platform just have a stubbed out implementation.
Comment 6 Darin Adler 2011-03-12 19:12:04 PST
Committed r80948: <http://trac.webkit.org/changeset/80948>
Comment 7 Darin Adler 2011-03-12 20:04:49 PST
(In reply to comment #2)
> (From update of attachment 85477 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=85477&action=review
> 
> The code looks good to me. It's a very clever way of solving the problem! I'm assuming you've verified that the rest of the image dragging works as before.

I did some testing. Not sure I covered everything.

> > Source/WebKit2/WebProcess/WebCoreSupport/mac/WebDragClientMac.mm:99
> > +    // FIXME: Seems this mesage should be named StartDrag, not SetDragImage.
> 
> typo message. It was named after the AppKit API, but I agree that this actually starts the dragging.

Oops, I missed your note and didn’t fix the typo.

The AppKit API is named dragImage... not setDragImage, so I think our name is not really in the same spirit.

> > Source/WebKit2/WebProcess/WebCoreSupport/mac/WebDragClientMac.mm:173
> 
> It is a bit hard to follow, given the amount of changes, but I don't understand why you're writing all the data to the pasteboard without checking the types required. Could you please clarify?

The way the code works is that it puts the list of types in, and then checks each type. Since the code put the types in, there is no need for it to double check what it put in!

Presumably the checks existed in the code we copied this from because there was some kind of separation of responsibilities of determining which types to include and putting the data on the pasteboard. But here I see no need for this checking.
Comment 8 Darin Adler 2011-03-12 20:09:53 PST
*** Bug 56224 has been marked as a duplicate of this bug. ***
Comment 9 Darin Adler 2011-03-12 20:10:34 PST
Oops, wrong bug.