Linux: write images to clipboard....

Linux: write images to clipboard.

Writing a bitmap to the clipboard is a rather slow operation, as it involves piping it over IPC and then converting it to a PNG.


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

[Evan Stade, 2009-03-25 04:53:53]

http://codereview.chromium.org/42592/diff/1/6
File base/clipboard_linux.cc (right):

http://codereview.chromium.org/42592/diff/1/6#newcode166
Line 166: // gtk_clipboard_request_image() and might change in the future.
If you just have "image/bmp" on the clipboard then
gtk_clipboard_wait_is_image_available() returns TRUE but
gtk_clipboard_request_image() returns NULL. This is quite annoying and in my
opinion a bug.

[Evan Stade, 2009-03-25 04:54:20]

On 2009/03/25 04:53:53, estade wrote:
> http://codereview.chromium.org/42592/diff/1/6
> File base/clipboard_linux.cc (right):
> 
> http://codereview.chromium.org/42592/diff/1/6#newcode166
> Line 166: // gtk_clipboard_request_image() and might change in the future.
> If you just have "image/bmp" on the clipboard then
> gtk_clipboard_wait_is_image_available() returns TRUE but
> gtk_clipboard_request_image() returns NULL. This is quite annoying and in my
> opinion a bug.

[Evan Martin, 2009-03-25 15:44:30]

LGTM, comments follow

http://codereview.chromium.org/42592/diff/1/6
File base/clipboard_linux.cc (right):

http://codereview.chromium.org/42592/diff/1/6#newcode166
Line 166: // gtk_clipboard_request_image() and might change in the future.
On 2009/03/25 04:53:54, estade wrote:
> If you just have "image/bmp" on the clipboard then
> gtk_clipboard_wait_is_image_available() returns TRUE but
> gtk_clipboard_request_image() returns NULL. This is quite annoying and in my
> opinion a bug.

It'd be a good thing for the world to track this down and file the bug upstream.
 Got some spare time?  :)

http://codereview.chromium.org/42592/diff/1/6#newcode175
Line 175: DCHECK(rv);
Should we do something smart in release builds when this fails?  E.g. just skip
the following code.

http://codereview.chromium.org/42592/diff/1/4
File base/scoped_clipboard_writer.cc (left):

http://codereview.chromium.org/42592/diff/1/4#oldcode123
Line 123: #if defined(OS_WIN)
What about OS_MAC?  Should we still protect against that?

http://codereview.chromium.org/42592/diff/1/5
File base/scoped_clipboard_writer.h (left):

http://codereview.chromium.org/42592/diff/1/5#oldcode57
Line 57: // memcpy the pixels into GDI and the blit the bitmap to the clipboard.
Is this comment no longer true?

http://codereview.chromium.org/42592/diff/1/8
File chrome/browser/renderer_host/resource_message_filter.cc (right):

http://codereview.chromium.org/42592/diff/1/8#newcode506
Line 506: // on Windows and it only applies to bitmaps. (On other platforms,
bitmaps
s/other platforms/Linux/  maybe?

[Evan Stade, 2009-03-25 18:30:00]

http://codereview.chromium.org/42592/diff/1/6
File base/clipboard_linux.cc (right):

http://codereview.chromium.org/42592/diff/1/6#newcode166
Line 166: // gtk_clipboard_request_image() and might change in the future.
http://bugzilla.gnome.org/show_bug.cgi?id=576749

http://codereview.chromium.org/42592/diff/1/4
File base/scoped_clipboard_writer.cc (left):

http://codereview.chromium.org/42592/diff/1/4#oldcode123
Line 123: #if defined(OS_WIN)
On 2009/03/25 15:44:30, Evan Martin wrote:
> What about OS_MAC?  Should we still protect against that?

AFAIK this code is valid on the mac. (Although it just so happens that it's
never called.)

http://codereview.chromium.org/42592/diff/1/5
File base/scoped_clipboard_writer.h (left):

http://codereview.chromium.org/42592/diff/1/5#oldcode57
Line 57: // memcpy the pixels into GDI and the blit the bitmap to the clipboard.
On 2009/03/25 15:44:30, Evan Martin wrote:
> Is this comment no longer true?

Not really, because it doesn't apply to other platforms, and also it's the
*only* way to copy a bitmap to the clipboard (in this class). On windows it's
overridden in renderer_glue to use shared memory, in which case it's not slow.

http://codereview.chromium.org/42592/diff/1/8
File chrome/browser/renderer_host/resource_message_filter.cc (right):

http://codereview.chromium.org/42592/diff/1/8#newcode506
Line 506: // on Windows and it only applies to bitmaps. (On other platforms,
bitmaps
On 2009/03/25 15:44:30, Evan Martin wrote:
> s/other platforms/Linux/  maybe?

Done.