ChromeOS MD: Fix cannot drag tab onto immersive fullscreen window

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

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/ui/views/tabs/tab_drag_controller.h"
 
 #include <math.h>
 #include <set>
 
 #include "base/auto_reset.h"
@@ skipping 93 matching lines @@
 gfx::NativeWindow GetModalTransient(gfx::NativeWindow window) {
   return wm::GetModalTransient(window);
 }
 #else
 gfx::NativeWindow GetModalTransient(gfx::NativeWindow window) {
   NOTIMPLEMENTED();
   return NULL;
 }
 #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(
     const gfx::Rect& bounds,
     int vertical_adjustment,
     int y) {
   int upper_threshold = bounds.bottom() + vertical_adjustment;
   int lower_threshold = bounds.y() - vertical_adjustment;
   return y >= lower_threshold && y <= upper_threshold;
 }
@@ skipping 725 matching lines @@
       attached_tabstrip_ ? attached_tabstrip_ : source_tabstrip_;
   DCHECK(tab_strip);
 
   return other_tabstrip->controller()->IsCompatibleWith(tab_strip) ?
       other_tabstrip : NULL;
 }
 
 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,
                                const gfx::Point& point_in_screen) {
   TRACE_EVENT1("views", "TabDragController::Attach",
                "point_in_screen", point_in_screen.ToString());
 
   DCHECK(!attached_tabstrip_);  // We should already have detached by the time
                                 // we get here.
 
   attached_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.
 
     selection_model_before_attach_.Copy(attached_tabstrip->GetSelectionModel());
 
     // Inserting counts as a move. We don't want the tabs to jitter when the
     // user moves the tab immediately after attaching it.
     last_move_screen_loc_ = point_in_screen.x();
 
@@ skipping 903 matching lines @@
   // TODO(pkotwicz): Fix this properly (crbug.com/358482)
   for (auto* browser : *BrowserList::GetInstance()) {
     if (browser->tab_strip_model()->empty())
       exclude.insert(browser->window()->GetNativeWindow());
   }
 #endif
   base::WeakPtr<TabDragController> ref(weak_factory_.GetWeakPtr());
   *window = window_finder_->GetLocalProcessWindowAtPoint(screen_point, exclude);
   return ref ? Liveness::ALIVE : Liveness::DELETED;
 }