Infobar Componentization Proof of Concept

chrome/browser/infobars/infobar_service.h

Index: chrome/browser/infobars/infobar_service.h
diff --git a/chrome/browser/infobars/infobar_service.h b/chrome/browser/infobars/infobar_service.h
index f5fd8b10b1e1742bbd557ad0daacfc339d6183d3..1eff3df8bf0c7390e576aa72fe3921658c744266 100644
--- a/chrome/browser/infobars/infobar_service.h
+++ b/chrome/browser/infobars/infobar_service.h
@@ -1,4 +1,4 @@
-// Copyright (c) 2012 The Chromium Authors. All rights reserved.
+// Copyright 2012 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.
 
@@ -8,70 +8,39 @@
 #include <vector>
 
 #include "base/memory/scoped_ptr.h"
+#include "chrome/browser/infobars/infobar_manager.h"
 #include "content/public/browser/web_contents_observer.h"
 #include "content/public/browser/web_contents_user_data.h"
 
-class InfoBar;
+namespace content {
+struct LoadCommittedDetails;
+}
 
-// Provides access to creating, removing and enumerating info bars
-// attached to a tab.
 class InfoBarService : public content::WebContentsObserver,

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

InfoBarService no longer manages the list of infobars, but rather associates an
InfoBarManager with a WebContents instance.
This file will eventually move to components/infobar/content/browser/ and will
be renamed as infobar_driver to follow the pattern from existing components.

-                       public content::WebContentsUserData<InfoBarService> {
+                       public content::WebContentsUserData<InfoBarService>,

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

Nit: Leave the WebContentsUserData<> listed last

+                       public InfoBarManager::Observer {

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

InfoBarService is a InfoBarManager::Observer so that it can emit the
infobar-related notifications.
These notifications won't be used in the component, but rather only in chrome
client code.

  public:
-  // Adds the specified |infobar|, which already owns a delegate.
-  //
-  // If infobars are disabled for this tab or the tab already has an infobar
-  // whose delegate returns true for
-  // InfoBarDelegate::EqualsDelegate(infobar->delegate()), |infobar| is deleted
-  // immediately without being added.
-  //
-  // Returns the infobar if it was successfully added.
-  virtual InfoBar* AddInfoBar(scoped_ptr<InfoBar> infobar);
-
-  // Removes the specified |infobar|.  This in turn may close immediately or
-  // animate closed; at the end the infobar will delete itself.
-  //
-  // If infobars are disabled for this tab, this will do nothing, on the
-  // assumption that the matching AddInfoBar() call will have already deleted
-  // the infobar (see above).
-  void RemoveInfoBar(InfoBar* infobar);
-
-  // Replaces one infobar with another, without any animation in between.  This
-  // will result in |old_infobar| being synchronously deleted.
-  //
-  // If infobars are disabled for this tab, |new_infobar| is deleted immediately
-  // without being added, and nothing else happens.
-  //
-  // Returns the new infobar if it was successfully added.
-  //
-  // NOTE: This does not perform any EqualsDelegate() checks like AddInfoBar().
+  // Helper functions.

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

The functions are just pass through to call the corresponding functions on
InfoBarManager.
The only thing they do in addition to calling InfoBarManager is to compute the
entry ID.

+  InfoBar* AddInfoBar(scoped_ptr<InfoBar> infobar);
   InfoBar* ReplaceInfoBar(InfoBar* old_infobar,
                           scoped_ptr<InfoBar> new_infobar);
 
-  // Returns the number of infobars for this tab.
-  size_t infobar_count() const { return infobars_.size(); }
-
-  // Returns the infobar at the given |index|.  The InfoBarService retains
-  // ownership.
-  //
-  // Warning: Does not sanity check |index|.
-  InfoBar* infobar_at(size_t index) { return infobars_[index]; }
+  static InfoBarDelegate::NavigationDetails

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

Converts content::LoadCommittedDetails to InfoBarDelegate::NavigationDetails.

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

Nit: Do this and GetActiveEntryID() need to be public?

It seems like GetActiveEntryID() is only public because
PluginInstallerInfoBarDelegate::Replace() calls it.  But that function has
access to the WebContents, so it could get at the InfoBarService instead of the
InfoBarManager and thus avoid needing to make that call.

NavigationDetailsFromLoadCommittedDetails() is mainly public because of the
managed_mode_interstitial.cc code, and while I don't have a specific solution
there, something feels wrong about that -- like this expiry ought to be somehow
happening automatically elsewhere instead of needing to be manually triggered. 
Like, maybe we can start by converting the InfoBarDelegate::ShouldExpire() call
to an InfoBarManager::OnNavigation() call, and then perhaps replace that with
some kind of call to InfobarService::NavigationEntryCommitted() or something.

+      NavigationDetailsFromLoadCommittedDetails(
+          const content::LoadCommittedDetails& details);
 
   // Retrieve the WebContents for the tab this service is associated with.
   content::WebContents* web_contents() {
     return content::WebContentsObserver::web_contents();
   }
 
+  InfoBarManager& infobar_manager() { return infobar_manager_; }
+
+  static int GetActiveEntryID(content::WebContents* web_contents);

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

InfoBarService computes the entry_id and explicitly passes it to InfoBarManager.

+
  private:
   friend class content::WebContentsUserData<InfoBarService>;
 
-  // InfoBars associated with this InfoBarService.  We own these pointers.
-  // However, this is not a ScopedVector, because we don't delete the infobars
-  // directly once they've been added to this; instead, when we're done with an
-  // infobar, we instruct it to delete itself and then orphan it.  See
-  // RemoveInfoBarInternal().
-  typedef std::vector<InfoBar*> InfoBars;
-
   explicit InfoBarService(content::WebContents* web_contents);
   virtual ~InfoBarService();
 
@@ -83,15 +52,17 @@ class InfoBarService : public content::WebContentsObserver,
       content::WebContents* web_contents) OVERRIDE;
   virtual bool OnMessageReceived(const IPC::Message& message) OVERRIDE;
 
-  void RemoveInfoBarInternal(InfoBar* infobar, bool animate);
-  void RemoveAllInfoBars(bool animate);
-
   // Message handlers.
   void OnDidBlockDisplayingInsecureContent();
   void OnDidBlockRunningInsecureContent();
 
-  InfoBars infobars_;
-  bool infobars_enabled_;
+  // InfoBarManager::Observer:

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

Nit: Put this just below the WebContentsObserver overrides

+  virtual void OnInfoBarAdded(InfoBar* infobar) OVERRIDE;
+  virtual void OnInfoBarReplaced(InfoBar* old_infobar,
+                                 InfoBar* new_infobar) OVERRIDE;
+  virtual void OnInfoBarRemoved(InfoBar* infobar, bool animate) OVERRIDE;
+
+  InfoBarManager infobar_manager_;
 
   DISALLOW_COPY_AND_ASSIGN(InfoBarService);
 };