Add a histogram for tracking web store promo

Add a histogram for tracking web store promo

Creates an enumeration histogram called Extensions.AppsPromo with the following buckets:
0: launched an application from the promo
1: launched the web store from the promo
2: manually closed the promo (and uninstalled the applications)
3: promo expired

BUG=61017
TEST=

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=66006

[jstritar, 2010-11-08 22:55:39]

Could you take a look at this?

[Erik does not do reviews, 2010-11-08 23:44:27]

+aa to look at the JS code which seems odd to me

I think we also want another histogram for non-promo app launches.  It would be
cool to figure them out by how they launched.  I'll file a separate bug for
that.

http://codereview.chromium.org/4708002/diff/1/2
File chrome/browser/dom_ui/app_launcher_handler.cc (right):

http://codereview.chromium.org/4708002/diff/1/2#newcode282
chrome/browser/dom_ui/app_launcher_handler.cc:282:
UMA_HISTOGRAM_ENUMERATION("Extensions.AppsPromo", 2, 3);
replace bare numbers with enum values (all other cases as well)

http://codereview.chromium.org/4708002/diff/1/2#newcode297
chrome/browser/dom_ui/app_launcher_handler.cc:297:
UMA_HISTOGRAM_ENUMERATION("Extensions.AppsPromo", 1, 3);
This should only be if (promo_active_)

http://codereview.chromium.org/4708002/diff/1/4
File chrome/browser/extensions/default_apps.cc (right):

http://codereview.chromium.org/4708002/diff/1/4#newcode90
chrome/browser/extensions/default_apps.cc:90:
UMA_HISTOGRAM_ENUMERATION("Extensions.AppsPromo", 3, 3);
shouldn't this be inside of SetPromoCounter?

http://codereview.chromium.org/4708002/diff/1/5
File chrome/browser/resources/new_new_tab.js (right):

http://codereview.chromium.org/4708002/diff/1/5#newcode1216
chrome/browser/resources/new_new_tab.js:1216:
promoLink.addEventListener('mousedown', recordWebStoreLaunch);
shouldn't both of these be replaced by click?  you can have a mousedown without
a click right?  also, not every keypress would cause a launch.

http://codereview.chromium.org/4708002/diff/1/6
File chrome/browser/resources/ntp/apps.js (right):

http://codereview.chromium.org/4708002/diff/1/6#newcode247
chrome/browser/resources/ntp/apps.js:247: a.onmousedown =
handleIfMiddleClick(recordAppLaunch);
This seems weird to me that you have to special case this.  I guess this means
that all launches don't actually go through our launch code?  Doesn't this mean
that middle clicking a panel app will cause it to open in a tab?

[jstritar, 2010-11-09 17:24:07]

On 2010/11/08 23:44:27, Erik Kay wrote:
> http://codereview.chromium.org/4708002/diff/1/5
> File chrome/browser/resources/new_new_tab.js (right):
> 
> http://codereview.chromium.org/4708002/diff/1/5#newcode1216
> chrome/browser/resources/new_new_tab.js:1216:
> promoLink.addEventListener('mousedown', recordWebStoreLaunch);
> shouldn't both of these be replaced by click?  you can have a mousedown
without
> a click right?  also, not every keypress would cause a launch.

For some reason the 'click' event isn't fired when you middle click a link, so I
think we need to listen for 'mousedown' and filter based on which mouse button.
Unfortunately, you can also open a new tab by right clicking and selecting "Open
Link in New Tab". I'm also not sure how this works when you remap mouse buttons
to different functions. You can also get to the web store via the web store app,
which I'm currently not tracking (didn't realize it was there).

I don't think we should track web store referrals on the client side like this.
I suggest monitoring them on the server side using a custom link or redirect.
For example, we could have two separate links, one for the promo text link and
one for the web store app:

https://chrome.google.com/extensions?hl=en-US&ref=web-store-promo
https://chrome.google.com/extensions?hl=en-US&ref=web-store-app

With this approach, it wouldn't matter how the user launches the web store.

> http://codereview.chromium.org/4708002/diff/1/6
> File chrome/browser/resources/ntp/apps.js (right):
> 
> http://codereview.chromium.org/4708002/diff/1/6#newcode247
> chrome/browser/resources/ntp/apps.js:247: a.onmousedown =
> handleIfMiddleClick(recordAppLaunch);
> This seems weird to me that you have to special case this.  I guess this means
> that all launches don't actually go through our launch code?  Doesn't this
mean
> that middle clicking a panel app will cause it to open in a tab?

Yeah, this seemed odd to me too-- you can middle click a panel app, like Google
scratch pad, and open it in a new tab since they are all just <a> links. This
does bypass the launch app code. Luckily you can't right click and "Open Link in
a New Tab" since they have custom context menus.

[jstritar, 2010-11-10 19:55:16]

I uploaded a new CL that addresses your feedback. I still need to do the awkward
JS event listening, since launching apps and the web store don't always go
through a central code path. I think the only case we don't record is if you
right click the promo link and select "open in new tab / window".

On 2010/11/08 23:44:27, Erik Kay wrote:
> +aa to look at the JS code which seems odd to me
> 
> I think we also want another histogram for non-promo app launches.  It would
be
> cool to figure them out by how they launched.  I'll file a separate bug for
> that.
> 
> http://codereview.chromium.org/4708002/diff/1/2
> File chrome/browser/dom_ui/app_launcher_handler.cc (right):
> 
> http://codereview.chromium.org/4708002/diff/1/2#newcode282
> chrome/browser/dom_ui/app_launcher_handler.cc:282:
> UMA_HISTOGRAM_ENUMERATION("Extensions.AppsPromo", 2, 3);
> replace bare numbers with enum values (all other cases as well)

Done.

> http://codereview.chromium.org/4708002/diff/1/2#newcode297
> chrome/browser/dom_ui/app_launcher_handler.cc:297:
> UMA_HISTOGRAM_ENUMERATION("Extensions.AppsPromo", 1, 3);
> This should only be if (promo_active_)

Done.

> http://codereview.chromium.org/4708002/diff/1/4
> File chrome/browser/extensions/default_apps.cc (right):
> 
> http://codereview.chromium.org/4708002/diff/1/4#newcode90
> chrome/browser/extensions/default_apps.cc:90:
> UMA_HISTOGRAM_ENUMERATION("Extensions.AppsPromo", 3, 3);
> shouldn't this be inside of SetPromoCounter?

Manually closing the promo also calls SetPromoCounter, so I keep the histogram
outside of the method to distinguish between that and the promo expiring.

> http://codereview.chromium.org/4708002/diff/1/5
> File chrome/browser/resources/new_new_tab.js (right):
> 
> http://codereview.chromium.org/4708002/diff/1/5#newcode1216
> chrome/browser/resources/new_new_tab.js:1216:
> promoLink.addEventListener('mousedown', recordWebStoreLaunch);
> shouldn't both of these be replaced by click?  you can have a mousedown
without
> a click right?  also, not every keypress would cause a launch.

See my previous email about this and your next comment.

> http://codereview.chromium.org/4708002/diff/1/6
> File chrome/browser/resources/ntp/apps.js (right):
> 
> http://codereview.chromium.org/4708002/diff/1/6#newcode247
> chrome/browser/resources/ntp/apps.js:247: a.onmousedown =
> handleIfMiddleClick(recordAppLaunch);
> This seems weird to me that you have to special case this.  I guess this means
> that all launches don't actually go through our launch code?  Doesn't this
mean
> that middle clicking a panel app will cause it to open in a tab?

[Aaron Boodman, 2010-11-10 20:38:00]

The fact that there is no chokepoint for app launch is indeed ugly. I've poked
arv on this review to see if he has any suggestion on how to achieve one.

The only thing I can think of is using a custom chrome:// URL that redirects to
the real URL. That would work, but if possible, I'd rather have the real URL
present for copy/paste.

Maybe we could use the ping="" attribute and have it go to a chrome:// URL?

http://codereview.chromium.org/4708002/diff/9001/chrome/browser/resources/ntp...
File chrome/browser/resources/ntp/apps.js (right):

http://codereview.chromium.org/4708002/diff/9001/chrome/browser/resources/ntp...
chrome/browser/resources/ntp/apps.js:128: chrome.send('recordAppLaunch',
[appId]);
It seems it would be safer to send 'recordAppLaunch' first. The way it is, I
worry about 'recordAppLaunch' getting dropped since 'launchApp' results in
navigating the tab.

[Erik does not do reviews, 2010-11-10 23:29:51]

This is just feedback on the enumeration and histogram itself.  I'll stay out of
the JS and NTP while arv and aa weigh in.

http://codereview.chromium.org/4708002/diff/9001/chrome/browser/dom_ui/app_la...
File chrome/browser/dom_ui/app_launcher_handler.cc (right):

http://codereview.chromium.org/4708002/diff/9001/chrome/browser/dom_ui/app_la...
chrome/browser/dom_ui/app_launcher_handler.cc:284: extension_misc::MAX);
given that you're using a common namespace for these, "MAX" (and perhaps the
rest of the enum) should have some unique prefix on it

http://codereview.chromium.org/4708002/diff/9001/chrome/browser/dom_ui/app_la...
chrome/browser/dom_ui/app_launcher_handler.cc:312: // If we're launching the web
store application, then we may have to bump
if that's the case, then we shouldn't bump LAUNCH_APP, right?

http://codereview.chromium.org/4708002/diff/9001/chrome/common/extensions/ext...
File chrome/common/extensions/extension_constants.cc (right):

http://codereview.chromium.org/4708002/diff/9001/chrome/common/extensions/ext...
chrome/common/extensions/extension_constants.cc:307: const char*
kNTPWebStoreAppId = "web-store-entry";
this needs a comment.  it's not a valid id, so I assume there's some magic going
on.

http://codereview.chromium.org/4708002/diff/9001/chrome/common/extensions/ext...
File chrome/common/extensions/extension_constants.h (right):

http://codereview.chromium.org/4708002/diff/9001/chrome/common/extensions/ext...
chrome/common/extensions/extension_constants.h:227: // Web Store app id used in
the new tab page.
I don't get it.  How can we have two different app ids for the same app?  This
seems like it would break some important stuff.

http://codereview.chromium.org/4708002/diff/9001/chrome/common/extensions/ext...
chrome/common/extensions/extension_constants.h:252: MAX = EXPIRE_PROMO
this is wrong.  From histogram.h: "the samples should always be less than
boundary_value".

[Aaron Boodman, 2010-11-11 01:14:58]

On 2010/11/10 20:38:00, Aaron Boodman wrote:
> Maybe we could use the ping="" attribute and have it go to a chrome:// URL?

While we wait for arv, can you investigate this? Try having a ping attribute on
the links that points to a relative path like "record-launch". I think that
might show up as a request to NewTabHTMLSource::StartDataRequest() ?

[arv (Not doing code reviews), 2010-11-11 04:10:58]

On 2010/11/11 01:14:58, Aaron Boodman wrote:
> On 2010/11/10 20:38:00, Aaron Boodman wrote:
> > Maybe we could use the ping="" attribute and have it go to a chrome:// URL?
> 
> While we wait for arv, can you investigate this? Try having a ping attribute
on
> the links that points to a relative path like "record-launch". I think that
> might show up as a request to NewTabHTMLSource::StartDataRequest() ?

If this is about launching apps from the ntp then I would just change to use a
chrome.send instead of following a link.

[arv (Not doing code reviews), 2010-11-11 05:23:08]

I'm not sure I fully understand what we are trying to achieve here? If you
middle click a link to an app, is that considered launching the app?

Middle click no longer dispatches a click event (to match Firefox). It can be
emulated by adding a listener in mousedown and then check the target in mouseup.

[jstritar, 2010-11-12 00:03:34]

Thank you for the feedback everyone. I decided to go w/ the 'ping' route.
Unfortunately it also doesn't get fired when you right click and select "Open in
New Tab/Window" (I filed a bug for this: http://crbug.com/62866). This will only
affect the web store links, since the apps have a custom context menu. I also
bump the launch app histogram from the app launch handler, because the ping
isn't fired when the app launch handler is used (false is returned in the
onclick handler). I've verified that we're recording the correct values via the
histogram dumps.

On 2010/11/10 23:29:51, Erik Kay wrote:
> This is just feedback on the enumeration and histogram itself.  I'll stay out
of
> the JS and NTP while arv and aa weigh in.
> 
>
http://codereview.chromium.org/4708002/diff/9001/chrome/browser/dom_ui/app_la...
> File chrome/browser/dom_ui/app_launcher_handler.cc (right):
> 
>
http://codereview.chromium.org/4708002/diff/9001/chrome/browser/dom_ui/app_la...
> chrome/browser/dom_ui/app_launcher_handler.cc:284: extension_misc::MAX);
> given that you're using a common namespace for these, "MAX" (and perhaps the
> rest of the enum) should have some unique prefix on it

Sorry for the sloppiness... I cleaned all this up.

>
http://codereview.chromium.org/4708002/diff/9001/chrome/browser/dom_ui/app_la...
> chrome/browser/dom_ui/app_launcher_handler.cc:312: // If we're launching the
web
> store application, then we may have to bump
> if that's the case, then we shouldn't bump LAUNCH_APP, right?

I was bumping both since it's technically an app, but I no longer do this in the
new CL (which makes it a bit simpler too).

>
http://codereview.chromium.org/4708002/diff/9001/chrome/common/extensions/ext...
> File chrome/common/extensions/extension_constants.cc (right):
> 
>
http://codereview.chromium.org/4708002/diff/9001/chrome/common/extensions/ext...
> chrome/common/extensions/extension_constants.cc:307: const char*
> kNTPWebStoreAppId = "web-store-entry";
> this needs a comment.  it's not a valid id, so I assume there's some magic
going
> on.

This was removed from the CL.

>
http://codereview.chromium.org/4708002/diff/9001/chrome/common/extensions/ext...
> File chrome/common/extensions/extension_constants.h (right):
> 
>
http://codereview.chromium.org/4708002/diff/9001/chrome/common/extensions/ext...
> chrome/common/extensions/extension_constants.h:227: // Web Store app id used
in
> the new tab page.
> I don't get it.  How can we have two different app ids for the same app?  This
> seems like it would break some important stuff.

This is a special case for the NTP, probably for convenience. The web store app
is really just a link and never goes through the app launch handler, so the ID
shouldn't cause a problem. With the new CL I no longer need to deal with this
id.

>
http://codereview.chromium.org/4708002/diff/9001/chrome/common/extensions/ext...
> chrome/common/extensions/extension_constants.h:252: MAX = EXPIRE_PROMO
> this is wrong.  From histogram.h: "the samples should always be less than
> boundary_value".

I fixed this, but now the histogram dumps show a 5th, empty bucket. Also, the
histogram only throws DCHECKs if there's a sample thats > the boundary. Is this
an error in the documentation?

Thank you

[Aaron Boodman, 2010-11-12 00:29:20]

Cool I like this approach.

http://codereview.chromium.org/4708002/diff/26001/chrome/browser/dom_ui/new_t...
File chrome/browser/dom_ui/new_tab_ui.cc (right):

http://codereview.chromium.org/4708002/diff/26001/chrome/browser/dom_ui/new_t...
chrome/browser/dom_ui/new_tab_ui.cc:625: if
(path.find(extension_misc::kLaunchWebStorePingURL) != std::string::npos) {
Can you isolate all of this in AppLauncherHandler instead? The code sort of
makes more sense there than here. That way the constants can also be in
app_launcher_handler.* instead of in extension_constants.*.

You could have like static bool AppLauncherHandler::HandlePing(path). If it
returns true then it was handled, otherwise, this code goes on to do what it
usually does.

The RecordXYZ() methods would become private statics of AppLauncherHandler.

http://codereview.chromium.org/4708002/diff/26001/chrome/browser/dom_ui/new_t...
chrome/browser/dom_ui/new_tab_ui.cc:674: DCHECK(params.size() == 2);
You are going to crash below if size < 2, so might as well CHECK here. It's a
programmer error that this would ever not be 2.

http://codereview.chromium.org/4708002/diff/26001/chrome/browser/resources/nt...
File chrome/browser/resources/ntp/apps.js (right):

http://codereview.chromium.org/4708002/diff/26001/chrome/browser/resources/nt...
chrome/browser/resources/ntp/apps.js:242: setPromoActive:
function(isPromoActive) {
Nit: I think the setter is overkill here. You can just make it a data field,
like 'loaded' above.

[arv (Not doing code reviews), 2010-11-12 01:20:58]

I don't think a[ping] is sufficient since clicking on the link does
not trigger a navigation to the URL but an application launch.

Handling the middle click using a[ping] might be acceptable though.

erik



On Thu, Nov 11, 2010 at 16:03,  <jstritar@chromium.org> wrote:
> Thank you for the feedback everyone. I decided to go w/ the 'ping' route.
> Unfortunately it also doesn't get fired when you right click and select
> "Open in
> New Tab/Window" (I filed a bug for this: http://crbug.com/62866). This will
> only
> affect the web store links, since the apps have a custom context menu. I
> also
> bump the launch app histogram from the app launch handler, because the ping
> isn't fired when the app launch handler is used (false is returned in the
> onclick handler). I've verified that we're recording the correct values via
> the
> histogram dumps.
>
> On 2010/11/10 23:29:51, Erik Kay wrote:
>>
>> This is just feedback on the enumeration and histogram itself.  I'll stay
>> out
>
> of
>>
>> the JS and NTP while arv and aa weigh in.
>
>
>
http://codereview.chromium.org/4708002/diff/9001/chrome/browser/dom_ui/app_la...
>>
>> File chrome/browser/dom_ui/app_launcher_handler.cc (right):
>
>
>
http://codereview.chromium.org/4708002/diff/9001/chrome/browser/dom_ui/app_la...
>>
>> chrome/browser/dom_ui/app_launcher_handler.cc:284: extension_misc::MAX);
>> given that you're using a common namespace for these, "MAX" (and perhaps
>> the
>> rest of the enum) should have some unique prefix on it
>
> Sorry for the sloppiness... I cleaned all this up.
>
>
>
http://codereview.chromium.org/4708002/diff/9001/chrome/browser/dom_ui/app_la...
>>
>> chrome/browser/dom_ui/app_launcher_handler.cc:312: // If we're launching
>> the
>
> web
>>
>> store application, then we may have to bump
>> if that's the case, then we shouldn't bump LAUNCH_APP, right?
>
> I was bumping both since it's technically an app, but I no longer do this in
> the
> new CL (which makes it a bit simpler too).
>
>
>
http://codereview.chromium.org/4708002/diff/9001/chrome/common/extensions/ext...
>>
>> File chrome/common/extensions/extension_constants.cc (right):
>
>
>
http://codereview.chromium.org/4708002/diff/9001/chrome/common/extensions/ext...
>>
>> chrome/common/extensions/extension_constants.cc:307: const char*
>> kNTPWebStoreAppId = "web-store-entry";
>> this needs a comment.  it's not a valid id, so I assume there's some magic
>
> going
>>
>> on.
>
> This was removed from the CL.
>
>
>
http://codereview.chromium.org/4708002/diff/9001/chrome/common/extensions/ext...
>>
>> File chrome/common/extensions/extension_constants.h (right):
>
>
>
http://codereview.chromium.org/4708002/diff/9001/chrome/common/extensions/ext...
>>
>> chrome/common/extensions/extension_constants.h:227: // Web Store app id
>> used
>
> in
>>
>> the new tab page.
>> I don't get it.  How can we have two different app ids for the same app?
>>  This
>> seems like it would break some important stuff.
>
> This is a special case for the NTP, probably for convenience. The web store
> app
> is really just a link and never goes through the app launch handler, so the
> ID
> shouldn't cause a problem. With the new CL I no longer need to deal with
> this
> id.
>
>
>
http://codereview.chromium.org/4708002/diff/9001/chrome/common/extensions/ext...
>>
>> chrome/common/extensions/extension_constants.h:252: MAX = EXPIRE_PROMO
>> this is wrong.  From histogram.h: "the samples should always be less than
>> boundary_value".
>
> I fixed this, but now the histogram dumps show a 5th, empty bucket. Also,
> the
> histogram only throws DCHECKs if there's a sample thats > the boundary. Is
> this
> an error in the documentation?
>
> Thank you
>
> http://codereview.chromium.org/4708002/
>

[Erik does not do reviews, 2010-11-12 15:27:38]

The histogram part of this LGTM

To recap where we're at with the rest of this:
- ctrl-click and middle-click still force the app to open in a tab without going
through the launch logic (don't respect user  or developer launch settings)
- they also don't trigger the ping, so we don't record them in the histogram

Do I have this right?

[Aaron Boodman, 2010-11-12 15:31:05]

On Fri, Nov 12, 2010 at 7:27 AM,  <erikkay@chromium.org> wrote:
> To recap where we're at with the rest of this:
> - ctrl-click and middle-click still force the app to open in a tab without
> going

Whoops, I forgot about this. In order to get this right, the only
thing I can think of to do is implement our own middle click and
right-click heuristics. 'click' only means 'left click', and there is
no 'right click' and 'middle click' events afaik. Or we could just do
mousedown, but it is going to feel slightly wrong.

> through the launch logic (don't respect user  or developer launch settings)
> - they also don't trigger the ping, so we don't record them in the histogram

This is wrong. These two *do* trigger the ping.

- a

[jstritar, 2010-11-12 15:36:04]

On 2010/11/12 15:31:05, Aaron Boodman wrote:
> On Fri, Nov 12, 2010 at 7:27 AM,  <mailto:erikkay@chromium.org> wrote:
> > To recap where we're at with the rest of this:
> > - ctrl-click and middle-click still force the app to open in a tab without
> > going
> 
> Whoops, I forgot about this. In order to get this right, the only
> thing I can think of to do is implement our own middle click and
> right-click heuristics. 'click' only means 'left click', and there is
> no 'right click' and 'middle click' events afaik. Or we could just do
> mousedown, but it is going to feel slightly wrong.

Do you mean we don't want the middle click to bypass the launch handler anymore?
ctrl + click ends up being captured by the onclick and going through the launch
handler.

> > through the launch logic (don't respect user &nbsp;or developer launch
settings)
> > - they also don't trigger the ping, so we don't record them in the histogram
> 
> This is wrong. These two *do* trigger the ping.

Yeah- the ping basically gets everything that doesn't go through the launch
handler.

> - a

[jstritar, 2010-11-12 16:13:09]

Uploaded a new CL based on Aaron's feedback.

@arv: We also bump the histogram from within the application launch handler, so
the a[ping]'s capture all the launches that don't go through that handler.

On 2010/11/12 00:29:20, Aaron Boodman wrote:
> Cool I like this approach.
> 
>
http://codereview.chromium.org/4708002/diff/26001/chrome/browser/dom_ui/new_t...
> File chrome/browser/dom_ui/new_tab_ui.cc (right):
> 
>
http://codereview.chromium.org/4708002/diff/26001/chrome/browser/dom_ui/new_t...
> chrome/browser/dom_ui/new_tab_ui.cc:625: if
> (path.find(extension_misc::kLaunchWebStorePingURL) != std::string::npos) {
> Can you isolate all of this in AppLauncherHandler instead? The code sort of
> makes more sense there than here. That way the constants can also be in
> app_launcher_handler.* instead of in extension_constants.*.
> 
> You could have like static bool AppLauncherHandler::HandlePing(path). If it
> returns true then it was handled, otherwise, this code goes on to do what it
> usually does.
> 
> The RecordXYZ() methods would become private statics of AppLauncherHandler.

Nice, this cleaned it up a lot.

>
http://codereview.chromium.org/4708002/diff/26001/chrome/browser/dom_ui/new_t...
> chrome/browser/dom_ui/new_tab_ui.cc:674: DCHECK(params.size() == 2);
> You are going to crash below if size < 2, so might as well CHECK here. It's a
> programmer error that this would ever not be 2.

Done. 

>
http://codereview.chromium.org/4708002/diff/26001/chrome/browser/resources/nt...
> File chrome/browser/resources/ntp/apps.js (right):
> 
>
http://codereview.chromium.org/4708002/diff/26001/chrome/browser/resources/nt...
> chrome/browser/resources/ntp/apps.js:242: setPromoActive:
> function(isPromoActive) {
> Nit: I think the setter is overkill here. You can just make it a data field,
> like 'loaded' above.

Good point... Done.

[Aaron Boodman, 2010-11-12 19:01:02]

http://codereview.chromium.org/4708002/diff/44001/chrome/browser/dom_ui/app_l...
File chrome/browser/dom_ui/app_launcher_handler.h (right):

http://codereview.chromium.org/4708002/diff/44001/chrome/browser/dom_ui/app_l...
chrome/browser/dom_ui/app_launcher_handler.h:68: static bool HandlePing(const
std::string& path);
We seem to usually put the statics methods together at the top of the method
list in Chromium code (just after the constructor and destructor). Can you fix
CreateAppInfo too?

http://codereview.chromium.org/4708002/diff/44001/chrome/browser/dom_ui/app_l...
chrome/browser/dom_ui/app_launcher_handler.h:85: static void
RecordWebStoreLaunch(bool promo_active);
Same here.

http://codereview.chromium.org/4708002/diff/44001/chrome/common/extensions/ex...
File chrome/common/extensions/extension_constants.h (right):

http://codereview.chromium.org/4708002/diff/44001/chrome/common/extensions/ex...
chrome/common/extensions/extension_constants.h:247: enum AppsPromoBuckets {
Does this still need to be here or can it also move into AppLauncherHandler?

[jstritar, 2010-11-12 19:59:07]

Uploaded a new CL based on this feedback.

On 2010/11/12 19:01:02, Aaron Boodman wrote:
>
http://codereview.chromium.org/4708002/diff/44001/chrome/browser/dom_ui/app_l...
> File chrome/browser/dom_ui/app_launcher_handler.h (right):
> 
>
http://codereview.chromium.org/4708002/diff/44001/chrome/browser/dom_ui/app_l...
> chrome/browser/dom_ui/app_launcher_handler.h:68: static bool HandlePing(const
> std::string& path);
> We seem to usually put the statics methods together at the top of the method
> list in Chromium code (just after the constructor and destructor). Can you fix
> CreateAppInfo too?

I reordered the methods in app_launcher_handler.h so that the static methods
appear at the top of their sections. I also updated app_launcher_handler.cc so
that the method order matches the header file.

>
http://codereview.chromium.org/4708002/diff/44001/chrome/browser/dom_ui/app_l...
> chrome/browser/dom_ui/app_launcher_handler.h:85: static void
> RecordWebStoreLaunch(bool promo_active);
> Same here.
> 
>
http://codereview.chromium.org/4708002/diff/44001/chrome/common/extensions/ex...
> File chrome/common/extensions/extension_constants.h (right):
> 
>
http://codereview.chromium.org/4708002/diff/44001/chrome/common/extensions/ex...
> chrome/common/extensions/extension_constants.h:247: enum AppsPromoBuckets {
> Does this still need to be here or can it also move into AppLauncherHandler?

I kept it in extension_constants.h since we access them from default_apps.cc as
well. (We bump the promotion expiring bucket from there.)

[Aaron Boodman, 2010-11-12 20:53:20]

LGTM

Erik Kay: I thought about your concern (that middle click doesn't open a pinned
tab when the pinned tab pref is set), and my opinion is that behavior, we need
to approach launching differently.

I don't want to try and implement our own click heuristics, so instead, what I
would recommend is hooking in at a lower level. Perhaps we can get notifications
at the DOMUI layer, or lower, that RVH is creating a new tab, and create the
right kind of tab there.

So that's why I'm LG'ing this current code for now -- I think the middle-click
issue should be addressed separately.

[Erik does not do reviews, 2010-11-12 22:03:38]

SGTM - should we file a bug for the other bit?

On Fri, Nov 12, 2010 at 12:53 PM, <aa@chromium.org> wrote:

> LGTM
>
> Erik Kay: I thought about your concern (that middle click doesn't open a
> pinned
> tab when the pinned tab pref is set), and my opinion is that behavior, we
> need
> to approach launching differently.
>
> I don't want to try and implement our own click heuristics, so instead,
> what I
> would recommend is hooking in at a lower level. Perhaps we can get
> notifications
> at the DOMUI layer, or lower, that RVH is creating a new tab, and create
> the
> right kind of tab there.
>
> So that's why I'm LG'ing this current code for now -- I think the
> middle-click
> issue should be addressed separately.
>
>
> http://codereview.chromium.org/4708002/
>