[Extensions] Migrate ExtensionInstallPrompt::Delegate to be a callback

chrome/browser/extensions/extension_install_prompt.cc

Index: chrome/browser/extensions/extension_install_prompt.cc
diff --git a/chrome/browser/extensions/extension_install_prompt.cc b/chrome/browser/extensions/extension_install_prompt.cc
index 6345f4f65b39c0c21d2a39bf15d0bcbeac5cfad8..cd27d50b8f7332c5d6b2488bdac7f8097c7fd8de 100644
--- a/chrome/browser/extensions/extension_install_prompt.cc
+++ b/chrome/browser/extensions/extension_install_prompt.cc
@@ -6,6 +6,7 @@
 
 #include <utility>
 
+#include "base/callback_helpers.h"
 #include "base/location.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_util.h"
@@ -146,25 +147,22 @@ SkBitmap GetDefaultIconBitmapForMaxScaleFactor(bool is_app) {
 
 // If auto confirm is enabled then posts a task to proceed with or cancel the
 // install and returns true. Otherwise returns false.
-bool AutoConfirmPrompt(ExtensionInstallPrompt::Delegate* delegate) {
+bool AutoConfirmPrompt(ExtensionInstallPrompt::DoneCallback* callback) {
   switch (extensions::ScopedTestDialogAutoConfirm::GetAutoConfirmValue()) {
     case extensions::ScopedTestDialogAutoConfirm::NONE:
       return false;
-    // We use PostTask instead of calling the delegate directly here, because in
+    // We use PostTask instead of calling the callback directly here, because in
     // the real implementations it's highly likely the message loop will be
     // pumping a few times before the user clicks accept or cancel.
     case extensions::ScopedTestDialogAutoConfirm::ACCEPT:
       base::MessageLoop::current()->PostTask(
-          FROM_HERE,
-          base::Bind(&ExtensionInstallPrompt::Delegate::InstallUIProceed,
-                     base::Unretained(delegate)));
+          FROM_HERE, base::Bind(base::ResetAndReturn(callback),

[asargent_no_longer_on_chrome, 2015/12/31 00:45:56]

I'm probably missing something subtle here - can you explain why you're passing
in the callback by pointer and then using ResetAndReturn, instead of just having
the callback passed in as a const ref? 

[Devlin, 2016/01/04 22:59:23]

If the callback is run, then we have to reset the callback here (for lifetime
reasons), but if it's not, then we don't want to (because it will be called
later).  The alternative would be to pass it in as a const& here, and then if
this returns true, reset it, but this way ensures that the callback (and any
refs it holds) is reset before it is run, which is kind of nice.

+                                ExtensionInstallPrompt::Result::ACCEPTED));
       return true;
     case extensions::ScopedTestDialogAutoConfirm::CANCEL:
       base::ThreadTaskRunnerHandle::Get()->PostTask(
-          FROM_HERE,
-          base::Bind(&ExtensionInstallPrompt::Delegate::InstallUIAbort,
-                     base::Unretained(delegate),
-                     true));
+          FROM_HERE, base::Bind(base::ResetAndReturn(callback),
+                                ExtensionInstallPrompt::Result::USER_CANCELED));
       return true;
   }
 
@@ -624,7 +622,6 @@ ExtensionInstallPrompt::ExtensionInstallPrompt(content::WebContents* contents)
       install_ui_(extensions::CreateExtensionInstallUI(
           ProfileForWebContents(contents))),
       show_params_(new ExtensionInstallPromptShowParams(contents)),
-      delegate_(NULL),
       did_call_show_dialog_(false) {
 }
 
@@ -636,7 +633,6 @@ ExtensionInstallPrompt::ExtensionInstallPrompt(Profile* profile,
       install_ui_(extensions::CreateExtensionInstallUI(profile)),
       show_params_(
           new ExtensionInstallPromptShowParams(profile, native_window)),
-      delegate_(NULL),
       did_call_show_dialog_(false) {
 }
 
@@ -644,26 +640,26 @@ ExtensionInstallPrompt::~ExtensionInstallPrompt() {
 }
 
 void ExtensionInstallPrompt::ShowDialog(
-    Delegate* delegate,
+    const DoneCallback& done_callback,
     const Extension* extension,
     const SkBitmap* icon,
     const ShowDialogCallback& show_dialog_callback) {
-  ShowDialog(delegate, extension, icon,
+  ShowDialog(done_callback, extension, icon,
              make_scoped_ptr(new Prompt(INSTALL_PROMPT)), show_dialog_callback);
 }
 
 void ExtensionInstallPrompt::ShowDialog(
-    Delegate* delegate,
+    const DoneCallback& done_callback,
     const Extension* extension,
     const SkBitmap* icon,
     scoped_ptr<Prompt> prompt,
     const ShowDialogCallback& show_dialog_callback) {
-  ShowDialog(delegate, extension, icon, std::move(prompt), nullptr,
+  ShowDialog(done_callback, extension, icon, std::move(prompt), nullptr,
              show_dialog_callback);
 }
 
 void ExtensionInstallPrompt::ShowDialog(
-    Delegate* delegate,
+    const DoneCallback& done_callback,
     const Extension* extension,
     const SkBitmap* icon,
     scoped_ptr<Prompt> prompt,
@@ -672,7 +668,7 @@ void ExtensionInstallPrompt::ShowDialog(
   DCHECK(ui_loop_ == base::MessageLoop::current());
   DCHECK(prompt);
   extension_ = extension;
-  delegate_ = delegate;
+  done_callback_ = done_callback;
   if (icon && !icon->empty())
     SetIcon(icon);
   prompt_ = std::move(prompt);
@@ -689,7 +685,7 @@ void ExtensionInstallPrompt::ShowDialog(
   if (extension->is_theme()) {
     if (extension->from_webstore() ||
         extensions::FeatureSwitch::easy_off_store_install()->IsEnabled()) {
-      delegate->InstallUIProceed();
+      done_callback.Run(Result::ACCEPTED);
       return;
     }
   }
@@ -833,20 +829,19 @@ void ExtensionInstallPrompt::ShowConfirmation() {
   prompt_->set_icon(gfx::Image::CreateFrom1xBitmap(icon_));
 
   if (show_params_->WasParentDestroyed()) {
-    delegate_->InstallUIAbort(false);
+    base::ResetAndReturn(&done_callback_).Run(Result::ABORTED);
     return;
   }
 
   g_last_prompt_type_for_tests = prompt_->type();
   did_call_show_dialog_ = true;
 
-  if (AutoConfirmPrompt(delegate_))
+  if (AutoConfirmPrompt(&done_callback_))
     return;
 
   if (show_dialog_callback_.is_null())
-    GetDefaultShowDialogCallback().Run(show_params_.get(), delegate_,
-                                       std::move(prompt_));
-  else
-    show_dialog_callback_.Run(show_params_.get(), delegate_,
-                              std::move(prompt_));
+    show_dialog_callback_ = GetDefaultShowDialogCallback();
+  base::ResetAndReturn(&show_dialog_callback_)
+      .Run(show_params_.get(), base::ResetAndReturn(&done_callback_),
+           std::move(prompt_));

[asargent_no_longer_on_chrome, 2015/12/31 00:45:56]

Similar to my above question, I don't quite get why you're using ResetAndReturn
with the done_callback_ here (and you just call Run directly up on line 688). Is
the idea that you don't want to accidentally run done_callback_ twice, for
instance if someday we added code in the destructor like:

if (!done_callback_.is_null()) {
  done_callback_.Run(Result::ABORTED);
}

?

[Devlin, 2016/01/04 22:59:23]

Doing it directly on line 688 was a mistake.  Even though we, at some points, do
have checks in the dtor like that, the bigger reason is because callbacks can
contain ref counts.  So, if we have an object like:

InstallPromptCaller : RefCounted {
  void OnDone(Result result);
  scoped_ptr<ExtensionInstallPrompt> prompt;
}

InstallPromptCaller::RunPrompt() {
  prompt.reset(new ExtensionInstallPrompt());
  prompt->ShowConfirmation(..., base::Bind(&InstallPromptCaller::OnDone, this));
}

Now, if we look at the lifetime chain, we can see that:
- InstallPromptCaller owns ExtensionInstallPrompt.
- ExtensionInstallPrompt's done_callback_ is callback to OnDone, and it adds a
refcount to InstallPromptCaller.
That means that the InstallPromptCaller's ref count will never go to zero. 
Either ExtensionInstallPrompt needs to be destroyed or the done_callback_ needs
to be reset (or, perhaps, both).

I don't know if any callers do this here, but I've had enough bad luck with
callback refcounts to not want to find out.  I think it's always good habit to
reset member callbacks if they might have refcounts.

[asargent_no_longer_on_chrome, 2016/01/05 18:58:54]

Ok. I am slightly worried about an overall design that requires us to remember
to call the callback with ResetAndReturn everywhere, since that implies a "both
objects have references to each other" situation that I worry will prove to be
fragile. (Maybe that's an issue with our larger callback system though? I wonder
if it would be useful to draw a distinction between callbacks that are intended
to be called only once, and those that are intended to be called multiple times,
and automatically reset the former when they are run.)

But anyhow, I think this CL is overall a step in the right direction, and we can
possibly make more improvements in the future. 

[Devlin, 2016/01/05 21:30:16]

I agree - I'd like to not have to do this.  But there are enough places where
the lifetime is a little hazy that I think this way is safer.  That said, I
really do like the idea of a one-run callback that auto-resets after being
called.

 }