Ugh. It turns out we were never properly removing the dragged tab from the source tabstrip when a drag was completed. This was causing various problems, which were highlighted when you reverted a drag (e.g. press esc or alt+tab while dragging).

See my comments inline in the diff for explanations.

This manifests as Venkat's latest repro in:

B=1262392

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@57 0039d316-1c4b-4281-b951-d872f2087c98

5 files changed, 65 insertions, 11 deletions

diff --git a/chrome/browser/tabs/dragged_tab_controller.cc b/chrome/browser/tabs/dragged_tab_controller.cc
index 0a043e4..0a337df 100644
--- a/chrome/browser/tabs/dragged_tab_controller.cc
+++ b/chrome/browser/tabs/dragged_tab_controller.cc
@@ -112,7 +112,7 @@ DraggedTabController::DraggedTabController(Tab* source_tab,
       source_tab_(source_tab),
       source_tabstrip_(source_tabstrip),
       source_model_index_(source_tabstrip->GetIndexOfTab(source_tab)),
-      attached_tabstrip_(NULL),
+      attached_tabstrip_(source_tabstrip),
       old_focused_view_(NULL),
       in_destructor_(false),
       last_move_screen_x_(0) {
@@ -124,6 +124,7 @@ DraggedTabController::DraggedTabController(Tab* source_tab,
 
 DraggedTabController::~DraggedTabController() {
   in_destructor_ = true;
+  CleanUpSourceTab();
   MessageLoop::current()->RemoveObserver(this);
   ChangeDraggedContents(NULL); // This removes our observer.
 }
@@ -151,7 +152,9 @@ void DraggedTabController::EndDrag(bool canceled) {
 
 Tab* DraggedTabController::GetDragSourceTabForContents(
     TabContents* contents) const {
-  return contents == dragged_contents_ ? source_tab_ : NULL;
+  if (attached_tabstrip_ == source_tabstrip_)
+    return contents == dragged_contents_ ? source_tab_ : NULL;
+  return NULL;
 }
 
 bool DraggedTabController::IsDragSourceTab(Tab* tab) const {
@@ -669,6 +672,8 @@ void DraggedTabController::EndDragImpl(EndDragType type) {
 }
 
 void DraggedTabController::RevertDrag() {
+  // We save this here because code below will modify |attached_tabstrip_|.
+  bool restore_frame = attached_tabstrip_ != source_tabstrip_;
   if (attached_tabstrip_) {
     int index = attached_tabstrip_->model()->GetIndexOfTabContents(
         dragged_contents_);
@@ -676,6 +681,9 @@ void DraggedTabController::RevertDrag() {
       // The Tab was inserted into another TabStrip. We need to put it back
       // into the original one.
       attached_tabstrip_->model()->DetachTabContentsAt(index);
+      // TODO(beng): (Cleanup) seems like we should use Attach() for this
+      //             somehow.
+      attached_tabstrip_ = source_tabstrip_;
       source_tabstrip_->model()->InsertTabContentsAt(source_model_index_,
           dragged_contents_, true, false);
     } else {
@@ -684,6 +692,9 @@ void DraggedTabController::RevertDrag() {
       source_tabstrip_->model()->MoveTabContentsAt(index, source_model_index_);
     }
   } else {
+    // TODO(beng): (Cleanup) seems like we should use Attach() for this
+    //             somehow.
+    attached_tabstrip_ = source_tabstrip_;
     // The Tab was detached from the TabStrip where the drag began, and has not
     // been attached to any other TabStrip. We need to put it back into the
     // source TabStrip.
@@ -693,7 +704,7 @@ void DraggedTabController::RevertDrag() {
   // If we're not attached to any TabStrip, or attached to some other TabStrip,
   // we need to restore the bounds of the original TabStrip's frame, in case
   // it has been hidden.
-  if (!attached_tabstrip_ || attached_tabstrip_ != source_tabstrip_) {
+  if (restore_frame) {
     if (!restore_bounds_.IsEmpty()) {
       HWND frame_hwnd = source_tabstrip_->GetViewContainer()->GetHWND();
       MoveWindow(frame_hwnd, restore_bounds_.x(), restore_bounds_.y(),
@@ -785,6 +796,16 @@ void DraggedTabController::CleanUpHiddenFrame() {
     source_tabstrip_->model()->delegate()->CloseFrameAfterDragSession();
 }
 
+void DraggedTabController::CleanUpSourceTab() {
+  // If we were attached to the source TabStrip, source Tab will be in use
+  // as the Tab. If we were detached or attached to another TabStrip, we can
+  // safely remove this item and delete it now.
+  if (attached_tabstrip_ != source_tabstrip_) {
+    source_tabstrip_->DestroyDraggedSourceTab(source_tab_);
+    source_tab_ = NULL;
+  }
+}
+
 void DraggedTabController::OnAnimateToBoundsComplete() {
   // Sometimes, for some reason, in automation we can be called back on a
   // detach even though we aren't attached to a TabStrip. Guard against that.
diff --git a/chrome/browser/tabs/dragged_tab_controller.h b/chrome/browser/tabs/dragged_tab_controller.h
index 82fdb5a..690baf9 100644
--- a/chrome/browser/tabs/dragged_tab_controller.h
+++ b/chrome/browser/tabs/dragged_tab_controller.h
@@ -230,6 +230,9 @@ class DraggedTabController : public TabContentsDelegate,
   // Closes a hidden frame at the end of a drag session.
   void CleanUpHiddenFrame();
 
+  // Cleans up a source tab that is no longer used.
+  void CleanUpSourceTab();
+
   // Completes the drag session after the view has animated to its final
   // position.
   void OnAnimateToBoundsComplete();
diff --git a/chrome/browser/tabs/tab_strip.cc b/chrome/browser/tabs/tab_strip.cc
index 40233b5..71936b2 100644
--- a/chrome/browser/tabs/tab_strip.cc
+++ b/chrome/browser/tabs/tab_strip.cc
@@ -499,6 +499,14 @@ void TabStrip::DestroyDragController() {
     drag_controller_.reset(NULL);
 }
 
+void TabStrip::DestroyDraggedSourceTab(Tab* tab) {
+  // We could be running an animation that references this Tab.
+  if (active_animation_.get())
+    active_animation_->Stop();
+  tab->GetParent()->RemoveChildView(tab);
+  delete tab;
+}
+
 gfx::Rect TabStrip::GetIdealBounds(int index) {
   DCHECK(index >= 0 && index < GetTabCount());
   return tab_data_.at(index).ideal_bounds;
@@ -669,6 +677,7 @@ void TabStrip::TabInsertedAt(TabContents* contents,
   if (active_animation_.get())
     active_animation_->Stop();
 
+  bool contains_tab = false;
   Tab* tab = NULL;
   // First see if this Tab is one that was dragged out of this TabStrip and is
   // now being dragged back in. In this case, the DraggedTabController actually
@@ -681,6 +690,14 @@ void TabStrip::TabInsertedAt(TabContents* contents,
       // removed, so we need to reset this property.
       tab->set_closing(false);
       tab->ValidateLoadingAnimation(TabRenderer::ANIMATION_NONE);
+      tab->SetVisible(true);
+    }
+
+    // See if we're already in the list. We don't want to add ourselves twice.
+    std::vector<TabData>::const_iterator iter = tab_data_.begin();
+    for (; iter != tab_data_.end() && !contains_tab; ++iter) {
+      if (iter->tab == tab)
+        contains_tab = true;
     }
   }
 
@@ -688,15 +705,19 @@ void TabStrip::TabInsertedAt(TabContents* contents,
   if (!tab)
     tab = new Tab(this);
 
-  if (index == TabStripModel::kNoTab) {
-    TabData d = { tab, gfx::Rect() };
-    tab_data_.push_back(d);
-    tab->UpdateData(contents);
-  } else {
-    TabData d = { tab, gfx::Rect() };
-    tab_data_.insert(tab_data_.begin() + index, d);
-    tab->UpdateData(contents);
+  // Only insert if we're not already in the list.
+  if (!contains_tab) {
+    if (index == TabStripModel::kNoTab) {
+      TabData d = { tab, gfx::Rect() };
+      tab_data_.push_back(d);
+      tab->UpdateData(contents);
+    } else {
+      TabData d = { tab, gfx::Rect() };
+      tab_data_.insert(tab_data_.begin() + index, d);
+      tab->UpdateData(contents);
+    }
   }
+
   // We only add the tab to the child list if it's not already - an invisible
   // tab maintained by the DraggedTabController will already be parented.
   if (!tab->GetParent())
diff --git a/chrome/browser/tabs/tab_strip.h b/chrome/browser/tabs/tab_strip.h
index a9f7e51..64195a3 100644
--- a/chrome/browser/tabs/tab_strip.h
+++ b/chrome/browser/tabs/tab_strip.h
@@ -112,6 +112,9 @@ class TabStrip : public ChromeViews::View,
   // Destroys the active drag controller.
   void DestroyDragController();
 
+  // Removes the drag source Tab from this TabStrip, and deletes it.
+  void DestroyDraggedSourceTab(Tab* tab);
+
   // Retrieve the ideal bounds for the Tab at the specified index.
   gfx::Rect GetIdealBounds(int index);
 
diff --git a/chrome/browser/tabs/tab_strip_model.cc b/chrome/browser/tabs/tab_strip_model.cc
index ecb0ab4..a5e58f9 100644
--- a/chrome/browser/tabs/tab_strip_model.cc
+++ b/chrome/browser/tabs/tab_strip_model.cc
@@ -99,6 +99,12 @@ void TabStripModel::InsertTabContentsAt(int index,
                                         TabContents* contents,
                                         bool foreground,
                                         bool inherit_group) {
+  // In tab dragging situations, if the last tab in the window was detached
+  // then the user aborted the drag, we will have the |closing_all_| member
+  // set (see DetachTabContentsAt) which will mess with our mojo here. We need
+  // to clear this bit.
+  closing_all_ = false;
+
   // Have to get the selected contents before we monkey with |contents_|
   // otherwise we run into problems when we try to change the selected contents
   // since the old contents and the new contents will be the same...