Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(2536)

Unified Diff: chrome/browser/views/tabs/dragged_tab_controller.cc

Issue 19026: Fixes tab contents crash. This changes a number of things in dragged... (Closed) Base URL: svn://chrome-svn/chrome/trunk/src/
Patch Set: '' Created 11 years, 11 months ago
Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.
Jump to:
View side-by-side diff with in-line comments
Download patch
« no previous file with comments | « no previous file | no next file » | no next file with comments »
Expand Comments ('e') | Collapse Comments ('c') | Show Comments Hide Comments ('s')
Index: chrome/browser/views/tabs/dragged_tab_controller.cc
===================================================================
--- chrome/browser/views/tabs/dragged_tab_controller.cc (revision 8729)
+++ chrome/browser/views/tabs/dragged_tab_controller.cc (working copy)
@@ -399,16 +399,23 @@
void DraggedTabController::ReplaceContents(TabContents* source,
TabContents* new_contents) {
DCHECK(dragged_contents_ == source);
- source->set_delegate(NULL);
- new_contents->set_delegate(this);
// If we're attached to a TabStrip, we need to tell the TabStrip that this
// TabContents was replaced.
- if (attached_tabstrip_ && attached_tabstrip_->model() && dragged_contents_) {
- int index =
- attached_tabstrip_->model()->GetIndexOfTabContents(dragged_contents_);
- if (index != TabStripModel::kNoTab)
- attached_tabstrip_->model()->ReplaceTabContentsAt(index, new_contents);
+ if (attached_tabstrip_ && dragged_contents_) {
+ if (original_delegate_) {
+ original_delegate_->ReplaceContents(source, new_contents);
+ // ReplaceContents on the original delegate is going to reset the delegate
+ // for us. We need to unset original_delegate_ here so that
+ // ChangeDraggedContents doesn't attempt to restore the delegate to the
+ // wrong value.
+ original_delegate_ = NULL;
+ } else if (attached_tabstrip_->model()) {
+ int index =
+ attached_tabstrip_->model()->GetIndexOfTabContents(dragged_contents_);
+ if (index != TabStripModel::kNoTab)
+ attached_tabstrip_->model()->ReplaceTabContentsAt(index, new_contents);
+ }
}
// Update our internal state.
@@ -557,12 +564,22 @@
NotificationService::current()->RemoveObserver(this,
NOTIFY_TAB_CONTENTS_DESTROYED,
Source<TabContents>(dragged_contents_));
+ if (original_delegate_)
+ dragged_contents_->set_delegate(original_delegate_);
}
+ original_delegate_ = NULL;
dragged_contents_ = new_contents;
if (dragged_contents_) {
NotificationService::current()->AddObserver(this,
NOTIFY_TAB_CONTENTS_DESTROYED,
Source<TabContents>(dragged_contents_));
+
+ // We need to be the delegate so we receive messages about stuff,
+ // otherwise our dragged_contents() may be replaced and subsequently
+ // collected/destroyed while the drag is in process, leading to
+ // nasty crashes.
+ original_delegate_ = dragged_contents_->delegate();
+ dragged_contents_->set_delegate(this);
}
}
@@ -820,11 +837,7 @@
if (view_.get())
view_->Detach(photobooth_.get());
- // We need to be the delegate so we receive messages about stuff,
- // otherwise our dragged_contents() may be replaced and subsequently
- // collected/destroyed while the drag is in process, leading to
- // nasty crashes.
- original_delegate_ = dragged_contents_->delegate();
+ // Detaching resets the delegate, but we still want to be the delegate.
dragged_contents_->set_delegate(this);
attached_tabstrip_ = NULL;
@@ -922,6 +935,12 @@
}
bool DraggedTabController::EndDragImpl(EndDragType type) {
+ // WARNING: this may be invoked multiple times. In particular, if deletion
+ // occurs after a delay (as it does when the tab is released in the original
+ // tab strip) and the navigation controller/tab contents is deleted before
+ // the animation finishes, this is invoked twice. The second time through
+ // type == TAB_DESTROYED.
+
bring_to_front_timer_.Stop();
// Hide the current dock controllers.
@@ -947,6 +966,8 @@
destroy_now = CompleteDrag();
}
}
+ if (dragged_contents_ && dragged_contents_->delegate() == this)
+ dragged_contents_->set_delegate(original_delegate_);
} else {
// If we get here it means the NavigationController is going down. Don't
// attempt to do any cleanup other than resetting the delegate (if we're
@@ -954,8 +975,14 @@
if (dragged_contents_ && dragged_contents_->delegate() == this)
dragged_contents_->set_delegate(NULL);
dragged_contents_ = NULL;
- attached_tabstrip_ = NULL;
}
+
+ // The delegate of the dragged contents should have been reset. Unset the
+ // original delegate so that we don't attempt to reset the delegate when
+ // deleted.
+ DCHECK(!dragged_contents_ || dragged_contents_->delegate() != this);
+ original_delegate_ = NULL;
+
// If we're not destroyed now, we'll be destroyed asynchronously later.
if (destroy_now)
source_tabstrip_->DestroyDragController();
« no previous file with comments | « no previous file | no next file » | no next file with comments »

Powered by Google App Engine
This is Rietveld 408576698