Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10399052:
[Chromoting] Add a filter that will stop the host sending unnecessary clipboard events to the clien… (Closed)
|
Created:
8 years, 7 months ago by simonmorris Modified:
8 years, 7 months ago Reviewers:
Wez CC:
chromium-reviews, jamiewalch+watch_chromium.org, dcaiafa+watch_chromium.org, simonmorris+watch_chromium.org, hclam+watch_chromium.org, wez+watch_chromium.org, amit, sanjeevr, dcheng, garykac+watch_chromium.org, lambroslambrou+watch_chromium.org, alexeypa+watch_chromium.org, sergeyu+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
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
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=138377
Patch Set 1 #
Total comments: 22
Patch Set 2 : Review. #
Total comments: 7
Patch Set 3 : Review. #
Total comments: 12
Patch Set 4 : Improve comments and names. #
Total comments: 2
Patch Set 5 : Improve comments. #Patch Set 6 : Sync. #Patch Set 7 : Add an explicit destructor. #Patch Set 8 : Update copyright date. #Patch Set 9 : Sync. #Patch Set 10 : Sync. #
Messages
Total messages: 22 (0 generated)
ptal
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.c... 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.... 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_du... File remoting/protocol/clipboard_duplicate_filter.h (right): http://codereview.chromium.org/10399052/diff/1/remoting/protocol/clipboard_du... 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_du... 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_du... 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_du... File remoting/protocol/clipboard_duplicate_filter_unittest.cc (right): http://codereview.chromium.org/10399052/diff/1/remoting/protocol/clipboard_du... 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/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.c... 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.... 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_du... File remoting/protocol/clipboard_duplicate_filter.h (right): http://codereview.chromium.org/10399052/diff/1/remoting/protocol/clipboard_du... 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(); > ... > }; > > .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. > 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_du... 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_du... 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_du... File remoting/protocol/clipboard_duplicate_filter_unittest.cc (right): http://codereview.chromium.org/10399052/diff/1/remoting/protocol/clipboard_du... 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.
ping
http://codereview.chromium.org/10399052/diff/1/remoting/protocol/clipboard_du... File remoting/protocol/clipboard_duplicate_filter.h (right): http://codereview.chromium.org/10399052/diff/1/remoting/protocol/clipboard_du... 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 22:35:25, simonmorris 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(); > > ... > > }; > > > > .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. > > > > 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. 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_du... 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. > ... > }; > > .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_du... 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... File remoting/protocol/clipboard_duplicate_filter.cc (right): http://codereview.chromium.org/10399052/diff/5001/remoting/protocol/clipboard... 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... File remoting/protocol/clipboard_duplicate_filter.h (right): http://codereview.chromium.org/10399052/diff/5001/remoting/protocol/clipboard... 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... 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... File remoting/protocol/clipboard_duplicate_filter_unittest.cc (right): http://codereview.chromium.org/10399052/diff/5001/remoting/protocol/clipboard... 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/diff/1/remoting/protocol/clipboard_du... File remoting/protocol/clipboard_duplicate_filter.h (right): http://codereview.chromium.org/10399052/diff/1/remoting/protocol/clipboard_du... 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 22:35:25, simonmorris 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(); > > > ... > > > }; > > > > > > .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. > > > > > > > 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. > > As discussed offline, we'll have this class suppress local echos, but not > duplicates. > Done. > > 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). 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_du... 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. > > ... > > }; > > > > .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_du... remoting/protocol/clipboard_duplicate_filter.h:57: void InjectClipboardEventToClient(const ClipboardEvent& event); 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... File remoting/protocol/clipboard_duplicate_filter.cc (right): http://codereview.chromium.org/10399052/diff/5001/remoting/protocol/clipboard... 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... File remoting/protocol/clipboard_duplicate_filter.h (right): http://codereview.chromium.org/10399052/diff/5001/remoting/protocol/clipboard... 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... 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/diff/1/remoting/protocol/clipboard_du... File remoting/protocol/clipboard_duplicate_filter.h (right): http://codereview.chromium.org/10399052/diff/1/remoting/protocol/clipboard_du... 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. > > > ... > > > }; > > > > > > .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_du... remoting/protocol/clipboard_duplicate_filter.h:57: void InjectClipboardEventToClient(const ClipboardEvent& event); 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_sessio... 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... File remoting/protocol/clipboard_echo_filter.cc (right): http://codereview.chromium.org/10399052/diff/5003/remoting/protocol/clipboard... 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... File remoting/protocol/clipboard_echo_filter.h (right): http://codereview.chromium.org/10399052/diff/5003/remoting/protocol/clipboard... 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... File remoting/protocol/clipboard_echo_filter_unittest.cc (right): http://codereview.chromium.org/10399052/diff/5003/remoting/protocol/clipboard... 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... 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... 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/diff/1/remoting/protocol/clipboard_du... File remoting/protocol/clipboard_duplicate_filter.h (right): http://codereview.chromium.org/10399052/diff/1/remoting/protocol/clipboard_du... 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 22:33:15, Wez wrote: > 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. OK. Sounds like a question for a separate CL. > > > > > ... > > > > }; > > > > > > > > .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_du... remoting/protocol/clipboard_duplicate_filter.h:57: void InjectClipboardEventToClient(const ClipboardEvent& event); 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_sessio... 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... File remoting/protocol/clipboard_echo_filter.cc (right): http://codereview.chromium.org/10399052/diff/5003/remoting/protocol/clipboard... 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... File remoting/protocol/clipboard_echo_filter.h (right): http://codereview.chromium.org/10399052/diff/5003/remoting/protocol/clipboard... 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... File remoting/protocol/clipboard_echo_filter_unittest.cc (right): http://codereview.chromium.org/10399052/diff/5003/remoting/protocol/clipboard... 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... 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... 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.
lgtm http://codereview.chromium.org/10399052/diff/19001/remoting/protocol/clipboar... File remoting/protocol/clipboard_echo_filter_unittest.cc (right): http://codereview.chromium.org/10399052/diff/19001/remoting/protocol/clipboar... remoting/protocol/clipboard_echo_filter_unittest.cc:71: // stub. nit: "... whether or not there is a client stub"?
fyi http://codereview.chromium.org/10399052/diff/19001/remoting/protocol/clipboar... File remoting/protocol/clipboard_echo_filter_unittest.cc (right): http://codereview.chromium.org/10399052/diff/19001/remoting/protocol/clipboar... 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.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/simonmorris@chromium.org/10399052/12006
Try job failure for 10399052-12006 (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...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/simonmorris@chromium.org/10399052/16007
Try job failure for 10399052-16007 (retry) (retry) on win_rel for steps "base_unittests, sync_unit_tests". It's a second try, previously, steps "base_unittests, browser_tests, sync_unit_tests" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/simonmorris@chromium.org/10399052/16007
Try job failure for 10399052-16007 (retry) on linux_chromeos for step "compile" (clobber build). It's a second try, previously, step "compile" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/simonmorris@chromium.org/10399052/16007
Try job failure for 10399052-16007 on mac_rel for step "update". http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu... Step "update" is always a major failure. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/simonmorris@chromium.org/10399052/16007
Try job failure for 10399052-16007 (retry) on win for step "compile" (clobber build). It's a second try, previously, step "compile" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/simonmorris@chromium.org/10399052/17008
Try job failure for 10399052-17008 (retry) on win_rel for steps "base_unittests, sync_unit_tests" (clobber build). It's a second try, previously, step "compile" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
