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

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

The ExtensionInstallPrompt + Delegate setup is prone to lifetime issues because
the caller of the delegate is a UI element, whereas the delegate is usually
somehow part of the extension system. At best, this makes lifetime management a
little confusing, but at worst, it's fundamentally flawed because UI ownership
and extension system ownership are very complicated at times like shutdown.

Instead, migrate the delegate to be a callback, where lifetime management is
more explicit (and safe if the fka delegate is deleted). Also update a few
confusing parts of the Cocoa install prompt API.

BUG=567844
TBR=benwells@chromium.org (small c/b/ui/extensions changes)

Committed: https://crrev.com/415930557c280022fed6a6ddd01f80f443704683
Cr-Commit-Position: refs/heads/master@{#368238}

[Devlin, 2015-12-22 23:44:35]

Patchset #8 (id:140001) has been deleted

[Devlin, 2015-12-29 00:01:29]

Patchset #1 (id:1) has been deleted

[Devlin, 2015-12-29 00:01:41]

Patchset #1 (id:20001) has been deleted

[Devlin, 2015-12-29 00:01:52]

Patchset #1 (id:40001) has been deleted

[Devlin, 2015-12-29 00:02:01]

Patchset #1 (id:60001) has been deleted

[Devlin, 2015-12-29 00:02:16]

Patchset #1 (id:80001) has been deleted

[Devlin, 2015-12-29 00:02:32]

Patchset #1 (id:100001) has been deleted

[Devlin, 2015-12-29 00:02:39]

Patchset #1 (id:120001) has been deleted

[Devlin, 2015-12-29 00:02:47]

Patchset #1 (id:160001) has been deleted

[Devlin, 2015-12-29 00:09:00]

Description was changed from

==========
No delegate
BUG=
==========

to

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

The ExtensionInstallPrompt + Delegate setup is prone to lifetime issues because
the caller of the delegate is a UI element, whereas the delegate is usually
somehow part of the extension system. At best, this makes lifetime management a
little confusing, but at worst, it's fundamentally flawed because UI ownership
and extension system ownership are very complicated at times like shutdown.

Instead, migrate the delegate to be a callback, where lifetime management is
more explicit (and safe if the fka delegate is deleted). Also update a few
confusing parts of the Cocoa install prompt API.

BUG=567844
==========

[Devlin, 2015-12-29 00:09:47]

Description was changed from

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

The ExtensionInstallPrompt + Delegate setup is prone to lifetime issues because
the caller of the delegate is a UI element, whereas the delegate is usually
somehow part of the extension system. At best, this makes lifetime management a
little confusing, but at worst, it's fundamentally flawed because UI ownership
and extension system ownership are very complicated at times like shutdown.

Instead, migrate the delegate to be a callback, where lifetime management is
more explicit (and safe if the fka delegate is deleted). Also update a few
confusing parts of the Cocoa install prompt API.

BUG=567844
==========

to

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

The ExtensionInstallPrompt + Delegate setup is prone to lifetime issues because
the caller of the delegate is a UI element, whereas the delegate is usually
somehow part of the extension system. At best, this makes lifetime management a
little confusing, but at worst, it's fundamentally flawed because UI ownership
and extension system ownership are very complicated at times like shutdown.

Instead, migrate the delegate to be a callback, where lifetime management is
more explicit (and safe if the fka delegate is deleted). Also update a few
confusing parts of the Cocoa install prompt API.

BUG=567844
==========

[Devlin, 2015-12-29 00:12:51]

rdevlin.cronin@chromium.org changed reviewers:
+ asargent@chromium.org, avi@chromium.org

[Devlin, 2015-12-29 00:12:52]

Antony (everything), Avi (cocoa), please take a look when you can.  I know it's
the holidays, and this isn't urgent, so take your time. :)

Sorry for the size - I didn't really know how to break this up.  When possible,
I've tried to make this have no behavior change.

https://codereview.chromium.org/1534123002/diff/180001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/extensions/extension_install_view_controller.h
(right):

https://codereview.chromium.org/1534123002/diff/180001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/extensions/extension_install_view_controller.h:24: class
ExtensionInstallViewDelegate {
It was incredibly confusing to me that two different objects would conditionally
call the ExtensionInstallPrompt::Delegate methods, and this makes it much
clearer where division of control lies.  Let me know if you think differently.

[Avi (use Gerrit), 2015-12-29 03:10:45]

lgtm

Lookin' good, though you really need to iwyu.

https://codereview.chromium.org/1534123002/diff/180001/chrome/browser/extensi...
File chrome/browser/extensions/api/dashboard_private/dashboard_private_api.h
(right):

https://codereview.chromium.org/1534123002/diff/180001/chrome/browser/extensi...
chrome/browser/extensions/api/dashboard_private/dashboard_private_api.h:75:
DISALLOW_COPY_AND_ASSIGN(
#include "base/macros.h"

https://codereview.chromium.org/1534123002/diff/180001/chrome/browser/extensi...
File chrome/browser/extensions/api/permissions/permissions_api.h (right):

https://codereview.chromium.org/1534123002/diff/180001/chrome/browser/extensi...
chrome/browser/extensions/api/permissions/permissions_api.h:76:
DISALLOW_COPY_AND_ASSIGN(PermissionsRequestFunction);
base/macros.h

https://codereview.chromium.org/1534123002/diff/180001/chrome/browser/extensi...
File chrome/browser/extensions/extension_disabled_ui.cc (right):

https://codereview.chromium.org/1534123002/diff/180001/chrome/browser/extensi...
chrome/browser/extensions/extension_disabled_ui.cc:112:
DISALLOW_COPY_AND_ASSIGN(ExtensionDisabledDialogDelegate);
base/macros.h

https://codereview.chromium.org/1534123002/diff/180001/chrome/browser/extensi...
File chrome/browser/extensions/unpacked_installer.cc (right):

https://codereview.chromium.org/1534123002/diff/180001/chrome/browser/extensi...
chrome/browser/extensions/unpacked_installer.cc:67:
DISALLOW_COPY_AND_ASSIGN(SimpleExtensionLoadPrompt);
base/macros.h

https://codereview.chromium.org/1534123002/diff/180001/chrome/browser/extensi...
File chrome/browser/extensions/webstore_inline_installer_browsertest.cc (right):

https://codereview.chromium.org/1534123002/diff/180001/chrome/browser/extensi...
chrome/browser/extensions/webstore_inline_installer_browsertest.cc:95:
DISALLOW_COPY_AND_ASSIGN(ProgrammableInstallPrompt);
You know.

https://codereview.chromium.org/1534123002/diff/180001/chrome/browser/ui/view...
File chrome/browser/ui/views/extensions/extension_install_dialog_view.h (right):

https://codereview.chromium.org/1534123002/diff/180001/chrome/browser/ui/view...
chrome/browser/ui/views/extensions/extension_install_dialog_view.h:114: // has
been ran.
"... has been run"?

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

I found a couple of minor issues

https://codereview.chromium.org/1534123002/diff/180001/chrome/browser/extensi...
File
chrome/browser/extensions/api/developer_private/show_permissions_dialog_helper.cc
(right):

https://codereview.chromium.org/1534123002/diff/180001/chrome/browser/extensi...
chrome/browser/extensions/api/developer_private/show_permissions_dialog_helper.cc:87:
// deletes itself in OnInstallPromptDone().
also safe because it owns the prompt and so should be guaranteed to outlive it -
not sure if that's worth mentioning or not

https://codereview.chromium.org/1534123002/diff/180001/chrome/browser/extensi...
File chrome/browser/extensions/api/management/chrome_management_api_delegate.cc
(right):

https://codereview.chromium.org/1534123002/diff/180001/chrome/browser/extensi...
chrome/browser/extensions/api/management/chrome_management_api_delegate.cc:54:
install_prompt_.reset(new ExtensionInstallPrompt(web_contents));
nit: looks like you're initializing install_prompt_ twice (once in initializer
list above, and once here)

https://codereview.chromium.org/1534123002/diff/180001/chrome/browser/extensi...
chrome/browser/extensions/api/management/chrome_management_api_delegate.cc:73:
extensions::ManagementSetEnabledFunction* function_;
Can you get rid of function_? Looks like we aren't using it anymore?

https://codereview.chromium.org/1534123002/diff/180001/chrome/browser/extensi...
File chrome/browser/extensions/api/webstore_private/webstore_private_api.cc
(right):

https://codereview.chromium.org/1534123002/diff/180001/chrome/browser/extensi...
chrome/browser/extensions/api/webstore_private/webstore_private_api.cc:299: //
Control flow finishes up in InstallUIProceed or InstallUIAbort.
nit: update to "...finishes up in OnInstallPromptDone"

https://codereview.chromium.org/1534123002/diff/180001/chrome/browser/extensi...
File chrome/browser/extensions/extension_disabled_ui.cc (right):

https://codereview.chromium.org/1534123002/diff/180001/chrome/browser/extensi...
chrome/browser/extensions/extension_disabled_ui.cc:126: // itself only once the
prompt finishes.
also because it owns install_ui_ (not sure if worth mentioning or not)

https://codereview.chromium.org/1534123002/diff/180001/chrome/browser/extensi...
File chrome/browser/extensions/extension_install_prompt.cc (right):

https://codereview.chromium.org/1534123002/diff/180001/chrome/browser/extensi...
chrome/browser/extensions/extension_install_prompt.cc:159: FROM_HERE,
base::Bind(base::ResetAndReturn(callback),
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?

https://codereview.chromium.org/1534123002/diff/180001/chrome/browser/extensi...
chrome/browser/extensions/extension_install_prompt.cc:846: std::move(prompt_));
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);
}

?

https://codereview.chromium.org/1534123002/diff/180001/chrome/browser/extensi...
File chrome/browser/extensions/extension_install_prompt.h (right):

https://codereview.chromium.org/1534123002/diff/180001/chrome/browser/extensi...
chrome/browser/extensions/extension_install_prompt.h:321: // We *MUST*
eventually call either Proceed() or Abort() on |delegate|.
nit: need to update this comment since there's no longer a delegate with
proceed/abort functions

https://codereview.chromium.org/1534123002/diff/180001/chrome/browser/extensi...
File chrome/browser/extensions/unpacked_installer.cc (right):

https://codereview.chromium.org/1534123002/diff/180001/chrome/browser/extensi...
chrome/browser/extensions/unpacked_installer.cc:86: // ExtensionInstallPrompt
code, so these should be unnecessary.
Do you mean we don't need to bother looking at the value of GetAutoConfirmValue,
but rather can just rely on the call to ShowDialog to do the right thing?

That sounds right to me - did you try doing it?

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

https://codereview.chromium.org/1534123002/diff/180001/chrome/browser/extensi...
File chrome/browser/extensions/api/dashboard_private/dashboard_private_api.h
(right):

https://codereview.chromium.org/1534123002/diff/180001/chrome/browser/extensi...
chrome/browser/extensions/api/dashboard_private/dashboard_private_api.h:75:
DISALLOW_COPY_AND_ASSIGN(
On 2015/12/29 03:10:45, Avi wrote:
> #include "base/macros.h"

Done.

https://codereview.chromium.org/1534123002/diff/180001/chrome/browser/extensi...
File
chrome/browser/extensions/api/developer_private/show_permissions_dialog_helper.cc
(right):

https://codereview.chromium.org/1534123002/diff/180001/chrome/browser/extensi...
chrome/browser/extensions/api/developer_private/show_permissions_dialog_helper.cc:87:
// deletes itself in OnInstallPromptDone().
On 2015/12/31 00:45:56, Antony Sargent wrote:
> also safe because it owns the prompt and so should be guaranteed to outlive it
-
> not sure if that's worth mentioning or not

But that's part of the point in this change - it's not necessarily safe just
because the prompt is owned, because the prompt here is the
ExtensionInstallPrompt, whose lifetime is completely independent from the
ExtensionInstallPromptView/cocoa equivalent.  Just because this owns the prompt
doesn't mean that the install prompt view couldn't outlive this.

https://codereview.chromium.org/1534123002/diff/180001/chrome/browser/extensi...
File chrome/browser/extensions/api/management/chrome_management_api_delegate.cc
(right):

https://codereview.chromium.org/1534123002/diff/180001/chrome/browser/extensi...
chrome/browser/extensions/api/management/chrome_management_api_delegate.cc:54:
install_prompt_.reset(new ExtensionInstallPrompt(web_contents));
On 2015/12/31 00:45:56, Antony Sargent wrote:
> nit: looks like you're initializing install_prompt_ twice (once in initializer
> list above, and once here)

Whoops, thanks!

https://codereview.chromium.org/1534123002/diff/180001/chrome/browser/extensi...
chrome/browser/extensions/api/management/chrome_management_api_delegate.cc:73:
extensions::ManagementSetEnabledFunction* function_;
On 2015/12/31 00:45:56, Antony Sargent wrote:
> Can you get rid of function_? Looks like we aren't using it anymore?

Done.

https://codereview.chromium.org/1534123002/diff/180001/chrome/browser/extensi...
File chrome/browser/extensions/api/permissions/permissions_api.h (right):

https://codereview.chromium.org/1534123002/diff/180001/chrome/browser/extensi...
chrome/browser/extensions/api/permissions/permissions_api.h:76:
DISALLOW_COPY_AND_ASSIGN(PermissionsRequestFunction);
On 2015/12/29 03:10:45, Avi wrote:
> base/macros.h

Done.

https://codereview.chromium.org/1534123002/diff/180001/chrome/browser/extensi...
File chrome/browser/extensions/api/webstore_private/webstore_private_api.cc
(right):

https://codereview.chromium.org/1534123002/diff/180001/chrome/browser/extensi...
chrome/browser/extensions/api/webstore_private/webstore_private_api.cc:299: //
Control flow finishes up in InstallUIProceed or InstallUIAbort.
On 2015/12/31 00:45:56, Antony Sargent wrote:
> nit: update to "...finishes up in OnInstallPromptDone"

Done.

https://codereview.chromium.org/1534123002/diff/180001/chrome/browser/extensi...
File chrome/browser/extensions/extension_disabled_ui.cc (right):

https://codereview.chromium.org/1534123002/diff/180001/chrome/browser/extensi...
chrome/browser/extensions/extension_disabled_ui.cc:112:
DISALLOW_COPY_AND_ASSIGN(ExtensionDisabledDialogDelegate);
On 2015/12/29 03:10:45, Avi wrote:
> base/macros.h

Done.

https://codereview.chromium.org/1534123002/diff/180001/chrome/browser/extensi...
chrome/browser/extensions/extension_disabled_ui.cc:126: // itself only once the
prompt finishes.
On 2015/12/31 00:45:56, Antony Sargent wrote:
> also because it owns install_ui_ (not sure if worth mentioning or not)

See other comment.

https://codereview.chromium.org/1534123002/diff/180001/chrome/browser/extensi...
File chrome/browser/extensions/extension_install_prompt.cc (right):

https://codereview.chromium.org/1534123002/diff/180001/chrome/browser/extensi...
chrome/browser/extensions/extension_install_prompt.cc:159: FROM_HERE,
base::Bind(base::ResetAndReturn(callback),
On 2015/12/31 00:45:56, Antony Sargent wrote:
> 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? 

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.

https://codereview.chromium.org/1534123002/diff/180001/chrome/browser/extensi...
chrome/browser/extensions/extension_install_prompt.cc:846: std::move(prompt_));
On 2015/12/31 00:45:56, Antony Sargent wrote:
> 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);
> }
> 
> ?

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.

https://codereview.chromium.org/1534123002/diff/180001/chrome/browser/extensi...
File chrome/browser/extensions/extension_install_prompt.h (right):

https://codereview.chromium.org/1534123002/diff/180001/chrome/browser/extensi...
chrome/browser/extensions/extension_install_prompt.h:321: // We *MUST*
eventually call either Proceed() or Abort() on |delegate|.
On 2015/12/31 00:45:56, Antony Sargent wrote:
> nit: need to update this comment since there's no longer a delegate with
> proceed/abort functions

Done.

https://codereview.chromium.org/1534123002/diff/180001/chrome/browser/extensi...
File chrome/browser/extensions/unpacked_installer.cc (right):

https://codereview.chromium.org/1534123002/diff/180001/chrome/browser/extensi...
chrome/browser/extensions/unpacked_installer.cc:67:
DISALLOW_COPY_AND_ASSIGN(SimpleExtensionLoadPrompt);
On 2015/12/29 03:10:45, Avi wrote:
> base/macros.h

Should be fine, since macros.h is included in unpacked_installer.h (and we don't
need to iwyu when it's in the file's associated header).

https://codereview.chromium.org/1534123002/diff/180001/chrome/browser/extensi...
chrome/browser/extensions/unpacked_installer.cc:86: // ExtensionInstallPrompt
code, so these should be unnecessary.
On 2015/12/31 00:45:56, Antony Sargent wrote:
> Do you mean we don't need to bother looking at the value of
GetAutoConfirmValue,
> but rather can just rely on the call to ShowDialog to do the right thing?
> 
> That sounds right to me - did you try doing it? 

Haven't tried it yet - tried to keep this small and targeted (as odd as that
sounds from a N-hundred line patch).  I'll do it in a followup, just in case one
or the other breaks something. :)

https://codereview.chromium.org/1534123002/diff/180001/chrome/browser/extensi...
File chrome/browser/extensions/webstore_inline_installer_browsertest.cc (right):

https://codereview.chromium.org/1534123002/diff/180001/chrome/browser/extensi...
chrome/browser/extensions/webstore_inline_installer_browsertest.cc:95:
DISALLOW_COPY_AND_ASSIGN(ProgrammableInstallPrompt);
On 2015/12/29 03:10:45, Avi wrote:
> You know.

Done.

https://codereview.chromium.org/1534123002/diff/180001/chrome/browser/ui/view...
File chrome/browser/ui/views/extensions/extension_install_dialog_view.h (right):

https://codereview.chromium.org/1534123002/diff/180001/chrome/browser/ui/view...
chrome/browser/ui/views/extensions/extension_install_dialog_view.h:114: // has
been ran.
On 2015/12/29 03:10:45, Avi wrote:
> "... has been run"?

Dang English.  Done.

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

lgtm

https://codereview.chromium.org/1534123002/diff/180001/chrome/browser/extensi...
File chrome/browser/extensions/extension_install_prompt.cc (right):

https://codereview.chromium.org/1534123002/diff/180001/chrome/browser/extensi...
chrome/browser/extensions/extension_install_prompt.cc:846: std::move(prompt_));
On 2016/01/04 22:59:23, Devlin wrote:
> On 2015/12/31 00:45:56, Antony Sargent wrote:
> > 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);
> > }
> > 
> > ?
> 
> 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.

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.

https://codereview.chromium.org/1534123002/diff/180001/chrome/browser/extensi...
File chrome/browser/extensions/unpacked_installer.cc (right):

https://codereview.chromium.org/1534123002/diff/180001/chrome/browser/extensi...
chrome/browser/extensions/unpacked_installer.cc:86: // ExtensionInstallPrompt
code, so these should be unnecessary.
On 2016/01/04 22:59:23, Devlin wrote:
> On 2015/12/31 00:45:56, Antony Sargent wrote:
> > Do you mean we don't need to bother looking at the value of
> GetAutoConfirmValue,
> > but rather can just rely on the call to ShowDialog to do the right thing?
> > 
> > That sounds right to me - did you try doing it? 
> 
> Haven't tried it yet - tried to keep this small and targeted (as odd as that
> sounds from a N-hundred line patch).  I'll do it in a followup, just in case
one
> or the other breaks something. :)

Ok

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

https://codereview.chromium.org/1534123002/diff/180001/chrome/browser/extensi...
File chrome/browser/extensions/extension_install_prompt.cc (right):

https://codereview.chromium.org/1534123002/diff/180001/chrome/browser/extensi...
chrome/browser/extensions/extension_install_prompt.cc:846: std::move(prompt_));
On 2016/01/05 18:58:54, Antony Sargent wrote:
> On 2016/01/04 22:59:23, Devlin wrote:
> > On 2015/12/31 00:45:56, Antony Sargent wrote:
> > > 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);
> > > }
> > > 
> > > ?
> > 
> > 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.
> 
> 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. 
> 
> 
> 

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.

[Devlin, 2016-01-05 21:30:34]

rdevlin.cronin@chromium.org changed reviewers:
+ finnur@chromium.org

[Devlin, 2016-01-05 21:30:34]

+Finnur for c/b/ui/views/extensions

[Devlin, 2016-01-05 21:30:37]

+Finnur for c/b/ui/views/extensions

[Finnur, 2016-01-06 10:36:19]

c/b/ui/views/extensions LGTM

[Devlin, 2016-01-08 00:20:36]

Description was changed from

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

The ExtensionInstallPrompt + Delegate setup is prone to lifetime issues because
the caller of the delegate is a UI element, whereas the delegate is usually
somehow part of the extension system. At best, this makes lifetime management a
little confusing, but at worst, it's fundamentally flawed because UI ownership
and extension system ownership are very complicated at times like shutdown.

Instead, migrate the delegate to be a callback, where lifetime management is
more explicit (and safe if the fka delegate is deleted). Also update a few
confusing parts of the Cocoa install prompt API.

BUG=567844
==========

to

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

The ExtensionInstallPrompt + Delegate setup is prone to lifetime issues because
the caller of the delegate is a UI element, whereas the delegate is usually
somehow part of the extension system. At best, this makes lifetime management a
little confusing, but at worst, it's fundamentally flawed because UI ownership
and extension system ownership are very complicated at times like shutdown.

Instead, migrate the delegate to be a callback, where lifetime management is
more explicit (and safe if the fka delegate is deleted). Also update a few
confusing parts of the Cocoa install prompt API.

BUG=567844
TBR=benwells@chromium.org (small c/b/ui/extensions changes)
==========

[Devlin, 2016-01-08 01:30:48]

The CQ bit was checked by rdevlin.cronin@chromium.org

[Devlin, 2016-01-08 01:30:49]

The patchset sent to the CQ was uploaded after l-g-t-m from avi@chromium.org,
asargent@chromium.org
Link to the patchset: https://codereview.chromium.org/1534123002/#ps240001
(title: " ")

[commit-bot: I haz the power, 2016-01-08 01:33:02]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1534123002/240001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1534123002/240001

[commit-bot: I haz the power, 2016-01-08 01:40:20]

Description was changed from

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

The ExtensionInstallPrompt + Delegate setup is prone to lifetime issues because
the caller of the delegate is a UI element, whereas the delegate is usually
somehow part of the extension system. At best, this makes lifetime management a
little confusing, but at worst, it's fundamentally flawed because UI ownership
and extension system ownership are very complicated at times like shutdown.

Instead, migrate the delegate to be a callback, where lifetime management is
more explicit (and safe if the fka delegate is deleted). Also update a few
confusing parts of the Cocoa install prompt API.

BUG=567844
TBR=benwells@chromium.org (small c/b/ui/extensions changes)
==========

to

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

The ExtensionInstallPrompt + Delegate setup is prone to lifetime issues because
the caller of the delegate is a UI element, whereas the delegate is usually
somehow part of the extension system. At best, this makes lifetime management a
little confusing, but at worst, it's fundamentally flawed because UI ownership
and extension system ownership are very complicated at times like shutdown.

Instead, migrate the delegate to be a callback, where lifetime management is
more explicit (and safe if the fka delegate is deleted). Also update a few
confusing parts of the Cocoa install prompt API.

BUG=567844
TBR=benwells@chromium.org (small c/b/ui/extensions changes)
==========

[commit-bot: I haz the power, 2016-01-08 01:40:20]

Committed patchset #4 (id:240001)

[commit-bot: I haz the power, 2016-01-08 01:41:26]

Description was changed from

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

The ExtensionInstallPrompt + Delegate setup is prone to lifetime issues because
the caller of the delegate is a UI element, whereas the delegate is usually
somehow part of the extension system. At best, this makes lifetime management a
little confusing, but at worst, it's fundamentally flawed because UI ownership
and extension system ownership are very complicated at times like shutdown.

Instead, migrate the delegate to be a callback, where lifetime management is
more explicit (and safe if the fka delegate is deleted). Also update a few
confusing parts of the Cocoa install prompt API.

BUG=567844
TBR=benwells@chromium.org (small c/b/ui/extensions changes)
==========

to

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

The ExtensionInstallPrompt + Delegate setup is prone to lifetime issues because
the caller of the delegate is a UI element, whereas the delegate is usually
somehow part of the extension system. At best, this makes lifetime management a
little confusing, but at worst, it's fundamentally flawed because UI ownership
and extension system ownership are very complicated at times like shutdown.

Instead, migrate the delegate to be a callback, where lifetime management is
more explicit (and safe if the fka delegate is deleted). Also update a few
confusing parts of the Cocoa install prompt API.

BUG=567844
TBR=benwells@chromium.org (small c/b/ui/extensions changes)

Committed: https://crrev.com/415930557c280022fed6a6ddd01f80f443704683
Cr-Commit-Position: refs/heads/master@{#368238}
==========

[commit-bot: I haz the power, 2016-01-08 01:41:27]

Patchset 4 (id:??) landed as
https://crrev.com/415930557c280022fed6a6ddd01f80f443704683
Cr-Commit-Position: refs/heads/master@{#368238}