[Chromoting] Let the Windows IT2Me host send clipboard events to the client.

[Chromoting] Let the Windows IT2Me host send clipboard events to the client.

This CL lets ClipboardWin send clipboard events to ClientSession.

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

[simonmorris, 2012-05-22 22:11:38]

The approach taken here feels very non-standard. Maybe it would be better to
make ClientSession ref-counted, or support a weak pointer? Or maybe there's some
other approach?

PTAL

[Wez, 2012-05-23 00:38:54]

On 2012/05/22 22:11:38, simonmorris wrote:
> The approach taken here feels very non-standard. Maybe it would be better to
> make ClientSession ref-counted, or support a weak pointer? Or maybe there's
some
> other approach?
> 
> PTAL

Can you provide a brief summary of the architecture, ideally in the design doc,
to guide the review, please?

[simonmorris, 2012-05-23 15:59:54]

On 2012/05/23 00:38:54, Wez wrote:
> On 2012/05/22 22:11:38, simonmorris wrote:
> > The approach taken here feels very non-standard. Maybe it would be better to
> > make ClientSession ref-counted, or support a weak pointer? Or maybe there's
> some
> > other approach?
> > 
> > PTAL
> 
> Can you provide a brief summary of the architecture, ideally in the design
doc,
> to guide the review, please?

This CL lets ClipboardWin send clipboard events to ClientSession (I've updated
the CL description to say so).

The awkward part is making sure that ClipboardWin won't send those events after
the ClientSession is destroyed.

[simonmorris, 2012-05-25 15:36:19]

On 2012/05/23 15:59:54, simonmorris wrote:
> > > The approach taken here feels very non-standard. Maybe it would be better
to
> > > make ClientSession ref-counted, or support a weak pointer? Or maybe
there's
> > > some other approach?
> > > 

Maybe FrameConsumerProxy's approach is the right way to go here?
Let me know if you think so, and I'll update the CL accordingly.

[simonmorris, 2012-05-26 00:10:56]

On 2012/05/25 15:36:19, simonmorris wrote:
> On 2012/05/23 15:59:54, simonmorris wrote:
> > > > The approach taken here feels very non-standard. Maybe it would be
better
> to
> > > > make ClientSession ref-counted, or support a weak pointer? Or maybe
> there's
> > > > some other approach?
> > > > 
> 
> Maybe FrameConsumerProxy's approach is the right way to go here?
> Let me know if you think so, and I'll update the CL accordingly.

ping

[Wez, 2012-05-26 02:49:21]

On 2012/05/25 15:36:19, simonmorris wrote:
> On 2012/05/23 15:59:54, simonmorris wrote:
> > > > The approach taken here feels very non-standard. Maybe it would be
better
> to
> > > > make ClientSession ref-counted, or support a weak pointer? Or maybe
> there's
> > > > some other approach?
> > > > 
> 
> Maybe FrameConsumerProxy's approach is the right way to go here?
> Let me know if you think so, and I'll update the CL accordingly.

FYI, I've just uploaded http://codereview.chromium.org/10454040/ which replaces
uses of ScopedThreadProxy with MessageLoopProxy+WeakPtr.

[simonmorris, 2012-05-27 03:20:12]

On 2012/05/26 02:49:21, Wez wrote:
> On 2012/05/25 15:36:19, simonmorris wrote:
> > On 2012/05/23 15:59:54, simonmorris wrote:
> > > > > The approach taken here feels very non-standard. Maybe it would be
> better
> > to
> > > > > make ClientSession ref-counted, or support a weak pointer? Or maybe
> > there's
> > > > > some other approach?
> > > > > 
> > 
> > Maybe FrameConsumerProxy's approach is the right way to go here?
> > Let me know if you think so, and I'll update the CL accordingly.
> 
> FYI, I've just uploaded http://codereview.chromium.org/10454040/ which
replaces
> uses of ScopedThreadProxy with MessageLoopProxy+WeakPtr.

OK. FrameConsumerProxy uses MessageLoopProxy+WeakPtr, so I'll take that as a
"yes" to my question, and rework this CL accordingly.

[simonmorris, 2012-05-29 22:14:55]

ptal

[Wez, 2012-05-30 01:15:42]

http://codereview.chromium.org/10413060/diff/9001/remoting/host/client_sessio...
File remoting/host/client_session.cc (right):

http://codereview.chromium.org/10413060/diff/9001/remoting/host/client_sessio...
remoting/host/client_session.cc:174: return client_clipboard;
Since you're creating a new ClipboardProxy every time ClientClipboard gets
called, there's no real benefit in it being refcounted.

http://codereview.chromium.org/10413060/diff/9001/remoting/host/client_session.h
File remoting/host/client_session.h (right):

http://codereview.chromium.org/10413060/diff/9001/remoting/host/client_sessio...
remoting/host/client_session.h:127: scoped_refptr<protocol::ClipboardProxy>
ClientClipboard();
Since this creates a ClipboardProxy it should have a name like
CreateClipboardProxy, but see comments in implementation.

http://codereview.chromium.org/10413060/diff/9001/remoting/host/client_sessio...
remoting/host/client_session.h:162: protocol::ClipboardWeakPtr
client_clipboard_;
I think you can achieve the same effect without the ClipboardWeakPtr class by
creating a base::WeakPtrFactory<ClipboardStub> and passing the target
ClipboardStub to the constructor.

http://codereview.chromium.org/10413060/diff/9001/remoting/host/desktop_envir...
File remoting/host/desktop_environment.h (right):

http://codereview.chromium.org/10413060/diff/9001/remoting/host/desktop_envir...
remoting/host/desktop_environment.h:50: const
scoped_refptr<protocol::ClipboardProxy>& client_clipboard);
If you switch ClientSession::ClientClipboard to return a scoped_ptr<> then this
can become OnSessionStarted(scoped_ptr<ClipboardStub> client_clipboard)

http://codereview.chromium.org/10413060/diff/9001/remoting/protocol/clipboard...
File remoting/protocol/clipboard_proxy.h (right):

http://codereview.chromium.org/10413060/diff/9001/remoting/protocol/clipboard...
remoting/protocol/clipboard_proxy.h:22: class ClipboardProxy
ClipboardThreadProxy?

http://codereview.chromium.org/10413060/diff/9001/remoting/protocol/clipboard...
remoting/protocol/clipboard_proxy.h:36: void Attach(const
base::WeakPtr<ClipboardStub>& clipboard_stub);
Why not have |clipboard_stub| as a constructor parameter?

[simonmorris, 2012-05-30 16:54:05]

http://codereview.chromium.org/10413060/diff/9001/remoting/host/client_sessio...
File remoting/host/client_session.cc (right):

http://codereview.chromium.org/10413060/diff/9001/remoting/host/client_sessio...
remoting/host/client_session.cc:174: return client_clipboard;
On 2012/05/30 01:15:43, Wez wrote:
> Since you're creating a new ClipboardProxy every time ClientClipboard gets
> called, there's no real benefit in it being refcounted.

I want ClipboardThreadProxy to be refcounted so that it can safely post messages
to itself on another thread.

http://codereview.chromium.org/10413060/diff/9001/remoting/host/client_session.h
File remoting/host/client_session.h (right):

http://codereview.chromium.org/10413060/diff/9001/remoting/host/client_sessio...
remoting/host/client_session.h:127: scoped_refptr<protocol::ClipboardProxy>
ClientClipboard();
On 2012/05/30 01:15:43, Wez wrote:
> Since this creates a ClipboardProxy it should have a name like
> CreateClipboardProxy, but see comments in implementation.

Done.

http://codereview.chromium.org/10413060/diff/9001/remoting/host/client_sessio...
remoting/host/client_session.h:162: protocol::ClipboardWeakPtr
client_clipboard_;
On 2012/05/30 01:15:43, Wez wrote:
> I think you can achieve the same effect without the ClipboardWeakPtr class by
> creating a base::WeakPtrFactory<ClipboardStub> and passing the target
> ClipboardStub to the constructor.

Done.

http://codereview.chromium.org/10413060/diff/9001/remoting/protocol/clipboard...
File remoting/protocol/clipboard_proxy.h (right):

http://codereview.chromium.org/10413060/diff/9001/remoting/protocol/clipboard...
remoting/protocol/clipboard_proxy.h:22: class ClipboardProxy
On 2012/05/30 01:15:43, Wez wrote:
> ClipboardThreadProxy?

Done.

http://codereview.chromium.org/10413060/diff/9001/remoting/protocol/clipboard...
remoting/protocol/clipboard_proxy.h:36: void Attach(const
base::WeakPtr<ClipboardStub>& clipboard_stub);
On 2012/05/30 01:15:43, Wez wrote:
> Why not have |clipboard_stub| as a constructor parameter?

Done.

[Wez, 2012-05-30 17:07:46]

http://codereview.chromium.org/10413060/diff/9001/remoting/host/client_sessio...
File remoting/host/client_session.cc (right):

http://codereview.chromium.org/10413060/diff/9001/remoting/host/client_sessio...
remoting/host/client_session.cc:174: return client_clipboard;
On 2012/05/30 16:54:06, simonmorris wrote:
> On 2012/05/30 01:15:43, Wez wrote:
> > Since you're creating a new ClipboardProxy every time ClientClipboard gets
> > called, there's no real benefit in it being refcounted.
> 
> I want ClipboardThreadProxy to be refcounted so that it can safely post
messages
> to itself on another thread.

You can do that if instead of storing a WeakPtr to the target ClipboardStub in
the ClipboardThreadProxy, you store a WeakPtr to the ClipboardThreadProxy
itself.  Any tasks it posts to itself will be dropped by the task queue if they
are processed after the proxy('s WeakPtrFactory) has been deleted.

(The caller must then guarantee that the ClipboardStub outlive the
ClipboardThreadProxy, but that should be easy enough to arrange.)

[simonmorris, 2012-05-30 23:18:26]

ptal

http://codereview.chromium.org/10413060/diff/9001/remoting/host/client_sessio...
File remoting/host/client_session.cc (right):

http://codereview.chromium.org/10413060/diff/9001/remoting/host/client_sessio...
remoting/host/client_session.cc:174: return client_clipboard;
On 2012/05/30 17:07:47, Wez wrote:
> On 2012/05/30 16:54:06, simonmorris wrote:
> > On 2012/05/30 01:15:43, Wez wrote:
> > > Since you're creating a new ClipboardProxy every time ClientClipboard gets
> > > called, there's no real benefit in it being refcounted.
> > 
> > I want ClipboardThreadProxy to be refcounted so that it can safely post
> messages
> > to itself on another thread.
> 
> You can do that if instead of storing a WeakPtr to the target ClipboardStub in
> the ClipboardThreadProxy, you store a WeakPtr to the ClipboardThreadProxy
> itself.  Any tasks it posts to itself will be dropped by the task queue if
they
> are processed after the proxy('s WeakPtrFactory) has been deleted.
> 
> (The caller must then guarantee that the ClipboardStub outlive the
> ClipboardThreadProxy, but that should be easy enough to arrange.)

As discussed offline, ClipboardThreadProxy does need to outlive ClipboardStub,
but WeakPtr can be used instead of ref-counting anyway.

[Wez, 2012-05-30 23:58:02]

http://codereview.chromium.org/10413060/diff/16001/remoting/host/client_sessi...
File remoting/host/client_session.h (right):

http://codereview.chromium.org/10413060/diff/16001/remoting/host/client_sessi...
remoting/host/client_session.h:160: // Factory for weak pointers to the client
clipboard.
nit: ... to the client clipboard stub.

http://codereview.chromium.org/10413060/diff/16001/remoting/host/client_sessi...
remoting/host/client_session.h:161: base::WeakPtrFactory<ClipboardStub>
client_clipboard_factory_;
nit: Consider clarifying that this _must_ appear after ClipboardEchoFilter, so
it won't outlive it?

http://codereview.chromium.org/10413060/diff/16001/remoting/host/event_execut...
File remoting/host/event_executor_win.cc (right):

http://codereview.chromium.org/10413060/diff/16001/remoting/host/event_execut...
remoting/host/event_executor_win.cc:117:
base::Passed(client_clipboard.Pass())));
I think you want base::Passed(&client_clipboard) here.

http://codereview.chromium.org/10413060/diff/16001/remoting/protocol/clipboar...
File remoting/protocol/clipboard_thread_proxy.cc (right):

http://codereview.chromium.org/10413060/diff/16001/remoting/protocol/clipboar...
remoting/protocol/clipboard_thread_proxy.cc:22: if
(!clipboard_stub_message_loop_->BelongsToCurrentThread()) {
Since you're no longer trampolining the same method to a different thread, you
can dispense with the thread check, and with the InjectClipboardEventStatic call
below.

http://codereview.chromium.org/10413060/diff/16001/remoting/protocol/clipboar...
remoting/protocol/clipboard_thread_proxy.cc:36: if (clipboard_stub) {
nit: clipboard_stub.get(), for clarity?

http://codereview.chromium.org/10413060/diff/16001/remoting/protocol/clipboar...
remoting/protocol/clipboard_thread_proxy.cc:41:
ClipboardThreadProxy::~ClipboardThreadProxy() {
nit: Move this above ctor to match the header.

http://codereview.chromium.org/10413060/diff/16001/remoting/protocol/clipboar...
File remoting/protocol/clipboard_thread_proxy.h (right):

http://codereview.chromium.org/10413060/diff/16001/remoting/protocol/clipboar...
remoting/protocol/clipboard_thread_proxy.h:23: ~ClipboardThreadProxy();
virtual

http://codereview.chromium.org/10413060/diff/16001/remoting/protocol/clipboar...
remoting/protocol/clipboard_thread_proxy.h:29:
scoped_refptr<base::MessageLoopProxy> clipboard_stub_message_loop);
nit: We're moving to SingleThreadTaskRunner, which MessageLoopProxy implements.

Looking at InjectClipboardEvent(), you don't even need a SingleThreadTaskRunner,
so you might consider just scoped_refptr<TaskRunner>.

http://codereview.chromium.org/10413060/diff/16001/remoting/protocol/clipboar...
remoting/protocol/clipboard_thread_proxy.h:38: const ClipboardEvent& event);
Make this private, since it's an internal implementation detail?

http://codereview.chromium.org/10413060/diff/16001/remoting/protocol/clipboar...
remoting/protocol/clipboard_thread_proxy.h:42:
scoped_refptr<base::MessageLoopProxy> clipboard_stub_message_loop_;
nit: These could be target_stub_ and target_message_loop_/target_task_runner_ to
reduce name lengths. :)

[simonmorris, 2012-05-31 00:21:18]

http://codereview.chromium.org/10413060/diff/16001/remoting/host/client_sessi...
File remoting/host/client_session.h (right):

http://codereview.chromium.org/10413060/diff/16001/remoting/host/client_sessi...
remoting/host/client_session.h:160: // Factory for weak pointers to the client
clipboard.
On 2012/05/30 23:58:02, Wez wrote:
> nit: ... to the client clipboard stub.

Done.

http://codereview.chromium.org/10413060/diff/16001/remoting/host/client_sessi...
remoting/host/client_session.h:161: base::WeakPtrFactory<ClipboardStub>
client_clipboard_factory_;
On 2012/05/30 23:58:02, Wez wrote:
> nit: Consider clarifying that this _must_ appear after ClipboardEchoFilter, so
> it won't outlive it?

Done.

http://codereview.chromium.org/10413060/diff/16001/remoting/host/event_execut...
File remoting/host/event_executor_win.cc (right):

http://codereview.chromium.org/10413060/diff/16001/remoting/host/event_execut...
remoting/host/event_executor_win.cc:117:
base::Passed(client_clipboard.Pass())));
On 2012/05/30 23:58:02, Wez wrote:
> I think you want base::Passed(&client_clipboard) here.

Done.

http://codereview.chromium.org/10413060/diff/16001/remoting/protocol/clipboar...
File remoting/protocol/clipboard_thread_proxy.cc (right):

http://codereview.chromium.org/10413060/diff/16001/remoting/protocol/clipboar...
remoting/protocol/clipboard_thread_proxy.cc:22: if
(!clipboard_stub_message_loop_->BelongsToCurrentThread()) {
On 2012/05/30 23:58:02, Wez wrote:
> Since you're no longer trampolining the same method to a different thread, you
> can dispense with the thread check, and with the InjectClipboardEventStatic
call
> below.

Done.

http://codereview.chromium.org/10413060/diff/16001/remoting/protocol/clipboar...
remoting/protocol/clipboard_thread_proxy.cc:36: if (clipboard_stub) {
On 2012/05/30 23:58:02, Wez wrote:
> nit: clipboard_stub.get(), for clarity?

Done.

http://codereview.chromium.org/10413060/diff/16001/remoting/protocol/clipboar...
remoting/protocol/clipboard_thread_proxy.cc:41:
ClipboardThreadProxy::~ClipboardThreadProxy() {
On 2012/05/30 23:58:02, Wez wrote:
> nit: Move this above ctor to match the header.

Done.

http://codereview.chromium.org/10413060/diff/16001/remoting/protocol/clipboar...
File remoting/protocol/clipboard_thread_proxy.h (right):

http://codereview.chromium.org/10413060/diff/16001/remoting/protocol/clipboar...
remoting/protocol/clipboard_thread_proxy.h:23: ~ClipboardThreadProxy();
On 2012/05/30 23:58:02, Wez wrote:
> virtual

Done.

http://codereview.chromium.org/10413060/diff/16001/remoting/protocol/clipboar...
remoting/protocol/clipboard_thread_proxy.h:29:
scoped_refptr<base::MessageLoopProxy> clipboard_stub_message_loop);
On 2012/05/30 23:58:02, Wez wrote:
> nit: We're moving to SingleThreadTaskRunner, which MessageLoopProxy
implements.
> 
> Looking at InjectClipboardEvent(), you don't even need a
SingleThreadTaskRunner,
> so you might consider just scoped_refptr<TaskRunner>.

Done.

http://codereview.chromium.org/10413060/diff/16001/remoting/protocol/clipboar...
remoting/protocol/clipboard_thread_proxy.h:38: const ClipboardEvent& event);
On 2012/05/30 23:58:02, Wez wrote:
> Make this private, since it's an internal implementation detail?

Done.

http://codereview.chromium.org/10413060/diff/16001/remoting/protocol/clipboar...
remoting/protocol/clipboard_thread_proxy.h:42:
scoped_refptr<base::MessageLoopProxy> clipboard_stub_message_loop_;
On 2012/05/30 23:58:02, Wez wrote:
> nit: These could be target_stub_ and target_message_loop_/target_task_runner_
to
> reduce name lengths. :)

I'd prefer to keep these names, for consistency with the other filter classes.

[Wez, 2012-05-31 00:23:53]

LGTM - looks great!

[commit-bot: I haz the power, 2012-05-31 15:44:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/simonmorris@chromium.org/10413060/15004

[commit-bot: I haz the power, 2012-05-31 16:00:04]

Try job failure for 10413060-15004 (retry) on mac_rel for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-05-31 16:20:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/simonmorris@chromium.org/10413060/19024

[commit-bot: I haz the power, 2012-05-31 16:31:01]

Try job failure for 10413060-19024 (retry) on mac_rel for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-05-31 17:25:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/simonmorris@chromium.org/10413060/18025

[commit-bot: I haz the power, 2012-05-31 20:26:36]

Change committed as 139854