[Chromoting] Add a filter that will stop the host sending unnecessary clipboard events to the clien… (issue 10399052)

[simon...@chromium.org]

Reviewers: Wez,

Message:
ptal


Description:
[Chromoting] Add a filter that will stop the host sending unnecessary
clipboard
events to the client.

The host does not yet send any clipboard events to the client. A follow-up
CL
will make that happen.

BUG=117473


Please review this at http://codereview.chromium.org/10399052/

SVN Base: svn://svn.chromium.org/chrome/trunk/src

Affected files:
M remoting/client/chromoting_client.h
M remoting/host/client_session.h
M remoting/host/client_session.cc
M remoting/protocol/client_stub.h
A remoting/protocol/clipboard_duplicate_filter.h
A remoting/protocol/clipboard_duplicate_filter.cc
A remoting/protocol/clipboard_duplicate_filter_unittest.cc
M remoting/protocol/host_control_dispatcher.h
M remoting/protocol/protocol_mock_objects.h
M remoting/remoting.gyp

[w...@chromium.org]

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

http://codereview.chromium.org/10399052/diff/1/remoting/host/client_session.cc#newcode39
remoting/host/client_session.cc:39:
clipboard_duplicate_filter_.set_to_client(connection_->client_stub());
Is client_stub() actually valid at this point, or do we need to wait and
set it only when we know we're connected?

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

http://codereview.chromium.org/10399052/diff/1/remoting/protocol/client_stub.h#newcode19
remoting/protocol/client_stub.h:19: class ClientStub : public
ClipboardStub {
nit: We discussed composition vs inheritance; do you see this becoming a
composed interface in future?

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

http://codereview.chromium.org/10399052/diff/1/remoting/protocol/clipboard_duplicate_filter.h#newcode18
remoting/protocol/clipboard_duplicate_filter.h:18: // client, if it
knows that the client already has that item on its clipboard.
I don't think the filter behaviour actually needs to differ between
client->host and host->client, in which case I think you can simplify
this class, e.g:

.h:

class ClipboardDeduplicator {
...
ClipboardFilter* host_filter();
ClipboardFilter* client_filter();
...
};

.cc:

class ClipboardDuplicateFilter : public ClipboardFilter {
...
ClipboardDuplicateFilter(ClipboardDeduplicator* deduplicator);
...
void InjectClipboardEvent(...) {
...
// Check whether it's new content
// Inject it
// Store it.
...
}
...
};

ClipboardDeduplicator::ClipboadDeduplicator() {
host_filter_ = new ClipboardDuplicateFilter(this);
client_filter_ = ...
}

However, I think we actually want to suppress local clipboard "echos",
i.e. when we are notified of clipboard content that we set ourself? In
that case the filter (possibly ClipboardEchoFilter?) does need to be
assymmetric, as you have it here, but we should use terms "local" and
"remote" rather than "client" and "host", since it's equally valid at
both ends.

Is there no way to filter echos in the per-platform implementations,
e.g. by putting a custom clipboard format alongside the real data, so
that we can distinguish local vs echo changes?

http://codereview.chromium.org/10399052/diff/1/remoting/protocol/clipboard_duplicate_filter.h#newcode31
remoting/protocol/clipboard_duplicate_filter.h:31: ClipboardStub*
get_filter_to_client();
teensy nit: get_client_filter()?

http://codereview.chromium.org/10399052/diff/1/remoting/protocol/clipboard_duplicate_filter.h#newcode60
remoting/protocol/clipboard_duplicate_filter.h:60: ClipboardStub*
to_client_;
teensy nit: I think these should be client_stub_ and host_stub_.

http://codereview.chromium.org/10399052/diff/1/remoting/protocol/clipboard_duplicate_filter_unittest.cc
File remoting/protocol/clipboard_duplicate_filter_unittest.cc (right):

http://codereview.chromium.org/10399052/diff/1/remoting/protocol/clipboard_duplicate_filter_unittest.cc#newcode31
remoting/protocol/clipboard_duplicate_filter_unittest.cc:31: //
identical event from the host.
This case actually tests more than that; it also tests that contents
that don't match are _not_ filtered; update description?

http://codereview.chromium.org/10399052/

[simon...@chromium.org]

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

http://codereview.chromium.org/10399052/diff/1/remoting/host/client_session.cc#newcode39
remoting/host/client_session.cc:39:
clipboard_duplicate_filter_.set_to_client(connection_->client_stub());

On 2012/05/16 20:51:43, Wez wrote:
> Is client_stub() actually valid at this point, or do we need to wait
and set it
> only when we know we're connected?

I've moved this line to OnConnectionAuthenticated(), as we shouldn't
send anything to the client before then anyway.

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

http://codereview.chromium.org/10399052/diff/1/remoting/protocol/client_stub.h#newcode19
remoting/protocol/client_stub.h:19: class ClientStub : public
ClipboardStub {

On 2012/05/16 20:51:43, Wez wrote:
> nit: We discussed composition vs inheritance; do you see this becoming
a
> composed interface in future?

Maybe. I don't think that possibility needs to affect this CL, though.

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

http://codereview.chromium.org/10399052/diff/1/remoting/protocol/clipboard_duplicate_filter.h#newcode18
remoting/protocol/clipboard_duplicate_filter.h:18: // client, if it
knows that the client already has that item on its clipboard.

I agree that we want to suppress local clipboard echos. This class
suppresses those, as well as clipboard events that would be sent twice
from host to client.

I considered using "local/remote" rather than "host/client", but (i) we
won't use this class on the client, as the filter is already implemented
in the webapp, where it can be changed more easily if needed, and (ii) I
think "host" and "client" are less likely to be confused than "local"
and "remote".

An echo is a duplicate, so I'm not sure there's much to be gained by
renaming ClipboardDuplicateFilter to ClipboardEchoFilter.

> Is there no way to filter echos in the per-platform implementations,
e.g. by
> putting a custom clipboard format alongside the real data, so that we
can
> distinguish local vs echo changes?

I expect there is. But is there a good reason to implement the
functionality separately on each platform, instead of using
platform-independent code, as in this CL?

http://codereview.chromium.org/10399052/diff/1/remoting/protocol/clipboard_duplicate_filter.h#newcode31
remoting/protocol/clipboard_duplicate_filter.h:31: ClipboardStub*
get_filter_to_client();

On 2012/05/16 20:51:43, Wez wrote:
> teensy nit: get_client_filter()?

Done.

http://codereview.chromium.org/10399052/diff/1/remoting/protocol/clipboard_duplicate_filter.h#newcode60
remoting/protocol/clipboard_duplicate_filter.h:60: ClipboardStub*
to_client_;

On 2012/05/16 20:51:43, Wez wrote:
> teensy nit: I think these should be client_stub_ and host_stub_.

Done.

http://codereview.chromium.org/10399052/diff/1/remoting/protocol/clipboard_duplicate_filter_unittest.cc
File remoting/protocol/clipboard_duplicate_filter_unittest.cc (right):

http://codereview.chromium.org/10399052/diff/1/remoting/protocol/clipboard_duplicate_filter_unittest.cc#newcode31
remoting/protocol/clipboard_duplicate_filter_unittest.cc:31: //
identical event from the host.

On 2012/05/16 20:51:43, Wez wrote:
> This case actually tests more than that; it also tests that contents
that don't
> match are _not_ filtered; update description?

Done.

http://codereview.chromium.org/10399052/

[simon...@chromium.org]

ping


http://codereview.chromium.org/10399052/

[w...@chromium.org]

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

http://codereview.chromium.org/10399052/diff/1/remoting/protocol/clipboard_duplicate_filter.h#newcode18
remoting/protocol/clipboard_duplicate_filter.h:18: // client, if it
knows that the client already has that item on its clipboard.

As discussed offline, we'll have this class suppress local echos, but
not duplicates.

> I considered using "local/remote" rather than "host/client", but (i)
we won't
> use this class on the client, as the filter is already implemented in
the
> webapp, where it can be changed more easily if needed, and (ii) I
think "host"
> and "client" are less likely to be confused than "local" and "remote".

This helper's not intrinsically tied to being a host-side component - we
could equally well use it in the client plugin, except that we already
have code in the JS to do this work. It suppresses echos of content you
inject locally from being re-transmitted remotely, so I think local &
remote are more appropriate names.

That said, since we will use it only in hosts initially, client & host
are valid and we can agree to disagree. ;)

> An echo is a duplicate, so I'm not sure there's much to be gained by
renaming
> ClipboardDuplicateFilter to ClipboardEchoFilter.

ClipboardEchoFilter makes sense if all we do is filter echos, and is
shorter. ;) I'm fine with duplicate, too, but consider renaming to
DuplicateClipboardFilter (i.e. word ordering more akin to spoken
English).

> > Is there no way to filter echos in the per-platform implementations,
e.g. by
> > putting a custom clipboard format alongside the real data, so that
we can
> > distinguish local vs echo changes?

> I expect there is. But is there a good reason to implement the
functionality
> separately on each platform, instead of using platform-independent
code, as in
> this CL?

As discussed offline, the benefit is in being able to disambiguate echos
of injected content from host-side copy operations without actually
comparing the content. The comparing scheme should be fine, but we can
revisit other options if we hit problems.

http://codereview.chromium.org/10399052/diff/1/remoting/protocol/clipboard_duplicate_filter.h#newcode18
remoting/protocol/clipboard_duplicate_filter.h:18: // client, if it
knows that the client already has that item on its clipboard.
On 2012/05/16 20:51:43, Wez wrote:
> I don't think the filter behaviour actually needs to differ between
client->host
> and host->client, in which case I think you can simplify this class,
e.g:

> .h:

> class ClipboardDeduplicator {
> ...
> ClipboardFilter* host_filter();
> ClipboardFilter* client_filter();

Please consider this change to derived the filter implementations from
ClipboardFilter, so that you don't then need a separate
set_client_stub() on ClipboardDuplicateFilter.

> Is there no way to filter echos in the per-platform implementations,
e.g. by
> putting a custom clipboard format alongside the real data, so that we
can
> distinguish local vs echo changes?

http://codereview.chromium.org/10399052/diff/1/remoting/protocol/clipboard_duplicate_filter.h#newcode57
remoting/protocol/clipboard_duplicate_filter.h:57: void
InjectClipboardEventToClient(const ClipboardEvent& event);
nit: If we're moving to just have this class suppress echos then you
could replace these with StoreClipboardFromClient() and
IsEchoOfClientClipboard(), for the InjectClipboard() methods on the
filters to use.

http://codereview.chromium.org/10399052/diff/5001/remoting/protocol/clipboard_duplicate_filter.cc
File remoting/protocol/clipboard_duplicate_filter.cc (right):

http://codereview.chromium.org/10399052/diff/5001/remoting/protocol/clipboard_duplicate_filter.cc#newcode1
remoting/protocol/clipboard_duplicate_filter.cc:1: #include
"remoting/protocol/clipboard_duplicate_filter.h"
Copyright notice?

http://codereview.chromium.org/10399052/diff/5001/remoting/protocol/clipboard_duplicate_filter.h
File remoting/protocol/clipboard_duplicate_filter.h (right):

http://codereview.chromium.org/10399052/diff/5001/remoting/protocol/clipboard_duplicate_filter.h#newcode62
remoting/protocol/clipboard_duplicate_filter.h:62: HostFilter
host_filter_;
If you make these scoped_ptr<Client/HostFilter>, or
scoped_ptr<ClipboardFilter>, then you can move the filter class
definitions into the .cc.

http://codereview.chromium.org/10399052/diff/5001/remoting/protocol/clipboard_duplicate_filter.h#newcode66
remoting/protocol/clipboard_duplicate_filter.h:66: std::string
recent_data_;
nit: Since these will now reflect the most recent clipboard from the
client, consider |client_[latest_]mime_type_| and
|client_[latest_]data_|

http://codereview.chromium.org/10399052/diff/5001/remoting/protocol/clipboard_duplicate_filter_unittest.cc
File remoting/protocol/clipboard_duplicate_filter_unittest.cc (right):

http://codereview.chromium.org/10399052/diff/5001/remoting/protocol/clipboard_duplicate_filter_unittest.cc#newcode32
remoting/protocol/clipboard_duplicate_filter_unittest.cc:32:
TEST(ClipboardDuplicateFilterTest,
FromClientBlocksIdenticalEventToClient) {
I'll review the unit-tests after you've update the implementation to
remove the host-duplicate suppression.

http://codereview.chromium.org/10399052/

[simon...@chromium.org]

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

http://codereview.chromium.org/10399052/diff/1/remoting/protocol/clipboard_duplicate_filter.h#newcode18
remoting/protocol/clipboard_duplicate_filter.h:18: // client, if it
knows that the client already has that item on its clipboard.

Done.

I agree that ClipboardEchoFilter is clearer, given that it's now not
filtering
host->client duplicates.

Done.

> > > Is there no way to filter echos in the per-platform
implementations, e.g. by
> > > putting a custom clipboard format alongside the real data, so that
we can
> > > distinguish local vs echo changes?
> >
> > I expect there is. But is there a good reason to implement the
functionality
> > separately on each platform, instead of using platform-independent
code, as in
> > this CL?

> As discussed offline, the benefit is in being able to disambiguate
echos of
> injected content from host-side copy operations without actually
comparing the
> content. The comparing scheme should be fine, but we can revisit
other options
> if we hit problems.

http://codereview.chromium.org/10399052/diff/1/remoting/protocol/clipboard_duplicate_filter.h#newcode18
remoting/protocol/clipboard_duplicate_filter.h:18: // client, if it
knows that the client already has that item on its clipboard.

On 2012/05/18 18:51:52, Wez wrote:
> On 2012/05/16 20:51:43, Wez wrote:
> > I don't think the filter behaviour actually needs to differ between
> client->host
> > and host->client, in which case I think you can simplify this class,
e.g:
> >
> > .h:
> >
> > class ClipboardDeduplicator {
> > ...
> > ClipboardFilter* host_filter();
> > ClipboardFilter* client_filter();

> Please consider this change to derived the filter implementations from
> ClipboardFilter, so that you don't then need a separate
set_client_stub() on
> ClipboardDuplicateFilter.

I'd prefer not to inherit an implementation.

On 2012/05/18 18:51:52, Wez wrote:
> nit: If we're moving to just have this class suppress echos then you
could
> replace these with StoreClipboardFromClient() and
IsEchoOfClientClipboard(), for
> the InjectClipboard() methods on the filters to use.

But then the logic is split across two classes. It's clearer to have the
inner classes be pure delegates.

http://codereview.chromium.org/10399052/diff/5001/remoting/protocol/clipboard_duplicate_filter.cc
File remoting/protocol/clipboard_duplicate_filter.cc (right):

http://codereview.chromium.org/10399052/diff/5001/remoting/protocol/clipboard_duplicate_filter.cc#newcode1
remoting/protocol/clipboard_duplicate_filter.cc:1: #include
"remoting/protocol/clipboard_duplicate_filter.h"

On 2012/05/18 18:51:52, Wez wrote:
> Copyright notice?

Done.

http://codereview.chromium.org/10399052/diff/5001/remoting/protocol/clipboard_duplicate_filter.h
File remoting/protocol/clipboard_duplicate_filter.h (right):

http://codereview.chromium.org/10399052/diff/5001/remoting/protocol/clipboard_duplicate_filter.h#newcode62
remoting/protocol/clipboard_duplicate_filter.h:62: HostFilter
host_filter_;

On 2012/05/18 18:51:52, Wez wrote:
> If you make these scoped_ptr<Client/HostFilter>, or
scoped_ptr<ClipboardFilter>,
> then you can move the filter class definitions into the .cc.

If the filter class definitions are in an anonymous namespace, then we'd
have to resort to making them friend classes in order to protect the
ClipboardEchoFilter interface that they use. I don't think declaring
(not defining) the classes here slows the compiler down much.

http://codereview.chromium.org/10399052/diff/5001/remoting/protocol/clipboard_duplicate_filter.h#newcode66
remoting/protocol/clipboard_duplicate_filter.h:66: std::string
recent_data_;

On 2012/05/18 18:51:52, Wez wrote:
> nit: Since these will now reflect the most recent clipboard from the
client,
> consider |client_[latest_]mime_type_| and |client_[latest_]data_|

Done.

http://codereview.chromium.org/10399052/

[w...@chromium.org]

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

http://codereview.chromium.org/10399052/diff/1/remoting/protocol/clipboard_duplicate_filter.h#newcode18
remoting/protocol/clipboard_duplicate_filter.h:18: // client, if it
knows that the client already has that item on its clipboard.

On 2012/05/21 17:15:58, simonmorris wrote:
> On 2012/05/18 18:51:52, Wez wrote:
> > On 2012/05/16 20:51:43, Wez wrote:
> > > I don't think the filter behaviour actually needs to differ
between
> > client->host
> > > and host->client, in which case I think you can simplify this
class, e.g:
> > >
> > > .h:
> > >
> > > class ClipboardDeduplicator {
> > > ...
> > > ClipboardFilter* host_filter();
> > > ClipboardFilter* client_filter();
> >
> > Please consider this change to derived the filter implementations
from
> > ClipboardFilter, so that you don't then need a separate
set_client_stub() on
> > ClipboardDuplicateFilter.
> >

> I'd prefer not to inherit an implementation.

That's a fair point, and it's not something I'd suggest in general;
However, ClipboardFilter is specifically intended as a base-class to
avoid duplicating a bunch of boiler-plate. It's arguably an interface
that we should arguably separate the implementation of into a
ClipboardFilterBase.

On 2012/05/21 17:15:58, simonmorris wrote:
> On 2012/05/18 18:51:52, Wez wrote:
> > nit: If we're moving to just have this class suppress echos then you
could
> > replace these with StoreClipboardFromClient() and
IsEchoOfClientClipboard(),
> for
> > the InjectClipboard() methods on the filters to use.

> But then the logic is split across two classes. It's clearer to have
the inner
> classes be pure delegates.

Actually, what I have in mind is pushing the logic into the two
end-point classes, so that the ClipboardEchoFilter itself becomes little
more than shared data between the two.

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

http://codereview.chromium.org/10399052/diff/5003/remoting/host/client_session.h#newcode149
remoting/host/client_session.h:149: // Filter to used to stop clipboard
items sent from the client being sent
nit: Suggest "... items sent by the client being echoed back to it."

http://codereview.chromium.org/10399052/diff/5003/remoting/protocol/clipboard_echo_filter.cc
File remoting/protocol/clipboard_echo_filter.cc (right):

http://codereview.chromium.org/10399052/diff/5003/remoting/protocol/clipboard_echo_filter.cc#newcode37
remoting/protocol/clipboard_echo_filter.cc:37: if (!client_stub_) {
nit: Move this test to the InjectClipboardEvent call, to match the
layout of InjectClipboardEventToHost?

http://codereview.chromium.org/10399052/diff/5003/remoting/protocol/clipboard_echo_filter.h
File remoting/protocol/clipboard_echo_filter.h (right):

http://codereview.chromium.org/10399052/diff/5003/remoting/protocol/clipboard_echo_filter.h#newcode31
remoting/protocol/clipboard_echo_filter.h:31: ClipboardStub*
get_client_filter();
nit: Since these are simple getters for members that can't be set(),
they don't actually need the get_ prefix, I don't think?

http://codereview.chromium.org/10399052/diff/5003/remoting/protocol/clipboard_echo_filter_unittest.cc
File remoting/protocol/clipboard_echo_filter_unittest.cc (right):

http://codereview.chromium.org/10399052/diff/5003/remoting/protocol/clipboard_echo_filter_unittest.cc#newcode31
remoting/protocol/clipboard_echo_filter_unittest.cc:31: // identical
event from the host.
nit: Suggest "... the host only filters out events identical to the most
recent clipboard content from the client".

http://codereview.chromium.org/10399052/diff/5003/remoting/protocol/clipboard_echo_filter_unittest.cc#newcode57
remoting/protocol/clipboard_echo_filter_unittest.cc:57: // the same
item.
nit: This reads as documentation of what we're doing; we actually _do_
want to inject the same content, and verify that it's properly filtered.

http://codereview.chromium.org/10399052/diff/5003/remoting/protocol/clipboard_echo_filter_unittest.cc#newcode70
remoting/protocol/clipboard_echo_filter_unittest.cc:70: // Check what
happens if no host stub is set.
Can you update this description to indicate the expected behaviour that
you're testing for? Otherwise it's not clear why we even need to test
it the behaviour rather than relay on non-NULL CHECKs.

http://codereview.chromium.org/10399052/

[simon...@chromium.org]

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

http://codereview.chromium.org/10399052/diff/1/remoting/protocol/clipboard_duplicate_filter.h#newcode18
remoting/protocol/clipboard_duplicate_filter.h:18: // client, if it
knows that the client already has that item on its clipboard.

OK. Sounds like a question for a separate CL.

On 2012/05/21 22:33:15, Wez wrote:
> On 2012/05/21 17:15:58, simonmorris wrote:
> > On 2012/05/18 18:51:52, Wez wrote:
> > > nit: If we're moving to just have this class suppress echos then
you could
> > > replace these with StoreClipboardFromClient() and
IsEchoOfClientClipboard(),
> > for
> > > the InjectClipboard() methods on the filters to use.
> >
> > But then the logic is split across two classes. It's clearer to have
the inner
> > classes be pure delegates.

> Actually, what I have in mind is pushing the logic into the two
end-point
> classes, so that the ClipboardEchoFilter itself becomes little more
than shared
> data between the two.

The code would still be split across two separate classes, and the data
would be in a third. I don't see a clear advantage to doing this.

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

http://codereview.chromium.org/10399052/diff/5003/remoting/host/client_session.h#newcode149
remoting/host/client_session.h:149: // Filter to used to stop clipboard
items sent from the client being sent

On 2012/05/21 22:33:15, Wez wrote:
> nit: Suggest "... items sent by the client being echoed back to it."

Done.

http://codereview.chromium.org/10399052/diff/5003/remoting/protocol/clipboard_echo_filter.cc
File remoting/protocol/clipboard_echo_filter.cc (right):

http://codereview.chromium.org/10399052/diff/5003/remoting/protocol/clipboard_echo_filter.cc#newcode37
remoting/protocol/clipboard_echo_filter.cc:37: if (!client_stub_) {

On 2012/05/21 22:33:15, Wez wrote:
> nit: Move this test to the InjectClipboardEvent call, to match the
layout of
> InjectClipboardEventToHost?

Then ClientFilter::InjectClipboardEvent would no longer be a pure
delegate. So someone working out what it does would have to look in
ClientFilter::InjectClipboardFilter and
ClipboardEchoFilter::InjectClipboardEventToClient. It's clearer to keep
the code in one place.

http://codereview.chromium.org/10399052/diff/5003/remoting/protocol/clipboard_echo_filter.h
File remoting/protocol/clipboard_echo_filter.h (right):

http://codereview.chromium.org/10399052/diff/5003/remoting/protocol/clipboard_echo_filter.h#newcode31
remoting/protocol/clipboard_echo_filter.h:31: ClipboardStub*
get_client_filter();

On 2012/05/21 22:33:15, Wez wrote:
> nit: Since these are simple getters for members that can't be set(),
they don't
> actually need the get_ prefix, I don't think?

Done.

http://codereview.chromium.org/10399052/diff/5003/remoting/protocol/clipboard_echo_filter_unittest.cc
File remoting/protocol/clipboard_echo_filter_unittest.cc (right):

http://codereview.chromium.org/10399052/diff/5003/remoting/protocol/clipboard_echo_filter_unittest.cc#newcode31
remoting/protocol/clipboard_echo_filter_unittest.cc:31: // identical
event from the host.

On 2012/05/21 22:33:15, Wez wrote:
> nit: Suggest "... the host only filters out events identical to the
most recent
> clipboard content from the client".

Done.

http://codereview.chromium.org/10399052/diff/5003/remoting/protocol/clipboard_echo_filter_unittest.cc#newcode57
remoting/protocol/clipboard_echo_filter_unittest.cc:57: // the same
item.

On 2012/05/21 22:33:15, Wez wrote:
> nit: This reads as documentation of what we're doing; we actually _do_
want to
> inject the same content, and verify that it's properly filtered.

Done.

http://codereview.chromium.org/10399052/diff/5003/remoting/protocol/clipboard_echo_filter_unittest.cc#newcode70
remoting/protocol/clipboard_echo_filter_unittest.cc:70: // Check what
happens if no host stub is set.

On 2012/05/21 22:33:15, Wez wrote:
> Can you update this description to indicate the expected behaviour
that you're
> testing for? Otherwise it's not clear why we even need to test it the
behaviour
> rather than relay on non-NULL CHECKs.

Done.

http://codereview.chromium.org/10399052/

[w...@chromium.org]

lgtm




http://codereview.chromium.org/10399052/diff/19001/remoting/protocol/clipboard_echo_filter_unittest.cc
File remoting/protocol/clipboard_echo_filter_unittest.cc (right):

http://codereview.chromium.org/10399052/diff/19001/remoting/protocol/clipboard_echo_filter_unittest.cc#newcode71
remoting/protocol/clipboard_echo_filter_unittest.cc:71: // stub.
nit: "... whether or not there is a client stub"?

http://codereview.chromium.org/10399052/

[simon...@chromium.org]

fyi

http://codereview.chromium.org/10399052/diff/19001/remoting/protocol/clipboard_echo_filter_unittest.cc
File remoting/protocol/clipboard_echo_filter_unittest.cc (right):

http://codereview.chromium.org/10399052/diff/19001/remoting/protocol/clipboard_echo_filter_unittest.cc#newcode71
remoting/protocol/clipboard_echo_filter_unittest.cc:71: // stub.

On 2012/05/21 23:06:40, Wez wrote:
> nit: "... whether or not there is a client stub"?

Done.

http://codereview.chromium.org/10399052/