Add the plumbing that will carry a clipboard item from a chromoting client to a host.

Add the plumbing that will carry a clipboard item from a chromoting client to a host.

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

[simonmorris, 2012-03-09 00:04:36]

PTAL

[Sergey Ulanov, 2012-03-09 00:23:40]

http://codereview.chromium.org/9646013/diff/1/remoting/protocol/clipboard_stub.h
File remoting/protocol/clipboard_stub.h (right):

http://codereview.chromium.org/9646013/diff/1/remoting/protocol/clipboard_stu...
remoting/protocol/clipboard_stub.h:18: class ClipboardStub {
Do we need separate stub for it? Can it be part of the InputStub? As it
currently works each stub is responsible for one direction of a channel. If we
add ClipboardStub it means that client to host messages sent over the event
channel are split between two interfaces (InputStub and ClipboardStub)

http://codereview.chromium.org/9646013/diff/1/remoting/protocol/clipboard_stu...
remoting/protocol/clipboard_stub.h:23: virtual void InjectClipboardEvent(const
ClipboardEvent& event) = 0;
It's not clear whether this goes from the client to the host or from host to
client?

http://codereview.chromium.org/9646013/diff/1/remoting/protocol/host_event_di...
File remoting/protocol/host_event_dispatcher.h (right):

http://codereview.chromium.org/9646013/diff/1/remoting/protocol/host_event_di...
remoting/protocol/host_event_dispatcher.h:30: void
set_clipboard_stub(ClipboardStub* clipboard_stub) {
Would it be better to use control channel for clipboard. It will always be
reliable, while we may change event channel to be unreliable.

[Wez, 2012-03-09 00:34:00]

http://codereview.chromium.org/9646013/diff/1/remoting/proto/event.proto
File remoting/proto/event.proto (right):

http://codereview.chromium.org/9646013/diff/1/remoting/proto/event.proto#newc...
remoting/proto/event.proto:52: // Defines an event that sends data from one
peer's clipboard into the other
nit: ... sends clipboard data between peers. ?

http://codereview.chromium.org/9646013/diff/1/remoting/proto/event.proto#newc...
remoting/proto/event.proto:58: DATA_TYPE_TEXT_UTF8 = 0;
If we're going to go down the route of coding specific types in to the protocol
then wouldn't we be better off defining a ClipboardEvent message for each type,
with type-specific fields, so that this becomes TextClipboardEvent?

Alternatively, we could have a single ClipboardEvent containing just MIME-type
and data fields, and avoid the need to code specific types into the protocol.

[simonmorris, 2012-03-09 00:34:14]

http://codereview.chromium.org/9646013/diff/1/remoting/protocol/clipboard_stub.h
File remoting/protocol/clipboard_stub.h (right):

http://codereview.chromium.org/9646013/diff/1/remoting/protocol/clipboard_stu...
remoting/protocol/clipboard_stub.h:18: class ClipboardStub {
On 2012/03/09 00:23:41, sergeyu wrote:
> Do we need separate stub for it? Can it be part of the InputStub? As it
> currently works each stub is responsible for one direction of a channel. If we
> add ClipboardStub it means that client to host messages sent over the event
> channel are split between two interfaces (InputStub and ClipboardStub)

We'll need to send clipboards from host to client as well (but not in this CL).
So the host will have a ClipboardStub, as well as the client, but it shouldn't
have an InputStub.

http://codereview.chromium.org/9646013/diff/1/remoting/protocol/clipboard_stu...
remoting/protocol/clipboard_stub.h:23: virtual void InjectClipboardEvent(const
ClipboardEvent& event) = 0;
On 2012/03/09 00:23:41, sergeyu wrote:
> It's not clear whether this goes from the client to the host or from host to
> client?

It can go in either direction (but not in this CL).

http://codereview.chromium.org/9646013/diff/1/remoting/protocol/host_event_di...
File remoting/protocol/host_event_dispatcher.h (right):

http://codereview.chromium.org/9646013/diff/1/remoting/protocol/host_event_di...
remoting/protocol/host_event_dispatcher.h:30: void
set_clipboard_stub(ClipboardStub* clipboard_stub) {
On 2012/03/09 00:23:41, sergeyu wrote:
> Would it be better to use control channel for clipboard. It will always be
> reliable, while we may change event channel to be unreliable.

I think it's better to send clipboard events over the same channel as key and
mouse events, if only for consistency.

If the event channel becomes unreliable, presumably the key events (at least)
will move to a reliable channel, so the clipboard events could move at the same
time.

[Wez, 2012-03-09 00:44:07]

http://codereview.chromium.org/9646013/diff/1/remoting/proto/event.proto
File remoting/proto/event.proto (right):

http://codereview.chromium.org/9646013/diff/1/remoting/proto/event.proto#newc...
remoting/proto/event.proto:54: message ClipboardEvent {
We probably want clipboard data on its own channel, so that it interferes less
with input.

http://codereview.chromium.org/9646013/diff/1/remoting/protocol/clipboard_stub.h
File remoting/protocol/clipboard_stub.h (right):

http://codereview.chromium.org/9646013/diff/1/remoting/protocol/clipboard_stu...
remoting/protocol/clipboard_stub.h:18: class ClipboardStub {
On 2012/03/09 00:34:14, simonmorris wrote:
> On 2012/03/09 00:23:41, sergeyu wrote:
> > Do we need separate stub for it? Can it be part of the InputStub? As it
> > currently works each stub is responsible for one direction of a channel. If
we
> > add ClipboardStub it means that client to host messages sent over the event
> > channel are split between two interfaces (InputStub and ClipboardStub)
> 
> We'll need to send clipboards from host to client as well (but not in this
CL).
> So the host will have a ClipboardStub, as well as the client, but it shouldn't
> have an InputStub.

Yes.  Plus, we don't really want the clipboard data passing via the event
channel anyway, since large transfers will then delay input events.

http://codereview.chromium.org/9646013/diff/1/remoting/protocol/host_event_di...
File remoting/protocol/host_event_dispatcher.h (right):

http://codereview.chromium.org/9646013/diff/1/remoting/protocol/host_event_di...
remoting/protocol/host_event_dispatcher.h:30: void
set_clipboard_stub(ClipboardStub* clipboard_stub) {
On 2012/03/09 00:34:14, simonmorris wrote:
> On 2012/03/09 00:23:41, sergeyu wrote:
> > Would it be better to use control channel for clipboard. It will always be
> > reliable, while we may change event channel to be unreliable.
> 
> I think it's better to send clipboard events over the same channel as key and
> mouse events, if only for consistency.
> 
> If the event channel becomes unreliable, presumably the key events (at least)
> will move to a reliable channel, so the clipboard events could move at the
same
> time.

We'll have a keyboard-event-specific reliability protocol when we move to an
unreliable channel, so to get reliable clipboard transfer without building a
specialised reliability layer for it, we'll need to put clipboard data on a
separate channel, and used PseudoTcp for that.

[simonmorris, 2012-03-09 00:44:46]

http://codereview.chromium.org/9646013/diff/1/remoting/proto/event.proto
File remoting/proto/event.proto (right):

http://codereview.chromium.org/9646013/diff/1/remoting/proto/event.proto#newc...
remoting/proto/event.proto:52: // Defines an event that sends data from one
peer's clipboard into the other
On 2012/03/09 00:34:00, Wez wrote:
> nit: ... sends clipboard data between peers. ?

Done.

http://codereview.chromium.org/9646013/diff/1/remoting/proto/event.proto#newc...
remoting/proto/event.proto:58: DATA_TYPE_TEXT_UTF8 = 0;
On 2012/03/09 00:34:00, Wez wrote:
> If we're going to go down the route of coding specific types in to the
protocol
> then wouldn't we be better off defining a ClipboardEvent message for each
type,
> with type-specific fields, so that this becomes TextClipboardEvent?
> 

If there are useful type-specific fields for some types, that would make sense.
Do you have examples in mind?

> Alternatively, we could have a single ClipboardEvent containing just MIME-type
> and data fields, and avoid the need to code specific types into the protocol.

That works too, if MIME-type is always unambiguous enough for us. It just means
there are strings in our code instead of enums, but I don't feel strongly about
it.

[garykac, 2012-03-09 00:51:19]

http://codereview.chromium.org/9646013/diff/1026/remoting/proto/event.proto
File remoting/proto/event.proto (right):

http://codereview.chromium.org/9646013/diff/1026/remoting/proto/event.proto#n...
remoting/proto/event.proto:57: DATA_TYPE_TEXT_UTF8 = 0;
Is the UTF8 suffix redundant or is the plan to support different text encodings
(SJIS, KOI8, Big5)?  Or is this just to allow alternate Unicode formats (like
UCS2)?

Supporting different text encodings doesn't seem worthwhile nor does supporting
UCS2 as an option (Japanese text requires 50% more storage with UTF8 vs UCS2,
but this isn't critical).

I would recommend using only UTF8 for text and naming this simply DATA_TYPE_TEXT
(with a comment noting that it's UTF8 encoded).

[simonmorris, 2012-03-09 00:53:24]

http://codereview.chromium.org/9646013/diff/1026/remoting/proto/event.proto
File remoting/proto/event.proto (right):

http://codereview.chromium.org/9646013/diff/1026/remoting/proto/event.proto#n...
remoting/proto/event.proto:57: DATA_TYPE_TEXT_UTF8 = 0;
On 2012/03/09 00:51:19, garykac wrote:
> Is the UTF8 suffix redundant or is the plan to support different text
encodings
> (SJIS, KOI8, Big5)?  Or is this just to allow alternate Unicode formats (like
> UCS2)?
> 
> Supporting different text encodings doesn't seem worthwhile nor does
supporting
> UCS2 as an option (Japanese text requires 50% more storage with UTF8 vs UCS2,
> but this isn't critical).
> 
> I would recommend using only UTF8 for text and naming this simply
DATA_TYPE_TEXT
> (with a comment noting that it's UTF8 encoded).

OK - I agree that we only want UTF-8.

[Wez, 2012-03-09 01:12:40]

http://codereview.chromium.org/9646013/diff/1/remoting/proto/event.proto
File remoting/proto/event.proto (right):

http://codereview.chromium.org/9646013/diff/1/remoting/proto/event.proto#newc...
remoting/proto/event.proto:58: DATA_TYPE_TEXT_UTF8 = 0;
On 2012/03/09 00:44:47, simonmorris wrote:
> On 2012/03/09 00:34:00, Wez wrote:
> > If we're going to go down the route of coding specific types in to the
> protocol
> > then wouldn't we be better off defining a ClipboardEvent message for each
> type,
> > with type-specific fields, so that this becomes TextClipboardEvent?
> > 
> 
> If there are useful type-specific fields for some types, that would make
sense.
> Do you have examples in mind?

Text, images and files would likely need different treatment on-the-wire.  Files
may be very large, so we'd want to break them into chunks, provide file
meta-data, etc.  Images may be on the local clipboard in several formats that we
can offer on the remote clipboard, and then just transfer whichever format the
remote end requests.

> > Alternatively, we could have a single ClipboardEvent containing just
MIME-type
> > and data fields, and avoid the need to code specific types into the
protocol.
> 
> That works too, if MIME-type is always unambiguous enough for us. It just
means
> there are strings in our code instead of enums, but I don't feel strongly
about
> it.

The client is going to see clipboard data classified by MIME-type anyway, so I
think it'd make sense to take advantage of that existing cross-platform scheme
rather than mapping from it to something Chromoting specific.

Given that hosts might have clipboard data in several formats, advertising
multiple MIME-types and letting the peer choose which to fetch is something we
should consider (although for simple text just pushing the data immediately with
a single type is probably best).

[simonmorris, 2012-03-10 01:16:47]

On 2012/03/09 00:44:07, Wez wrote:
> http://codereview.chromium.org/9646013/diff/1/remoting/proto/event.proto
> File remoting/proto/event.proto (right):
> 
>
http://codereview.chromium.org/9646013/diff/1/remoting/proto/event.proto#newc...
> remoting/proto/event.proto:54: message ClipboardEvent {
> We probably want clipboard data on its own channel, so that it interferes less
> with input.
> 
>
http://codereview.chromium.org/9646013/diff/1/remoting/protocol/clipboard_stub.h
> File remoting/protocol/clipboard_stub.h (right):
> 
>
http://codereview.chromium.org/9646013/diff/1/remoting/protocol/clipboard_stu...
> remoting/protocol/clipboard_stub.h:18: class ClipboardStub {
> On 2012/03/09 00:34:14, simonmorris wrote:
> > On 2012/03/09 00:23:41, sergeyu wrote:
> > > Do we need separate stub for it? Can it be part of the InputStub? As it
> > > currently works each stub is responsible for one direction of a channel.
If
> we
> > > add ClipboardStub it means that client to host messages sent over the
event
> > > channel are split between two interfaces (InputStub and ClipboardStub)
> > 
> > We'll need to send clipboards from host to client as well (but not in this
> CL).
> > So the host will have a ClipboardStub, as well as the client, but it
shouldn't
> > have an InputStub.
> 
> Yes.  Plus, we don't really want the clipboard data passing via the event
> channel anyway, since large transfers will then delay input events.
> 
>
http://codereview.chromium.org/9646013/diff/1/remoting/protocol/host_event_di...
> File remoting/protocol/host_event_dispatcher.h (right):
> 
>
http://codereview.chromium.org/9646013/diff/1/remoting/protocol/host_event_di...
> remoting/protocol/host_event_dispatcher.h:30: void
> set_clipboard_stub(ClipboardStub* clipboard_stub) {
> On 2012/03/09 00:34:14, simonmorris wrote:
> > On 2012/03/09 00:23:41, sergeyu wrote:
> > > Would it be better to use control channel for clipboard. It will always be
> > > reliable, while we may change event channel to be unreliable.
> > 
> > I think it's better to send clipboard events over the same channel as key
and
> > mouse events, if only for consistency.
> > 
> > If the event channel becomes unreliable, presumably the key events (at
least)
> > will move to a reliable channel, so the clipboard events could move at the
> same
> > time.
> 
> We'll have a keyboard-event-specific reliability protocol when we move to an
> unreliable channel, so to get reliable clipboard transfer without building a
> specialised reliability layer for it, we'll need to put clipboard data on a
> separate channel, and used PseudoTcp for that.

Adding a channel for clipboard data appears to break backwards compatibility.
Session::OnRemoteCandidates rejects a channel with a name it doesn't recognise.

[Sergey Ulanov, 2012-03-12 19:00:01]

On 2012/03/10 01:16:47, simonmorris wrote:
> > We'll have a keyboard-event-specific reliability protocol when we move to an
> > unreliable channel, so to get reliable clipboard transfer without building a
> > specialised reliability layer for it, we'll need to put clipboard data on a
> > separate channel, and used PseudoTcp for that.
> 
> Adding a channel for clipboard data appears to break backwards compatibility.
> Session::OnRemoteCandidates rejects a channel with a name it doesn't
recognise.

We don't use libjingle's Session class anymore (on both ends of the connection).
remoting::protocol::JingleSession just ignores candidates for unknown channels.
But that doesn't mean that we don't need to worry about backward compatibility
when adding new channels. If we add a new channel then it needs to be added to
the session config, so that we the peers can negotiate when the channel is not
supported.

In any case, I don't agree that adding a new channel specifically for clipboard
is the right thing to do. I would prefer reusing control channel for that if
possible.

[simonmorris, 2012-03-12 19:14:44]

On 2012/03/12 19:00:01, sergeyu wrote:
> On 2012/03/10 01:16:47, simonmorris wrote:
> > > We'll have a keyboard-event-specific reliability protocol when we move to
an
> > > unreliable channel, so to get reliable clipboard transfer without building
a
> > > specialised reliability layer for it, we'll need to put clipboard data on
a
> > > separate channel, and used PseudoTcp for that.
> > 
> > Adding a channel for clipboard data appears to break backwards
compatibility.
> > Session::OnRemoteCandidates rejects a channel with a name it doesn't
> recognise.
> 
> We don't use libjingle's Session class anymore (on both ends of the
connection).

The error I got when I tried to connect a client/host with a clipboard channel
to an old host/client seemed to come from Session::OnRemoteCandidates. If there
are clients/hosts in the field that reject connections with unknown channel
names, then I don't think we can add a channel without making new clients/hosts
handle that rejection - perhaps by retrying without the extra channel?

> remoting::protocol::JingleSession just ignores candidates for unknown
channels.
> But that doesn't mean that we don't need to worry about backward compatibility
> when adding new channels. If we add a new channel then it needs to be added to
> the session config, so that we the peers can negotiate when the channel is not
> supported.
> 
> In any case, I don't agree that adding a new channel specifically for
clipboard
> is the right thing to do. I would prefer reusing control channel for that if
> possible.

The simplest plan might be to put the clipboard events into an existing channel
(I don't mind whether it's the event or control channel). If and when we make
changes that let us add channels without breaking backward compatibility, we can
stop using those channels to send clipboard events. That way, for now, we only
change protobufs, which handle backward compatibility well.

[simonmorris, 2012-03-12 23:51:12]

The clipboard events are now on the control channel.

PTAL

[simonmorris, 2012-03-14 01:17:22]

ping

[simonmorris, 2012-03-14 18:47:39]

ping

[Sergey Ulanov, 2012-03-14 20:27:07]

LGTM with some nits.

http://codereview.chromium.org/9646013/diff/9002/remoting/host/client_session.cc
File remoting/host/client_session.cc (right):

http://codereview.chromium.org/9646013/diff/9002/remoting/host/client_session...
remoting/host/client_session.cc:25: using protocol::ClipboardEvent;
There is only one place when this is used, so maybe remove this and just specify
the namespace directly.

http://codereview.chromium.org/9646013/diff/9002/remoting/host/client_session...
File remoting/host/client_session_unittest.cc (right):

http://codereview.chromium.org/9646013/diff/9002/remoting/host/client_session...
remoting/host/client_session_unittest.cc:13: using protocol::MockClipboardStub;
Same here. There is only one place where this is needed, so better not use
"using" here.

http://codereview.chromium.org/9646013/diff/9002/remoting/host/event_executor...
File remoting/host/event_executor_linux.cc (right):

http://codereview.chromium.org/9646013/diff/9002/remoting/host/event_executor...
remoting/host/event_executor_linux.cc:23: using protocol::ClipboardEvent;
nit: no need to have it here.

http://codereview.chromium.org/9646013/diff/9002/remoting/host/event_executor...
remoting/host/event_executor_linux.cc:38: virtual void InjectKeyEvent(const
KeyEvent& event) OVERRIDE;
nit: better to separate implementation of ClipboardStub from InputStub, and add
comments, e.g.
 // InputStub interface.

http://codereview.chromium.org/9646013/diff/9002/remoting/host/event_executor...
File remoting/host/event_executor_mac.cc (right):

http://codereview.chromium.org/9646013/diff/9002/remoting/host/event_executor...
remoting/host/event_executor_mac.cc:22: using protocol::ClipboardEvent;
nit: Don't need this.

http://codereview.chromium.org/9646013/diff/9002/remoting/host/event_executor...
File remoting/host/event_executor_win.cc (right):

http://codereview.chromium.org/9646013/diff/9002/remoting/host/event_executor...
remoting/host/event_executor_win.cc:18: using protocol::ClipboardEvent;
nit: don't need this.

http://codereview.chromium.org/9646013/diff/9002/remoting/proto/event.proto
File remoting/proto/event.proto (right):

http://codereview.chromium.org/9646013/diff/9002/remoting/proto/event.proto#n...
remoting/proto/event.proto:56: required string mime_type = 1;
It's usually better to mark all fields in protobuf messages as optional.
Otherwise it would be more difficult to make them optional in the future if we
need to.

[Wez, 2012-03-14 20:40:56]

http://codereview.chromium.org/9646013/diff/9002/remoting/proto/event.proto
File remoting/proto/event.proto (right):

http://codereview.chromium.org/9646013/diff/9002/remoting/proto/event.proto#n...
remoting/proto/event.proto:53: message ClipboardEvent {
I thought that we had agreed on TextClipboardEvent containing just a UTF-8
encoded string for now, with the intent being to move to a
multi-MIME-type-capable multi-channel protocol in future?

http://codereview.chromium.org/9646013/diff/9002/remoting/protocol/host_contr...
File remoting/protocol/host_control_dispatcher.h (right):

http://codereview.chromium.org/9646013/diff/9002/remoting/protocol/host_contr...
remoting/protocol/host_control_dispatcher.h:35: // message. Doesn't take
ownership of |clipboard_stub|. It must outlive
nit: ". It must" -> ", which must"

[Wez, 2012-03-14 20:45:39]

lgtm

[simonmorris, 2012-03-14 21:20:21]

http://codereview.chromium.org/9646013/diff/9002/remoting/host/client_session.cc
File remoting/host/client_session.cc (right):

http://codereview.chromium.org/9646013/diff/9002/remoting/host/client_session...
remoting/host/client_session.cc:25: using protocol::ClipboardEvent;
On 2012/03/14 20:27:07, sergeyu wrote:
> There is only one place when this is used, so maybe remove this and just
specify
> the namespace directly.

Done.

http://codereview.chromium.org/9646013/diff/9002/remoting/host/client_session...
File remoting/host/client_session_unittest.cc (right):

http://codereview.chromium.org/9646013/diff/9002/remoting/host/client_session...
remoting/host/client_session_unittest.cc:13: using protocol::MockClipboardStub;
On 2012/03/14 20:27:07, sergeyu wrote:
> Same here. There is only one place where this is needed, so better not use
> "using" here.

Done.

http://codereview.chromium.org/9646013/diff/9002/remoting/host/event_executor...
File remoting/host/event_executor_linux.cc (right):

http://codereview.chromium.org/9646013/diff/9002/remoting/host/event_executor...
remoting/host/event_executor_linux.cc:23: using protocol::ClipboardEvent;
On 2012/03/14 20:27:07, sergeyu wrote:
> nit: no need to have it here.

Done.

http://codereview.chromium.org/9646013/diff/9002/remoting/host/event_executor...
remoting/host/event_executor_linux.cc:38: virtual void InjectKeyEvent(const
KeyEvent& event) OVERRIDE;
On 2012/03/14 20:27:07, sergeyu wrote:
> nit: better to separate implementation of ClipboardStub from InputStub, and
add
> comments, e.g.
>  // InputStub interface.

Done.

http://codereview.chromium.org/9646013/diff/9002/remoting/host/event_executor...
File remoting/host/event_executor_mac.cc (right):

http://codereview.chromium.org/9646013/diff/9002/remoting/host/event_executor...
remoting/host/event_executor_mac.cc:22: using protocol::ClipboardEvent;
On 2012/03/14 20:27:07, sergeyu wrote:
> nit: Don't need this.

Done.

http://codereview.chromium.org/9646013/diff/9002/remoting/host/event_executor...
File remoting/host/event_executor_win.cc (right):

http://codereview.chromium.org/9646013/diff/9002/remoting/host/event_executor...
remoting/host/event_executor_win.cc:18: using protocol::ClipboardEvent;
On 2012/03/14 20:27:07, sergeyu wrote:
> nit: don't need this.

Done.

http://codereview.chromium.org/9646013/diff/9002/remoting/proto/event.proto
File remoting/proto/event.proto (right):

http://codereview.chromium.org/9646013/diff/9002/remoting/proto/event.proto#n...
remoting/proto/event.proto:53: message ClipboardEvent {
On 2012/03/14 20:40:56, Wez wrote:
> I thought that we had agreed on TextClipboardEvent containing just a UTF-8
> encoded string for now, with the intent being to move to a
> multi-MIME-type-capable multi-channel protocol in future?

We may be able to use this message as it stands in such a protocol. Keeping the
MIME type in the message costs very little, and keeps that option open.

http://codereview.chromium.org/9646013/diff/9002/remoting/proto/event.proto#n...
remoting/proto/event.proto:56: required string mime_type = 1;
On 2012/03/14 20:27:07, sergeyu wrote:
> It's usually better to mark all fields in protobuf messages as optional.
> Otherwise it would be more difficult to make them optional in the future if we
> need to.

Done.

http://codereview.chromium.org/9646013/diff/9002/remoting/protocol/host_contr...
File remoting/protocol/host_control_dispatcher.h (right):

http://codereview.chromium.org/9646013/diff/9002/remoting/protocol/host_contr...
remoting/protocol/host_control_dispatcher.h:35: // message. Doesn't take
ownership of |clipboard_stub|. It must outlive
On 2012/03/14 20:40:56, Wez wrote:
> nit: ". It must" -> ", which must"

Done.

[simonmorris, 2012-03-15 00:36:20]

wez@, ptal at the ClipboardStub/InputStub refactoring in this patch set.

[Wez, 2012-03-15 01:14:44]

This looks good but while InputStub derives from ClipboardStub I don't think
there's much value in passing the two interfaces separately to ClientSession and
friends.

LGTM otherwise, with a few nits!

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

http://codereview.chromium.org/9646013/diff/15003/remoting/host/client_sessio...
remoting/host/client_session.h:64: protocol::ClipboardStub* clipboard_stub,
While the ClipboardStub is part of the InputStub, and the EventInjector
implements it, I suggest we don't bother separating the two at this level, i.e.
just pass the InputStub here, as we did previously.

http://codereview.chromium.org/9646013/diff/15003/remoting/host/event_executor.h
File remoting/host/event_executor.h (right):

http://codereview.chromium.org/9646013/diff/15003/remoting/host/event_executo...
remoting/host/event_executor.h:9: #include "remoting/protocol/clipboard_stub.h"
Don't need this include any more?

http://codereview.chromium.org/9646013/diff/15003/remoting/host/event_executo...
File remoting/host/event_executor_linux.cc (right):

http://codereview.chromium.org/9646013/diff/15003/remoting/host/event_executo...
remoting/host/event_executor_linux.cc:41: virtual void
InjectClipboardEvent(const protocol::ClipboardEvent& event)
nit: Be consist about protocol::ClipboardEvent vs KeyEvent / MouseEvent?  Have
you avoided using for ClipboardEvent because it's only used in two places?

http://codereview.chromium.org/9646013/diff/15003/remoting/protocol/client_ev...
File remoting/protocol/client_event_dispatcher.cc (right):

http://codereview.chromium.org/9646013/diff/15003/remoting/protocol/client_ev...
remoting/protocol/client_event_dispatcher.cc:37: // ClipboardEvent.
nit: NOTIMPLEMENTED() (or NOTREACHED()) here?

http://codereview.chromium.org/9646013/diff/15003/remoting/protocol/connectio...
File remoting/protocol/connection_to_client.h (right):

http://codereview.chromium.org/9646013/diff/15003/remoting/protocol/connectio...
remoting/protocol/connection_to_client.h:89: virtual void
set_clipboard_stub(ClipboardStub* clipboard_stub);
See earlier comment re not splitting clipboard_stub from input_stub at this
level just yet.

http://codereview.chromium.org/9646013/diff/15003/remoting/protocol/protocol_...
File remoting/protocol/protocol_mock_objects.h (right):

http://codereview.chromium.org/9646013/diff/15003/remoting/protocol/protocol_...
remoting/protocol/protocol_mock_objects.h:63: class MockClipboardStub : public
ClipboardStub {
nit: At this point we don't actually need a MockClipboardStub, since a
MockInputStub can serve in its place?

[simonmorris, 2012-03-15 16:53:19]

I've refactored so that, instead of InputStub inheriting from ClipboardStub,
there's a new HostEventStub that inherits from both.

PTYAL

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

http://codereview.chromium.org/9646013/diff/15003/remoting/host/client_sessio...
remoting/host/client_session.h:64: protocol::ClipboardStub* clipboard_stub,
On 2012/03/15 01:14:44, Wez wrote:
> While the ClipboardStub is part of the InputStub, and the EventInjector
> implements it, I suggest we don't bother separating the two at this level,
i.e.
> just pass the InputStub here, as we did previously.

ClipboardStub is no longer part of InputStub. But if it's clearer, I could just
pass a HostEventStub here.

http://codereview.chromium.org/9646013/diff/15003/remoting/host/event_executor.h
File remoting/host/event_executor.h (right):

http://codereview.chromium.org/9646013/diff/15003/remoting/host/event_executo...
remoting/host/event_executor.h:9: #include "remoting/protocol/clipboard_stub.h"
On 2012/03/15 01:14:44, Wez wrote:
> Don't need this include any more?

Done.

http://codereview.chromium.org/9646013/diff/15003/remoting/host/event_executo...
File remoting/host/event_executor_linux.cc (right):

http://codereview.chromium.org/9646013/diff/15003/remoting/host/event_executo...
remoting/host/event_executor_linux.cc:41: virtual void
InjectClipboardEvent(const protocol::ClipboardEvent& event)
On 2012/03/15 01:14:44, Wez wrote:
> nit: Be consist about protocol::ClipboardEvent vs KeyEvent / MouseEvent?  Have
> you avoided using for ClipboardEvent because it's only used in two places?

I was consistent originally, but another reviewer asked for the change. Now it's
2-1, so I've gone back to the consistent approach.

http://codereview.chromium.org/9646013/diff/15003/remoting/protocol/client_ev...
File remoting/protocol/client_event_dispatcher.cc (right):

http://codereview.chromium.org/9646013/diff/15003/remoting/protocol/client_ev...
remoting/protocol/client_event_dispatcher.cc:37: // ClipboardEvent.
On 2012/03/15 01:14:44, Wez wrote:
> nit: NOTIMPLEMENTED() (or NOTREACHED()) here?

No longer needed.

http://codereview.chromium.org/9646013/diff/15003/remoting/protocol/connectio...
File remoting/protocol/connection_to_client.h (right):

http://codereview.chromium.org/9646013/diff/15003/remoting/protocol/connectio...
remoting/protocol/connection_to_client.h:89: virtual void
set_clipboard_stub(ClipboardStub* clipboard_stub);
On 2012/03/15 01:14:44, Wez wrote:
> See earlier comment re not splitting clipboard_stub from input_stub at this
> level just yet.

Now that InputStub doesn't inherit from ClipboardStub, I think these can stay
separate. But they could merge to a set_host_event_stub(), if that's clearer.

http://codereview.chromium.org/9646013/diff/15003/remoting/protocol/protocol_...
File remoting/protocol/protocol_mock_objects.h (right):

http://codereview.chromium.org/9646013/diff/15003/remoting/protocol/protocol_...
remoting/protocol/protocol_mock_objects.h:63: class MockClipboardStub : public
ClipboardStub {
On 2012/03/15 01:14:44, Wez wrote:
> nit: At this point we don't actually need a MockClipboardStub, since a
> MockInputStub can serve in its place?

They're separate again now.

[Wez, 2012-03-15 20:37:18]

LGTM, but the addition of HostEventStub means you can avoid splitting the
interfaces passed to ClientSession at this stage.

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

http://codereview.chromium.org/9646013/diff/14006/remoting/host/client_sessio...
remoting/host/client_session.h:25: class ClientSession : public
protocol::ClipboardStub,
Implement HostEventStub rather than ClipboardStub and InputStub?

http://codereview.chromium.org/9646013/diff/14006/remoting/host/client_sessio...
remoting/host/client_session.h:65: protocol::ClipboardStub* clipboard_stub,
Use HostEventStub here rather than separate InputStub and ClipboardStub?

http://codereview.chromium.org/9646013/diff/14006/remoting/protocol/input_stub.h
File remoting/protocol/input_stub.h (right):

http://codereview.chromium.org/9646013/diff/14006/remoting/protocol/input_stu...
remoting/protocol/input_stub.h:6: // This interface handles some event messages
defined in event.proto.
nit: some -> input

[simonmorris, 2012-03-15 21:49:30]

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

http://codereview.chromium.org/9646013/diff/14006/remoting/host/client_sessio...
remoting/host/client_session.h:25: class ClientSession : public
protocol::ClipboardStub,
On 2012/03/15 20:37:19, Wez wrote:
> Implement HostEventStub rather than ClipboardStub and InputStub?

Done.

http://codereview.chromium.org/9646013/diff/14006/remoting/host/client_sessio...
remoting/host/client_session.h:65: protocol::ClipboardStub* clipboard_stub,
On 2012/03/15 20:37:19, Wez wrote:
> Use HostEventStub here rather than separate InputStub and ClipboardStub?

Done.

http://codereview.chromium.org/9646013/diff/14006/remoting/protocol/input_stub.h
File remoting/protocol/input_stub.h (right):

http://codereview.chromium.org/9646013/diff/14006/remoting/protocol/input_stu...
remoting/protocol/input_stub.h:6: // This interface handles some event messages
defined in event.proto.
On 2012/03/15 20:37:19, Wez wrote:
> nit: some -> input

Done.

[commit-bot: I haz the power, 2012-03-15 23:10:20]

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

[commit-bot: I haz the power, 2012-03-16 01:23:45]

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

[commit-bot: I haz the power, 2012-03-16 15:46:26]

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

[commit-bot: I haz the power, 2012-03-16 17:28:05]

Change committed as 127195