Move tab drag checking from Ash to Chrome

To avoid duplicating the tab drag MIME types, there are two options:
move the strings to a common dependency between Ash and Chrome, or move
the check to the Chrome side through a delegate.

This CL does the latter as it is more straightforward.

Bug: 1069869
Change-Id: I3d0c77cd2d7c6130d075074698cb8717d30362e1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2198650
Commit-Queue: Collin Baker <collinbaker@chromium.org>
Reviewed-by: John Lee <johntlee@chromium.org>
Reviewed-by: Xiaoqian Dai <xdai@chromium.org>
Reviewed-by: Mitsuru Oshima <oshima@chromium.org>
Cr-Commit-Position: refs/heads/master@{#768577}

ash/drag_drop/tab_drag_drop_delegate.cc

@@ -41,30 +41,7 @@ bool TabDragDropDelegate::IsChromeTabDrag(const ui::OSExchangeData& drag_data) {
   if (!features::IsWebUITabStripTabDragIntegrationEnabled())
     return false;
 
-  base::Pickle pickle;
-  drag_data.GetPickledData(ui::ClipboardFormatType::GetWebCustomDataType(),
-                           &pickle);
-  base::PickleIterator iter(pickle);
-
-  uint32_t entry_count = 0;
-  if (!iter.ReadUInt32(&entry_count))
-    return false;
-
-  for (uint32_t i = 0; i < entry_count; ++i) {
-    base::StringPiece16 type;
-    base::StringPiece16 data;
-    if (!iter.ReadStringPiece16(&type) || !iter.ReadStringPiece16(&data))
-      return false;
-
-    // TODO(https://crbug.com/1069869): share this constant between Ash
-    // and Chrome instead of hardcoding it in both places.
-    static const base::NoDestructor<base::string16> chrome_tab_type(
-        base::ASCIIToUTF16("application/vnd.chromium.tab"));
-    if (type == *chrome_tab_type)
-      return true;
-  }
-
-  return false;
+  return Shell::Get()->shell_delegate()->IsTabDrag(drag_data);
 }
 
 TabDragDropDelegate::TabDragDropDelegate(

ash/drag_drop/tab_drag_drop_delegate_unittest.cc

@@ -5,6 +5,7 @@
 #include "ash/drag_drop/tab_drag_drop_delegate.h"
 
 #include <memory>
+#include <utility>
 #include <vector>
 
 #include "ash/public/cpp/ash_features.h"
@@ -29,40 +30,26 @@
 #include "ui/gfx/geometry/vector2d.h"
 
 using ::testing::_;
+using ::testing::NiceMock;
 using ::testing::Return;
 
 namespace ash {
 
 namespace {
 
-std::unique_ptr<ui::OSExchangeData> MakeDragData(const std::string& mime_type,
-                                                 const std::string& data) {
-  auto result = std::make_unique<ui::OSExchangeData>();
-
-  base::flat_map<base::string16, base::string16> data_map;
-  data_map.emplace(base::ASCIIToUTF16(mime_type), base::ASCIIToUTF16(data));
-
-  base::Pickle inner_data;
-  ui::WriteCustomDataToPickle(data_map, &inner_data);
-
-  result->SetPickledData(ui::ClipboardFormatType::GetWebCustomDataType(),
-                         inner_data);
-  return result;
-}
-
 class MockShellDelegate : public TestShellDelegate {
  public:
   MockShellDelegate() = default;
   ~MockShellDelegate() override = default;
 
+  MOCK_METHOD(bool, IsTabDrag, (const ui::OSExchangeData&), (override));
+
   MOCK_METHOD(aura::Window*,
               CreateBrowserForTabDrop,
               (aura::Window*, const ui::OSExchangeData&),
               (override));
 };
 
-static constexpr char kTabMimeType[] = "application/vnd.chromium.tab";
-
 }  // namespace
 
 class TabDragDropDelegateTest : public AshTestBase {
@@ -75,7 +62,7 @@ class TabDragDropDelegateTest : public AshTestBase {
 
   // AshTestBase:
   void SetUp() override {
-    auto mock_shell_delegate = std::make_unique<MockShellDelegate>();
+    auto mock_shell_delegate = std::make_unique<NiceMock<MockShellDelegate>>();
     mock_shell_delegate_ = mock_shell_delegate.get();
     AshTestBase::SetUp(std::move(mock_shell_delegate));
     ash::TabletModeControllerTestApi().EnterTabletMode();
@@ -91,17 +78,15 @@ class TabDragDropDelegateTest : public AshTestBase {
 
  private:
   base::test::ScopedFeatureList scoped_feature_list_;
-  MockShellDelegate* mock_shell_delegate_ = nullptr;
+  NiceMock<MockShellDelegate>* mock_shell_delegate_ = nullptr;
 };
 
-TEST_F(TabDragDropDelegateTest, AcceptsValidDrags) {
-  EXPECT_TRUE(
-      TabDragDropDelegate::IsChromeTabDrag(*MakeDragData(kTabMimeType, "foo")));
-}
+TEST_F(TabDragDropDelegateTest, ForwardsDragCheckToShellDelegate) {
+  ON_CALL(*mock_shell_delegate(), IsTabDrag(_)).WillByDefault(Return(false));
+  EXPECT_FALSE(TabDragDropDelegate::IsChromeTabDrag(ui::OSExchangeData()));
 
-TEST_F(TabDragDropDelegateTest, RejectsInvalidDrags) {
-  EXPECT_FALSE(
-      TabDragDropDelegate::IsChromeTabDrag(*MakeDragData("text/plain", "bar")));
+  ON_CALL(*mock_shell_delegate(), IsTabDrag(_)).WillByDefault(Return(true));
+  EXPECT_TRUE(TabDragDropDelegate::IsChromeTabDrag(ui::OSExchangeData()));
 }
 
 TEST_F(TabDragDropDelegateTest, DragToExistingTabStrip) {
@@ -146,8 +131,8 @@ TEST_F(TabDragDropDelegateTest, DragToNewWindow) {
       .Times(1)
       .WillOnce(Return(new_window.get()));
 
-  auto drop_data = MakeDragData(kTabMimeType, "fake_id");
-  delegate.Drop(drag_start_location + gfx::Vector2d(2, 0), *drop_data);
+  delegate.Drop(drag_start_location + gfx::Vector2d(2, 0),
+                ui::OSExchangeData());
 
   EXPECT_FALSE(
       SplitViewController::Get(source_window.get())->InTabletSplitViewMode());
@@ -176,8 +161,7 @@ TEST_F(TabDragDropDelegateTest, DropOnEdgeEntersSplitView) {
       .Times(1)
       .WillOnce(Return(new_window.get()));
 
-  auto drop_data = MakeDragData(kTabMimeType, "fake_id");
-  delegate.Drop(drag_end_location, *drop_data);
+  delegate.Drop(drag_end_location, ui::OSExchangeData());
 
   SplitViewController* const split_view_controller =
       SplitViewController::Get(source_window.get());

ash/shell_delegate.cc

@@ -6,6 +6,10 @@
 
 namespace ash {
 
+bool ShellDelegate::IsTabDrag(const ui::OSExchangeData& drop_data) {
+  return false;
+}
+
 aura::Window* ShellDelegate::CreateBrowserForTabDrop(
     aura::Window* source_window,
     const ui::OSExchangeData& drop_data) {

ash/shell_delegate.h

@@ -58,6 +58,11 @@ class ASH_EXPORT ShellDelegate {
   // Check whether the current tab of the browser window can go back.
   virtual bool CanGoBack(gfx::NativeWindow window) const = 0;
 
+  // Checks whether a drag-drop operation is a tab drag.
+  virtual bool IsTabDrag(const ui::OSExchangeData& drop_data);
+
+  // Drops tab in a new browser window. |drop_data| must be from a tab
+  // drag as determined by IsTabDrag() above.
   virtual aura::Window* CreateBrowserForTabDrop(
       aura::Window* source_window,
       const ui::OSExchangeData& drop_data);

chrome/browser/ui/ash/chrome_shell_delegate.cc

@@ -5,6 +5,7 @@
 #include "chrome/browser/ui/ash/chrome_shell_delegate.h"
 
 #include <memory>
+#include <utility>
 
 #include "ash/public/cpp/ash_features.h"
 #include "ash/screenshot_delegate.h"
@@ -73,10 +74,15 @@ bool ChromeShellDelegate::CanGoBack(gfx::NativeWindow window) const {
   return contents->GetController().CanGoBack();
 }
 
+bool ChromeShellDelegate::IsTabDrag(const ui::OSExchangeData& drop_data) {
+  DCHECK(ash::features::IsWebUITabStripTabDragIntegrationEnabled());
+  return tab_strip_ui::IsDraggedTab(drop_data);
+}
+
 aura::Window* ChromeShellDelegate::CreateBrowserForTabDrop(
     aura::Window* source_window,
     const ui::OSExchangeData& drop_data) {
-  CHECK(ash::features::IsWebUITabStripTabDragIntegrationEnabled());
+  DCHECK(ash::features::IsWebUITabStripTabDragIntegrationEnabled());
 
   BrowserView* source_view = BrowserView::GetBrowserViewForNativeWindow(
       source_window->GetToplevelWindow());

chrome/browser/ui/ash/chrome_shell_delegate.h

@@ -5,6 +5,8 @@
 #ifndef CHROME_BROWSER_UI_ASH_CHROME_SHELL_DELEGATE_H_
 #define CHROME_BROWSER_UI_ASH_CHROME_SHELL_DELEGATE_H_
 
+#include <memory>
+
 #include "ash/shell_delegate.h"
 #include "base/macros.h"
 
@@ -22,6 +24,7 @@ class ChromeShellDelegate : public ash::ShellDelegate {
       ash::BackGestureContextualNudgeController* controller) override;
   void OpenKeyboardShortcutHelpPage() const override;
   bool CanGoBack(gfx::NativeWindow window) const override;
+  bool IsTabDrag(const ui::OSExchangeData& drop_data) override;
   aura::Window* CreateBrowserForTabDrop(
       aura::Window* source_window,
       const ui::OSExchangeData& drop_data) override;

chrome/browser/ui/webui/tab_strip/tab_strip_ui_util.cc

@@ -4,6 +4,10 @@
 
 #include "chrome/browser/ui/webui/tab_strip/tab_strip_ui_util.h"
 
+#include <memory>
+#include <string>
+#include <utility>
+
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/extensions/extension_tab_util.h"
@@ -72,12 +76,37 @@ void MoveTabAcrossWindows(Browser* source_browser,
       to_index, std::move(detached_contents), add_types, to_group_id);
 }
 
+bool IsDraggedTab(const ui::OSExchangeData& drop_data) {
+  base::Pickle pickle;
+  drop_data.GetPickledData(ui::ClipboardFormatType::GetWebCustomDataType(),
+                           &pickle);
+  base::PickleIterator iter(pickle);
+
+  uint32_t entry_count = 0;
+  if (!iter.ReadUInt32(&entry_count))
+    return false;
+
+  for (uint32_t i = 0; i < entry_count; ++i) {
+    base::StringPiece16 type;
+    base::StringPiece16 data;
+    if (!iter.ReadStringPiece16(&type) || !iter.ReadStringPiece16(&data))
+      return false;
+
+    // TODO(https://crbug.com/1069869): handle tab group drags.
+    if (type == base::ASCIIToUTF16(kWebUITabIdDataType))
+      return true;
+  }
+
+  return false;
+}
+
 bool DropTabsInNewBrowser(Browser* new_browser,
                           const ui::OSExchangeData& drop_data) {
   base::Pickle pickle;
   drop_data.GetPickledData(ui::ClipboardFormatType::GetWebCustomDataType(),
                            &pickle);
 
+  // TODO(https://crbug.com/1069869): handle tab group drags.
   base::string16 tab_id_str;
   ui::ReadCustomDataForType(pickle.data(), pickle.size(),
                             base::ASCIIToUTF16(kWebUITabIdDataType),

chrome/browser/ui/webui/tab_strip/tab_strip_ui_util.h

@@ -5,6 +5,8 @@
 #ifndef CHROME_BROWSER_UI_WEBUI_TAB_STRIP_TAB_STRIP_UI_UTIL_H_
 #define CHROME_BROWSER_UI_WEBUI_TAB_STRIP_TAB_STRIP_UI_UTIL_H_
 
+#include <string>
+
 #include "base/optional.h"
 #include "components/tab_groups/tab_group_id.h"
 
@@ -31,6 +33,10 @@ void MoveTabAcrossWindows(
     int to_index,
     base::Optional<tab_groups::TabGroupId> to_group_id = base::nullopt);
 
+// Returns whether |drop_data| is a tab drag originating from a WebUI
+// tab strip.
+bool IsDraggedTab(const ui::OSExchangeData& drop_data);
+
 // Handles dropping tabs not destined for an existing tab strip.
 // |new_browser| should be the newly created Browser with no tabs, and
 // must have the same profile as the drag source. |drop_data| must have