Add stubs and glue for routing a request for an image from the clipboard.

Add stubs and glue for routing a request for an image from the clipboard.

BUG=75237
TEST=none

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

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

[jam, 2011-03-08 04:44:01]

http://codereview.chromium.org/6625075/diff/7002/content/browser/renderer_hos...
File content/browser/renderer_host/render_message_filter.cc (right):

http://codereview.chromium.org/6625075/diff/7002/content/browser/renderer_hos...
content/browser/renderer_host/render_message_filter.cc:399:
IPC_MESSAGE_HANDLER_DELAY_REPLY(ViewHostMsg_ClipboardReadImage,
nit: it's about time that we move all these clipboard messages to their own
filter

http://codereview.chromium.org/6625075/diff/7002/content/browser/renderer_hos...
File content/browser/renderer_host/render_message_filter_gtk.cc (right):

http://codereview.chromium.org/6625075/diff/7002/content/browser/renderer_hos...
content/browser/renderer_host/render_message_filter_gtk.cc:135: void
RenderMessageFilter::DoOnClipboardReadHTML(ui::Clipboard::Buffer buffer,
i think this is supposed to be DoOnClipboardReadImage right?

http://codereview.chromium.org/6625075/diff/7002/content/browser/renderer_hos...
content/browser/renderer_host/render_message_filter_gtk.cc:237: this,
&RenderMessageFilter::DoOnClipboardReadImage, buffer,
you post a task from the IO thread to the UI thread, and the renderer is waiting
the reply to be sent.  this is equivalent to just dispatching the message on the
UI thread (i.e. this is what IPC::ChannelProxy does).

This will lead to hangs, and running a nested message loop in the renderer will
lead to crashes from reentrancy.

When Dimtry was in MTV, we discussed Darin's suggestion of how to do this
without these two problems.  Basically, when the clipboard is accessed for an
image object, a sync message to the IO thread is sent to return a blob id.  The
renderer then receives its reply.  A task is posted to the UI thread to start
reading the image data and associate it with the blob url.  Since blobs are
accessed asynchronously, the renderer main thread never blocks on the browser UI
thread.

[jam, 2011-03-08 04:54:35]

also, I think there's a simpler way.  Looking at RenderMessageFilter, it
currently dispatches clipboard messages on the IO thread on Windows and Mac, and
on the UI thread on X11.  There's a comment there that says:

" On non-X11 platforms, clipboard actions can be performed on the IO thread.
// On X11, since the clipboard is linked with GTK, we either have to do this
// with GTK on the UI thread, or with Xlib on the BACKGROUND_X11 thread. "

If the clipboard can be accessed on threads other than the UI thread on all
platforms, then there's no problem.  First, the clipboard messages should be
separated out into their own filter.  Then it becomes easy to have a
ClipboardMessageFilter::OverrideThreadForMessage that dispatches the clipboard
messages on BACKGROUND_X11 just for Linux (see FileUtilitiesMessageFilter for an
example).

[dcheng, 2011-03-08 05:08:55]

I wrote some of the original comments before your second reply. I plan on
uploading a separate patch first that implements the message filter change you
described.

http://codereview.chromium.org/6625075/diff/7002/content/browser/renderer_hos...
File content/browser/renderer_host/render_message_filter_gtk.cc (right):

http://codereview.chromium.org/6625075/diff/7002/content/browser/renderer_hos...
content/browser/renderer_host/render_message_filter_gtk.cc:135: void
RenderMessageFilter::DoOnClipboardReadHTML(ui::Clipboard::Buffer buffer,
On 2011/03/08 04:44:01, John Abd-El-Malek wrote:
> i think this is supposed to be DoOnClipboardReadImage right?

I messed up splitting up the stack of patches I had locally.

http://codereview.chromium.org/6625075/diff/7002/content/browser/renderer_hos...
content/browser/renderer_host/render_message_filter_gtk.cc:237: this,
&RenderMessageFilter::DoOnClipboardReadImage, buffer,
On 2011/03/08 04:44:01, John Abd-El-Malek wrote:
> you post a task from the IO thread to the UI thread, and the renderer is
waiting
> the reply to be sent.  this is equivalent to just dispatching the message on
the
> UI thread (i.e. this is what IPC::ChannelProxy does).
> 
> This will lead to hangs, and running a nested message loop in the renderer
will
> lead to crashes from reentrancy.
> 
> When Dimtry was in MTV, we discussed Darin's suggestion of how to do this
> without these two problems.  Basically, when the clipboard is accessed for an
> image object, a sync message to the IO thread is sent to return a blob id. 
The
> renderer then receives its reply.  A task is posted to the UI thread to start
> reading the image data and associate it with the blob url.  Since blobs are
> accessed asynchronously, the renderer main thread never blocks on the browser
UI
> thread.

For what it's worth, we'd ideally send these messages to the X11 background
thread. This is the way all clipboard messages are handled on GTK today though.

In the long term, it would be nice to do this with blob URLs, but I believe that
will require non-trivial changes to Blob (Blob always expects to know the size
of the data when you're constructing the Blob; unfortunately, when accessing the
clipboard, we won't know the size until we've actually gone and read the data).

[jam, 2011-03-08 05:23:40]

On Mon, Mar 7, 2011 at 9:08 PM, <dcheng@chromium.org> wrote:

> I wrote some of the original comments before your second reply. I plan on
> uploading a separate patch first that implements the message filter change
> you
> described.
>
>
>
>
>
http://codereview.chromium.org/6625075/diff/7002/content/browser/renderer_hos...
> File content/browser/renderer_host/render_message_filter_gtk.cc (right):
>
>
>
http://codereview.chromium.org/6625075/diff/7002/content/browser/renderer_hos...
> content/browser/renderer_host/render_message_filter_gtk.cc:135: void
> RenderMessageFilter::DoOnClipboardReadHTML(ui::Clipboard::Buffer buffer,
> On 2011/03/08 04:44:01, John Abd-El-Malek wrote:
>
>> i think this is supposed to be DoOnClipboardReadImage right?
>>
>
> I messed up splitting up the stack of patches I had locally.
>
>
>
>
http://codereview.chromium.org/6625075/diff/7002/content/browser/renderer_hos...
> content/browser/renderer_host/render_message_filter_gtk.cc:237: this,
> &RenderMessageFilter::DoOnClipboardReadImage, buffer,
> On 2011/03/08 04:44:01, John Abd-El-Malek wrote:
>
>> you post a task from the IO thread to the UI thread, and the renderer
>>
> is waiting
>
>> the reply to be sent.  this is equivalent to just dispatching the
>>
> message on the
>
>> UI thread (i.e. this is what IPC::ChannelProxy does).
>>
>
>  This will lead to hangs, and running a nested message loop in the
>>
> renderer will
>
>> lead to crashes from reentrancy.
>>
>
>  When Dimtry was in MTV, we discussed Darin's suggestion of how to do
>>
> this
>
>> without these two problems.  Basically, when the clipboard is accessed
>>
> for an
>
>> image object, a sync message to the IO thread is sent to return a blob
>>
> id.  The
>
>> renderer then receives its reply.  A task is posted to the UI thread
>>
> to start
>
>> reading the image data and associate it with the blob url.  Since
>>
> blobs are
>
>> accessed asynchronously, the renderer main thread never blocks on the
>>
> browser UI
>
>> thread.
>>
>
> For what it's worth, we'd ideally send these messages to the X11
> background thread. This is the way all clipboard messages are handled on
> GTK today though.
>
> In the long term, it would be nice to do this with blob URLs, but I
> believe that will require non-trivial changes to Blob (Blob always
> expects to know the size of the data when you're constructing the Blob;
> unfortunately, when accessing the clipboard, we won't know the size
> until we've actually gone and read the data).


So why not send this to the X11 thread?  Has anyone looked into it?  The old
clipboard code was written before we had things like BrowserThread and
BrowserMessageFilter, so it was a lot harder back then to go back and forth
between threads safely.

We shouldn't be adding code that can cause deadlocks.


>
> http://codereview.chromium.org/6625075/
>

[dcheng, 2011-03-08 05:24:53]

On 2011/03/08 05:23:40, John Abd-El-Malek wrote:
> On Mon, Mar 7, 2011 at 9:08 PM, <mailto:dcheng@chromium.org> wrote:
> 
> > I wrote some of the original comments before your second reply. I plan on
> > uploading a separate patch first that implements the message filter change
> > you
> > described.
> >
> >
> >
> >
> >
>
http://codereview.chromium.org/6625075/diff/7002/content/browser/renderer_hos...
> > File content/browser/renderer_host/render_message_filter_gtk.cc (right):
> >
> >
> >
>
http://codereview.chromium.org/6625075/diff/7002/content/browser/renderer_hos...
> > content/browser/renderer_host/render_message_filter_gtk.cc:135: void
> > RenderMessageFilter::DoOnClipboardReadHTML(ui::Clipboard::Buffer buffer,
> > On 2011/03/08 04:44:01, John Abd-El-Malek wrote:
> >
> >> i think this is supposed to be DoOnClipboardReadImage right?
> >>
> >
> > I messed up splitting up the stack of patches I had locally.
> >
> >
> >
> >
>
http://codereview.chromium.org/6625075/diff/7002/content/browser/renderer_hos...
> > content/browser/renderer_host/render_message_filter_gtk.cc:237: this,
> > &RenderMessageFilter::DoOnClipboardReadImage, buffer,
> > On 2011/03/08 04:44:01, John Abd-El-Malek wrote:
> >
> >> you post a task from the IO thread to the UI thread, and the renderer
> >>
> > is waiting
> >
> >> the reply to be sent.  this is equivalent to just dispatching the
> >>
> > message on the
> >
> >> UI thread (i.e. this is what IPC::ChannelProxy does).
> >>
> >
> >  This will lead to hangs, and running a nested message loop in the
> >>
> > renderer will
> >
> >> lead to crashes from reentrancy.
> >>
> >
> >  When Dimtry was in MTV, we discussed Darin's suggestion of how to do
> >>
> > this
> >
> >> without these two problems.  Basically, when the clipboard is accessed
> >>
> > for an
> >
> >> image object, a sync message to the IO thread is sent to return a blob
> >>
> > id.  The
> >
> >> renderer then receives its reply.  A task is posted to the UI thread
> >>
> > to start
> >
> >> reading the image data and associate it with the blob url.  Since
> >>
> > blobs are
> >
> >> accessed asynchronously, the renderer main thread never blocks on the
> >>
> > browser UI
> >
> >> thread.
> >>
> >
> > For what it's worth, we'd ideally send these messages to the X11
> > background thread. This is the way all clipboard messages are handled on
> > GTK today though.
> >
> > In the long term, it would be nice to do this with blob URLs, but I
> > believe that will require non-trivial changes to Blob (Blob always
> > expects to know the size of the data when you're constructing the Blob;
> > unfortunately, when accessing the clipboard, we won't know the size
> > until we've actually gone and read the data).
> 
> 
> So why not send this to the X11 thread?  Has anyone looked into it?  The old
> clipboard code was written before we had things like BrowserThread and
> BrowserMessageFilter, so it was a lot harder back then to go back and forth
> between threads safely.
> 
> We shouldn't be adding code that can cause deadlocks.
> 
> 
> >
> > http://codereview.chromium.org/6625075/
> >

LIke I said, I'm working on the message filter change now =)

[dcheng, 2011-03-12 03:44:21]

I've moved the stubs for ClipboardHostMsg_ReadImage and its handlers into this
change. To help with the incremental changes for Linux, I've explicitly
overridden the message to be handled in the X11 background thread as I believe
it can be done without changing all the code at once.

[jam, 2011-03-14 20:53:40]

On 2011/03/12 03:44:21, dcheng wrote:
> I've moved the stubs for ClipboardHostMsg_ReadImage and its handlers into this
> change. To help with the incremental changes for Linux, I've explicitly
> overridden the message to be handled in the X11 background thread as I believe
> it can be done without changing all the code at once.

I'm curious, what makes ReadImage safe to use on the X11 thread using the gtk
functions, as opposed to the other clipboard functions?  Someone more familiar
than me with Linux should chime in.

[dcheng, 2011-03-14 21:03:20]

On 2011/03/14 20:53:40, John Abd-El-Malek wrote:
> On 2011/03/12 03:44:21, dcheng wrote:
> > I've moved the stubs for ClipboardHostMsg_ReadImage and its handlers into
this
> > change. To help with the incremental changes for Linux, I've explicitly
> > overridden the message to be handled in the X11 background thread as I
believe
> > it can be done without changing all the code at once.
> 
> I'm curious, what makes ReadImage safe to use on the X11 thread using the gtk
> functions, as opposed to the other clipboard functions?  Someone more familiar
> than me with Linux should chime in.

Nothing at the moment. I plan to implement it using Xlib though, which we can
use from the X11 background thread. The rest of the code in clipboard_linux.cc
uses GTK, which is unusable from the background thread. Clipboard::ReadImage
would be a proof-of-concept to test Xlib support on the background thread before
migrating the rest of the Clipboard class.

[jam, 2011-03-14 22:03:36]

ah i see, ok lgtm

On Mon, Mar 14, 2011 at 2:03 PM, <dcheng@chromium.org> wrote:

> On 2011/03/14 20:53:40, John Abd-El-Malek wrote:
>
>> On 2011/03/12 03:44:21, dcheng wrote:
>> > I've moved the stubs for ClipboardHostMsg_ReadImage and its handlers
>> into
>>
> this
>
>> > change. To help with the incremental changes for Linux, I've explicitly
>> > overridden the message to be handled in the X11 background thread as I
>>
> believe
>
>> > it can be done without changing all the code at once.
>>
>
>  I'm curious, what makes ReadImage safe to use on the X11 thread using the
>> gtk
>> functions, as opposed to the other clipboard functions?  Someone more
>> familiar
>> than me with Linux should chime in.
>>
>
> Nothing at the moment. I plan to implement it using Xlib though, which we
> can
> use from the X11 background thread. The rest of the code in
> clipboard_linux.cc
> uses GTK, which is unusable from the background thread.
> Clipboard::ReadImage
> would be a proof-of-concept to test Xlib support on the background thread
> before
> migrating the rest of the Clipboard class.
>
>
> http://codereview.chromium.org/6625075/
>

[dcheng, 2011-03-15 03:39:46]

Updated the change with one more missing file.