Make ExtensionInstallPrompt::Prompt ref-counted

Make ExtensionInstallPrompt::Prompt ref-counted

We pass around ExtensionInstallPrompt::Prompt a lot by value and reference,
but should probably just have it ref-counted instead.  It's an expensive
object to copy (which we do far too much right now), and passing around
weak references is dangerous (and leads to crashing).

TBR=ben@chromium.org (c/b/ui minus c/b/ui/cocoa)
TBR=kinaba@chromium.org (c/b/chromeos/file_manager)
TBR=koz@chromium.org (c/b/apps)

All TBRs are for strictly mechanical changes.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=277823

[Devlin, 2014-06-04 22:01:25]

Yoyo, please take a look.  I think this is the reason for crbug.com/380739, and
the revert of https://codereview.chromium.org/309643007/, because
ExtensionInstallDialogView uses a const& for its prompt (after its passed
through various callbacks, etc).  Rather than make ExtensionInstallDialogView
make yet another copy of the prompt (which is expensive, and we already do far
too often) it seems that the better solution is to refcount the object.  Does
this make sense?

[Yoyo Zhou, 2014-06-04 22:14:48]

I'm not thrilled about more ref-counting, but LGTM, because we don't seem to be
able to consistently keep track of this object's lifetime (and doing so probably
imposes additional code complexity costs).

[Devlin, 2014-06-11 13:33:54]

+shess.  Mind taking a look at c/b/ui/cocoa?  There shouldn't be any functional
changes, but it's more files than a TBR.

[Devlin, 2014-06-16 19:10:03]

On 2014/06/11 13:33:54, Devlin wrote:
> +shess.  Mind taking a look at c/b/ui/cocoa?  There shouldn't be any
functional
> changes, but it's more files than a TBR.

Scott, friendly ping?

[Scott Hess - ex-Googler, 2014-06-16 19:29:04]

lgtm for cocoa/, minor nits, feel free to send me another review if you decide
to follow the suggestions.

https://codereview.chromium.org/313203004/diff/100001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/extensions/extension_install_view_controller.mm
(right):

https://codereview.chromium.org/313203004/diff/100001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/extensions/extension_install_view_controller.mm:593:
(scoped_refptr<ExtensionInstallPrompt::Prompt>)prompt {
Since you don't actually take a ref of |prompt| in here, a solution which
side-steps my comments below would be to just leave this as a const ref.  Just
because your object is ref-countable doesn't mean that you have to allow
everyone to ref-count it if you know that the original value can't go out of
scope.

https://codereview.chromium.org/313203004/diff/100001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/extensions/extension_install_view_controller.mm:607:
children:nil]];
You should feel free to use temporaries to tame the wrapping.  I don't think it
will take any more space, but it will be easier to change going forward. 
Likewise in the three cases below.  This could either be something like:
      NSString* title = SysUTF16ToNSString(prompt->GetPermission(i));
      id child = [self buildItemWithTitle:title
                           cellAttributes:kUseBullet
                                 children:nil];
      [children addObject:child];
or:
      NSString* title = SysUTF16ToNSString(prompt->GetPermission(i));
      [children addObject:[self buildItemWithTitle:title
                                    cellAttributes:kUseBullet
                                          children:nil]];
since each one varies a bit, maybe there's one pattern which is more applicable
than the others.

Of course, it would also work to push the SysUTF16ToNSString() conversion into
-buildItemWithTitle:*, since there are no callers which actually have an
NSString to hand.

[Devlin, 2014-06-17 16:05:27]

https://codereview.chromium.org/313203004/diff/100001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/extensions/extension_install_view_controller.mm
(right):

https://codereview.chromium.org/313203004/diff/100001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/extensions/extension_install_view_controller.mm:593:
(scoped_refptr<ExtensionInstallPrompt::Prompt>)prompt {
On 2014/06/16 19:29:04, shess wrote:
> Since you don't actually take a ref of |prompt| in here, a solution which
> side-steps my comments below would be to just leave this as a const ref.  Just
> because your object is ref-countable doesn't mean that you have to allow
> everyone to ref-count it if you know that the original value can't go out of
> scope.

Went with this option.  Thanks! :)

[Devlin, 2014-06-17 16:10:05]

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

[commit-bot: I haz the power, 2014-06-17 16:11:49]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/rdevlin.cronin@chromium.org/313203004/...

[Devlin, 2014-06-17 16:12:46]

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

[Devlin, 2014-06-17 16:15:01]

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

[commit-bot: I haz the power, 2014-06-17 16:16:05]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/rdevlin.cronin@chromium.org/313203004/...

[commit-bot: I haz the power, 2014-06-17 18:50:26]

Change committed as 277823