linux: rearrange clipboard code

Once I understood what was going on, I attempted to clarify
the comment in the header.  The code seemed totally wrong on
first read and I think a comment like the one I wrote would've helped.

Concrete changes: DCHECK() if a key is set twice; don't call
WriteText() from WriteBookmark().

BUG=22697

Review URL: http://codereview.chromium.org/307003

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@29523 0039d316-1c4b-4281-b951-d872f2087c98

3 files changed, 45 insertions, 46 deletions

diff --git a/app/clipboard/clipboard.h b/app/clipboard/clipboard.h
index 6dd3927..61e8b96 100644
--- a/app/clipboard/clipboard.h
+++ b/app/clipboard/clipboard.h
@@ -21,10 +21,6 @@ class FilePath;
 class Clipboard {
  public:
   typedef std::string FormatType;
-#if defined(USE_X11)
-  typedef struct _GtkClipboard GtkClipboard;
-  typedef std::map<FormatType, std::pair<char*, size_t> > TargetMap;
-#endif
 
   // ObjectType designates the type of data to be stored in the clipboard. This
   // designation is shared across all OSes. The system-specific designation
@@ -233,19 +229,26 @@ class Clipboard {
   // True if we can create a window.
   bool create_window_;
 #elif defined(USE_X11)
-  // Data is stored in the |clipboard_data_| map until it is saved to the system
-  // clipboard. The Store* functions save data to the |clipboard_data_| map. The
-  // SetGtkClipboard function replaces whatever is on the system clipboard with
-  // the contents of |clipboard_data_|.
-  // The Write* functions make a deep copy of the data passed to them an store
-  // it in |clipboard_data_|.
+  // The public API is via WriteObjects() which dispatches to multiple
+  // Write*() calls, but on GTK we must write all the clipboard types
+  // in a single GTK call.  To support this we store the current set
+  // of data we intend to put on the clipboard on clipboard_data_ as
+  // WriteObjects is running, and then at the end call SetGtkClipboard
+  // which replaces whatever is on the system clipboard with the
+  // contents of clipboard_data_.
+
+ public:
+  typedef std::map<FormatType, std::pair<char*, size_t> > TargetMap;
+
+ private:
+  typedef struct _GtkClipboard GtkClipboard;
 
   // Write changes to gtk clipboard.
   void SetGtkClipboard();
   // Insert a mapping into clipboard_data_.
   void InsertMapping(const char* key, char* data, size_t data_len);
 
-  // find the gtk clipboard for the passed buffer enum
+  // Find the gtk clipboard for the passed buffer enum.
   GtkClipboard* LookupBackingClipboard(Buffer clipboard) const;
 
   TargetMap* clipboard_data_;
diff --git a/app/clipboard/clipboard_linux.cc b/app/clipboard/clipboard_linux.cc
index bc8a0b4..4e2a241 100644
--- a/app/clipboard/clipboard_linux.cc
+++ b/app/clipboard/clipboard_linux.cc
@@ -32,7 +32,7 @@ std::string GdkAtomToString(const GdkAtom& atom) {
 }
 
 GdkAtom StringToGdkAtom(const std::string& str) {
-  return gdk_atom_intern(str.c_str(), false);
+  return gdk_atom_intern(str.c_str(), FALSE);
 }
 
 // GtkClipboardGetFunc callback.
@@ -67,11 +67,13 @@ void GetData(GtkClipboard* clipboard,
 
 // GtkClipboardClearFunc callback.
 // We are guaranteed this will be called exactly once for each call to
-// gtk_clipboard_set_with_data
+// gtk_clipboard_set_with_data.
 void ClearData(GtkClipboard* clipboard,
                gpointer user_data) {
   Clipboard::TargetMap* map =
       reinterpret_cast<Clipboard::TargetMap*>(user_data);
+  // The same data may be inserted under multiple keys, so use a set to
+  // uniq them.
   std::set<char*> ptrs;
 
   for (Clipboard::TargetMap::iterator iter = map->begin();
@@ -135,12 +137,9 @@ void Clipboard::SetGtkClipboard() {
   int i = 0;
   for (Clipboard::TargetMap::iterator iter = clipboard_data_->begin();
        iter != clipboard_data_->end(); ++iter, ++i) {
-    targets[i].target = static_cast<gchar*>(malloc(iter->first.size() + 1));
-    memcpy(targets[i].target, iter->first.data(), iter->first.size());
-    targets[i].target[iter->first.size()] = '\0';
-
+    targets[i].target = const_cast<char*>(iter->first.c_str());
     targets[i].flags = 0;
-    targets[i].info = i;
+    targets[i].info = 0;
   }
 
   gtk_clipboard_set_with_data(clipboard_, targets.get(),
@@ -148,8 +147,8 @@ void Clipboard::SetGtkClipboard() {
                               GetData, ClearData,
                               clipboard_data_);
 
-  for (size_t i = 0; i < clipboard_data_->size(); i++)
-    free(targets[i].target);
+  // clipboard_data_ now owned by the GtkClipboard.
+  clipboard_data_ = NULL;
 }
 
 void Clipboard::WriteText(const char* text_data, size_t text_len) {
@@ -198,9 +197,6 @@ void Clipboard::WriteBitmap(const char* pixel_data, const char* size_data) {
 
 void Clipboard::WriteBookmark(const char* title_data, size_t title_len,
                               const char* url_data, size_t url_len) {
-  // Write as plain text.
-  WriteText(url_data, url_len);
-
   // Write as a URI.
   char* data = new char[url_len + 1];
   memcpy(data, url_data, url_len);
@@ -371,40 +367,21 @@ Clipboard::FormatType Clipboard::GetWebKitSmartPasteFormatType() {
   return std::string(kMimeWebkitSmartPaste);
 }
 
-// Insert the key/value pair in the clipboard_data structure. If
-// the mapping already exists, it frees the associated data. Don't worry
-// about double freeing because if the same key is inserted into the
-// map twice, it must have come from different Write* functions and the
-// data pointer cannot be the same.
 void Clipboard::InsertMapping(const char* key,
                               char* data,
                               size_t data_len) {
-  TargetMap::iterator iter = clipboard_data_->find(key);
-
-  if (iter != clipboard_data_->end()) {
-    if (strcmp(kMimeBmp, key) == 0)
-      g_object_unref(reinterpret_cast<GdkPixbuf*>(iter->second.first));
-    else
-      delete[] iter->second.first;
-  }
-
+  DCHECK(clipboard_data_->find(key) == clipboard_data_->end());
   (*clipboard_data_)[key] = std::make_pair(data, data_len);
 }
 
 GtkClipboard* Clipboard::LookupBackingClipboard(Buffer clipboard) const {
-  GtkClipboard* result;
-
   switch (clipboard) {
     case BUFFER_STANDARD:
-      result = clipboard_;
-      break;
+      return clipboard_;
     case BUFFER_SELECTION:
-      result = primary_selection_;
-      break;
+      return primary_selection_;
     default:
       NOTREACHED();
-      result = NULL;
-      break;
+      return NULL;
   }
-  return result;
 }
diff --git a/app/clipboard/clipboard_unittest.cc b/app/clipboard/clipboard_unittest.cc
index 5b1d48a8..e4f288a 100644
--- a/app/clipboard/clipboard_unittest.cc
+++ b/app/clipboard/clipboard_unittest.cc
@@ -335,3 +335,22 @@ TEST_F(ClipboardTest, BitmapTest) {
                                           Clipboard::BUFFER_STANDARD));
 }
 #endif  // defined(OS_WIN)
+
+// Test writing all formats we have simultaneously.
+TEST_F(ClipboardTest, WriteEverything) {
+  Clipboard clipboard;
+
+  {
+    ScopedClipboardWriter writer(&clipboard);
+    writer.WriteText(UTF8ToUTF16("foo"));
+    writer.WriteURL(UTF8ToUTF16("foo"));
+    writer.WriteHTML(UTF8ToUTF16("foo"), "bar");
+    writer.WriteBookmark(UTF8ToUTF16("foo"), "bar");
+    writer.WriteHyperlink("foo", "bar");
+    writer.WriteWebSmartPaste();
+    // Left out: WriteFile, WriteFiles, WriteBitmapFromPixels, WritePickledData.
+  }
+
+  // Passes if we don't crash.
+}
+