Bug 56193 – Dragging image to desktop gives webloc instead of image file in WebKit2

• Component: New Bugs
• Status: RESOLVED FIXED

[Darin Adler]

Dragging image to desktop gives webloc instead of image file in WebKit2

[Darin Adler]

Created attachment 85477 [details]
Patch

[Enrica Casucci]

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?

[Eric Seidel (no email)]

Attachment 85477 [details] did not build on mac:
Build output: http://queues.webkit.org/results/8144005

[Darin Adler]

Created attachment 85589 [details]
Patch

[Sam Weinig]

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.

[Darin Adler]

Committed r80948: <http://trac.webkit.org/changeset/80948>

[Darin Adler]

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

[Darin Adler]

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

[Darin Adler]

Oops, wrong bug.