Minor cleanup to how infobars handle expiry:

* Make StoreActiveEntryUniqueID() take no args -- the arg was unnecessary.
* Remove the unique ID setter and make its lone caller override ShouldExpireInternal() instead, to reduce the overall API surface area.
* Prefer to override ShouldExpireInternal() instead of ShouldExpire() where possible, as that's a safer pattern if others copy-and-paste it, since it will change less behavior.

BUG=none
TEST=none
Review URL: https://codereview.chromium.org/11748012

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@175362 0039d316-1c4b-4281-b951-d872f2087c98

chrome/browser/api/infobars/infobar_delegate.cc

@@ -98,18 +98,21 @@ InfoBarDelegate::InfoBarDelegate(InfoBarService* infobar_service)
     : contents_unique_id_(0),
       owner_(infobar_service) {
   if (infobar_service)
-    StoreActiveEntryUniqueID(infobar_service);
+    StoreActiveEntryUniqueID();
 }
 
-void InfoBarDelegate::StoreActiveEntryUniqueID(
-    InfoBarService* infobar_service) {
+void InfoBarDelegate::StoreActiveEntryUniqueID() {
+  content::WebContents* web_contents = owner_->GetWebContents();
+  DCHECK(web_contents);
   NavigationEntry* active_entry =
-      infobar_service->GetWebContents()->GetController().GetActiveEntry();
+      web_contents->GetController().GetActiveEntry();
   contents_unique_id_ = active_entry ? active_entry->GetUniqueID() : 0;
 }
 
 bool InfoBarDelegate::ShouldExpireInternal(
     const content::LoadCommittedDetails& details) const {
+  // NOTE: If you change this, be sure to check and adjust the behavior of
+  // anyone who overrides this as necessary!
   return (contents_unique_id_ != details.entry->GetUniqueID()) ||
       (content::PageTransitionStripQualifier(
           details.entry->GetTransitionType()) ==

chrome/browser/api/infobars/infobar_delegate.h

@@ -114,16 +114,12 @@ class InfoBarDelegate {
   // using StoreActiveEntryUniqueID automatically.
   explicit InfoBarDelegate(InfoBarService* infobar_service);
 
-  // Store the unique id for the active entry in the specified WebContents, to
-  // be used later upon navigation to determine if this InfoBarDelegate should
-  // be expired from |contents_|.
-  void StoreActiveEntryUniqueID(InfoBarService* infobar_service);
+  // Store the unique id for the active entry in our WebContents, to be used
+  // later upon navigation to determine if this InfoBarDelegate should be
+  // expired.
+  void StoreActiveEntryUniqueID();
 
-  // Direct accessors for subclasses that need to do something special.
   int contents_unique_id() const { return contents_unique_id_; }
-  void set_contents_unique_id(int contents_unique_id) {
-    contents_unique_id_ = contents_unique_id;
-  }
 
   // Returns true if the navigation is to a new URL or a reload occured.
   virtual bool ShouldExpireInternal(

chrome/browser/autofill/autofill_cc_infobar_delegate.cc

@@ -43,14 +43,6 @@ void AutofillCCInfoBarDelegate::LogUserAction(
   had_user_interaction_ = true;
 }
 
-bool AutofillCCInfoBarDelegate::ShouldExpire(
-    const content::LoadCommittedDetails& details) const {
-  // The user has submitted a form, causing the page to navigate elsewhere. We
-  // don't want the infobar to be expired at this point, because the user won't
-  // get a chance to answer the question.
-  return false;
-}
-
 void AutofillCCInfoBarDelegate::InfoBarDismissed() {
   LogUserAction(AutofillMetrics::INFOBAR_DENIED);
 }
@@ -64,6 +56,14 @@ InfoBarDelegate::Type AutofillCCInfoBarDelegate::GetInfoBarType() const {
   return PAGE_ACTION_TYPE;
 }
 
+bool AutofillCCInfoBarDelegate::ShouldExpireInternal(
+    const content::LoadCommittedDetails& details) const {
+  // The user has submitted a form, causing the page to navigate elsewhere. We
+  // don't want the infobar to be expired at this point, because the user won't
+  // get a chance to answer the question.
+  return false;
+}
+
 string16 AutofillCCInfoBarDelegate::GetMessageText() const {
   return l10n_util::GetStringUTF16(IDS_AUTOFILL_CC_INFOBAR_TEXT);
 }

chrome/browser/autofill/autofill_cc_infobar_delegate.h

@@ -35,11 +35,11 @@ class AutofillCCInfoBarDelegate : public ConfirmInfoBarDelegate {
   void LogUserAction(AutofillMetrics::InfoBarMetric user_action);
 
   // ConfirmInfoBarDelegate:
-  virtual bool ShouldExpire(
-      const content::LoadCommittedDetails& details) const OVERRIDE;
   virtual void InfoBarDismissed() OVERRIDE;
   virtual gfx::Image* GetIcon() const OVERRIDE;
   virtual Type GetInfoBarType() const OVERRIDE;
+  virtual bool ShouldExpireInternal(
+      const content::LoadCommittedDetails& details) const OVERRIDE;
   virtual string16 GetMessageText() const OVERRIDE;
   virtual string16 GetButtonLabel(InfoBarButton button) const OVERRIDE;
   virtual bool Accept() OVERRIDE;

chrome/browser/chrome_quota_permission_context.cc

@@ -60,7 +60,7 @@ class RequestQuotaInfoBarDelegate : public ConfirmInfoBarDelegate {
           QuotaPermissionContext::QUOTA_PERMISSION_RESPONSE_CANCELLED);
   }
 
-  virtual bool ShouldExpire(
+  virtual bool ShouldExpireInternal(
       const content::LoadCommittedDetails& details)
       const OVERRIDE {
     return false;

chrome/browser/extensions/api/debugger/debugger_api.cc

@@ -71,10 +71,10 @@ class ExtensionDevToolsInfoBarDelegate : public ConfirmInfoBarDelegate {
 
  private:
   // ConfirmInfoBarDelegate:
-  virtual bool ShouldExpire(
-      const content::LoadCommittedDetails& details) const OVERRIDE;
   virtual int GetButtons() const OVERRIDE;
   virtual Type GetInfoBarType() const OVERRIDE;
+  virtual bool ShouldExpireInternal(
+      const content::LoadCommittedDetails& details) const OVERRIDE;
   virtual string16 GetMessageText() const OVERRIDE;
   virtual void InfoBarDismissed() OVERRIDE;
   virtual bool Cancel() OVERRIDE;
@@ -363,11 +363,6 @@ void ExtensionDevToolsInfoBarDelegate::DiscardClientHost() {
   client_host_ = NULL;
 }
 
-bool ExtensionDevToolsInfoBarDelegate::ShouldExpire(
-    const content::LoadCommittedDetails& details) const {
-  return false;
-}
-
 int ExtensionDevToolsInfoBarDelegate::GetButtons() const {
   return BUTTON_CANCEL;
 }
@@ -376,6 +371,11 @@ InfoBarDelegate::Type ExtensionDevToolsInfoBarDelegate::GetInfoBarType() const {
   return WARNING_TYPE;
 }
 
+bool ExtensionDevToolsInfoBarDelegate::ShouldExpireInternal(
+    const content::LoadCommittedDetails& details) const {
+  return false;
+}
+
 string16 ExtensionDevToolsInfoBarDelegate::GetMessageText() const {
   return l10n_util::GetStringFUTF16(IDS_DEV_TOOLS_INFOBAR_LABEL,
                                     UTF8ToUTF16(client_name_));

chrome/browser/geolocation/chrome_geolocation_permission_context_unittest.cc

@@ -670,14 +670,13 @@ TEST_F(GeolocationPermissionContextTests, InfoBarUsesCommittedEntry) {
   ASSERT_EQ(1U, infobar_service()->GetInfoBarCount());
   InfoBarDelegate* infobar_0 = infobar_service()->GetInfoBarDelegateAt(0);
   ASSERT_TRUE(infobar_0);
-  // Ensure the infobar is not yet expired.
+  // Ensure the infobar wouldn't expire for a navigation to the committed entry.
   content::LoadCommittedDetails details;
   details.entry = web_contents()->GetController().GetLastCommittedEntry();
-  ASSERT_FALSE(infobar_0->ShouldExpire(details));
-  // Commit the "GoBack()" above, and ensure the infobar is now expired.
-  content::WebContentsTester::For(web_contents())->CommitPendingNavigation();
-  details.entry = web_contents()->GetController().GetLastCommittedEntry();
-  ASSERT_TRUE(infobar_0->ShouldExpire(details));
+  EXPECT_FALSE(infobar_0->ShouldExpire(details));
+  // Ensure the infobar will expire when we commit the pending navigation.
+  details.entry = web_contents()->GetController().GetActiveEntry();
+  EXPECT_TRUE(infobar_0->ShouldExpire(details));
 
   // Delete the tab contents.
   DeleteContents();

chrome/browser/geolocation/geolocation_confirm_infobar_delegate.cc

@@ -31,7 +31,7 @@ GeolocationConfirmInfoBarDelegate::GeolocationConfirmInfoBarDelegate(
       display_languages_(display_languages) {
   const content::NavigationEntry* committed_entry = infobar_service->
       GetWebContents()->GetController().GetLastCommittedEntry();
-  set_contents_unique_id(committed_entry ? committed_entry->GetUniqueID() : 0);
+   contents_unique_id_ = committed_entry ? committed_entry->GetUniqueID() : 0;
 }
 
 gfx::Image* GeolocationConfirmInfoBarDelegate::GetIcon() const {
@@ -44,6 +44,17 @@ InfoBarDelegate::Type
   return PAGE_ACTION_TYPE;
 }
 
+bool GeolocationConfirmInfoBarDelegate::ShouldExpireInternal(
+    const content::LoadCommittedDetails& details) const {
+  // This implementation matches InfoBarDelegate::ShouldExpireInternal(), but
+  // uses the unique ID we set in the constructor instead of that stored in the
+  // base class.
+  return (contents_unique_id_ != details.entry->GetUniqueID()) ||
+      (content::PageTransitionStripQualifier(
+          details.entry->GetTransitionType()) ==
+              content::PAGE_TRANSITION_RELOAD);
+}
+
 string16 GeolocationConfirmInfoBarDelegate::GetMessageText() const {
   return l10n_util::GetStringFUTF16(IDS_GEOLOCATION_INFOBAR_QUESTION,
       net::FormatUrl(requesting_frame_.GetOrigin(), display_languages_));

chrome/browser/geolocation/geolocation_confirm_infobar_delegate.h

@@ -32,6 +32,8 @@ class GeolocationConfirmInfoBarDelegate : public ConfirmInfoBarDelegate {
   // ConfirmInfoBarDelegate:
   virtual gfx::Image* GetIcon() const OVERRIDE;
   virtual Type GetInfoBarType() const OVERRIDE;
+  virtual bool ShouldExpireInternal(
+      const content::LoadCommittedDetails& details) const OVERRIDE;
   virtual string16 GetMessageText() const OVERRIDE;
   virtual string16 GetButtonLabel(InfoBarButton button) const OVERRIDE;
   virtual bool Accept() OVERRIDE;
@@ -46,6 +48,7 @@ class GeolocationConfirmInfoBarDelegate : public ConfirmInfoBarDelegate {
   GeolocationInfoBarQueueController* controller_;
   const GeolocationPermissionRequestID id_;
   GURL requesting_frame_;
+  int contents_unique_id_;
   std::string display_languages_;
 
   DISALLOW_IMPLICIT_CONSTRUCTORS(GeolocationConfirmInfoBarDelegate);

chrome/browser/google/google_url_tracker_infobar_delegate.cc

@@ -58,7 +58,7 @@ bool GoogleURLTrackerInfoBarDelegate::ShouldExpireInternal(
 }
 
 void GoogleURLTrackerInfoBarDelegate::Update(const GURL& search_url) {
-  StoreActiveEntryUniqueID(owner());
+  StoreActiveEntryUniqueID();
   search_url_ = search_url;
   pending_id_ = 0;
 }