Modified the flash cipboard interface to add html clipboard support.

ppapi/tests/test_flash_clipboard.cc

Index: ppapi/tests/test_flash_clipboard.cc
diff --git a/ppapi/tests/test_flash_clipboard.cc b/ppapi/tests/test_flash_clipboard.cc
index 3022705cc98a6873c677a4d17fd4b7cdc84d1acd..d0e7b66767cf8650e89b237c7e2a18f5fba37383 100644
--- a/ppapi/tests/test_flash_clipboard.cc
+++ b/ppapi/tests/test_flash_clipboard.cc
@@ -11,6 +11,12 @@
 
 REGISTER_TEST_CASE(FlashClipboard);
 
+// WriteData() causes an async request sent to the browser process.

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

nit: request "to be" sent?
Or just say "sends an async request to".

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

Done.

+// As a result, the string written may not be reflected by IsFormatAvailable()
+// or ReadPlainText() immediately. We need to wait and retry.
+const int kIntervalMs = 250;
+const int kMaxIntervals = kActionTimeoutMs / kIntervalMs;
+
 TestFlashClipboard::TestFlashClipboard(TestingInstance* instance)
     : TestCase(instance),
       clipboard_interface_(NULL) {
@@ -18,53 +24,125 @@ TestFlashClipboard::TestFlashClipboard(TestingInstance* instance)
 
 bool TestFlashClipboard::Init() {
   clipboard_interface_ = static_cast<const PPB_Flash_Clipboard*>(
-      pp::Module::Get()->GetBrowserInterface(PPB_FLASH_CLIPBOARD_INTERFACE));
+      pp::Module::Get()->GetBrowserInterface(
+          PPB_FLASH_CLIPBOARD_INTERFACE));
   return !!clipboard_interface_;
 }
 
 void TestFlashClipboard::RunTests(const std::string& filter) {
-  RUN_TEST(ReadWrite, filter);
+  RUN_TEST(ReadWritePlainText, filter);
+  RUN_TEST(ReadWriteHTML, filter);
+  RUN_TEST(ReadWriteMultipleFormats, filter);

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

Don't you also need to change ppapi_uitest.cc to get these to run?

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

I don't know about that, they seem to be running already (I've seen them fail on
trybots, for example). ppapi_uitest already contains:
TEST_PPAPI_IN_PROCESS(FlashClipboard)
TEST_PPAPI_OUT_OF_PROCESS(FlashClipboard)

Does something else need to be done?

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

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

You're right. There are two ways to run tests in ppapi_uitest.cc. You can either
do it this way:
TEST_PPAPI_IN_PROCESS(FlashClipboard)
...which runs all the tests listed in RunTests. Or you can do them individually:
TEST_PPAPI_IN_PROCESS(FlashClipboard_ReadWritePlainText)
TEST_PPAPI_IN_PROCESS(FlashClipboard_ReadWriteHTML)
TEST_PPAPI_IN_PROCESS(FlashClipboard_ReadWriteMultipleFormats)

The latter has the advantage that you can enable/disable individual tests. I'd
like to eventually get all the tests running that way, but I haven't been
pushing for it since it makes the test suite slower overall. What you have is
fine.

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

Ah I see, thanks for the explanation.

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

 }
 
-std::string TestFlashClipboard::TestReadWrite() {
-  std::string input_str("Hello, world");
-  pp::Var input_var(input_str);
-  clipboard_interface_->WritePlainText(instance_->pp_instance(),
-                                       PP_FLASH_CLIPBOARD_TYPE_STANDARD,
-                                       input_var.pp_var());
-
-  // WritePlainText() causes an async request sent to the browser process.
-  // As a result, the string written may not be reflected by IsFormatAvailable()
-  // or ReadPlainText() immediately. We need to wait and retry.
-  const int kIntervalMs = 250;
-  const int kMaxIntervals = kActionTimeoutMs / kIntervalMs;
-
+PP_Bool TestFlashClipboard::IsFormatAvailable(
+    PP_Flash_Clipboard_Format format) {
   PP_Bool is_available = PP_FALSE;
   for (int i = 0; i < kMaxIntervals; ++i) {
     is_available = clipboard_interface_->IsFormatAvailable(
         instance_->pp_instance(),
         PP_FLASH_CLIPBOARD_TYPE_STANDARD,
-        PP_FLASH_CLIPBOARD_FORMAT_PLAINTEXT);
+        format);
     if (is_available)
       break;
 
     PlatformSleep(kIntervalMs);
   }
-  ASSERT_TRUE(is_available);
+  return is_available;
+}
 
+std::string TestFlashClipboard::ReadStringVar(
+    PP_Flash_Clipboard_Format format) {
   std::string result_str;
-  for (int i = 0; i < kMaxIntervals; ++i) {
-    pp::Var result_var(pp::Var::PassRef(),
-        clipboard_interface_->ReadPlainText(instance_->pp_instance(),
-                                            PP_FLASH_CLIPBOARD_TYPE_STANDARD));
-    ASSERT_TRUE(result_var.is_string());
+  pp::Var result_var(pp::Var::PassRef(),

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

style nit: I'd usually put the first param on the next line in this case.

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

Done.

+      clipboard_interface_->ReadData(instance_->pp_instance(),
+                                     PP_FLASH_CLIPBOARD_TYPE_STANDARD,
+                                     format));
+

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

style nit: unnecessary blank line. Blank line on 65 is kind of unnecessary too.

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

Done.

+  if (result_var.is_string()) {
     result_str = result_var.AsString();
-    if (result_str == input_str)
-      break;
+  }

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

style nit: maybe no {} here to be consistent with 46,47 (and elsewhere...  we
mostly leave them out when they're unnecessary in ppapi code)

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

Done.

+
+  return result_str;
+}
+
+int32_t TestFlashClipboard::WriteStringVar(PP_Flash_Clipboard_Format format,
+                                           const std::string& input) {
+  pp::Var input_var(input);
+  PP_Flash_Clipboard_Data_Item item = {format, input_var.pp_var()};

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

style nit: space after { and before }

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

Done.

+  int32_t success = clipboard_interface_->WriteData(
+      instance_->pp_instance(),
+      PP_FLASH_CLIPBOARD_TYPE_STANDARD,
+      1,
+      &item);

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

Does WriteData expect to be passed a reference count for the var in item? If so,
I think the reference counting here is wrong, since input_var has 1 ref count
which it will release at the end. So if WriteData is "given" a ref-count, then
we increase it once (input_var constructor) and decrease it twice (input_var
destructor and WriteData when it's finished with it).

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

Not sure what you mean exactly. WriteData finishes immediately and AFAIK does
not decrement the reference count. 

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

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

I guess I wanted to make sure you had thought about how the reference counting
worked for your API, but I do think it's right. Sorry for muddying the waters.

+  return success;
+}
+
+bool TestFlashClipboard::ReadAndMatchPlainText(const std::string& input) {
+  for (int i = 0; i < kMaxIntervals; ++i) {
+    if (ReadStringVar(PP_FLASH_CLIPBOARD_FORMAT_PLAINTEXT) == input) {
+      return true;
+    }
+    PlatformSleep(kIntervalMs);
+  }
+  return false;
+}
 
+bool TestFlashClipboard::ReadAndMatchHTML(const std::string& input) {
+  for (int i = 0; i < kMaxIntervals; ++i) {
+    std::string result = ReadStringVar(PP_FLASH_CLIPBOARD_FORMAT_HTML);
+    // Markup is inserted around the copied html, so just check that
+    // the pasted string contains the copied string.
+    bool match = result.find(input) != std::string::npos;
+    if (match) {
+      return true;
+    }
     PlatformSleep(kIntervalMs);
   }
+  return false;
+}
+
+std::string TestFlashClipboard::TestReadWritePlainText() {
+  std::string input = "Hello world plain text!";
+  ASSERT_TRUE(WriteStringVar(PP_FLASH_CLIPBOARD_FORMAT_PLAINTEXT,
+                             input) == PP_OK);
+  ASSERT_TRUE(IsFormatAvailable(PP_FLASH_CLIPBOARD_FORMAT_PLAINTEXT));
+  ASSERT_TRUE(ReadAndMatchPlainText(input));
+
+  PASS();
+}
+
+std::string TestFlashClipboard::TestReadWriteHTML() {
+  std::string input = "Hello world html!";
+  ASSERT_TRUE(WriteStringVar(PP_FLASH_CLIPBOARD_FORMAT_HTML,
+                             input) == PP_OK);
+  ASSERT_TRUE(IsFormatAvailable(PP_FLASH_CLIPBOARD_FORMAT_HTML));
+  ASSERT_TRUE(ReadAndMatchHTML(input));
+
+  PASS();
+}
+
+std::string TestFlashClipboard::TestReadWriteMultipleFormats() {
+  std::string plain_text("plain text");
+  std::string html("html");
+  pp::Var plain_text_var(plain_text);
+  pp::Var html_var(html);

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

nit: I think you could do each of these in one line, e.g.
pp::Var html_var("html");

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

Done.

+  PP_Flash_Clipboard_Data_Item items[] = {
+      {PP_FLASH_CLIPBOARD_FORMAT_PLAINTEXT, plain_text_var.pp_var()},
+      {PP_FLASH_CLIPBOARD_FORMAT_HTML, html_var.pp_var()}
+  };
+  int32_t success = clipboard_interface_->WriteData(
+      instance_->pp_instance(),
+      PP_FLASH_CLIPBOARD_TYPE_STANDARD,
+      2,

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

nit: best practice would probably be to compute the number of elements...  e.g.:
sizeof(items)/sizeof(*items)
(note in regular chromium code you'd use arraysize, but we can't use that here)

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

Done.

+      items);
+  ASSERT_TRUE(success == PP_OK);
+
+  ASSERT_TRUE(IsFormatAvailable(PP_FLASH_CLIPBOARD_FORMAT_PLAINTEXT));
+  ASSERT_TRUE(IsFormatAvailable(PP_FLASH_CLIPBOARD_FORMAT_HTML));
+
+  ASSERT_TRUE(ReadAndMatchPlainText(plain_text));
+  ASSERT_TRUE(ReadAndMatchHTML(html));
 
-  ASSERT_TRUE(result_str == input_str);
   PASS();
 }