Bug 53645 – [GTK] editing/pasteboard/copy-standalone-image.html fails

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Martin Robinson]

The image in this test is not copied properly. This can be observed by looking at the pixel results.

[Martin Robinson]

Created attachment 81157 [details]
Patch

This issue was that images were not written to the markup and URL portion of the clipboard. I've corrected this in my attached patch.

[Eric Seidel (no email)]

Comment on attachment 81157 [details]
Patch

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

> Source/WebCore/platform/gtk/PasteboardGtk.cpp:115
> +    // FIXME: Later this code should be shared with Chromium somehow. Chances are all platforms want it.

Which code?  Can we abstract said code into a helper method (even if we don't share it now)?

> Source/WebCore/platform/gtk/PasteboardGtk.cpp:130
> +    if (!url.isEmpty()) {

This whole block feels like a helper method to me. :)

[Ryosuke Niwa]

Comment on attachment 81157 [details]
Patch

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

>> Source/WebCore/platform/gtk/PasteboardGtk.cpp:130
>> +    if (!url.isEmpty()) {
> 
> This whole block feels like a helper method to me. :)

Indeed, it seems like we can add a function to markup.h/cpp.

[Ryosuke Niwa]

Comment on attachment 81157 [details]
Patch

r- due to style issues.

[Martin Robinson]

Created attachment 89416 [details]
Updated patch

[Martin Robinson]

(In reply to comment #2)
> (From update of attachment 81157 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=81157&action=review

Thank you both for your comments!

> > Source/WebCore/platform/gtk/PasteboardGtk.cpp:115
> > +    // FIXME: Later this code should be shared with Chromium somehow. Chances are all platforms want it.
> Which code?  Can we abstract said code into a helper method (even if we don't share it now)?

I've abstracted this code into a helper.

> > Source/WebCore/platform/gtk/PasteboardGtk.cpp:130
> > +    if (!url.isEmpty()) {
> eric: This whole block feels like a helper method to me. :)
> rniwa: Indeed, it seems like we can add a function to markup.h/cpp.

I've abstracted this into a helper based on a version of this code in the chromium platform directory. I've also replaced another implementation of it in the win platform directory.

[WebKit Review Bot]

Attachment 89416 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8401399

[Martin Robinson]

Created attachment 89439 [details]
Fix build for Chromium and Windows

[Build Bot]

Attachment 89416 [details] did not build on win:
Build output: http://queues.webkit.org/results/8401402

[Build Bot]

Attachment 89439 [details] did not build on win:
Build output: http://queues.webkit.org/results/8403349

[Martin Robinson]

Created attachment 89587 [details]
Try to fix the Windows build

[Build Bot]

Attachment 89587 [details] did not build on win:
Build output: http://queues.webkit.org/results/8404661

[Martin Robinson]

Created attachment 89593 [details]
One more shot at fixing the Windows build

[Xan Lopez]

Comment on attachment 89593 [details]
One more shot at fixing the Windows build

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

This looks sane to me.

> Source/WebCore/ChangeLog:26
> +        * platform/win/ClipboardWin.cpp: Remove the imageToMarkup helper and the

and the -> and use the

[Martin Robinson]

Committed r85064: <http://trac.webkit.org/changeset/85064>

[Benjamin Poulain]

Some time ago I noticed Chrome has some weird behavior regarding drag and drop due to imageToMarkup(): https://bugs.webkit.org/show_bug.cgi?id=58043

Unfortunately I had no time to pursue at the time and now I see imageToMarkup has been generalized :)

Is there a reason why you spread imageToMarkup() instead of using the existing createMarkup()?