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

remoting/protocol/clipboard_duplicate_filter.h

Index: remoting/protocol/clipboard_duplicate_filter.h
diff --git a/remoting/protocol/clipboard_duplicate_filter.h b/remoting/protocol/clipboard_duplicate_filter.h
new file mode 100644
index 0000000000000000000000000000000000000000..1de148c8297ba62715193012665cd69e9bd26e64
--- /dev/null
+++ b/remoting/protocol/clipboard_duplicate_filter.h
@@ -0,0 +1,74 @@
+// Copyright (c) 2012 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef REMOTING_PROTOCOL_CLIPBOARD_DUPLICATE_FILTER_H_
+#define REMOTING_PROTOCOL_CLIPBOARD_DUPLICATE_FILTER_H_
+
+#include <map>
+#include <string>
+
+#include "base/compiler_specific.h"
+#include "remoting/protocol/clipboard_stub.h"
+
+namespace remoting {
+namespace protocol {
+
+// ClipboardDuplicateFilter stops the host sending a clipboard item to the
+// client, if it knows that the client already has that item on its clipboard.

[Wez, 2012/05/16 20:51:43]

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?

[simonmorris, 2012/05/16 22:35:25]

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.
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?

[Wez, 2012/05/18 18:51:52]

As discussed offline, we'll have this class suppress local echos, but not
duplicates.
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. ;)
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).
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.

[Wez, 2012/05/18 18:51:52]

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.

[simonmorris, 2012/05/21 17:15:58]

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

Done.

[simonmorris, 2012/05/21 17:15:58]

I'd prefer not to inherit an implementation.

[Wez, 2012/05/21 22:33:15]

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.

[simonmorris, 2012/05/21 23:01:33]

OK. Sounds like a question for a separate CL.

+class ClipboardDuplicateFilter {
+ public:
+  ClipboardDuplicateFilter();
+
+  // Sets the ClipboardStub that sends events to the client.
+  void set_to_client(ClipboardStub* clipboard_stub);
+
+  // Sets the ClipboardStub that sends events to the host.
+  void set_to_host(ClipboardStub* clipboard_stub);
+
+  // Gets the ClipboardStub that sends events through this filter and on to the
+  // client.
+  ClipboardStub* get_filter_to_client();

[Wez, 2012/05/16 20:51:43]

teensy nit: get_client_filter()?

[simonmorris, 2012/05/16 22:35:25]

Done.

+
+  // Gets the ClipboardStub that sends events through this filter and on to the
+  // host.
+  ClipboardStub* get_filter_to_host();
+
+ private:
+  class FilterToClient : public ClipboardStub {
+   public:
+    FilterToClient(ClipboardDuplicateFilter* filter);
+    virtual void InjectClipboardEvent(const ClipboardEvent& event) OVERRIDE;
+
+   private:
+    ClipboardDuplicateFilter* filter_;
+  };
+
+  class FilterToHost : public ClipboardStub {
+   public:
+    FilterToHost(ClipboardDuplicateFilter* filter);
+    virtual void InjectClipboardEvent(const ClipboardEvent& event) OVERRIDE;
+
+   private:
+    ClipboardDuplicateFilter* filter_;
+  };
+
+  void InjectClipboardEventToHost(const ClipboardEvent& event);
+  void InjectClipboardEventToClient(const ClipboardEvent& event);

[Wez, 2012/05/18 18:51:52]

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.

[simonmorris, 2012/05/21 17:15:58]

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

[Wez, 2012/05/21 22:33:15]

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.

[simonmorris, 2012/05/21 23:01:33]

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.

+
+  ClipboardStub* to_host_;
+  ClipboardStub* to_client_;

[Wez, 2012/05/16 20:51:43]

teensy nit: I think these should be client_stub_ and host_stub_.

[simonmorris, 2012/05/16 22:35:25]

Done.

+  FilterToClient filter_to_client_;
+  FilterToHost filter_to_host_;
+
+  // The most recent item sent to the client or host.
+  std::string recent_mime_type_;
+  std::string recent_data_;
+
+  DISALLOW_COPY_AND_ASSIGN(ClipboardDuplicateFilter);
+};
+
+}  // namespace protocol
+}  // namespace remoting
+
+#endif  // REMOTING_PROTOCOL_CLIPBOARD_DUPLICATE_FILTER_H_