Add metrics to help measure effects of installing default apps in a profile.

Add metrics to help measure effects of installing default apps in a profile
for the defaults field trial.

Evan: please review the webui changes.
Antony: plase review the extension changes.
Miranda: please review the profile changes.
Scott: please review the browser.cc

BUG=94920
TEST=No user visible change.  New metrics should be available in UMA.

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

[Roger Tawa OOO till Jul 10th, 2011-09-29 17:43:14]

Hi Antony, Evan, Miranda, Scott,

Please take a look.  Thanks.

[csilv, 2011-09-29 18:32:04]

webui lgtm.  please consider suggested changes in comments.

http://codereview.chromium.org/8086002/diff/1/chrome/browser/ui/webui/ntp/new...
File chrome/browser/ui/webui/ntp/new_tab_page_handler.cc (right):

http://codereview.chromium.org/8086002/diff/1/chrome/browser/ui/webui/ntp/new...
chrome/browser/ui/webui/ntp/new_tab_page_handler.cc:36: kDefaultAppsTrial_Name);
style nit: line 36 should be indented 4 spaces.  Please consider the following
alternative wrapping for readability:

static bool default_apps_trial_exists =
    base::FieldTrialList::TrialExists(kDefaultAppsTrial_Name);

http://codereview.chromium.org/8086002/diff/1/chrome/browser/ui/webui/ntp/new...
chrome/browser/ui/webui/ntp/new_tab_page_handler.cc:83: kDefaultAppsTrial_Name);
ditto above comment

http://codereview.chromium.org/8086002/diff/1/chrome/browser/ui/webui/ntp/new...
chrome/browser/ui/webui/ntp/new_tab_page_handler.cc:89: }
The above block is mostly a repeat of the contents of attach.  Might be worth
putting into a separate method that can be called from both places.

[Roger Tawa OOO till Jul 10th, 2011-09-29 19:10:58]

Thanks Chris.  Comments addressed, changes uploaded.

http://codereview.chromium.org/8086002/diff/1/chrome/browser/ui/webui/ntp/new...
File chrome/browser/ui/webui/ntp/new_tab_page_handler.cc (right):

http://codereview.chromium.org/8086002/diff/1/chrome/browser/ui/webui/ntp/new...
chrome/browser/ui/webui/ntp/new_tab_page_handler.cc:36: kDefaultAppsTrial_Name);
On 2011/09/29 18:32:04, csilv wrote:
> style nit: line 36 should be indented 4 spaces.  Please consider the following
> alternative wrapping for readability:
> 
> static bool default_apps_trial_exists =
>     base::FieldTrialList::TrialExists(kDefaultAppsTrial_Name);

Done.

http://codereview.chromium.org/8086002/diff/1/chrome/browser/ui/webui/ntp/new...
chrome/browser/ui/webui/ntp/new_tab_page_handler.cc:83: kDefaultAppsTrial_Name);
On 2011/09/29 18:32:04, csilv wrote:
> ditto above comment

Done.

http://codereview.chromium.org/8086002/diff/1/chrome/browser/ui/webui/ntp/new...
chrome/browser/ui/webui/ntp/new_tab_page_handler.cc:89: }
On 2011/09/29 18:32:04, csilv wrote:
> The above block is mostly a repeat of the contents of attach.  Might be worth
> putting into a separate method that can be called from both places.

The way the UMA_HISTOGRAM_ENUMERATION macro works (creating a static variable)
that does not work.

[sky, 2011-09-29 19:11:09]

LGTM. If you haven't already, you should make sure Jim sees this too.

http://codereview.chromium.org/8086002/diff/1/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

http://codereview.chromium.org/8086002/diff/1/chrome/browser/ui/browser.cc#ne...
chrome/browser/ui/browser.cc:706: launch_type, 100);
nit: spacing looks weird on this line.

[Roger Tawa OOO till Jul 10th, 2011-09-29 19:20:18]

http://codereview.chromium.org/8086002/diff/1/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

http://codereview.chromium.org/8086002/diff/1/chrome/browser/ui/browser.cc#ne...
chrome/browser/ui/browser.cc:706: launch_type, 100);
On 2011/09/29 19:11:09, sky wrote:
> nit: spacing looks weird on this line.

Adjusting the spacing in a few places makes the coder a little easier to read,
thanks.

[asargent_no_longer_on_chrome, 2011-09-29 20:46:29]

extensions part LGTM

http://codereview.chromium.org/8086002/diff/17/chrome/browser/extensions/crx_...
File chrome/browser/extensions/crx_installer.cc (right):

http://codereview.chromium.org/8086002/diff/17/chrome/browser/extensions/crx_...
chrome/browser/extensions/crx_installer.cc:543:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
I assume you already noticed that this method is running on the file thread, and
verified that it's safe to call the FieldTrial methods below when you're not on
the UI thread (I don't know one way or the other off the top of my head).

http://codereview.chromium.org/8086002/diff/17/chrome/browser/extensions/crx_...
chrome/browser/extensions/crx_installer.cc:555: kDefaultAppsTrial_Name));
You might consider adding the following towards the top of the .cc file:

using base::FieldTrial;
using base::FieldTrialList;

Then this code can just be:

if (FieldTrialList::TrialExists(kDefaultAppsTrial_Name)) {
  UserMetrics::RecordComputedAction(
      FieldTrial::MakeName("Extensions.ExtensionInstalled",
                           kDefaultAppsTrial_Name));
}

(If you choose to do it that way here, do the same thing in extension_service.cc
too). 

Your call either way.

[Roger Tawa OOO till Jul 10th, 2011-09-29 22:17:38]

Thanks Antony, see below.

http://codereview.chromium.org/8086002/diff/17/chrome/browser/extensions/crx_...
File chrome/browser/extensions/crx_installer.cc (right):

http://codereview.chromium.org/8086002/diff/17/chrome/browser/extensions/crx_...
chrome/browser/extensions/crx_installer.cc:543:
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
On 2011/09/29 20:46:29, Antony Sargent wrote:
> I assume you already noticed that this method is running on the file thread,
and
> verified that it's safe to call the FieldTrial methods below when you're not
on
> the UI thread (I don't know one way or the other off the top of my head).

Yes.  TrialExists() is thread safe.  MakeName() is a static function that only
depends on its arguments and uses no globals or other static members.

http://codereview.chromium.org/8086002/diff/17/chrome/browser/extensions/crx_...
chrome/browser/extensions/crx_installer.cc:555: kDefaultAppsTrial_Name));
On 2011/09/29 20:46:29, Antony Sargent wrote:
> You might consider adding the following towards the top of the .cc file:
> 
> using base::FieldTrial;
> using base::FieldTrialList;
> 
> Then this code can just be:
> 
> if (FieldTrialList::TrialExists(kDefaultAppsTrial_Name)) {
>   UserMetrics::RecordComputedAction(
>       FieldTrial::MakeName("Extensions.ExtensionInstalled",
>                            kDefaultAppsTrial_Name));
> }
> 
> (If you choose to do it that way here, do the same thing in
extension_service.cc
> too). 
> 
> Your call either way.
> 

If its my call, then I'll leave it as is.  I don't really like using 'using' :-)
 Independently of this, I would not change the code to be as you described
because I want the return value of TrialExists to be assigned to a local static
bool.  This is the same pattern used in all field trial code.

[Miranda Callahan, 2011-09-30 09:28:31]

On 2011/09/29 22:17:38, Roger Tawa wrote:
> Thanks Antony, see below.
> 
>
http://codereview.chromium.org/8086002/diff/17/chrome/browser/extensions/crx_...
> File chrome/browser/extensions/crx_installer.cc (right):
> 
>
http://codereview.chromium.org/8086002/diff/17/chrome/browser/extensions/crx_...
> chrome/browser/extensions/crx_installer.cc:543:
> DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
> On 2011/09/29 20:46:29, Antony Sargent wrote:
> > I assume you already noticed that this method is running on the file thread,
> and
> > verified that it's safe to call the FieldTrial methods below when you're not
> on
> > the UI thread (I don't know one way or the other off the top of my head).
> 
> Yes.  TrialExists() is thread safe.  MakeName() is a static function that only
> depends on its arguments and uses no globals or other static members.
> 
>
http://codereview.chromium.org/8086002/diff/17/chrome/browser/extensions/crx_...
> chrome/browser/extensions/crx_installer.cc:555: kDefaultAppsTrial_Name));
> On 2011/09/29 20:46:29, Antony Sargent wrote:
> > You might consider adding the following towards the top of the .cc file:
> > 
> > using base::FieldTrial;
> > using base::FieldTrialList;
> > 
> > Then this code can just be:
> > 
> > if (FieldTrialList::TrialExists(kDefaultAppsTrial_Name)) {
> >   UserMetrics::RecordComputedAction(
> >       FieldTrial::MakeName("Extensions.ExtensionInstalled",
> >                            kDefaultAppsTrial_Name));
> > }
> > 
> > (If you choose to do it that way here, do the same thing in
> extension_service.cc
> > too). 
> > 
> > Your call either way.
> > 
> 
> If its my call, then I'll leave it as is.  I don't really like using 'using'
:-)
>  Independently of this, I would not change the code to be as you described
> because I want the return value of TrialExists to be assigned to a local
static
> bool.  This is the same pattern used in all field trial code.

Profiles code LGTM.

[Evan Stade, 2011-10-04 16:58:32]

http://codereview.chromium.org/8086002/diff/9001/chrome/browser/ui/webui/ntp/...
File chrome/browser/ui/webui/ntp/new_tab_page_handler.cc (right):

http://codereview.chromium.org/8086002/diff/9001/chrome/browser/ui/webui/ntp/...
chrome/browser/ui/webui/ntp/new_tab_page_handler.cc:26: }  // anonymous
is there a difference?

http://codereview.chromium.org/8086002/diff/9001/chrome/browser/ui/webui/ntp/...
chrome/browser/ui/webui/ntp/new_tab_page_handler.cc:41:
base::FieldTrialList::TrialExists(kDefaultAppsTrial_Name);
why does this variable have an underscore in it

http://codereview.chromium.org/8086002/diff/9001/chrome/browser/ui/webui/ntp/...
chrome/browser/ui/webui/ntp/new_tab_page_handler.cc:98:
UMA_HISTOGRAM_ENUMERATION("NtpHandler.SelectedShownPageType",
don't you have to add this name to a list somewhere? Have you done this and have
you pinged jar about this review?

[Roger Tawa OOO till Jul 10th, 2011-10-04 17:38:45]

http://codereview.chromium.org/8086002/diff/9001/chrome/browser/ui/webui/ntp/...
File chrome/browser/ui/webui/ntp/new_tab_page_handler.cc (right):

http://codereview.chromium.org/8086002/diff/9001/chrome/browser/ui/webui/ntp/...
chrome/browser/ui/webui/ntp/new_tab_page_handler.cc:26: }  // anonymous
On 2011/10/04 16:58:33, Evan Stade wrote:
> is there a difference?

I believe unnamed namespaces are preferred, but I can revert this if you like.

http://codereview.chromium.org/8086002/diff/9001/chrome/browser/ui/webui/ntp/...
chrome/browser/ui/webui/ntp/new_tab_page_handler.cc:41:
base::FieldTrialList::TrialExists(kDefaultAppsTrial_Name);
On 2011/10/04 16:58:33, Evan Stade wrote:
> why does this variable have an underscore in it

Because that's the way its declared in default_apps_trial.h

http://codereview.chromium.org/8086002/diff/9001/chrome/browser/ui/webui/ntp/...
chrome/browser/ui/webui/ntp/new_tab_page_handler.cc:98:
UMA_HISTOGRAM_ENUMERATION("NtpHandler.SelectedShownPageType",
On 2011/10/04 16:58:33, Evan Stade wrote:
> don't you have to add this name to a list somewhere? Have you done this and
have
> you pinged jar about this review?

That change is in a separate repo.

[Evan Stade, 2011-10-04 17:54:35]

http://codereview.chromium.org/8086002/diff/9001/chrome/browser/ui/webui/ntp/...
File chrome/browser/ui/webui/ntp/new_tab_page_handler.cc (right):

http://codereview.chromium.org/8086002/diff/9001/chrome/browser/ui/webui/ntp/...
chrome/browser/ui/webui/ntp/new_tab_page_handler.cc:26: }  // anonymous
On 2011/10/04 17:38:46, Roger Tawa wrote:
> On 2011/10/04 16:58:33, Evan Stade wrote:
> > is there a difference?
> 
> I believe unnamed namespaces are preferred, but I can revert this if you like.

I think it's the same thing, but for very short namespaces static is less
verbose

http://codereview.chromium.org/8086002/diff/9001/chrome/browser/ui/webui/ntp/...
chrome/browser/ui/webui/ntp/new_tab_page_handler.cc:41:
base::FieldTrialList::TrialExists(kDefaultAppsTrial_Name);
On 2011/10/04 17:38:46, Roger Tawa wrote:
> On 2011/10/04 16:58:33, Evan Stade wrote:
> > why does this variable have an underscore in it
> 
> Because that's the way its declared in default_apps_trial.h

right. I couldn't comment there because it's not in this changelist. Pretend I
put this comment in the right place. There shouldn't be an underscore unless
there's a style rule I'm unaware of.

[Roger Tawa OOO till Jul 10th, 2011-10-04 18:15:25]

Hi Evan, comments addressed, changes uploaded.  Please take another look.

http://codereview.chromium.org/8086002/diff/9001/chrome/browser/ui/webui/ntp/...
File chrome/browser/ui/webui/ntp/new_tab_page_handler.cc (right):

http://codereview.chromium.org/8086002/diff/9001/chrome/browser/ui/webui/ntp/...
chrome/browser/ui/webui/ntp/new_tab_page_handler.cc:26: }  // anonymous
On 2011/10/04 17:54:35, Evan Stade wrote:
> On 2011/10/04 17:38:46, Roger Tawa wrote:
> > On 2011/10/04 16:58:33, Evan Stade wrote:
> > > is there a difference?
> > 
> > I believe unnamed namespaces are preferred, but I can revert this if you
like.
> 
> I think it's the same thing, but for very short namespaces static is less
> verbose

Put back static.

http://codereview.chromium.org/8086002/diff/9001/chrome/browser/ui/webui/ntp/...
chrome/browser/ui/webui/ntp/new_tab_page_handler.cc:41:
base::FieldTrialList::TrialExists(kDefaultAppsTrial_Name);
On 2011/10/04 17:54:35, Evan Stade wrote:
> On 2011/10/04 17:38:46, Roger Tawa wrote:
> > On 2011/10/04 16:58:33, Evan Stade wrote:
> > > why does this variable have an underscore in it
> > 
> > Because that's the way its declared in default_apps_trial.h
> 
> right. I couldn't comment there because it's not in this changelist. Pretend I
> put this comment in the right place. There shouldn't be an underscore unless
> there's a style rule I'm unaware of.

That can be addressed in a follow up CL.

[Evan Stade, 2011-10-04 18:22:32]

I find style followups don't tend to get prioritized and this one would only
take about 1.5 minutes to do. But if you prefer a different CL, LGTM.

[Roger Tawa OOO till Jul 10th, 2011-10-04 18:26:47]

Thanks Evan.  I prefer a different CL to keep each changed focused.

[jar (doing other things), 2011-10-05 17:42:16]

http://codereview.chromium.org/8086002/diff/9008/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_service.cc (right):

http://codereview.chromium.org/8086002/diff/9008/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_service.cc:971:
UserMetrics::RecordComputedAction(
I suspect you'd be much better off using a histogram.  I'll send off-line email
discussing pros and cons, given the way this info is surfaced in dashboards.

[Roger Tawa OOO till Jul 10th, 2011-10-12 14:46:22]

http://codereview.chromium.org/8086002/diff/9008/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_service.cc (right):

http://codereview.chromium.org/8086002/diff/9008/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_service.cc:971:
UserMetrics::RecordComputedAction(
On 2011/10/05 17:42:16, jar wrote:
> I suspect you'd be much better off using a histogram.  I'll send off-line
email
> discussing pros and cons, given the way this info is surfaced in dashboards.

Done.

[Evan Stade, 2011-10-13 00:07:50]

lgtm pending nits

http://codereview.chromium.org/8086002/diff/18001/chrome/browser/ui/webui/ntp...
File chrome/browser/ui/webui/ntp/new_tab_page_handler.cc (right):

http://codereview.chromium.org/8086002/diff/18001/chrome/browser/ui/webui/ntp...
chrome/browser/ui/webui/ntp/new_tab_page_handler.cc:36: shown_page_type, 4);
ah I just had a CL to do this. You have saved me the effort of fixing it up. Two
things:

1. name should be "NewTabPage.DefaultPageType"
2. don't use magic constants

http://codereview.chromium.org/8086002/diff/18001/chrome/browser/ui/webui/ntp...
File chrome/browser/ui/webui/ntp/new_tab_page_handler.h (right):

http://codereview.chromium.org/8086002/diff/18001/chrome/browser/ui/webui/ntp...
chrome/browser/ui/webui/ntp/new_tab_page_handler.h:57: PAGE_ID_OFFSET = 10,
do not add this as an enum value, it doesn't make sense

http://codereview.chromium.org/8086002/diff/18001/chrome/browser/ui/webui/ntp...
chrome/browser/ui/webui/ntp/new_tab_page_handler.h:61: BOOKMARKS_PAGE_ID = 3 <<
PAGE_ID_OFFSET,
LAST_PAGE_ID = BOOKMARKS_PAGE_ID

[Roger Tawa OOO till Jul 10th, 2011-10-13 00:39:43]

http://codereview.chromium.org/8086002/diff/18001/chrome/browser/ui/webui/ntp...
File chrome/browser/ui/webui/ntp/new_tab_page_handler.cc (right):

http://codereview.chromium.org/8086002/diff/18001/chrome/browser/ui/webui/ntp...
chrome/browser/ui/webui/ntp/new_tab_page_handler.cc:36: shown_page_type, 4);
On 2011/10/13 00:07:51, Evan Stade wrote:
> ah I just had a CL to do this. You have saved me the effort of fixing it up.
Two
> things:
> 
> 1. name should be "NewTabPage.DefaultPageType"
> 2. don't use magic constants

Done.

http://codereview.chromium.org/8086002/diff/18001/chrome/browser/ui/webui/ntp...
File chrome/browser/ui/webui/ntp/new_tab_page_handler.h (right):

http://codereview.chromium.org/8086002/diff/18001/chrome/browser/ui/webui/ntp...
chrome/browser/ui/webui/ntp/new_tab_page_handler.h:57: PAGE_ID_OFFSET = 10,
On 2011/10/13 00:07:51, Evan Stade wrote:
> do not add this as an enum value, it doesn't make sense

Done.

http://codereview.chromium.org/8086002/diff/18001/chrome/browser/ui/webui/ntp...
chrome/browser/ui/webui/ntp/new_tab_page_handler.h:61: BOOKMARKS_PAGE_ID = 3 <<
PAGE_ID_OFFSET,
On 2011/10/13 00:07:51, Evan Stade wrote:
> LAST_PAGE_ID = BOOKMARKS_PAGE_ID

Done.

[commit-bot: I haz the power, 2011-10-13 01:42:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rogerta@chromium.org/8086002/24001

[commit-bot: I haz the power, 2011-10-13 03:47:20]

Change committed as 105253

[jar (doing other things), 2011-10-13 22:16:53]

http://codereview.chromium.org/8086002/diff/18001/chrome/browser/ui/webui/ntp...
File chrome/browser/ui/webui/ntp/new_tab_page_handler.h (right):

http://codereview.chromium.org/8086002/diff/18001/chrome/browser/ui/webui/ntp...
chrome/browser/ui/webui/ntp/new_tab_page_handler.h:61: BOOKMARKS_PAGE_ID = 3 <<
PAGE_ID_OFFSET,
This looks like you're going for a histogram with about 4K buckets, which is
probably much larger than you need.  The cost is about 4 bytes per bucket
contents, plus 4 bytes to define the range stored in teh bucket, or a total of 8
bytes per bucket, and around 32K total.   ...but... UMA also needs a snapshot to
send up deltas, so that may mean it may cost closer to 64K total :-(.

I suspect you're gather much more data than you need, as I don't know how all
this data will be used (or analyzed).  64K isn't the end of the world... but it
is a measurable fraction of memory utilization.

[jar (doing other things), 2011-10-13 22:22:07]

http://codereview.chromium.org/8086002/diff/18001/chrome/browser/ui/webui/ntp...
File chrome/browser/ui/webui/ntp/new_tab_page_handler.h (right):

http://codereview.chromium.org/8086002/diff/18001/chrome/browser/ui/webui/ntp...
chrome/browser/ui/webui/ntp/new_tab_page_handler.h:61: BOOKMARKS_PAGE_ID = 3 <<
PAGE_ID_OFFSET,
I retract my comment.  Sorry about that.... the histogram scales back down the
size.

On 2011/10/13 22:16:53, jar wrote:
> This looks like you're going for a histogram with about 4K buckets, which is
> probably much larger than you need.  The cost is about 4 bytes per bucket
> contents, plus 4 bytes to define the range stored in teh bucket, or a total of
8
> bytes per bucket, and around 32K total.   ...but... UMA also needs a snapshot
to
> send up deltas, so that may mean it may cost closer to 64K total :-(.
> 
> I suspect you're gather much more data than you need, as I don't know how all
> this data will be used (or analyzed).  64K isn't the end of the world... but
it
> is a measurable fraction of memory utilization.

[Evan Stade, 2011-10-14 17:05:52]

http://codereview.chromium.org/8086002/diff/24001/chrome/browser/ui/webui/ntp...
File chrome/browser/ui/webui/ntp/new_tab_page_handler.cc (right):

http://codereview.chromium.org/8086002/diff/24001/chrome/browser/ui/webui/ntp...
chrome/browser/ui/webui/ntp/new_tab_page_handler.cc:44: shown_page_type, 4);
sorry for not being specific enough. My intent was that you would use
LAST_PAGE_ID here instead of 4.

[Roger Tawa OOO till Jul 10th, 2011-10-14 19:21:12]

http://codereview.chromium.org/8086002/diff/24001/chrome/browser/ui/webui/ntp...
File chrome/browser/ui/webui/ntp/new_tab_page_handler.cc (right):

http://codereview.chromium.org/8086002/diff/24001/chrome/browser/ui/webui/ntp...
chrome/browser/ui/webui/ntp/new_tab_page_handler.cc:44: shown_page_type, 4);
On 2011/10/14 17:05:52, Evan Stade wrote:
> sorry for not being specific enough. My intent was that you would use
> LAST_PAGE_ID here instead of 4.

Right, I should have done that.  Will fix.

[Evan Stade, 2011-11-15 18:04:44]

how's that style-fixing follow up CL coming?

[Roger Tawa OOO till Jul 10th, 2011-11-15 18:14:06]

On 2011/11/15 18:04:44, Evan Stade wrote:
> how's that style-fixing follow up CL coming?

Still on my radar.  Glad I hadn't tackled it yet actually, since there were some
follow up CL that needed to be merged into M16.  Once I know default apps are
working smoothly in M16, you'll get to review it.