Rewrite clipboard write IPC handling to be easier to understand.

ui/base/clipboard/clipboard.cc

Index: ui/base/clipboard/clipboard.cc
diff --git a/ui/base/clipboard/clipboard.cc b/ui/base/clipboard/clipboard.cc
index 84371acea734acdf5af8947848cb6e320970d617..dae4840b125617b97f8c67aad783541ec4f1d9b7 100644
--- a/ui/base/clipboard/clipboard.cc
+++ b/ui/base/clipboard/clipboard.cc
@@ -18,23 +18,6 @@ namespace ui {
 
 namespace {
 
-// Valides a shared bitmap on the clipboard.
-// Returns true if the clipboard data makes sense and it's safe to access the
-// bitmap.
-bool ValidateAndMapSharedBitmap(size_t bitmap_bytes,
-                                base::SharedMemory* bitmap_data) {
-  using base::SharedMemory;
-
-  if (!bitmap_data || !SharedMemory::IsHandleValid(bitmap_data->handle()))
-    return false;
-
-  if (!bitmap_data->Map(bitmap_bytes)) {
-    PLOG(ERROR) << "Failed to map bitmap memory";
-    return false;
-  }
-  return true;
-}
-
 // A list of allowed threads. By default, this is empty and no thread checking
 // is done (in the unit test case), but a user (like content) can set which
 // threads are allowed to call this method.
@@ -106,13 +89,6 @@ void Clipboard::DestroyClipboardForCurrentThread() {
 }
 
 void Clipboard::DispatchObject(ObjectType type, const ObjectMapParams& params) {
-  // All types apart from CBF_WEBKIT need at least 1 non-empty param.
-  if (type != CBF_WEBKIT && (params.empty() || params[0].empty()))
-    return;
-  // Some other types need a non-empty 2nd param.
-  if ((type == CBF_BOOKMARK || type == CBF_SMBITMAP || type == CBF_DATA) &&
-      (params.size() != 2 || params[1].empty()))
-    return;
   switch (type) {
     case CBF_TEXT:
       WriteText(&(params[0].front()), params[0].size());
@@ -143,41 +119,8 @@ void Clipboard::DispatchObject(ObjectType type, const ObjectMapParams& params) {
       break;
 
     case CBF_SMBITMAP: {
-      using base::SharedMemory;
-      using base::SharedMemoryHandle;
-
-      if (params[0].size() != sizeof(SharedMemory*) ||
-          params[1].size() != sizeof(gfx::Size)) {
-        return;
-      }
-
-      SkBitmap bitmap;
-      const gfx::Size* unvalidated_size =
-          reinterpret_cast<const gfx::Size*>(&params[1].front());
-      // Let Skia do some sanity checking for us (no negative widths/heights, no
-      // overflows while calculating bytes per row, etc).
-      if (!bitmap.setInfo(SkImageInfo::MakeN32Premul(
-          unvalidated_size->width(), unvalidated_size->height()))) {
-        return;
-      }
-      // Make sure the size is representable as a signed 32-bit int, so
-      // SkBitmap::getSize() won't be truncated.
-      if (!sk_64_isS32(bitmap.computeSize64()))
-        return;
-
-      // It's OK to cast away constness here since we map the handle as
-      // read-only.
-      const char* raw_bitmap_data_const =
-          reinterpret_cast<const char*>(&params[0].front());
-      char* raw_bitmap_data = const_cast<char*>(raw_bitmap_data_const);
-      scoped_ptr<SharedMemory> bitmap_data(
-          *reinterpret_cast<SharedMemory**>(raw_bitmap_data));
-
-      if (!ValidateAndMapSharedBitmap(bitmap.getSize(), bitmap_data.get()))
-        return;
-      bitmap.setPixels(bitmap_data->memory());
-
-      WriteBitmap(bitmap);
+      WriteBitmap(
+          **reinterpret_cast<const SkBitmap* const*>(&(params[0].front())));

[jamesr, 2014/10/09 22:34:12]

i can't follow this expression. what is the type of param[0].front()? could you
use some intermediate vars here with the types spelled out to see what's
happening?

[dcheng, 2014/10/09 22:39:17]

Would you find this clearer if it used vector_as_array and aliased that into a
char* pointer?
The only reason I used params[0].front() is to match the existing style.

[jamesr, 2014/10/09 22:41:28]

I'd find it clearer if the intermediates of this expression were spelled out. 
For instance, I can't tell easily what the type of params[0].front() is, so it's
hard to understand what the address of it means and how it's being cast to a
const SkBitmap* const* or why that's being dereferenced twice.

[dcheng, 2014/10/13 05:52:21]

Done. I've added some intermediate types in both
ScopedClipboardWriter::WriteImage and in here to make it a little more obvious
what's going on.

       break;
     }
 
@@ -194,40 +137,4 @@ void Clipboard::DispatchObject(ObjectType type, const ObjectMapParams& params) {
   }
 }
 
-// static
-bool Clipboard::ReplaceSharedMemHandle(ObjectMap* objects,
-                                       base::SharedMemoryHandle bitmap_handle,
-                                       base::ProcessHandle process) {
-  using base::SharedMemory;
-  bool has_shared_bitmap = false;
-
-  for (ObjectMap::iterator iter = objects->begin(); iter != objects->end();
-       ++iter) {
-    if (iter->first == CBF_SMBITMAP) {
-      // The code currently only accepts sending a single bitmap over this way.
-      // Fail if we ever encounter more than one shmem bitmap structure to fill.
-      if (has_shared_bitmap)
-        return false;
-
-#if defined(OS_WIN)
-      SharedMemory* bitmap = new SharedMemory(bitmap_handle, true, process);
-#else
-      SharedMemory* bitmap = new SharedMemory(bitmap_handle, true);
-#endif
-
-      // There must always be two parameters associated with each shmem bitmap.
-      if (iter->second.size() != 2)
-        return false;
-
-      // We store the shared memory object pointer so it can be retrieved by the
-      // UI thread (see DispatchObject()).
-      iter->second[0].clear();
-      for (size_t i = 0; i < sizeof(SharedMemory*); ++i)
-        iter->second[0].push_back(reinterpret_cast<char*>(&bitmap)[i]);
-      has_shared_bitmap = true;
-    }
-  }
-  return true;
-}
-
 }  // namespace ui