Cocoa extension install dialog instrumentation.

Cocoa extension install dialog instrumentation.

This adds Experience Sampling instrumentation to the Cocoa version of
the extension install dialog.

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

[Chris Thompson, 2014-08-14 17:45:12]

groby@ could you PTAL? This is another piece of the experience sampling
instrumentation for Cocoa. This one should hopefully be relatively
straightforward (compared to the Downloads CL).

[groby-ooo-7-16, 2014-08-14 18:00:05]

https://codereview.chromium.org/462853002/diff/1/chrome/browser/ui/cocoa/exte...
File chrome/browser/ui/cocoa/extensions/extension_install_dialog_controller.h
(right):

https://codereview.chromium.org/462853002/diff/1/chrome/browser/ui/cocoa/exte...
chrome/browser/ui/cocoa/extensions/extension_install_dialog_controller.h:55:
scoped_ptr<extensions::ExperienceSamplingEvent> sampling_event_;
Question - why does this have object lifetime? Shouldn't that be created when
you raise the event?

Or does it signal when you don't raise an event, too?

https://codereview.chromium.org/462853002/diff/1/chrome/browser/ui/cocoa/exte...
File chrome/browser/ui/cocoa/extensions/extension_install_dialog_controller.mm
(right):

https://codereview.chromium.org/462853002/diff/1/chrome/browser/ui/cocoa/exte...
chrome/browser/ui/cocoa/extensions/extension_install_dialog_controller.mm:60:
event_name.append(ExtensionInstallPrompt::PromptTypeToString(prompt->type());
Is this a constant shared with other platforms? In that case, please factor out.

https://codereview.chromium.org/462853002/diff/1/chrome/browser/ui/cocoa/exte...
chrome/browser/ui/cocoa/extensions/extension_install_dialog_controller.mm:68: if
(sampling_event_.get())
No need to check - it's set in the ctor.

https://codereview.chromium.org/462853002/diff/1/chrome/browser/ui/cocoa/exte...
chrome/browser/ui/cocoa/extensions/extension_install_dialog_controller.mm:70:
delegate_->InstallUIProceed();
Shouldn't the delegate handle this, so you can be cross-platform?

https://codereview.chromium.org/462853002/diff/1/chrome/browser/ui/cocoa/exte...
chrome/browser/ui/cocoa/extensions/extension_install_dialog_controller.mm:76: if
(sampling_event_.get())
see above

[Chris Thompson, 2014-08-14 18:06:21]

https://codereview.chromium.org/462853002/diff/1/chrome/browser/ui/cocoa/exte...
File chrome/browser/ui/cocoa/extensions/extension_install_dialog_controller.h
(right):

https://codereview.chromium.org/462853002/diff/1/chrome/browser/ui/cocoa/exte...
chrome/browser/ui/cocoa/extensions/extension_install_dialog_controller.h:55:
scoped_ptr<extensions::ExperienceSamplingEvent> sampling_event_;
On 2014/08/14 18:00:05, groby wrote:
> Question - why does this have object lifetime? Shouldn't that be created when
> you raise the event?
> 
> Or does it signal when you don't raise an event, too?

We want to also track when the UI was created/shown to the user (the
ExperienceSamplingEvent tracks some of these details).

https://codereview.chromium.org/462853002/diff/1/chrome/browser/ui/cocoa/exte...
File chrome/browser/ui/cocoa/extensions/extension_install_dialog_controller.mm
(right):

https://codereview.chromium.org/462853002/diff/1/chrome/browser/ui/cocoa/exte...
chrome/browser/ui/cocoa/extensions/extension_install_dialog_controller.mm:60:
event_name.append(ExtensionInstallPrompt::PromptTypeToString(prompt->type());
On 2014/08/14 18:00:05, groby wrote:
> Is this a constant shared with other platforms? In that case, please factor
out.

Done. I've moved it out to the main ExperienceSampling files in the upstream CL.

https://codereview.chromium.org/462853002/diff/1/chrome/browser/ui/cocoa/exte...
chrome/browser/ui/cocoa/extensions/extension_install_dialog_controller.mm:68: if
(sampling_event_.get())
On 2014/08/14 18:00:05, groby wrote:
> No need to check - it's set in the ctor.

The Create(event_name) call can return an empty scoped_ptr if it can't find a
BrowserContext, so this is a safety measure (this can come up in tests, for
example).

https://codereview.chromium.org/462853002/diff/1/chrome/browser/ui/cocoa/exte...
chrome/browser/ui/cocoa/extensions/extension_install_dialog_controller.mm:70:
delegate_->InstallUIProceed();
On 2014/08/14 18:00:05, groby wrote:
> Shouldn't the delegate handle this, so you can be cross-platform?

I looked into the delegate classes for extension installation, but overall the
instrumentation was simpler for Aura+Cocoa than for the wide variety of delegate
subclasses.

https://codereview.chromium.org/462853002/diff/1/chrome/browser/ui/cocoa/exte...
chrome/browser/ui/cocoa/extensions/extension_install_dialog_controller.mm:76: if
(sampling_event_.get())
On 2014/08/14 18:00:05, groby wrote:
> see above

Acknowledged.

[groby-ooo-7-16, 2014-08-14 18:34:05]

LGTM w/ nit

https://codereview.chromium.org/462853002/diff/1/chrome/browser/ui/cocoa/exte...
File chrome/browser/ui/cocoa/extensions/extension_install_dialog_controller.mm
(right):

https://codereview.chromium.org/462853002/diff/1/chrome/browser/ui/cocoa/exte...
chrome/browser/ui/cocoa/extensions/extension_install_dialog_controller.mm:68: if
(sampling_event_.get())
On 2014/08/14 18:06:20, Chris Thompson wrote:
> On 2014/08/14 18:00:05, groby wrote:
> > No need to check - it's set in the ctor.
> 
> The Create(event_name) call can return an empty scoped_ptr if it can't find a
> BrowserContext, so this is a safety measure (this can come up in tests, for
> example).

In that case, mind leaving a quick comment at the Create() site that it can be
NULL?

[Chris Thompson, 2014-08-14 19:51:47]

https://codereview.chromium.org/462853002/diff/1/chrome/browser/ui/cocoa/exte...
File chrome/browser/ui/cocoa/extensions/extension_install_dialog_controller.mm
(right):

https://codereview.chromium.org/462853002/diff/1/chrome/browser/ui/cocoa/exte...
chrome/browser/ui/cocoa/extensions/extension_install_dialog_controller.mm:68: if
(sampling_event_.get())
On 2014/08/14 18:34:05, groby wrote:
> On 2014/08/14 18:06:20, Chris Thompson wrote:
> > On 2014/08/14 18:00:05, groby wrote:
> > > No need to check - it's set in the ctor.
> > 
> > The Create(event_name) call can return an empty scoped_ptr if it can't find
a
> > BrowserContext, so this is a safety measure (this can come up in tests, for
> > example).
> 
> In that case, mind leaving a quick comment at the Create() site that it can be
> NULL?

Done (on upstream CL).

[groby-ooo-7-16, 2014-08-14 20:52:33]

SLGTM, but it looks like the try bots are lacking a prior CL. Did you run
against HEAD?

[Chris Thompson, 2014-08-14 20:53:22]

On 2014/08/14 20:52:33, groby wrote:
> SLGTM, but it looks like the try bots are lacking a prior CL. Did you run
> against HEAD?

Yeah, forgot that I need to land the parent CL first before the trybots will do
me any good for testing.

[Chris Thompson, 2014-08-15 16:06:28]

The CQ bit was checked by cthomp@chromium.org

[commit-bot: I haz the power, 2014-08-15 16:09:49]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/cthomp@chromium.org/462853002/60001

[commit-bot: I haz the power, 2014-08-15 18:35:50]

Committed patchset #4 (60001) as 289948