Infobar Componentization Proof of Concept

chrome/browser/infobars/infobar_service.cc

Index: chrome/browser/infobars/infobar_service.cc
diff --git a/chrome/browser/infobars/infobar_service.cc b/chrome/browser/infobars/infobar_service.cc
index b3bcaa09d2b633c57c52ee890f22547f0f04f56a..c21c5e5afb8951c7fca74b414e4efc09c0245c64 100644
--- a/chrome/browser/infobars/infobar_service.cc
+++ b/chrome/browser/infobars/infobar_service.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2013 The Chromium Authors. All rights reserved.
+// Copyright 2013 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.
 
@@ -12,100 +12,70 @@
 #include "chrome/common/chrome_switches.h"
 #include "chrome/common/render_messages.h"
 #include "content/public/browser/navigation_controller.h"
+#include "content/public/browser/navigation_details.h"
+#include "content/public/browser/navigation_entry.h"
 #include "content/public/browser/notification_service.h"
 #include "content/public/browser/web_contents.h"
 
 
 DEFINE_WEB_CONTENTS_USER_DATA_KEY(InfoBarService);
 
-InfoBar* InfoBarService::AddInfoBar(scoped_ptr<InfoBar> infobar) {
-  DCHECK(infobar);
-  if (!infobars_enabled_)
-    return NULL;
-
-  for (InfoBars::const_iterator i(infobars_.begin()); i != infobars_.end();
-       ++i) {
-    if ((*i)->delegate()->EqualsDelegate(infobar->delegate())) {
-      DCHECK_NE((*i)->delegate(), infobar->delegate());
-      return NULL;
-    }
-  }
-
-  InfoBar* infobar_ptr = infobar.release();
-  infobars_.push_back(infobar_ptr);
-  infobar_ptr->SetOwner(this);
-
-  content::NotificationService::current()->Notify(
-      chrome::NOTIFICATION_TAB_CONTENTS_INFOBAR_ADDED,
-      content::Source<InfoBarService>(this),
-      content::Details<InfoBar::AddedDetails>(infobar_ptr));
-  return infobar_ptr;
+InfoBarService::InfoBarService(content::WebContents* web_contents)
+    : content::WebContentsObserver(web_contents) {
+  DCHECK(web_contents);
 }
 
-void InfoBarService::RemoveInfoBar(InfoBar* infobar) {
-  RemoveInfoBarInternal(infobar, true);
+InfoBarService::~InfoBarService() {}
+
+InfoBar* InfoBarService::AddInfoBar(scoped_ptr<InfoBar> infobar) {
+  return infobar_manager_.AddInfoBar(infobar.Pass(),
+                                     GetActiveEntryID(web_contents()));
 }
 
 InfoBar* InfoBarService::ReplaceInfoBar(InfoBar* old_infobar,
                                         scoped_ptr<InfoBar> new_infobar) {
-  DCHECK(old_infobar);
-  if (!infobars_enabled_)
-    return AddInfoBar(new_infobar.Pass());  // Deletes the infobar.
-  DCHECK(new_infobar);
-
-  InfoBars::iterator i(std::find(infobars_.begin(), infobars_.end(),
-                                 old_infobar));
-  DCHECK(i != infobars_.end());
-
-  InfoBar* new_infobar_ptr = new_infobar.release();
-  i = infobars_.insert(i, new_infobar_ptr);
-  new_infobar_ptr->SetOwner(this);
-  InfoBar::ReplacedDetails replaced_details(old_infobar, new_infobar_ptr);
-
-  // Remove the old infobar before notifying, so that if any observers call back
-  // to AddInfoBar() or similar, we don't dupe-check against this infobar.
-  infobars_.erase(++i);
-
-  content::NotificationService::current()->Notify(
-      chrome::NOTIFICATION_TAB_CONTENTS_INFOBAR_REPLACED,
-      content::Source<InfoBarService>(this),
-      content::Details<InfoBar::ReplacedDetails>(&replaced_details));
-
-  old_infobar->CloseSoon();
-  return new_infobar_ptr;
+  return infobar_manager_.ReplaceInfoBar(
+      old_infobar, new_infobar.Pass(), GetActiveEntryID(web_contents()));
 }
 
-InfoBarService::InfoBarService(content::WebContents* web_contents)
-    : content::WebContentsObserver(web_contents),
-      infobars_enabled_(true) {
-  DCHECK(web_contents);
-  if (CommandLine::ForCurrentProcess()->HasSwitch(switches::kDisableInfoBars))
-    infobars_enabled_ = false;
+// static
+InfoBarDelegate::NavigationDetails
+InfoBarService::NavigationDetailsFromLoadCommittedDetails(

[Peter Kasting, 2014/03/18 18:29:17]

Nit: Indent 4

+    const content::LoadCommittedDetails& details) {
+  InfoBarDelegate::NavigationDetails navigation_details;
+  navigation_details.entry_id = details.entry->GetUniqueID();
+  navigation_details.is_reload = content::PageTransitionStripQualifier(
+                                     details.entry->GetTransitionType()) ==
+                                 content::PAGE_TRANSITION_RELOAD;
+  navigation_details.is_redirect =
+      (details.entry->GetTransitionType() &
+       content::PAGE_TRANSITION_IS_REDIRECT_MASK) != 0;
+  navigation_details.did_replace_entry = details.did_replace_entry;
+  navigation_details.is_navigation_to_different_page =
+      details.is_navigation_to_different_page();
+  navigation_details.is_main_frame = details.is_main_frame;
+  return navigation_details;
 }
 
-InfoBarService::~InfoBarService() {
-  // Destroy all remaining InfoBars.  It's important to not animate here so that
-  // we guarantee that we'll delete all delegates before we do anything else.
-  RemoveAllInfoBars(false);
+// static
+int InfoBarService::GetActiveEntryID(content::WebContents* web_contents) {
+  content::NavigationEntry* active_entry =
+      web_contents->GetController().GetActiveEntry();
+  return active_entry ? active_entry->GetUniqueID() : 0;
 }
 
 void InfoBarService::RenderProcessGone(base::TerminationStatus status) {
-  RemoveAllInfoBars(true);
+  infobar_manager_.RemoveAllInfoBars(true);
 }
 
 void InfoBarService::NavigationEntryCommitted(
     const content::LoadCommittedDetails& load_details) {
-  // NOTE: It is not safe to change the following code to count upwards or
-  // use iterators, as the RemoveInfoBar() call synchronously modifies our
-  // delegate list.
-  for (size_t i = infobars_.size(); i > 0; --i) {
-    InfoBar* infobar = infobars_[i - 1];
-    if (infobar->delegate()->ShouldExpire(load_details))
-      RemoveInfoBar(infobar);
-  }
+  infobar_manager_.OnNavigation(
+      NavigationDetailsFromLoadCommittedDetails(load_details));
 }
 
 void InfoBarService::WebContentsDestroyed(content::WebContents* web_contents) {
+  infobar_manager_.CleanUp();
   // The WebContents is going away; be aggressively paranoid and delete
   // ourselves lest other parts of the system attempt to add infobars or use
   // us otherwise during the destruction.
@@ -126,36 +96,6 @@ bool InfoBarService::OnMessageReceived(const IPC::Message& message) {
   return handled;
 }
 
-void InfoBarService::RemoveInfoBarInternal(InfoBar* infobar, bool animate) {
-  DCHECK(infobar);
-  if (!infobars_enabled_) {
-    DCHECK(infobars_.empty());
-    return;
-  }
-
-  InfoBars::iterator i(std::find(infobars_.begin(), infobars_.end(), infobar));
-  DCHECK(i != infobars_.end());
-
-  // Remove the infobar before notifying, so that if any observers call back to
-  // AddInfoBar() or similar, we don't dupe-check against this infobar.
-  infobars_.erase(i);
-
-  // This notification must happen before the call to CloseSoon() below, since
-  // observers may want to access |infobar| and that call can delete it.
-  InfoBar::RemovedDetails removed_details(infobar, animate);
-  content::NotificationService::current()->Notify(
-      chrome::NOTIFICATION_TAB_CONTENTS_INFOBAR_REMOVED,
-      content::Source<InfoBarService>(this),
-      content::Details<InfoBar::RemovedDetails>(&removed_details));
-
-  infobar->CloseSoon();
-}
-
-void InfoBarService::RemoveAllInfoBars(bool animate) {
-  while (!infobars_.empty())
-    RemoveInfoBarInternal(infobars_.back(), animate);
-}
-
 void InfoBarService::OnDidBlockDisplayingInsecureContent() {
   InsecureContentInfoBarDelegate::Create(
       this, InsecureContentInfoBarDelegate::DISPLAY);
@@ -165,3 +105,27 @@ void InfoBarService::OnDidBlockRunningInsecureContent() {
   InsecureContentInfoBarDelegate::Create(this,
                                          InsecureContentInfoBarDelegate::RUN);
 }
+
+void InfoBarService::OnInfoBarAdded(InfoBar* infobar) {

[droger, 2014/03/18 15:59:53]

These functions listen for InfoBarManager and emit the corresponding
notifications.

[Peter Kasting, 2014/03/18 18:29:17]

Since we're trying to kill notifications in Chrome anyway, it seems like we
should take the opportunity to clean these up.  Anyone who listens to these
should probably instead just be an InfoBarManager::Observer.

[blundell, 2014/03/19 15:08:09]

There are 20+ listeners for these notifications currently. As changing
notification listeners to Observers can be time-consuming, would you be OK with
us taking this approach to avoid the work being blocked on the cleanup task and
at the same time incrementally working to change the notification listeners to
be InfoBarManager::Observers?

On 2014/03/18 18:29:17, Peter Kasting wrote:

[Peter Kasting, 2014/03/19 18:11:11]

Splitting work into smaller pieces is always fine as long as we agree on the
ultimate endpoint.  I'm surprised there are that many observers of this.  I
would have expected more like 5.

+  content::NotificationService::current()->Notify(
+      chrome::NOTIFICATION_TAB_CONTENTS_INFOBAR_ADDED,
+      content::Source<InfoBarService>(this),
+      content::Details<InfoBar::AddedDetails>(infobar));
+}
+
+void InfoBarService::OnInfoBarReplaced(InfoBar* old_infobar,
+                                       InfoBar* new_infobar) {
+  InfoBar::ReplacedDetails replaced_details(old_infobar, new_infobar);
+  content::NotificationService::current()->Notify(
+      chrome::NOTIFICATION_TAB_CONTENTS_INFOBAR_REPLACED,
+      content::Source<InfoBarService>(this),
+      content::Details<InfoBar::ReplacedDetails>(&replaced_details));
+}
+
+void InfoBarService::OnInfoBarRemoved(InfoBar* infobar, bool animate) {
+  InfoBar::RemovedDetails removed_details(infobar, animate);
+  content::NotificationService::current()->Notify(
+      chrome::NOTIFICATION_TAB_CONTENTS_INFOBAR_REMOVED,
+      content::Source<InfoBarService>(this),
+      content::Details<InfoBar::RemovedDetails>(&removed_details));
+}