Copy selection to x clipboard.

Copy selection to x clipboard.


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

[Evan Stade, 2009-03-28 01:53:48]

This works, except for one case: where the user selects text and then deselects
and then tries to paste. You'd normally expect that deselecting text wouldn't
clear the x clipboard, but in our case it does because we never ask for the text
until the user tries to paste it, and by then the selection is gone. I don't see
an easy way around this.

(The FIXMEs are placeholders for where I need to add comments)

[Evan Stade, 2009-03-30 07:43:56]

on second thought.. is there some reason we can't let the renderer copy to the
clipboard directly? I thought we didn't use gtk anywhere in the renderer but I
just noticed I was wrong.

[darin (slow to review), 2009-03-30 16:18:29]

Is there a reason why we can't reuse all of the existing clipboard plumbing?

[darin (slow to review), 2009-03-30 16:21:36]

To elaborate:  it seems to me that text selection is just another reason to
generate clipboard writes.  so i would have expected instead to see some
webkit-side changes to trigger additional calls to WebClipboard.  i don't
understand why we need any new IPC.

[brettw, 2009-03-30 16:25:34]

Drive by

[darin (slow to review), 2009-03-30 16:26:37]

Alternatively, if you can't reuse WebClipboard, then it seems like the
SelectionChanged event should just carry with it the "selection text" that needs
to be written to the clipboard.  Then, in RenderWidgetHostViewGtk, you would
already have all of the data you need to put on the clipboard before you begin
talking to X.  In other words, I don't think modifying RenderWidgetHelper for
this purpose is correct.

[Evan Stade, 2009-03-30 17:11:10]

On 2009/03/30 16:26:37, darin wrote:
> Alternatively, if you can't reuse WebClipboard, then it seems like the
> SelectionChanged event should just carry with it the "selection text" that
needs
> to be written to the clipboard.  Then, in RenderWidgetHostViewGtk, you would
> already have all of the data you need to put on the clipboard before you begin
> talking to X.  In other words, I don't think modifying RenderWidgetHelper for
> this purpose is correct.

The reason for this approach is that we want to minimize the data transmitted
over IPC. Selection changed events are generated every time the selection
changes during a drag (not just at the end of the drag), so if a user
drag-selects a document that's O(n^2) data transmitted over IPC, which seems
especially bad considering that the user may not even intend to paste it
elsewhere.

[darin (slow to review), 2009-03-30 17:19:44]

So, you suspect that the time it takes for X to call you back is significantly
greater than the round-trip-time between browser UI thread and renderer webkit
thread?

How about this:

1) When selection changes, send event to RenderViewHost to inform that the
selection changed.
2) If RenderViewHost is not already waiting for an X selection callback, then
RenderViewHost can send a request to the RenderView to get the current selection
text.
3) When the RenderViewHost receives the selection text, he can then kick off the
request to X to write out the current selection.
4) When X calls back to the RenderViewHost (actually the
RenderWidgetHostViewGtk), he can give the selection text that he received in
step #3.

This is far simpler than trying to introduce blocking IPCs.  Blocking IPCs are
very bad for performance and should only be used when absolutely necessary.

[agl, 2009-03-30 17:27:02]

http://codereview.chromium.org/55052/diff/1001/1005
File chrome/browser/renderer_host/render_process_host.h (right):

http://codereview.chromium.org/55052/diff/1001/1005#newcode128
Line 128: // FIXME
Seems that this should be fixed :)

http://codereview.chromium.org/55052/diff/1001/1014
File chrome/browser/renderer_host/render_widget_helper.cc (right):

http://codereview.chromium.org/55052/diff/1001/1014#newcode16
Line 16: const int kMaxSelectionTextDelayMS = 40;
just use static here. Not worth an extra four lines.

http://codereview.chromium.org/55052/diff/1001/1014#newcode55
Line 55: paint_event_(CreateEvent(NULL, FALSE /* auto-reset */, FALSE, NULL)),
This #ifdef can probably be removed. One should be able to init paint_event_ and
selection_event_ with the same arguments on all platforms.

http://codereview.chromium.org/55052/diff/1001/1014#newcode128
Line 128: selection_event_.TimedWait(max_sleep_time);
Seems to me that a WaitableEvent is possible a little heavyweight here. Looks
like it should just be a condition variable, but this isn't the patch to change
it all.

http://codereview.chromium.org/55052/diff/1001/1004
File chrome/browser/renderer_host/render_widget_host_view.h (right):

http://codereview.chromium.org/55052/diff/1001/1004#newcode115
Line 115: // FIXME
(ping)

[Evan Stade, 2009-03-30 17:29:47]

On 2009/03/30 17:19:44, darin wrote:
> So, you suspect that the time it takes for X to call you back is significantly
> greater than the round-trip-time between browser UI thread and renderer webkit
> thread?
> 
> How about this:
> 
> 1) When selection changes, send event to RenderViewHost to inform that the
> selection changed.
> 2) If RenderViewHost is not already waiting for an X selection callback, then
> RenderViewHost can send a request to the RenderView to get the current
selection
> text.
> 3) When the RenderViewHost receives the selection text, he can then kick off
the
> request to X to write out the current selection.
> 4) When X calls back to the RenderViewHost (actually the
> RenderWidgetHostViewGtk), he can give the selection text that he received in
> step #3.
> 
> This is far simpler than trying to introduce blocking IPCs.  Blocking IPCs are
> very bad for performance and should only be used when absolutely necessary.

I meant I was worried about IPC between renderer and browser. The GetData
callback is only called by gtk when another application (typically the user
pasting) asks for the data on the clipboard.

[darin (slow to review), 2009-03-30 17:45:50]

Yes, I know... but you have to tell X that there is clipboard data, so you might
as well have copied the data to the browser process at that point.

[darin (slow to review), 2009-03-30 17:49:58]

If you really want to wait to copy the data until it is requested, then you
should probably do so using SyncChannel and SendWithTimeout.  The
RenderWidgetHelper mechanism used for painting is to support the case of an IPC
message that can either have an optional blocking sender.  This text selection
is not an example of that.  Instead, it is a straight synchronous IPC, which we
have nice support for.

[Evan Stade, 2009-03-30 23:05:09]

redesigned the patch. GTK imposes the requirement that we be able to supply the
data for the selection syncrhonously, but with a (sort of hacky) combination of
GtkSelection functions we can circumvent this.

We don't want to skip using Gtk/Gdk completely because we don't want to have to
re-implement the ICCCM conformance code and on-destroy handlers and other
behind-the-scenes magic (see _gtk_selection_request in gtkselection.c, for one).

This works with the exception that when you select some text then close the tab
without ever pasting, you will lose the text and the clipboard will be blank.
This could be avoided by

a) keeping the selection text as a browser property, but I'm not sure it's worth
it since SetSelectionText would have to be plumbed through the
RenderViewHostDelegate and the TabContentsDelegate, and RequestSelectionText
would be even more difficult. Or,

b) when the tab is closing while we hold the clipboard, request the selection
text (with a timeout). Paste this text to the clipboard without a callback. Then
allow the tab to close. I'm not sure where the proper place is during shutdown
to do this.

In any case I don't think this is a particularly common use case.

[darin (slow to review), 2009-03-30 23:50:57]

Chrome is like a window manager for renderers, so losing the text selection when
you close a tab seems totally reasonable to me.  I wouldn't expect my selection
to be retained by the X windows manager when I close my xterm ;-)

[Evan Stade, 2009-03-31 20:15:21]

ping

[darin (slow to review), 2009-04-02 19:44:47]

LGTM

[tony, 2009-04-02 20:58:56]

LG, some nits below.

http://codereview.chromium.org/55052/diff/1042/3018
File chrome/browser/renderer_host/render_widget_host_view_gtk.cc (right):

http://codereview.chromium.org/55052/diff/1042/3018#newcode85
Line 85: g_object_set_data(G_OBJECT(widget), "render-widget-host-view-gtk",
host_view);
Nit: 80 cols. Also, can you add a comment why we need this?

http://codereview.chromium.org/55052/diff/1042/3018#newcode168
Line 168: host_view->set_event_selection(event);
I see, this is why it's a friend class?  I guess I would rather these methods be
public on the RenderWidgetHostViewGtk.

http://codereview.chromium.org/55052/diff/1042/3022
File chrome/renderer/render_view.h (right):

http://codereview.chromium.org/55052/diff/1042/3022#newcode821
Line 821: // The currently selected text (currently only tracked on linux).
Nit: Might be nice to elaborate why this is only tracked on linux.

[Evan Stade, 2009-04-02 21:28:11]

thanks for speedy review!

http://codereview.chromium.org/55052/diff/1042/3018
File chrome/browser/renderer_host/render_widget_host_view_gtk.cc (right):

http://codereview.chromium.org/55052/diff/1042/3018#newcode85
Line 85: g_object_set_data(G_OBJECT(widget), "render-widget-host-view-gtk",
host_view);
On 2009/04/02 20:58:56, tony wrote:
> Nit: 80 cols. Also, can you add a comment why we need this?

Done.

http://codereview.chromium.org/55052/diff/1042/3018#newcode168
Line 168: host_view->set_event_selection(event);
On 2009/04/02 20:58:56, tony wrote:
> I see, this is why it's a friend class?  I guess I would rather these methods
be
> public on the RenderWidgetHostViewGtk.

It's not just these methods. RenderWidgetHostViewGtkWidget frequently needs to
access private member variables of RenderWidgetHostViewGtk, and we keep adding
more and more public setters and getters, but I don't think it really makes
sense because other classes (e.g. WebContentsViewGtk) shouldn't have access to
those variables.

I can change it back though if you feel strongly

http://codereview.chromium.org/55052/diff/1042/3022
File chrome/renderer/render_view.h (right):

http://codereview.chromium.org/55052/diff/1042/3022#newcode821
Line 821: // The currently selected text (currently only tracked on linux).
On 2009/04/02 20:58:56, tony wrote:
> Nit: Might be nice to elaborate why this is only tracked on linux.

Done.

[tony, 2009-04-02 21:55:07]

On 2009/04/02 21:28:11, estade wrote:
> It's not just these methods. RenderWidgetHostViewGtkWidget frequently needs to
> access private member variables of RenderWidgetHostViewGtk, and we keep adding
> more and more public setters and getters, but I don't think it really makes
> sense because other classes (e.g. WebContentsViewGtk) shouldn't have access to
> those variables.
> 
> I can change it back though if you feel strongly

I don't feel strongly about it, but I think the friend class means our design
doesn't fit quite right.  It would be nice to re-evaluate in the future and see
if we can refactor such that we don't need the friend class.