Bug 27411

Summary: [GTK] Clipboard data is lost on exit
Product: WebKit Reporter: ickard
Component: WebKit GtkAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: a.butenka, christian, commit-queue, gns, mrobinson, sarah.e.strong
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Linux   
Attachments:
Description Flags
Sets gtk_clipboard_set_can_store when clipboard is acquired to fix bug #27411: clipboard lost on exit.
gns: review-
Sets gtk_clipboard_set_can_store when clipboard is acquired. With changes suggested by kov: https://bugs.webkit.org/show_bug.cgi?id=27411#c6
none
Sets gtk_clipboard_set_can_store in WebCore. With changes suggested by mrobinson: https://bugs.webkit.org/show_bug.cgi?id=27411#c9
none
Sets gtk_clipboard_set_can_store in WebCore. With updated information about testing the behaviour.
gns: review+, commit-queue: commit-queue-
Sets gtk_clipboard_set_can_store in WebCore. With malformed patch fix none

Description ickard 2009-07-18 04:16:11 PDT
In gtk applications, one can use the clipboard in different ways: either just using the two convenience functions (if you will) gtk_clipboard_set_text() and gtk_clipboard_set_image(). When using these, clipboard data persists when one exits a program( this is due to gtk automatically calling gtk_clipboard_set_can_store() when one is calling these. If gtk_clipboard_set_can_store() is set properly, clipboard data can be stored to the freedesktop clipboard manager (http://www.freedesktop.org/wiki/Specifications/clipboard-manager-spec) which gnome implements. The transfer to the clipboard will happen once gtk_clipboard_store() is called, or when the application main loop exits.

Steps to reproduce:
1. open a webkit-gtk browser( I used epiphany)
2. copy some text to the clipboard (not selection clipboard)
3. open gedit, paste it there (this should work)
4. quit epiphany, try to paste again (this does not work)

This bug affects a couple of programs in the linux desktop (for instance firefox), so to see the clipboard manager working as intended try to use for instance gedit in a gnome environment (if you quit gedit, its clipboard data should persist).
Comment 1 Alexander Butenko 2009-07-18 10:09:58 PDT
as i know, its a longstanding GTK bug in general. not a webkit
Comment 2 ickard 2009-07-18 15:48:11 PDT
(In reply to comment #1)
> as i know, its a longstanding GTK bug in general. not a webkit

No it's not really a gtk bug, there are properly working gtk apps (such as gedit)
Comment 3 Christian Dywan 2009-12-20 03:05:28 PST
This works with Clipman in XFCE 4.6. So is this really a problem with WebKitGTK+? From a look at GTK+ sources the default clipboard is stored automatically. And surely GTK+ doesn't expect all existing applications to handle this manually. I might be wrong.
Comment 4 Sarah Strong 2010-07-09 11:20:13 PDT
I can confirm this bug in Epiphany and Empathy using Webkit-1.3.2 in Ubuntu 10.04.

Clipboard managers such as Klipper (a part of KDE), Glipper, Parcellite and Clipman work around this problem so it's expected that you wouldn't be able to reproduce it if you're running one of those.

It's not a GTK+ bug; that is to say, gtk provides the functions ickard mentioned so that applications can save their data before they exit. It would be nice if existing applications didn't have to handle it manually, but right now gtk_clipboard_set_can_store()/gtk_clipboard_store() are the right way of fixing it.
Comment 5 Sarah Strong 2010-07-12 07:16:41 PDT
Created attachment 61225 [details]
Sets gtk_clipboard_set_can_store when clipboard is acquired to fix bug #27411: clipboard lost on exit.
Comment 6 Gustavo Noronha (kov) 2010-07-12 07:29:21 PDT
Comment on attachment 61225 [details]
Sets gtk_clipboard_set_can_store when clipboard is acquired to fix bug #27411: clipboard lost on exit.

WebKit/gtk/webkit/webkitwebview.cpp:3565
 +                                                        NULL, 0);
Two things: no need to break the line here, in WebKit code we're used to longer lines, and this makes a bit more readable. Also, the style guidelines say we should use 0 instead of NULL (though we do use NULL in a few cases we get warnings otherwise).

As for the content, it looks sane to me, but I'd like to hear mrobinson's opinion as well, since he's the one who's been working on clipboard/dragndrop the most recently (I'll poke him). Also, we might also want to save the PRIMARY clipboard, right? I'll say r- for now, because we need a ChangeLog entry. I assume you are not a committer, so you may want to set commit-queue to ? as well, so that the person who reviews your patch can make the patch be automatic committed for you =).
Comment 7 Sarah Strong 2010-07-12 08:48:17 PDT
Created attachment 61234 [details]
Sets gtk_clipboard_set_can_store when clipboard is acquired. With changes suggested by kov: https://bugs.webkit.org/show_bug.cgi?id=27411#c6
Comment 8 Sarah Strong 2010-07-12 09:03:59 PDT
I didn't add a primary buffer save because there's some disagreement on whether it makes sense conceptually. I think the argument against it is that primary selection isn't meant for workflow where you'd need clipboard persistence. The argument for it is of course preserving more information the user might want.

It should be straightforward to add primary save support. I'll up a new patch once I've tested that.
Comment 9 Martin Robinson 2010-07-12 11:08:09 PDT
Patch looks good, but this may not be the correct place to call these functions. Having the change here means that not all code paths which set the clipboard will get this fix. Perhaps it would make more sense to do it here: http://trac.webkit.org/browser/trunk/WebCore/platform/gtk/PasteboardHelper.cpp#L286 in writeClipboardContents. Please consider this change. Thanks!
Comment 10 Sarah Strong 2010-07-12 12:24:10 PDT
Created attachment 61256 [details]
Sets gtk_clipboard_set_can_store in WebCore. With changes suggested by mrobinson: https://bugs.webkit.org/show_bug.cgi?id=27411#c9
Comment 11 Martin Robinson 2010-07-12 12:29:12 PDT
This looks good to me! A reviewer will have to do the r+.
Comment 12 Sarah Strong 2010-07-16 10:22:45 PDT
Created attachment 61822 [details]
Sets gtk_clipboard_set_can_store in WebCore. With updated information about testing the behaviour.
Comment 13 Gustavo Noronha (kov) 2010-07-16 10:29:44 PDT
Comment on attachment 61822 [details]
Sets gtk_clipboard_set_can_store in WebCore. With updated information about testing the behaviour.

Very good, thanks!
Comment 14 WebKit Commit Bot 2010-07-16 10:55:30 PDT
Comment on attachment 61822 [details]
Sets gtk_clipboard_set_can_store in WebCore. With updated information about testing the behaviour.

Rejecting patch 61822 from commit-queue.

Failed to run "[u'/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', u'--reviewer', u'Gustavo Noronha Silva', u'--force']" exit_code: 2
Parsed 2 diffs from patch file(s).
patching file WebCore/ChangeLog
patch: **** malformed patch at line 21: +        I haven't included an automated test because of the difficulty of testing behaviour after application exit.

patching file WebCore/platform/gtk/PasteboardHelper.cpp

Full output: http://webkit-commit-queue.appspot.com/results/3562073
Comment 15 Sarah Strong 2010-07-16 11:13:47 PDT
Created attachment 61827 [details]
Sets gtk_clipboard_set_can_store in WebCore. With malformed patch fix
Comment 16 Gustavo Noronha (kov) 2010-07-16 11:16:46 PDT
Comment on attachment 61827 [details]
Sets gtk_clipboard_set_can_store in WebCore. With malformed patch fix

let's try again
Comment 17 WebKit Commit Bot 2010-07-16 14:49:46 PDT
Comment on attachment 61827 [details]
Sets gtk_clipboard_set_can_store in WebCore. With malformed patch fix

Clearing flags on attachment: 61827

Committed r63587: <http://trac.webkit.org/changeset/63587>