Simultaneously store multiple formats on the linux clipboard.

Simultaneously store multiple formats on the linux clipboard.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=3819

[Evan Stade, 2008-10-21 22:35:09]

This is a bit of a rework from the first clipboard implementation. The reason is
because the gtk clipboard and the windows clipboard work in fundamentally
different ways. Every time you set something on the gtk clipboard, all previous
data is erased. The Chromium clipboard class interface is a bit awkward for us
to handle, but I don't think this design has any significant performance issues.

Another point of note is that the IsFormatAvailable function is a bit more
roundabout than we would like because of a bug in GTK. I have filed a bug report
with the GTK developers.

[Evan Martin, 2008-10-22 17:33:48]

I have a couple of style comments below, but more fundamentally I just don't
understand what this change is trying to do.  Sorry, I don't have all this
clipboard stuff in my head like you do.    :)  It might be good to find the
original author of the file and include them in the review.

Is the intent to put multiple things on the clipboard simultaneously in
different formats?

http://codereview.chromium.org/8038/diff/8/208
File base/clipboard.h (right):

http://codereview.chromium.org/8038/diff/8/208#newcode34
Line 34: typedef std::map<std::string, std::pair<uint8*, size_t> > TargetMap;
This could live down next to where it's used.

Is there a reason for the value not to be a std::string?  We use those elsewhere
for strings.

This needs documentation as to what it holds (e.g. what is the key?)

http://codereview.chromium.org/8038/diff/8/207
File base/clipboard_linux.cc (right):

http://codereview.chromium.org/8038/diff/8/207#newcode19
Line 19: void GetData(GtkClipboard* clipboard,
This (and all other file-local functions) need some doc comments.

http://codereview.chromium.org/8038/diff/8/207#newcode47
Line 47: void SetGtkClipboard(GtkClipboard* clipboard,
Shouldn't this be a private method on the class?

http://codereview.chromium.org/8038/diff/8/207#newcode74
Line 74: void FreeTargetMap(Clipboard::TargetMap* data_map) {
Shouldn't this be a private method of the class?  You could even embed it in
Clear() if you want.

[Evan Stade, 2008-10-22 20:35:14]

wall of text warning

You are correct about the intent of the change. We have to be able to store
multiple formats on the clipboard simultaneously. The Chrome Clipboard class
accomplishes this by expecting that consecutive calls to Write* (WriteHTML,
WriteText, etc.) will leave the clipboard with multiple representations. This is
tested by the MultiFormat clipboard unit test. This required a rework of
clipboard_linux.cc because the gtk clipboard interface does not naturally
support consecutive calls building on one another. So we keep all data in a map,
and always pass the map when we are setting the gtk clipboard. Consecutive calls
to Write* will write to the map and set the gtk clipboard, then add to the same
map and set the gtk clipboard again. GTK thinks it is wiping out the clipboard
but we are actually keeping the previous data. This only works (and only needs
to work) with consecutive calls to Write* within chrome.

Consecutive calls to gtk_clipboard_set_with_data is not the most efficient thing
ever but there is no *significant* performance compromise because "setting" the
gtk clipboard consists of providing a user_data pointer and a callback, so data
is never written except when the actual paste action occurs.

I am the original author of this file. (
http://codereview.chromium.org/7602/show ) I didn't realize the multiformat
problem when I first wrote it, which was a distinct lack of foresight.

http://codereview.chromium.org/8038/diff/8/208
File base/clipboard.h (right):

http://codereview.chromium.org/8038/diff/8/208#newcode34
Line 34: typedef std::map<std::string, std::pair<uint8*, size_t> > TargetMap;
On 2008/10/22 17:33:48, Evan Martin wrote:
> This could live down next to where it's used.

It needs to be public. Should I still move it?

> Is there a reason for the value not to be a std::string?  We use those
elsewhere
> for strings.

It needs to represent arbitrary data, including images. Also we can't escape
dynamic allocation because we want more than one target to be able to map to a
single value.

> 
> This needs documentation as to what it holds (e.g. what is the key?)

Done.

http://codereview.chromium.org/8038/diff/8/207
File base/clipboard_linux.cc (right):

http://codereview.chromium.org/8038/diff/8/207#newcode19
Line 19: void GetData(GtkClipboard* clipboard,
On 2008/10/22 17:33:48, Evan Martin wrote:
> This (and all other file-local functions) need some doc comments.
> 

Done.

http://codereview.chromium.org/8038/diff/8/207#newcode47
Line 47: void SetGtkClipboard(GtkClipboard* clipboard,
On 2008/10/22 17:33:48, Evan Martin wrote:
> Shouldn't this be a private method on the class?

Done.

http://codereview.chromium.org/8038/diff/8/207#newcode74
Line 74: void FreeTargetMap(Clipboard::TargetMap* data_map) {
On 2008/10/22 17:33:48, Evan Martin wrote:
> Shouldn't this be a private method of the class?  You could even embed it in
> Clear() if you want.

I made it private, but left it separate from Clear() because in the future we
may want to call it directly from the destructor.

The one below this is also private now.

[Evan Martin, 2008-10-22 22:24:24]

Thanks for the explanation!  Filing the bug is also a nice touch.

http://3.latest.chromiumcodereview.appspot.com/8038/diff/9/12
File base/clipboard.h (right):

http://3.latest.chromiumcodereview.appspot.com/8038/diff/9/12#newcode143
Line 143: void SetGtkClipboard();
Documentation on these functions please.

http://3.latest.chromiumcodereview.appspot.com/8038/diff/9/12#newcode146
Line 146: std::pair<std::string, std::pair<uint8*, size_t> > data);
Does it make sense for this to just take these parameters separately?

E.g. InsertOrOverwrite(string key, char* data, size_t data_len).

That way callers don't need to create all these pairs, which seem a bit weird
from a caller perspective.  (It would help as self-documentation as well.)

Your call though.  :)

http://3.latest.chromiumcodereview.appspot.com/8038/diff/9/11
File base/clipboard_linux.cc (right):

http://3.latest.chromiumcodereview.appspot.com/8038/diff/9/11#newcode71
Line 71: strcpy(text_data, utf8_text.c_str());
Should this be memcpy()?  (Can you copy text with embedded nulls?)

http://3.latest.chromiumcodereview.appspot.com/8038/diff/9/11#newcode97
Line 97: static_cast<size_t>(html.size() + 1))));
Isn't .size() already a size_t?

http://3.latest.chromiumcodereview.appspot.com/8038/diff/9/11#newcode113
Line 113: int num = 0, i;
Can you declare these where they're used?  (E.g. "i" belongs in the "for" loop,
and num is down by get_targets.)

http://3.latest.chromiumcodereview.appspot.com/8038/diff/9/11#newcode116
Line 116: GtkSelectionData* data = gtk_clipboard_wait_for_contents(clipboard_,
gdk_atom_intern("TARGETS", false));
80 columns.

http://3.latest.chromiumcodereview.appspot.com/8038/diff/9/11#newcode190
Line 190: // private
This isn't necessary.  (Don't ask me why we annotate only static -- *shrug*.)

http://3.latest.chromiumcodereview.appspot.com/8038/diff/9/11#newcode221
Line 221: for (iter = clipboard_data_.begin(); iter != clipboard_data_.end();
iter++)
Our typical style for for loops is to put the iterator inside.  That way you
don't have to make up a new name for the second iterator below.  It does mean
you end up wrapping the loop, which is gross but seen all over the code:

for (Clipboard::TargetMap::iterator iter = clipboard_data_.begin();
     iter != clipboard_data_.end(); iter++) {
  // use iter
}

This style would let you also put "i" in the loop in the previous function.

[Evan Martin, 2008-10-22 22:25:18]

On 2008/10/22 20:35:14, Evan Stade wrote:
> wall of text warning

Oh, and this wall of text would be nice to put in the file somewhere.  Perhaps
where targetmap is displayed, for example.

[Peter Kasting, 2008-10-22 22:29:23]

On 2008/10/22 20:35:14, Evan Stade wrote:
> The Chrome Clipboard class
> accomplishes this by expecting that consecutive calls to Write* (WriteHTML,
> WriteText, etc.) will leave the clipboard with multiple representations. This
is
> tested by the MultiFormat clipboard unit test. This required a rework of
> clipboard_linux.cc because the gtk clipboard interface does not naturally
> support consecutive calls building on one another. So we keep all data in a
map,
> and always pass the map when we are setting the gtk clipboard. Consecutive
calls
> to Write* will write to the map and set the gtk clipboard, then add to the
same
> map and set the gtk clipboard again. GTK thinks it is wiping out the clipboard
> but we are actually keeping the previous data. This only works (and only needs
> to work) with consecutive calls to Write* within chrome.

Might it be clearer, from an API perspective, to have functions that can add
formats to the list of things on the clipboard, and then flush them all to the
clipboard?  This would map nicely to the Linux implementation (although I
suppose there'd be debate over whether Windows should do the same thing, or just
do what it's doing now... hmm).

I say this because even in Windows it's not necessarily obvious that WriteXXX()
followed by WriteYYY() actually preserves XXX.

[Evan Stade, 2008-10-22 23:40:22]

> Might it be clearer, from an API perspective, to have functions that can add
> formats to the list of things on the clipboard, and then flush them all to the
> clipboard?  This would map nicely to the Linux implementation (although I
> suppose there'd be debate over whether Windows should do the same thing, or
just
> do what it's doing now... hmm).
> 
> I say this because even in Windows it's not necessarily obvious that
WriteXXX()
> followed by WriteYYY() actually preserves XXX.

Yes, that would probably be a better API in windows and linux. (I don't know
about mac.) Windows does drop the clipboard lock between writes so you could
potentially have data that was half yours and half belonging to some other app
on the clipboard. If we switched to that API it would be very easy to change
this linux implementation. (move SetGtkClipboard() to Clipboard::FlushContents()
or whatever it's called)

[Evan Stade, 2008-10-22 23:47:16]

http://codereview.chromium.org/8038/diff/9/12
File base/clipboard.h (right):

http://codereview.chromium.org/8038/diff/9/12#newcode143
Line 143: void SetGtkClipboard();
On 2008/10/22 22:24:24, Evan Martin wrote:
> Documentation on these functions please.

Done.

http://codereview.chromium.org/8038/diff/9/12#newcode146
Line 146: std::pair<std::string, std::pair<uint8*, size_t> > data);
On 2008/10/22 22:24:24, Evan Martin wrote:
> Does it make sense for this to just take these parameters separately?
> 
> E.g. InsertOrOverwrite(string key, char* data, size_t data_len).
> 
> That way callers don't need to create all these pairs, which seem a bit weird
> from a caller perspective.  (It would help as self-documentation as well.)
> 
> Your call though.  :)

good idea.

http://codereview.chromium.org/8038/diff/9/11
File base/clipboard_linux.cc (right):

http://codereview.chromium.org/8038/diff/9/11#newcode71
Line 71: strcpy(text_data, utf8_text.c_str());
On 2008/10/22 22:24:24, Evan Martin wrote:
> Should this be memcpy()?  (Can you copy text with embedded nulls?)

I thought C++ strings couldn't have embedded nulls but looks like I'm wrong.
Changed.

http://codereview.chromium.org/8038/diff/9/11#newcode97
Line 97: static_cast<size_t>(html.size() + 1))));
On 2008/10/22 22:24:24, Evan Martin wrote:
> Isn't .size() already a size_t?

Done.

http://codereview.chromium.org/8038/diff/9/11#newcode113
Line 113: int num = 0, i;
On 2008/10/22 22:24:24, Evan Martin wrote:
> Can you declare these where they're used?  (E.g. "i" belongs in the "for"
loop,
> and num is down by get_targets.)

Done.

http://codereview.chromium.org/8038/diff/9/11#newcode116
Line 116: GtkSelectionData* data = gtk_clipboard_wait_for_contents(clipboard_,
gdk_atom_intern("TARGETS", false));
On 2008/10/22 22:24:24, Evan Martin wrote:
> 80 columns.

Done.

http://codereview.chromium.org/8038/diff/9/11#newcode190
Line 190: // private
On 2008/10/22 22:24:24, Evan Martin wrote:
> This isn't necessary.  (Don't ask me why we annotate only static -- *shrug*.)

Done.

http://codereview.chromium.org/8038/diff/9/11#newcode221
Line 221: for (iter = clipboard_data_.begin(); iter != clipboard_data_.end();
iter++)
On 2008/10/22 22:24:24, Evan Martin wrote:
> Our typical style for for loops is to put the iterator inside.  That way you
> don't have to make up a new name for the second iterator below.  It does mean
> you end up wrapping the loop, which is gross but seen all over the code:
> 
> for (Clipboard::TargetMap::iterator iter = clipboard_data_.begin();
>      iter != clipboard_data_.end(); iter++) {
>   // use iter
> }
> 
> This style would let you also put "i" in the loop in the previous function.

Done.

[Peter Kasting, 2008-10-22 23:48:31]

On 2008/10/22 23:40:22, Evan Stade wrote:
> Yes, that would probably be a better API in windows and linux.

So, how do we move towards that?  Is it most convenient for you to go ahead and
check this change in first, or does it make sense to change change the clipboard
APIs immediately?

[Evan Stade, 2008-10-23 00:28:30]

> Is it most convenient for you to go ahead and
> check this change in first?

yes, it would be arguably more coherent and definitely more convenient.

[Evan Martin, 2008-10-23 00:45:03]

LGTM