Make TabContents own its infobar delegates....

chrome/browser/tab_contents/tab_contents.cc

Index: chrome/browser/tab_contents/tab_contents.cc
===================================================================
--- chrome/browser/tab_contents/tab_contents.cc	(revision 65711)
+++ chrome/browser/tab_contents/tab_contents.cc	(working copy)
@@ -477,10 +477,8 @@
   // notification may trigger infobars calls that access their delegate. (and
   // some implementations of InfoBarDelegate do delete themselves on
   // InfoBarClosed()).
-  for (int i = 0; i < infobar_delegate_count(); ++i) {
-    InfoBarDelegate* delegate = GetInfoBarDelegateAt(i);
-    delegate->InfoBarClosed();
-  }
+  std::for_each(infobar_delegates_.begin(), infobar_delegates_.end(),
+                std::mem_fun(&InfoBarDelegate::InfoBarClosed));
   infobar_delegates_.clear();
 
   // TODO(brettw) this should be moved to the view.
@@ -1066,6 +1064,7 @@
 }
 
 void TabContents::AddInfoBar(InfoBarDelegate* delegate) {
+  DCHECK(delegate != NULL);
   if (delegate_ && !delegate_->infobars_enabled()) {
     delegate->InfoBarClosed();
     return;
@@ -1073,19 +1072,19 @@
 
   // Look through the existing InfoBarDelegates we have for a match. If we've
   // already got one that matches, then we don't add the new one.
-  for (int i = 0; i < infobar_delegate_count(); ++i) {
-    if (GetInfoBarDelegateAt(i)->EqualsDelegate(delegate)) {
-      // Tell the new infobar to close so that it can clean itself up.
-      delegate->InfoBarClosed();
-      return;
-    }
+  InfoBarDelegates::iterator i(std::find_if(infobar_delegates_.begin(),
+    infobar_delegates_.end(),
+    std::bind2nd(std::mem_fun(&InfoBarDelegate::EqualsDelegate), delegate)));
+  if (i != infobar_delegates_.end()) {
+    // Tell the new infobar to close so that it can clean itself up.
+    delegate->InfoBarClosed();
+    return;
   }
 
   infobar_delegates_.push_back(delegate);
   NotificationService::current()->Notify(
       NotificationType::TAB_CONTENTS_INFOBAR_ADDED,
-      Source<TabContents>(this),
-      Details<InfoBarDelegate>(delegate));
+      Source<TabContents>(this), Details<InfoBarDelegate>(delegate));
 
   // Add ourselves as an observer for navigations the first time a delegate is
   // added. We use this notification to expire InfoBars that need to expire on
@@ -1097,72 +1096,79 @@
 }
 
 void TabContents::RemoveInfoBar(InfoBarDelegate* delegate) {
+  DCHECK(delegate != NULL);
   if (delegate_ && !delegate_->infobars_enabled()) {
+    DCHECK(infobar_delegates_.empty());
     return;
   }
 
-  std::vector<InfoBarDelegate*>::iterator it =
-      find(infobar_delegates_.begin(), infobar_delegates_.end(), delegate);
-  if (it != infobar_delegates_.end()) {
-    InfoBarDelegate* delegate = *it;
-    NotificationService::current()->Notify(
-        NotificationType::TAB_CONTENTS_INFOBAR_REMOVED,
-        Source<TabContents>(this),
-        Details<InfoBarDelegate>(delegate));
+  InfoBarDelegates::iterator i(std::find(infobar_delegates_.begin(),
+                                         infobar_delegates_.end(), delegate));
+  DCHECK(i != infobar_delegates_.end());
+  NotificationService::current()->Notify(
+      NotificationType::TAB_CONTENTS_INFOBAR_REMOVED,
+      Source<TabContents>(this), Details<InfoBarDelegate>(delegate));
 
-    // Just to be safe, make sure the delegate was not removed by an observer.
-    it = find(infobar_delegates_.begin(), infobar_delegates_.end(), delegate);
-    if (it != infobar_delegates_.end()) {
-      infobar_delegates_.erase(it);
-      // Remove ourselves as an observer if we are tracking no more InfoBars.
-      if (infobar_delegates_.empty()) {
-        registrar_.Remove(this, NotificationType::NAV_ENTRY_COMMITTED,
-                          Source<NavigationController>(&controller_));
-      }
-    } else {
-      // If you hit this NOTREACHED, please comment in bug
-      // http://crbug.com/50428 how you got there.
-      NOTREACHED();
-    }
+  // Just to be safe, make sure the delegate was not removed by an observer.  If
+  // you hit this DCHECK, please comment in http://crbug.com/50428 how you got
+  // there.
+  i = std::find(infobar_delegates_.begin(), infobar_delegates_.end(), delegate);
+  DCHECK(i != infobar_delegates_.end());
+  infobar_delegates_.erase(i);
+
+  // Remove ourselves as an observer if we are tracking no more InfoBars.
+  // NOTE: We must do this before calling InfoBarClosed() below.  Otherwise, if
+  // that call causes a call to AddInfoBar(), we'll try to register for the
+  // NAV_ENTRY_COMMITTED notification, which we won't have yet unregistered
+  // here, and we'll fail the "not already registered" DCHECK in
+  // NotificationRegistrar::Add().
+  if (infobar_delegates_.empty()) {
+    registrar_.Remove(this, NotificationType::NAV_ENTRY_COMMITTED,
+                      Source<NavigationController>(&controller_));
   }
+
+  delegate->InfoBarClosed();
 }
 
 void TabContents::ReplaceInfoBar(InfoBarDelegate* old_delegate,
                                  InfoBarDelegate* new_delegate) {
+  DCHECK(old_delegate != NULL);
+  DCHECK(new_delegate != NULL);
   if (delegate_ && !delegate_->infobars_enabled()) {
+    DCHECK(infobar_delegates_.empty());
     new_delegate->InfoBarClosed();
     return;
   }
 
-  std::vector<InfoBarDelegate*>::iterator it =
-      find(infobar_delegates_.begin(), infobar_delegates_.end(), old_delegate);
-  DCHECK(it != infobar_delegates_.end());
+  InfoBarDelegates::iterator i(std::find(infobar_delegates_.begin(),
+      infobar_delegates_.end(), old_delegate));
+  DCHECK(i != infobar_delegates_.end());
 
   // Notify the container about the change of plans.
-  scoped_ptr<std::pair<InfoBarDelegate*, InfoBarDelegate*> > details(
-    new std::pair<InfoBarDelegate*, InfoBarDelegate*>(
-        old_delegate, new_delegate));
+  typedef std::pair<InfoBarDelegate*, InfoBarDelegate*> DelegatePair;
+  scoped_ptr<DelegatePair> delegate_pair(new DelegatePair(old_delegate,
+                                                          new_delegate));
   NotificationService::current()->Notify(
       NotificationType::TAB_CONTENTS_INFOBAR_REPLACED,
-      Source<TabContents>(this),
-      Details<std::pair<InfoBarDelegate*, InfoBarDelegate*> >(details.get()));
+      Source<TabContents>(this), Details<DelegatePair>(delegate_pair.get()));
 
-  // Just to be safe, make sure the delegate was not removed by an observer.
-  it = find(infobar_delegates_.begin(), infobar_delegates_.end(), old_delegate);
-  if (it != infobar_delegates_.end()) {
-    // Remove the old one.
-    infobar_delegates_.erase(it);
-  } else {
-    // If you hit this NOTREACHED, please comment in bug
-    // http://crbug.com/50428 how you got there.
-    NOTREACHED();
-  }
+  // Just to be safe, make sure the delegate was not removed by an observer.  If
+  // you hit this DCHECK, please comment in http://crbug.com/50428 how you got
+  // there.
+  i = std::find(infobar_delegates_.begin(), infobar_delegates_.end(),
+                old_delegate);
+  DCHECK(i != infobar_delegates_.end());
+  // Remove the old one.
+  infobar_delegates_.erase(i);
 
-  // Add the new one.
-  DCHECK(find(infobar_delegates_.begin(),
-              infobar_delegates_.end(), new_delegate) ==
-         infobar_delegates_.end());
+  // Add the new one.  We do this before calling old_delegate->InfoBarClosed()
+  // for the same reason as why we call that function last in RemoveInfoBar()
+  // above; see comments there.
+  DCHECK(std::find(infobar_delegates_.begin(), infobar_delegates_.end(),
+                   new_delegate) == infobar_delegates_.end());
   infobar_delegates_.push_back(new_delegate);
+
+  old_delegate->InfoBarClosed();
 }
 
 bool TabContents::ShouldShowBookmarkBar() {
@@ -1178,9 +1184,8 @@
   // is so the bookmarks bar disappears at the same time the page does.
   if (controller_.GetLastCommittedEntry()) {
     // Not the first load, always use the committed DOM UI.
-    if (render_manager_.dom_ui())
-      return render_manager_.dom_ui()->force_bookmark_bar_visible();
-    return false;  // Default.
+    return (render_manager_.dom_ui() == NULL) ?
+        false : render_manager_.dom_ui()->force_bookmark_bar_visible();
   }
 
   // When it's the first load, we know either the pending one or the committed
@@ -1188,9 +1193,8 @@
   // of them will be valid, so we can just check both.
   if (render_manager_.pending_dom_ui())
     return render_manager_.pending_dom_ui()->force_bookmark_bar_visible();
-  if (render_manager_.dom_ui())
-    return render_manager_.dom_ui()->force_bookmark_bar_visible();
-  return false;  // Default.
+  return (render_manager_.dom_ui() == NULL) ?
+      false : render_manager_.dom_ui()->force_bookmark_bar_visible();
 }
 
 void TabContents::ToolbarSizeChanged(bool is_animating) {
@@ -1217,18 +1221,17 @@
 }
 
 void TabContents::WillClose(ConstrainedWindow* window) {
-  ConstrainedWindowList::iterator it =
-      find(child_windows_.begin(), child_windows_.end(), window);
-  bool removed_topmost_window = it == child_windows_.begin();
-  if (it != child_windows_.end())
-    child_windows_.erase(it);
-  if (child_windows_.size() > 0) {
-    if (removed_topmost_window) {
+  ConstrainedWindowList::iterator i(
+      std::find(child_windows_.begin(), child_windows_.end(), window));
+  bool removed_topmost_window = (i == child_windows_.begin());
+  if (i != child_windows_.end())
+    child_windows_.erase(i);
+  if (child_windows_.empty()) {
+    BlockTabContent(false);
+  } else {
+    if (removed_topmost_window)
       child_windows_[0]->ShowConstrainedWindow();
-    }
     BlockTabContent(true);
-  } else {
-    BlockTabContent(false);
   }
 }
 
@@ -1559,15 +1562,11 @@
   if (!details.is_user_initiated_main_frame_load())
     return;
 
+  // NOTE: It is not safe to change the following code to count upwards or use
+  // iterators, as the RemvoeInfoBar() call synchronously modifies our delegate

[Ben Goodger (Google), 2010/11/12 18:56:43]

Remove

+  // list.
   for (int i = infobar_delegate_count() - 1; i >= 0; --i) {
     InfoBarDelegate* delegate = GetInfoBarDelegateAt(i);
-    if (!delegate) {
-      // If you hit this NOTREACHED, please comment in bug
-      // http://crbug.com/50428 how you got there.
-      NOTREACHED();
-      continue;
-    }
-
     if (delegate->ShouldExpire(details))
       RemoveInfoBar(delegate);
   }
@@ -2420,8 +2419,8 @@
   SetIsCrashed(true);
 
   // Remove all infobars.
-  for (int i = infobar_delegate_count() - 1; i >=0 ; --i)
-    RemoveInfoBar(GetInfoBarDelegateAt(i));
+  while (!infobar_delegates_.empty())
+    RemoveInfoBar(infobar_delegates_.front());
 
   // Tell the view that we've crashed so it can prepare the sad tab page.
   // Only do this if we're not in browser shutdown, so that TabContents