ChromeOS MD: Fix cannot drag tab onto immersive fullscreen window

chrome/browser/ui/views/tabs/tab_drag_controller.cc

Index: chrome/browser/ui/views/tabs/tab_drag_controller.cc
diff --git a/chrome/browser/ui/views/tabs/tab_drag_controller.cc b/chrome/browser/ui/views/tabs/tab_drag_controller.cc
index 1997b873a2349ff601ee1869a531a63314eee050..c5a06f563401a5ac1a4f825552f682dd77951cf2 100644
--- a/chrome/browser/ui/views/tabs/tab_drag_controller.cc
+++ b/chrome/browser/ui/views/tabs/tab_drag_controller.cc
@@ -111,6 +111,22 @@ gfx::NativeWindow GetModalTransient(gfx::NativeWindow window) {
 }
 #endif
 
+// Returns the browser view that contains the |tabstrip|.
+BrowserView* GetBrowserViewForTabStrip(TabStrip* tabstrip) {
+  gfx::NativeWindow window = tabstrip->GetWidget()->GetNativeWindow();
+  return BrowserView::GetBrowserViewForNativeWindow(window);
+}
+
+// Returns the bounds of the tabstrip in browser view coordinates.
+gfx::Rect GetBoundsForTabStripInBrowserView(TabStrip* tabstrip) {
+  BrowserView* browser_view = GetBrowserViewForTabStrip(tabstrip);
+  gfx::RectF bounds_f(
+      browser_view->frame()->GetBoundsForTabStrip(browser_view->tabstrip()));
+  views::View::ConvertRectToTarget(browser_view->parent(), browser_view,
+                                   &bounds_f);
+  return gfx::ToEnclosingRect(bounds_f);
+}
+
 // Returns true if |bounds| contains the y-coordinate |y|. The y-coordinate
 // of |bounds| is adjusted by |vertical_adjustment|.
 bool DoesRectContainVerticalPointExpanded(
@@ -856,14 +872,17 @@ TabStrip* TabDragController::GetTabStripForWindow(gfx::NativeWindow window) {
 bool TabDragController::DoesTabStripContain(
     TabStrip* tabstrip,
     const gfx::Point& point_in_screen) const {
-  // Make sure the specified screen point is actually within the bounds of the
-  // specified tabstrip...
-  gfx::Rect tabstrip_bounds = GetViewScreenBounds(tabstrip);
-  return point_in_screen.x() < tabstrip_bounds.right() &&
-      point_in_screen.x() >= tabstrip_bounds.x() &&
-      DoesRectContainVerticalPointExpanded(tabstrip_bounds,
-                                           kVerticalDetachMagnetism,
-                                           point_in_screen.y());
+  // Make sure the specified screen point is within the bounds of the specified
+  // tabstrip where it should be laid when revealed.
+  gfx::Rect tabstrip_bounds = GetBoundsForTabStripInBrowserView(tabstrip);
+  BrowserView* browser_view = GetBrowserViewForTabStrip(tabstrip);
+  gfx::Point point_in_browser_view(point_in_screen.x(), point_in_screen.y());
+  views::View::ConvertPointFromScreen(browser_view, &point_in_browser_view);
+  return point_in_browser_view.x() < tabstrip_bounds.right() &&
+         point_in_browser_view.x() >= tabstrip_bounds.x() &&
+         DoesRectContainVerticalPointExpanded(tabstrip_bounds,
+                                              kVerticalDetachMagnetism,
+                                              point_in_browser_view.y());
 }
 
 void TabDragController::Attach(TabStrip* attached_tabstrip,
@@ -879,6 +898,10 @@ void TabDragController::Attach(TabStrip* attached_tabstrip,
   std::vector<Tab*> tabs =
       GetTabsMatchingDraggedContents(attached_tabstrip_);
 
+  // Make a StartedDraggingTabs call here to GetRevealedLock so that if
+  // immersive mode is unrevealed, reveal it.
+  attached_tabstrip_->StartedDraggingTabs(tabs);

[Qiang(Joe) Xu, 2017/01/22 01:43:57]

We should get ImmersiveModeCOntrollerAsh::OnImmersiveRevealedStarted called here
(which will layout tabstrip region). A better way might be calling
StartedDraggingTabs, to let TabStripController know that user started dragging
tabs, which will call GetRevealedLock to start revealing. There is also a
StartedDraggingTabs(tabs) call after InsertWebContentsAt call.

[sky, 2017/01/23 16:19:06]

StartedDraggingTabs is called on line 949.

[Qiang(Joe) Xu, 2017/01/23 17:50:37]

Yes, I am aware of that. The problem is here:
https://cs.chromium.org/chromium/src/chrome/browser/ui/views/tabs/tab_drag_co...,
the attached_->ideal_bounds(i) will give the minimum tab size(32, 29), not the
actual tab width, which makes the logic broken for GetInsertionIndexFrom. So we
have to reveal the tabstrip before insertion happens. Equivalently, make this
happen
https://cs.chromium.org/chromium/src/chrome/browser/ui/views/frame/browser_vi...
with reveal unlocked before insertion. I am calling StartedDraggingTabs here,
but the only call I need is
https://cs.chromium.org/chromium/src/chrome/browser/ui/views/tabs/tab_strip.c...
(controller_->OnStartedDraggingTabs()). I think make a call of
StartedDraggingTabs with |tabs| before insertion is OK. It is doing
layouting/animation the UI work with the state just before attaching new tab
https://cs.chromium.org/chromium/src/chrome/browser/ui/views/tabs/tab_strip.c...

[sky, 2017/01/23 20:07:26]

So, you're solution to this is to call the function twice? That doesn't strike
you as a hack?

[Qiang(Joe) Xu, 2017/01/26 17:43:08]

Hi, I am sorry I thought this could work. Another solution would be do the work
for ideal bounds for tabs, which I updated in the new patch.

+
   if (tabs.empty()) {
     // Transitioning from detached to attached to a new tabstrip. Add tabs to
     // the new model.