Fix multiple file paths issue for dragging

This CL reimplements to use one CF_HDROP clipboard format having
series of file path's strings instead of series of CF_HDROP having
a file path string to get multiple file paths correctly for
dragging.

BUG=878690

Change-Id: I167e93f6411f866cb143b910b1fc7d4896481ffc
Reviewed-on: https://chromium-review.googlesource.com/1195176Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Commit-Queue: Miyoung Shin <myid.shin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#595759}

services/ws/drag_drop_delegate_unittest.cc

@@ -26,15 +26,6 @@
 #include "ui/wm/core/default_screen_position_client.h"
 #include "url/gurl.h"
 
-namespace ui {
-
-// An equal-to operator to make EXPECT_EQ happy.
-bool operator==(const FileInfo& info1, const FileInfo& info2) {
-  return info1.path == info2.path && info1.display_name == info2.display_name;
-}
-
-}  // namespace ui
-
 namespace ws {
 
 class DragDropDelegateTest : public testing::Test {

ui/base/dragdrop/file_info.cc

@@ -14,4 +14,8 @@ FileInfo::FileInfo(const base::FilePath& path,
 
 FileInfo::~FileInfo() {}
 
+bool FileInfo::operator==(const FileInfo& other) const {
+  return path == other.path && display_name == other.display_name;
+}
+
 }  // namespace ui

ui/base/dragdrop/file_info.h

@@ -15,6 +15,7 @@ struct UI_BASE_EXPORT FileInfo {
   FileInfo();
   FileInfo(const base::FilePath& path, const base::FilePath& display_name);
   ~FileInfo();
+  bool operator==(const FileInfo& other) const;
 
   base::FilePath path;
   base::FilePath display_name;  // Optional.

ui/base/dragdrop/os_exchange_data_provider_win.cc

@@ -57,8 +57,9 @@ static void GetInternetShortcutFileContents(const GURL& url, std::string* data);
 static void CreateValidFileNameFromTitle(const GURL& url,
                                          const base::string16& title,
                                          base::string16* validated);
-// Creates a new STGMEDIUM object to hold a file.
-static STGMEDIUM* GetStorageForFileName(const base::FilePath& path);
+// Creates a new STGMEDIUM object to hold files.
+static STGMEDIUM* GetStorageForFileNames(
+    const std::vector<FileInfo>& filenames);
 static STGMEDIUM* GetIDListStorageForFileName(const base::FilePath& path);
 // Creates a File Descriptor for the creation of a file to the given URL and
 // returns a handle to it.
@@ -346,7 +347,7 @@ void OSExchangeDataProviderWin::SetURL(const GURL& url,
       Clipboard::GetUrlFormatType().ToFormatEtc(), storage));
 
   // TODO(beng): add CF_HTML.
-  // http://code.google.com/p/chromium/issues/detail?id=6767
+  // http://code.google.com/p/chromium/issues/detail?id=6767GetIDListStorageForFileName
 
   // Also add text representations (these should be last since they're the
   // least preferable).
@@ -354,11 +355,9 @@ void OSExchangeDataProviderWin::SetURL(const GURL& url,
 }
 
 void OSExchangeDataProviderWin::SetFilename(const base::FilePath& path) {
-  STGMEDIUM* storage = GetStorageForFileName(path);
-  data_->contents_.push_back(std::make_unique<DataObjectImpl::StoredDataInfo>(
-      Clipboard::GetCFHDropFormatType().ToFormatEtc(), storage));
+  SetFilenames({FileInfo(path, base::FilePath())});
 
-  storage = GetIDListStorageForFileName(path);
+  STGMEDIUM* storage = GetIDListStorageForFileName(path);
   if (!storage)
     return;
   data_->contents_.push_back(std::make_unique<DataObjectImpl::StoredDataInfo>(
@@ -367,11 +366,12 @@ void OSExchangeDataProviderWin::SetFilename(const base::FilePath& path) {
 
 void OSExchangeDataProviderWin::SetFilenames(
     const std::vector<FileInfo>& filenames) {
-  for (size_t i = 0; i < filenames.size(); ++i) {
-    STGMEDIUM* storage = GetStorageForFileName(filenames[i].path);
-    data_->contents_.push_back(std::make_unique<DataObjectImpl::StoredDataInfo>(
-        Clipboard::GetCFHDropFormatType().ToFormatEtc(), storage));
-  }
+  STGMEDIUM* storage = GetStorageForFileNames(filenames);
+  if (!storage)
+    return;
+
+  data_->contents_.push_back(std::make_unique<DataObjectImpl::StoredDataInfo>(
+      Clipboard::GetCFHDropFormatType().ToFormatEtc(), storage));
 }
 
 void OSExchangeDataProviderWin::SetPickledData(
@@ -537,7 +537,7 @@ void OSExchangeDataProviderWin::SetDownloadFileInfo(
   // think we always synthesize one in WebContentsDragWin.
   STGMEDIUM* storage = NULL;
   if (!download.filename.empty())
-    storage = GetStorageForFileName(download.filename);
+    GetStorageForFileNames({FileInfo(download.filename, base::FilePath())});
 
   // Add CF_HDROP.
   auto info = std::make_unique<DataObjectImpl::StoredDataInfo>(
@@ -752,8 +752,12 @@ void DataObjectImpl::OnDownloadCompleted(const base::FilePath& file_path) {
       }
 
       // Update the storage.
-      (*iter)->owns_medium = true;
-      (*iter)->medium = GetStorageForFileName(file_path);
+      STGMEDIUM* storage =
+          GetStorageForFileNames({FileInfo(file_path, base::FilePath())});
+      if (storage) {
+        (*iter)->owns_medium = true;
+        (*iter)->medium = storage;
+      }
 
       break;
     }
@@ -1003,26 +1007,52 @@ static void CreateValidFileNameFromTitle(const GURL& url,
   *validated += extension;
 }
 
-static STGMEDIUM* GetStorageForFileName(const base::FilePath& path) {
-  const size_t kDropSize = sizeof(DROPFILES);
-  const size_t kTotalBytes =
-      kDropSize + (path.value().length() + 2) * sizeof(wchar_t);
-  HANDLE hdata = GlobalAlloc(GMEM_MOVEABLE, kTotalBytes);
+static STGMEDIUM* GetStorageForFileNames(
+    const std::vector<FileInfo>& filenames) {
+  // CF_HDROP clipboard format consists of DROPFILES structure, a series of file
+  // names including the terminating null character and the additional null
+  // character at the tail to terminate the array.
+  // For example,
+  //| DROPFILES | FILENAME 1 | NULL | ... | FILENAME n | NULL | NULL |
+  // For more details, please refer to
+  // https://docs.microsoft.com/ko-kr/windows/desktop/shell/clipboard#cf_hdrop
+
+  if (filenames.empty())
+    return nullptr;
+
+  const size_t kDropFilesHeaderSizeInBytes = sizeof(DROPFILES);
+  size_t total_bytes = kDropFilesHeaderSizeInBytes;
+  for (const auto& filename : filenames) {
+    // Allocate memory of the filename's length including the null
+    // character.
+    total_bytes += (filename.path.value().length() + 1) * sizeof(wchar_t);
+  }
+  // |data| needs to be terminated by an additional null character.
+  total_bytes += sizeof(wchar_t);
+
+  // GHND combines GMEM_MOVEABLE and GMEM_ZEROINIT, and GMEM_ZEROINIT
+  // initializes memory contents to zero.
+  HANDLE hdata = GlobalAlloc(GHND, total_bytes);
 
   base::win::ScopedHGlobal<DROPFILES*> locked_mem(hdata);
   DROPFILES* drop_files = locked_mem.get();
   drop_files->pFiles = sizeof(DROPFILES);
   drop_files->fWide = TRUE;
+
   wchar_t* data = reinterpret_cast<wchar_t*>(
-      reinterpret_cast<BYTE*>(drop_files) + kDropSize);
-  const size_t copy_size = (path.value().length() + 1) * sizeof(wchar_t);
-  memcpy(data, path.value().c_str(), copy_size);
-  data[path.value().length() + 1] = L'\0';  // Double NULL
+      reinterpret_cast<BYTE*>(drop_files) + kDropFilesHeaderSizeInBytes);
+
+  size_t next_filename_offset = 0;
+  for (const auto& filename : filenames) {
+    wcscpy(data + next_filename_offset, filename.path.value().c_str());
+    // Skip the terminating null character of the filename.
+    next_filename_offset += filename.path.value().length() + 1;
+  }
 
   STGMEDIUM* storage = new STGMEDIUM;
   storage->tymed = TYMED_HGLOBAL;
   storage->hGlobal = hdata;
-  storage->pUnkForRelease = NULL;
+  storage->pUnkForRelease = nullptr;
   return storage;
 }
 

ui/base/dragdrop/os_exchange_data_win_unittest.cc

@@ -11,6 +11,7 @@
 #include "base/win/scoped_hglobal.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "ui/base/clipboard/clipboard.h"
+#include "ui/base/dragdrop/file_info.h"
 #include "ui/base/dragdrop/os_exchange_data_provider_win.h"
 #include "url/gurl.h"
 
@@ -303,6 +304,23 @@ TEST(OSExchangeDataWinTest, FileContents) {
   EXPECT_EQ(file_contents, read_contents);
 }
 
+TEST(OSExchangeDataWinTest, Filenames) {
+  OSExchangeData data;
+  const std::vector<FileInfo> kTestFilenames = {
+      {base::FilePath(FILE_PATH_LITERAL("C:\\tmp\\test_file1")),
+       base::FilePath()},
+      {base::FilePath(FILE_PATH_LITERAL("C:\\tmp\\test_file2")),
+       base::FilePath()},
+  };
+  data.SetFilenames(kTestFilenames);
+
+  OSExchangeData copy(data.provider().Clone());
+  std::vector<FileInfo> dropped_filenames;
+
+  EXPECT_TRUE(copy.GetFilenames(&dropped_filenames));
+  EXPECT_EQ(kTestFilenames, dropped_filenames);
+}
+
 TEST(OSExchangeDataWinTest, CFHtml) {
   OSExchangeData data;
   GURL url("http://www.google.com/");