Use WeakPtr<> to break DownloadRequestInfobarDelegate dependency.

DownloadRequestInfobarDelegate has a pointer back to
DownloadRequestLimiter::TabDownloadState to inform it of user response
to the download throttling prompt.  That used to be broken on
TabDownloadState destruction by a callback to DownloadRequestInfobarDelegate 
resetting the pointer.  This patch uses a WeakPtr<> instead.

BUG=168968
R=benjhayden@chromium.org
TBR=pkasting@chromium.org


Review URL: https://chromiumcodereview.appspot.com/11884005

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

5 files changed, 39 insertions, 31 deletions

diff --git a/chrome/browser/download/download_request_infobar_delegate.cc b/chrome/browser/download/download_request_infobar_delegate.cc
index b27ebb8..e5dc325 100644
--- a/chrome/browser/download/download_request_infobar_delegate.cc
+++ b/chrome/browser/download/download_request_infobar_delegate.cc
@@ -18,14 +18,14 @@ DownloadRequestInfoBarDelegate::~DownloadRequestInfoBarDelegate() {
 // static
 void DownloadRequestInfoBarDelegate::Create(
     InfoBarService* infobar_service,
-    DownloadRequestLimiter::TabDownloadState* host) {
+    base::WeakPtr<DownloadRequestLimiter::TabDownloadState> host) {
   infobar_service->AddInfoBar(scoped_ptr<InfoBarDelegate>(
       new DownloadRequestInfoBarDelegate(infobar_service, host)));
 }
 
 DownloadRequestInfoBarDelegate::DownloadRequestInfoBarDelegate(
     InfoBarService* infobar_service,
-    DownloadRequestLimiter::TabDownloadState* host)
+    base::WeakPtr<DownloadRequestLimiter::TabDownloadState> host)
     : ConfirmInfoBarDelegate(infobar_service),
       host_(host) {
 }
@@ -47,7 +47,8 @@ string16 DownloadRequestInfoBarDelegate::GetButtonLabel(
 
 bool DownloadRequestInfoBarDelegate::Accept() {
   if (host_) {
-    // Accept() call will nullify host_ if no further prompts are required.
+    // Accept() call will invalidate host_ weak pointer if no further
+    // prompts are required.
     host_->Accept();
   }
 
diff --git a/chrome/browser/download/download_request_infobar_delegate.h b/chrome/browser/download/download_request_infobar_delegate.h
index e4a9ab0..b3558ae 100644
--- a/chrome/browser/download/download_request_infobar_delegate.h
+++ b/chrome/browser/download/download_request_infobar_delegate.h
@@ -6,6 +6,7 @@
 #define CHROME_BROWSER_DOWNLOAD_DOWNLOAD_REQUEST_INFOBAR_DELEGATE_H_
 
 #include "base/basictypes.h"
+#include "base/memory/weak_ptr.h"
 #include "chrome/browser/api/infobars/confirm_infobar_delegate.h"
 #include "chrome/browser/download/download_request_limiter.h"
 
@@ -20,25 +21,22 @@ class DownloadRequestInfoBarDelegate : public ConfirmInfoBarDelegate {
   virtual ~DownloadRequestInfoBarDelegate();
 
   // Creates a download request delegate and adds it to |infobar_service|.
-  static void Create(InfoBarService* infobar_service,
-                     DownloadRequestLimiter::TabDownloadState* host);
+  static void Create(
+      InfoBarService* infobar_service,
+      base::WeakPtr<DownloadRequestLimiter::TabDownloadState> host);
 
 #if defined(UNIT_TEST)
   static scoped_ptr<DownloadRequestInfoBarDelegate> Create(
-      DownloadRequestLimiter::TabDownloadState* host) {
+      base::WeakPtr<DownloadRequestLimiter::TabDownloadState> host) {
     return scoped_ptr<DownloadRequestInfoBarDelegate>(
         new DownloadRequestInfoBarDelegate(NULL, host));
   }
 #endif
 
-  void set_host(DownloadRequestLimiter::TabDownloadState* host) {
-    host_ = host;
-  }
-
  private:
   DownloadRequestInfoBarDelegate(
       InfoBarService* infobar_service,
-      DownloadRequestLimiter::TabDownloadState* host);
+      base::WeakPtr<DownloadRequestLimiter::TabDownloadState> host);
 
   // ConfirmInfoBarDelegate:
   virtual gfx::Image* GetIcon() const OVERRIDE;
@@ -46,7 +44,7 @@ class DownloadRequestInfoBarDelegate : public ConfirmInfoBarDelegate {
   virtual string16 GetButtonLabel(InfoBarButton button) const OVERRIDE;
   virtual bool Accept() OVERRIDE;
 
-  DownloadRequestLimiter::TabDownloadState* host_;
+  base::WeakPtr<DownloadRequestLimiter::TabDownloadState> host_;
 
   DISALLOW_COPY_AND_ASSIGN(DownloadRequestInfoBarDelegate);
 };
diff --git a/chrome/browser/download/download_request_infobar_delegate_unittest.cc b/chrome/browser/download/download_request_infobar_delegate_unittest.cc
index d8e070a..aefeef0 100644
--- a/chrome/browser/download/download_request_infobar_delegate_unittest.cc
+++ b/chrome/browser/download/download_request_infobar_delegate_unittest.cc
@@ -3,6 +3,7 @@
 // found in the LICENSE file.
 
 #include "base/memory/scoped_ptr.h"
+#include "base/memory/weak_ptr.h"
 #include "chrome/browser/download/download_request_infobar_delegate.h"
 #include "chrome/browser/download/download_request_limiter.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -24,6 +25,9 @@ class MockTabDownloadState : public DownloadRequestLimiter::TabDownloadState {
   bool accepted() const { return accepted_; }
 
  private:
+  // To produce weak pointers for infobar_ construction.
+  base::WeakPtrFactory<DownloadRequestLimiter::TabDownloadState> factory_;
+
   // The actual infobar delegate we're listening to.
   scoped_ptr<DownloadRequestInfoBarDelegate> infobar_;
 
@@ -33,13 +37,14 @@ class MockTabDownloadState : public DownloadRequestLimiter::TabDownloadState {
   // True if we have gotten a Accept response. Meaningless if |responded_| is
   // not true.
   bool accepted_;
+
 };
 
 MockTabDownloadState::MockTabDownloadState()
-    : infobar_(DownloadRequestInfoBarDelegate::Create(this)),
+    : factory_(ALLOW_THIS_IN_INITIALIZER_LIST(this)),
+      infobar_(DownloadRequestInfoBarDelegate::Create(factory_.GetWeakPtr())),
       responded_(false),
-      accepted_(false) {
-}
+      accepted_(false) {}
 
 MockTabDownloadState::~MockTabDownloadState() {
   EXPECT_TRUE(responded_);
@@ -55,7 +60,7 @@ void MockTabDownloadState::Accept() {
   EXPECT_FALSE(responded_);
   responded_ = true;
   accepted_ = true;
-  infobar_->set_host(NULL);
+  factory_.InvalidateWeakPtrs();
 }
 
 
diff --git a/chrome/browser/download/download_request_limiter.cc b/chrome/browser/download/download_request_limiter.cc
index 1b7d08b..762fea6 100644
--- a/chrome/browser/download/download_request_limiter.cc
+++ b/chrome/browser/download/download_request_limiter.cc
@@ -34,7 +34,7 @@ DownloadRequestLimiter::TabDownloadState::TabDownloadState(
       host_(host),
       status_(DownloadRequestLimiter::ALLOW_ONE_DOWNLOAD),
       download_count_(0),
-      infobar_(NULL) {
+      factory_(ALLOW_THIS_IN_INITIALIZER_LIST(this)) {
   content::Source<NavigationController> notification_source(
       &contents->GetController());
   content::Source<content::WebContents> web_contents_source(contents);
@@ -54,8 +54,8 @@ DownloadRequestLimiter::TabDownloadState::~TabDownloadState() {
   // We should only be destroyed after the callbacks have been notified.
   DCHECK(callbacks_.empty());
 
-  // And we should have closed the infobar.
-  DCHECK(!infobar_);
+  // And we should have invalidated the back pointer.
+  DCHECK(!factory_.HasWeakPtrs());
 }
 
 void DownloadRequestLimiter::TabDownloadState::DidGetUserGesture() {
@@ -106,7 +106,8 @@ void DownloadRequestLimiter::TabDownloadState::PromptUserForDownload(
     Cancel();
     return;
   }
-  DownloadRequestInfoBarDelegate::Create(infobar_service, this);
+  DownloadRequestInfoBarDelegate::Create(infobar_service,
+                                         factory_.GetWeakPtr());
 }
 
 void DownloadRequestLimiter::TabDownloadState::Cancel() {
@@ -121,7 +122,7 @@ DownloadRequestLimiter::TabDownloadState::TabDownloadState()
     : host_(NULL),
       status_(DownloadRequestLimiter::ALLOW_ONE_DOWNLOAD),
       download_count_(0),
-      infobar_(NULL) {
+      factory_(ALLOW_THIS_IN_INITIALIZER_LIST(this)) {
 }
 
 void DownloadRequestLimiter::TabDownloadState::Observe(
@@ -179,11 +180,8 @@ void DownloadRequestLimiter::TabDownloadState::NotifyCallbacks(bool allow) {
   // don't close it. If allow is false, we send all the notifications to cancel
   // all remaining downloads and close the infobar.
   if (!allow || (callbacks_.size() < kMaxDownloadsAtOnce)) {
-    if (infobar_) {
-      // Reset the delegate so we don't get notified again.
-      infobar_->set_host(NULL);
-      infobar_ = NULL;
-    }
+    // Null the generated weak pointer so we don't get notified again.
+    factory_.InvalidateWeakPtrs();
     callbacks.swap(callbacks_);
   } else {
     std::vector<DownloadRequestLimiter::Callback>::iterator start, end;
diff --git a/chrome/browser/download/download_request_limiter.h b/chrome/browser/download/download_request_limiter.h
index 794cc38..8f341e7 100644
--- a/chrome/browser/download/download_request_limiter.h
+++ b/chrome/browser/download/download_request_limiter.h
@@ -12,6 +12,7 @@
 #include "base/callback.h"
 #include "base/gtest_prod_util.h"
 #include "base/memory/ref_counted.h"
+#include "base/memory/weak_ptr.h"
 #include "content/public/browser/notification_observer.h"
 #include "content/public/browser/notification_registrar.h"
 #include "content/public/browser/web_contents_observer.h"
@@ -114,9 +115,6 @@ class DownloadRequestLimiter
         content::WebContents* tab,
         const DownloadRequestLimiter::Callback& callback);
 
-    // Are we showing a prompt to the user?
-    bool is_showing_prompt() const { return (infobar_ != NULL); }
-
     // Invoked from DownloadRequestDialogDelegate. Notifies the delegates and
     // changes the status appropriately. Virtual for testing.
     virtual void Cancel();
@@ -127,6 +125,11 @@ class DownloadRequestLimiter
     TabDownloadState();
 
    private:
+    // Are we showing a prompt to the user?  Determined by whether
+    // we have an outstanding weak pointer--weak pointers are only
+    // given to the info bar delegate.
+    bool is_showing_prompt() const { return factory_.HasWeakPtrs(); }
+
     // content::NotificationObserver method.
     virtual void Observe(int type,
                          const content::NotificationSource& source,
@@ -154,8 +157,11 @@ class DownloadRequestLimiter
     // Used to remove observers installed on NavigationController.
     content::NotificationRegistrar registrar_;
 
-    // Handles showing the infobar to the user, may be null.
-    DownloadRequestInfoBarDelegate* infobar_;
+    // Weak pointer factory for generating a weak pointer to pass to the
+    // infobar.  User responses to the throttling prompt will be returned
+    // through this channel, and it can be revoked if the user prompt result
+    // becomes moot.
+    base::WeakPtrFactory<DownloadRequestLimiter::TabDownloadState> factory_;
 
     DISALLOW_COPY_AND_ASSIGN(TabDownloadState);
   };