Modified the flash cipboard interface to add html clipboard support.

ppapi/cpp/private/flash_clipboard.cc

Index: ppapi/cpp/private/flash_clipboard.cc
diff --git a/ppapi/cpp/private/flash_clipboard.cc b/ppapi/cpp/private/flash_clipboard.cc
index 8634eafc8adde463a9fa2be7609ab2a8dfca857d..32b71d9a4590d23dd4aa5dcac8a09a1f6bba850e 100644
--- a/ppapi/cpp/private/flash_clipboard.cc
+++ b/ppapi/cpp/private/flash_clipboard.cc
@@ -4,6 +4,8 @@
 
 #include "ppapi/cpp/private/flash_clipboard.h"
 
+#include <vector>
+
 #include "ppapi/c/pp_bool.h"
 #include "ppapi/c/pp_errors.h"
 #include "ppapi/cpp/instance.h"
@@ -18,13 +20,18 @@ template <> const char* interface_name<PPB_Flash_Clipboard>() {
   return PPB_FLASH_CLIPBOARD_INTERFACE;
 }
 
+template <> const char* interface_name<PPB_Flash_Clipboard_3_0>() {
+  return PPB_FLASH_CLIPBOARD_INTERFACE_3_0;
+}
+
 }  // namespace
 
 namespace flash {
 
 // static
 bool Clipboard::IsAvailable() {
-  return has_interface<PPB_Flash_Clipboard>();
+  return has_interface<PPB_Flash_Clipboard>() ||
+      has_interface<PPB_Flash_Clipboard_3_0>();
 }
 
 // static
@@ -40,34 +47,56 @@ bool Clipboard::IsFormatAvailable(Instance* instance,
 }
 
 // static
-bool Clipboard::ReadPlainText(Instance* instance,
-                              PP_Flash_Clipboard_Type clipboard_type,
-                              std::string* text_out) {
+bool Clipboard::ReadData(
+    Instance* instance,

[dmichael (off chromium), 2012/02/23 18:13:25]

brettw's been working on a change to replace Instance* in the C++ layer with
InstanceHandle. Assuming he lands first, you should adapt to that new style:
http://codereview.chromium.org/9381010/

[raymes, 2012/02/24 07:28:28]

Ok

On 2012/02/23 18:13:25, dmichael wrote:

+    PP_Flash_Clipboard_Type clipboard_type,
+    PP_Flash_Clipboard_Format clipboard_format,
+    PP_Var* out) {
   bool rv = false;
   if (has_interface<PPB_Flash_Clipboard>()) {
-    Var v(Var::PassRef(),
-          get_interface<PPB_Flash_Clipboard>()->ReadPlainText(
-              instance->pp_instance(),
-              clipboard_type));
-    if (v.is_string()) {
-      rv = true;
-      *text_out = v.AsString();
-    }
+    *out = get_interface<PPB_Flash_Clipboard>()->ReadData(
+        instance->pp_instance(),
+        clipboard_type,
+        clipboard_format);
+    rv = true;
+  } else if (has_interface<PPB_Flash_Clipboard_3_0>() &&
+           clipboard_format == PP_FLASH_CLIPBOARD_FORMAT_PLAINTEXT) {
+    *out = get_interface<PPB_Flash_Clipboard_3_0>()->ReadPlainText(
+        instance->pp_instance(),
+        clipboard_type);

[dmichael (off chromium), 2012/02/23 18:13:25]

Do you really need backwards compatibility here? This would only help if you're
building a new Flash plugin on the new API and pushing it to older versions of
Chrome that don't support the old API. It's up to you guys, but if you don't
need it, leave it out.

In any case, as soon as you're not using the old interface anymore, you should
probably get rid of any and all of the backwards compat code for the API.
Backwards compat is a pain in the neck, and we should only do it when we have to
(mostly only public interfaces or to prevent breakages on DEPS rolls). For
private interfaces, we can often avoid it.

[raymes, 2012/02/24 07:28:28]

Spoke to vtl and he says that the backward compat code is necessary for handling
multiple versions of flash.

On 2012/02/23 18:13:25, dmichael wrote:

[dmichael (off chromium), 2012/02/24 19:35:44]

Okay, vtl understands what you need for Flash more than I do. But bear in mind,
you're implementing two distinct forms  of compatibility.
1) Making new Chrome support older versions of the plugin.
2) Making new plugins support older versions of Chrome.
In this case, you're doing (2), because this code gets built in to the plugin
itself. I've done this before for a change to PPB_Var, but it turned out to not
really help much because we weren't doing it consistently everywhere in the C++
layer...  IIRC, plugins using the C++ layer and targeting (say) 18 don't work
with Chrome 16 because of other interface changes.

So I'd bet that you want the backwards-compat code that's elsewhere in this CL,
but you *might* not need it here. Again, vtl understands the needs for Flash
better than I do.

[raymes, 2012/02/24 21:38:17]

According to vtl, both are needed.
On 2012/02/24 19:35:44, dmichael wrote:

+    rv = true;
   }
   return rv;
 }
 
 // static
-bool Clipboard::WritePlainText(Instance* instance,
-                               PP_Flash_Clipboard_Type clipboard_type,
-                               const std::string& text) {
+bool Clipboard::WriteData(
+    Instance* instance,
+    PP_Flash_Clipboard_Type clipboard_type,
+    const std::vector<PP_Flash_Clipboard_Data_Item>& data_items) {
   bool rv = false;
+
   if (has_interface<PPB_Flash_Clipboard>()) {
-    rv = (get_interface<PPB_Flash_Clipboard>()->WritePlainText(
-              instance->pp_instance(),
-              clipboard_type,
-              Var(text).pp_var()) == PP_OK);
+        new PP_Flash_Clipboard_Data_Item[data_items.size()];

[dmichael (off chromium), 2012/02/23 18:13:25]

What is this line doing? Looks like a blatant leak from an unused expression...

[raymes, 2012/02/24 07:28:28]

=( undeleted line from an older patchset. Good catch.
Done.

+    rv = (get_interface<PPB_Flash_Clipboard>()->WriteData(
+        instance->pp_instance(),
+        clipboard_type,
+        data_items.size(),
+        &data_items[0]) == PP_OK);

[dmichael (off chromium), 2012/02/23 18:13:25]

probably best to return early if data_items.empty() to avoid indexing out of
bounds here.

[raymes, 2012/02/24 07:28:28]

Done.

+  } else if (has_interface<PPB_Flash_Clipboard_3_0>()) {
+    // Take the last plaintext item and write it according to the API.

[dmichael (off chromium), 2012/02/23 18:13:25]

I think this backwards-compat part should probably go away. But if I'm wrong
about that, it would probably better to send write them all, wouldn't it? It
also looks like you're writing the first one, not the last one.

[raymes, 2012/02/24 07:28:28]

The backward compat code is apparently necessary. Why do you suggest that it
would be better to write all of them? I'm pretty sure it's correctly writing the
last one, not the first. (Iterating from rbegin()). This code changed slightly
in the latest patchset anyhow.

On 2012/02/23 18:13:25, dmichael wrote:

[dmichael (off chromium), 2012/02/24 19:35:44]

It might be that I don't understand how the clipboard works well enough. But
imagine you're the client of this C++ wrapper, and you Write 10 things. If your
plugin is running on a newer version of Chrome, all 10 things get written to the
clipboard. If your plugin is running on an older version of Chrome, then only
the last item gets written. It seems to me that it would be less surprising for
the author of the plugin if the behavior was the same in both cases, if we can
make it so.
Ah, didn't catch it was a reverse iterator. My bad.

[raymes, 2012/02/24 21:38:17]

Ah I see what you're saying. The thing is that each time an item of the same
format is written, the prior item is overwritten. We are only writing plain text
items here, so writing the last has the same effect as writing them all.

On 2012/02/24 19:35:44, dmichael wrote:

[dmichael (off chromium), 2012/02/24 21:49:36]

That makes sense, thanks for explaining. I think a comment here to that effect
would be helpful.

+    for (std::vector<PP_Flash_Clipboard_Data_Item>::const_reverse_iterator
+        i = data_items.rbegin(); i != data_items.rend(); ++i) {
+      if (i->format == PP_FLASH_CLIPBOARD_FORMAT_PLAINTEXT) {
+        rv = (get_interface<PPB_Flash_Clipboard_3_0>()->WritePlainText(
+            instance->pp_instance(),
+            clipboard_type,
+            i->data) == PP_OK);
+        break;
+      }
+    }
   }
+
   return rv;
 }