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 65558)
+++ chrome/browser/tab_contents/tab_contents.cc	(working copy)
@@ -477,10 +477,9 @@
   // 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();
-  }
+  for (InfoBarDelegates::reverse_iterator i(infobar_delegates_.rbegin());
+       i != infobar_delegates_.rend(); ++i)
+    (*i)->InfoBarClosed();
   infobar_delegates_.clear();
 
   // TODO(brettw) this should be moved to the view.
@@ -1066,6 +1065,7 @@
 }
 
 void TabContents::AddInfoBar(InfoBarDelegate* delegate) {
+  DCHECK(delegate != NULL);
   if (delegate_ && !delegate_->infobars_enabled()) {
     delegate->InfoBarClosed();
     return;
@@ -1073,19 +1073,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,71 +1097,67 @@
 }
 
 void TabContents::RemoveInfoBar(InfoBarDelegate* delegate) {
+  DCHECK(delegate != NULL);
   if (delegate_ && !delegate_->infobars_enabled()) {
+    delegate->InfoBarClosed();
     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);
+  delegate->InfoBarClosed();
+
+  // 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_));
   }
 }
 
 void TabContents::ReplaceInfoBar(InfoBarDelegate* old_delegate,
                                  InfoBarDelegate* new_delegate) {
+  DCHECK(old_delegate != NULL);
+  DCHECK(new_delegate != NULL);
   if (delegate_ && !delegate_->infobars_enabled()) {
     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*> Delegates;
+  scoped_ptr<Delegates> details(new Delegates(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<Delegates>(details.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);
+  old_delegate->InfoBarClosed();
 
   // Add the new one.
-  DCHECK(find(infobar_delegates_.begin(),
-              infobar_delegates_.end(), new_delegate) ==
-         infobar_delegates_.end());
+  DCHECK(std::find(infobar_delegates_.begin(), infobar_delegates_.end(),
+                   new_delegate) == infobar_delegates_.end());
   infobar_delegates_.push_back(new_delegate);
 }
 
@@ -1178,9 +1174,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) ?

[tfarina, 2010/11/10 13:11:57]

This change to ternary operator just make it *less* readable. I really don't
like the change to ternary operator when it does not fit into one line. The same
thing below. Also if the check is not wrong, why change it at all? It's just
increasing the size of the diff.

+        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 +1183,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 +1211,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,17 +1552,10 @@
   if (!details.is_user_initiated_main_frame_load())
     return;
 
-  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);
+  for (InfoBarDelegates::reverse_iterator i(infobar_delegates_.rbegin());
+       i != infobar_delegates_.rend(); ++i) {
+    if ((*i)->ShouldExpire(details))
+      RemoveInfoBar(*i);
   }
 }
 
@@ -2420,8 +2406,8 @@
   SetIsCrashed(true);
 
   // Remove all infobars.
-  for (int i = infobar_delegate_count() - 1; i >=0 ; --i)
-    RemoveInfoBar(GetInfoBarDelegateAt(i));
+  while (!infobar_delegates_.empty())

[tfarina, 2010/11/10 13:11:57]

What is wrong with the for loop? I guess this is superfluous.

+    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