Add PluginMessageLoopProxy and use it for Host plugin UI thread.

Add PluginMessageLoopProxy and use it for Host plugin UI thread.

The new class will also be used in the client plugin for the main plugin thread.

BUG=None
TEST=Everything works.

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

[Sergey Ulanov, 2011-08-12 21:00:59]

-ajwong, +awong

[awong, 2011-08-12 21:15:26]

http://codereview.chromium.org/7635030/diff/1/remoting/base/plugin_message_lo...
File remoting/base/plugin_message_loop_proxy.cc (right):

http://codereview.chromium.org/7635030/diff/1/remoting/base/plugin_message_lo...
remoting/base/plugin_message_loop_proxy.cc:45: return PostTask(from_here, task);
I'm not certain we should even allow this API to be called without a CHECK
failure.

Given that we're just delegating to the plugin thread, and that servicing
synchronous Javascript is one of the main reasons we use nested message loops, I
don't think we can guarantee that the task can't be nested...

http://codereview.chromium.org/7635030/diff/1/remoting/base/plugin_message_lo...
remoting/base/plugin_message_loop_proxy.cc:70: base::Closure* task_in_heap = new
base::Closure(task);
task_in_heap -> task_on_heap

http://codereview.chromium.org/7635030/diff/1/remoting/host/plugin/host_plugi...
File remoting/host/plugin/host_plugin_utils.cc (right):

http://codereview.chromium.org/7635030/diff/1/remoting/host/plugin/host_plugi...
remoting/host/plugin/host_plugin_utils.cc:1: 
spurious newline.

http://codereview.chromium.org/7635030/diff/2001/remoting/base/plugin_message...
File remoting/base/plugin_message_loop_proxy.cc (right):

http://codereview.chromium.org/7635030/diff/2001/remoting/base/plugin_message...
remoting/base/plugin_message_loop_proxy.cc:91: bool
PluginMessageLoopProxy::BelongsToCurrentThread() {
lock?

[Sergey Ulanov, 2011-08-12 22:45:25]

http://codereview.chromium.org/7635030/diff/1/remoting/base/plugin_message_lo...
File remoting/base/plugin_message_loop_proxy.cc (right):

http://codereview.chromium.org/7635030/diff/1/remoting/base/plugin_message_lo...
remoting/base/plugin_message_loop_proxy.cc:45: return PostTask(from_here, task);
On 2011/08/12 21:15:26, awong wrote:
> I'm not certain we should even allow this API to be called without a CHECK
> failure.
> 
> Given that we're just delegating to the plugin thread, and that servicing
> synchronous Javascript is one of the main reasons we use nested message loops,
I
> don't think we can guarantee that the task can't be nested...

That is right, but tasks scheduled with this proxy will never call
MessageLoop::Run(), so for whoever uses this object nested and non-nested tasks
will look the same, i.e. one task posted with this proxy will never be nested in
another tasks posted using this proxy. Or am I missing some other subtle
differences between nested and non-nested tasks?

http://codereview.chromium.org/7635030/diff/1/remoting/base/plugin_message_lo...
remoting/base/plugin_message_loop_proxy.cc:70: base::Closure* task_in_heap = new
base::Closure(task);
On 2011/08/12 21:15:26, awong wrote:
> task_in_heap -> task_on_heap

Done.

http://codereview.chromium.org/7635030/diff/1/remoting/host/plugin/host_plugi...
File remoting/host/plugin/host_plugin_utils.cc (right):

http://codereview.chromium.org/7635030/diff/1/remoting/host/plugin/host_plugi...
remoting/host/plugin/host_plugin_utils.cc:1: 
On 2011/08/12 21:15:26, awong wrote:
> spurious newline.

Done.

http://codereview.chromium.org/7635030/diff/2001/remoting/base/plugin_message...
File remoting/base/plugin_message_loop_proxy.cc (right):

http://codereview.chromium.org/7635030/diff/2001/remoting/base/plugin_message...
remoting/base/plugin_message_loop_proxy.cc:91: bool
PluginMessageLoopProxy::BelongsToCurrentThread() {
On 2011/08/12 21:15:26, awong wrote:
> lock?

Done.

[Sergey Ulanov, 2011-08-15 18:48:26]

ping

[awong, 2011-08-15 20:17:53]

LGTM

http://codereview.chromium.org/7635030/diff/8001/remoting/client/client_conte...
File remoting/client/client_context.cc (right):

http://codereview.chromium.org/7635030/diff/8001/remoting/client/client_conte...
remoting/client/client_context.cc:12: namespace {
add newline.

[Wez, 2011-08-15 20:28:19]

It looks to me like PluginMessageLoopProxy is over-generalised; all it actually
does is collapse the MessageLoopProxy interface on to two methods provided by
the Delegate, and map between Closure and a callback+data signature.

This just seems to add another level of indirection; MessageLoopProxy requires
one interface to be implemented, and NPAPI provides a different one, and the
Delegate adds a third.  Why not flatten things and just have an NPAPI-specific
MessageLoopProxy implementation, and get rid of the extra Delegate interface?

There was talk of providing a simpler common interface, implemented by
MessageLoop, MessageLoopProxy, etc, for posting tasks safely to message-loops;
if MessageLoopProxy already had that "clean" interface then I suspect it would
look sufficiently like your Delegate that you wouldn't have added the extra
layer of indirection?

http://codereview.chromium.org/7635030/diff/8007/remoting/base/plugin_message...
File remoting/base/plugin_message_loop_proxy.h (right):

http://codereview.chromium.org/7635030/diff/8007/remoting/base/plugin_message...
remoting/base/plugin_message_loop_proxy.h:23: int delay_ms,
void(function)(void*), void* data) = 0;
Given that there needs to be a separate Delegate implemented specific to the
plugin environment anyway, why not make this interface take a const-ref Closure,
so that it is responsible for doing any necessary heapifying, etc?  That way the
environment-specific implementation can avoid unnecessary copies and bounces
through task-runners.

http://codereview.chromium.org/7635030/diff/8007/remoting/base/plugin_message...
remoting/base/plugin_message_loop_proxy.h:40: Task* task) OVERRIDE;
nit: Can we wrap the parameters for these three similarly to
PostNonNestableDelayedTask(), to make this block easier to read?

http://codereview.chromium.org/7635030/diff/8007/remoting/host/desktop_enviro...
File remoting/host/desktop_environment.cc (right):

http://codereview.chromium.org/7635030/diff/8007/remoting/host/desktop_enviro...
remoting/host/desktop_environment.cc:69: };
Isn't this entire class just a wannabe-MessageLoopProxy, in which case it can
disappear entirely now that |message_loop| is a MessageLoopProxy?

[Sergey Ulanov, 2011-08-15 22:39:52]

http://codereview.chromium.org/7635030/diff/8001/remoting/client/client_conte...
File remoting/client/client_context.cc (right):

http://codereview.chromium.org/7635030/diff/8001/remoting/client/client_conte...
remoting/client/client_context.cc:12: namespace {
On 2011/08/15 20:17:53, awong wrote:
> add newline.
This code is gone now.

http://codereview.chromium.org/7635030/diff/8007/remoting/base/plugin_message...
File remoting/base/plugin_message_loop_proxy.h (right):

http://codereview.chromium.org/7635030/diff/8007/remoting/base/plugin_message...
remoting/base/plugin_message_loop_proxy.h:23: int delay_ms,
void(function)(void*), void* data) = 0;
On 2011/08/15 20:28:19, Wez wrote:
> Given that there needs to be a separate Delegate implemented specific to the
> plugin environment anyway, why not make this interface take a const-ref
Closure,
> so that it is responsible for doing any necessary heapifying, etc?  That way
the
> environment-specific implementation can avoid unnecessary copies and bounces
> through task-runners.

We will need to move the closure to the heap anyway for both NPAPI and PPAPI. If
I pass void* here then it needs to be done only in one place instead of in two
delegate implementations. 
The goal is to make the delegate implementation for NPAPI and PPAPI as thin as
possible, and so the Delegate interface must be as close as possible to what is
provided by the plugin APIs.

http://codereview.chromium.org/7635030/diff/8007/remoting/base/plugin_message...
remoting/base/plugin_message_loop_proxy.h:40: Task* task) OVERRIDE;
On 2011/08/15 20:28:19, Wez wrote:
> nit: Can we wrap the parameters for these three similarly to
> PostNonNestableDelayedTask(), to make this block easier to read?

Done.

http://codereview.chromium.org/7635030/diff/8007/remoting/host/desktop_enviro...
File remoting/host/desktop_environment.cc (right):

http://codereview.chromium.org/7635030/diff/8007/remoting/host/desktop_enviro...
remoting/host/desktop_environment.cc:69: };
On 2011/08/15 20:28:19, Wez wrote:
> Isn't this entire class just a wannabe-MessageLoopProxy, in which case it can
> disappear entirely now that |message_loop| is a MessageLoopProxy?

Not really. This class is used to schedule tasks for the same object and then
Detach() is called by that object to cancel these tasks.  In other words
Detach() in MessageLoopProxy detaches the thread, but Deatch() in UIThreadProxy
detaches the target of the pending tasks.
I see how it may be confusing, I will clean it up later, added TODO for this.
Maybe Detach() here should be renamed to CancelPendingTasks() or something like
that.

[Sergey Ulanov, 2011-08-15 22:47:13]

On 2011/08/15 20:28:19, Wez wrote:
> It looks to me like PluginMessageLoopProxy is over-generalised; all it
actually
> does is collapse the MessageLoopProxy interface on to two methods provided by
> the Delegate, and map between Closure and a callback+data signature.
> 
> This just seems to add another level of indirection; MessageLoopProxy requires
> one interface to be implemented, and NPAPI provides a different one, and the
> Delegate adds a third.  Why not flatten things and just have an NPAPI-specific
> MessageLoopProxy implementation, and get rid of the extra Delegate interface?
> 
> There was talk of providing a simpler common interface, implemented by
> MessageLoop, MessageLoopProxy, etc, for posting tasks safely to message-loops;
> if MessageLoopProxy already had that "clean" interface then I suspect it would
> look sufficiently like your Delegate that you wouldn't have added the extra
> layer of indirection?

I'm trying to avoid implementing two separate MessageLoopProxy for PPAPI and
NPAPI. Delegate interface is as close as possible to what is provided in PPAPI
and NPAPI. For example implementation of this interface for PPAPI is less than
10 lines of code (not in this CL). If we had a cleaner version of
MessageLoopProxy its implementation would still be a bit more complicated than
the implementation of proxy delegate (it would have to deal with closures
instead of function pointers, etc).

[Wez, 2011-08-16 00:16:09]

On 2011/08/15 22:47:13, sergeyu wrote:
> On 2011/08/15 20:28:19, Wez wrote:
> > It looks to me like PluginMessageLoopProxy is over-generalised; all it
> actually
> > does is collapse the MessageLoopProxy interface on to two methods provided
by
> > the Delegate, and map between Closure and a callback+data signature.
> > 
> > This just seems to add another level of indirection; MessageLoopProxy
requires
> > one interface to be implemented, and NPAPI provides a different one, and the
> > Delegate adds a third.  Why not flatten things and just have an
NPAPI-specific
> > MessageLoopProxy implementation, and get rid of the extra Delegate
interface?
> > 
> > There was talk of providing a simpler common interface, implemented by
> > MessageLoop, MessageLoopProxy, etc, for posting tasks safely to
message-loops;
> > if MessageLoopProxy already had that "clean" interface then I suspect it
would
> > look sufficiently like your Delegate that you wouldn't have added the extra
> > layer of indirection?
> 
> I'm trying to avoid implementing two separate MessageLoopProxy for PPAPI and
> NPAPI. Delegate interface is as close as possible to what is provided in PPAPI
> and NPAPI. For example implementation of this interface for PPAPI is less than
> 10 lines of code (not in this CL). If we had a cleaner version of
> MessageLoopProxy its implementation would still be a bit more complicated than
> the implementation of proxy delegate (it would have to deal with closures
> instead of function pointers, etc).

My point is just that you end up with two Delegates for the two plugin types
(albeit with one of them being quite minimal) anyway.  The Closure to
function+data adaptation could be done in a general-purpose way, I think,
outside PluginMessageLoopProxy, in which case we'd be back to
PluginMessageLoopProxy being little more than an interface shim.

LGTM, though - we can argue about the details off-line. ;)
Please check whether we need to delete queued delayed tasks when tearing down
the plugin, though.

[awong, 2011-08-18 01:49:14]

rereading code because of Lambros's bug.

http://codereview.chromium.org/7635030/diff/17001/remoting/host/plugin/host_s...
File remoting/host/plugin/host_script_object.cc (left):

http://codereview.chromium.org/7635030/diff/17001/remoting/host/plugin/host_s...
remoting/host/plugin/host_script_object.cc:131: destructing_.Set();
Doesn't this still need to be around Sergey?