[cros] Added histograms for user image usage

[cros] Added histograms for user image usage

R=altimofeev@chromium.org
BUG=chromium-os:18916
TEST=Sign in as a new or existing user with different images chosen. Change picture to different ones. Check that each time the appropriate histogram is updated at chrome://histograms.

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

[altimofeev, 2011-08-10 08:48:53]

LGTM

http://codereview.chromium.org/7590002/diff/1/chrome/browser/chromeos/login/u...
File chrome/browser/chromeos/login/user_manager.cc (right):

http://codereview.chromium.org/7590002/diff/1/chrome/browser/chromeos/login/u...
chrome/browser/chromeos/login/user_manager.cc:325: user_images_[email] =
user.image();
Why this is needed? AFAIU, now default images will be reloaded every time. BTW,
is it intentional?

http://codereview.chromium.org/7590002/diff/1/chrome/browser/chromeos/login/u...
chrome/browser/chromeos/login/user_manager.cc:403: int metric =
kDefaultImagesCount;
nit, optional: '?:' operator

[whywhat, 2011-08-10 11:34:38]

http://codereview.chromium.org/7590002/diff/1/chrome/browser/chromeos/login/u...
File chrome/browser/chromeos/login/user_manager.cc (right):

http://codereview.chromium.org/7590002/diff/1/chrome/browser/chromeos/login/u...
chrome/browser/chromeos/login/user_manager.cc:325: user_images_[email] =
user.image();
On 2011/08/10 08:48:54, altimofeev wrote:
> Why this is needed? AFAIU, now default images will be reloaded every time.
BTW,
> is it intentional?

We have to check if the image is default one no matter if we have image caches
or not. If it is a default image, we can get it from resources at no cost except
on the first call I think.

http://codereview.chromium.org/7590002/diff/1/chrome/browser/chromeos/login/u...
chrome/browser/chromeos/login/user_manager.cc:403: int metric =
kDefaultImagesCount;
On 2011/08/10 08:48:54, altimofeev wrote:
> nit, optional: '?:' operator

I like more words :)

[jar (doing other things), 2011-08-10 17:48:52]

Drive by comments (since I was asked to look at this code during a review of the
documentation of histograms).

http://codereview.chromium.org/7590002/diff/2002/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/user_manager.cc (right):

http://codereview.chromium.org/7590002/diff/2002/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/user_manager.cc:405: metric =
logged_in_user_.default_image_index();
personal style nit: not critical:  I prefer to call functions only once, as the
code is sometimes faster and race-free, and often easier to read.  The above
could be:

int metric = logged_in_user_.default_image_index();
if (metric == -1)
  metric = kDefaultImagesCount;

http://codereview.chromium.org/7590002/diff/2002/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options/chromeos/change_picture_options_handler.cc
(right):

http://codereview.chromium.org/7590002/diff/2002/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/chromeos/change_picture_options_handler.cc:157:
kDefaultImagesCount + 2);
nit suggestion: Since you name and define (the size of) this histogram three
times, it is probably worth factoring it out to a little local static function
that just takes the sample.  Too many folks instigate typos in one of these
definitions, and it is just easier to avoid replicating the macro invocation. 
In addition to readability, it might actually compile a tad smaller (macro
invocation has a few lines of code)... but I'm focusing on readability and
maintainability.

[whywhat, 2011-08-10 17:54:52]

Thanks Jim! Will address these comments in the following changelist.
My main concern was about passing different enumeration size to histogram macros
though on the server side these all are considered to be the same enum.
Maybe I should pass the same size everywhere even if one histogram bucket will
always be empty?

[jar (doing other things), 2011-08-10 18:08:33]

The histogram system always "pads" ENUMERATED_HISTOGRAMS with an empty slot
so that they cane be expanded in the future.  As a result, you don't need to
bother with padding in other cases (which should have no samples, and not be
visible anyway).

One trick you could look at would be to put the "default" at the bottom of
the histogram (slots 0 or 1), so that you can grow the array by adding new
built-in photos at the end of your enum list.  As currently designed, when
you decide to add new built-in-photos, you'll have to renumber your
kDefault, which means you'll need a new histogram... by a different name (to
avoid blurring data with different means when you select an aggregatino of
EVERYTHING combining multiple versions).

This is not a big deal, as I'm guessing the pictures are not that critical.
 Still, it is probably interesting to see what pictures are popular, and
which unpopular pictures should be replaced.

Jim



On Wed, Aug 10, 2011 at 10:54 AM, <avayvod@chromium.org> wrote:

> Thanks Jim! Will address these comments in the following changelist.
> My main concern was about passing different enumeration size to histogram
> macros
> though on the server side these all are considered to be the same enum.
> Maybe I should pass the same size everywhere even if one histogram bucket
> will
> always be empty?
>
>
>
http://codereview.chromium.**org/7590002/<http://codereview.chromium.org/7590...
>

[jar (doing other things), 2011-08-10 18:17:41]

Rereading what I wrote... I might have caused confusion:

On Wed, Aug 10, 2011 at 11:08 AM, Jim Roskind <jar@chromium.org> wrote:

> The histogram system always "pads" ENUMERATED_HISTOGRAMS with an empty slot
> so that they cane be expanded in the future.  As a result, you don't need to
> bother with padding in other cases (which should have no samples, and not be
> visible anyway).


I was noting that you don't have to have all your different histograms with
different enumerated list.  It is alsa fine to re-use the XML documentation
for an enumeration when there are some extra bucket definitions that don't
come into play in a shorter histogram.

You must always specify a single named histogram with exactly the same specs
(min/max/count of elements) in all places in your code.  I hope I didn't
confuse that point.

Jim




>
> One trick you could look at would be to put the "default" at the bottom of
> the histogram (slots 0 or 1), so that you can grow the array by adding new
> built-in photos at the end of your enum list.  As currently designed, when
> you decide to add new built-in-photos, you'll have to renumber your
> kDefault, which means you'll need a new histogram... by a different name (to
> avoid blurring data with different means when you select an aggregatino of
> EVERYTHING combining multiple versions).
>
> This is not a big deal, as I'm guessing the pictures are not that critical.
>  Still, it is probably interesting to see what pictures are popular, and
> which unpopular pictures should be replaced.
>
> Jim
>
>
>
> On Wed, Aug 10, 2011 at 10:54 AM, <avayvod@chromium.org> wrote:
>
>> Thanks Jim! Will address these comments in the following changelist.
>> My main concern was about passing different enumeration size to histogram
>> macros
>> though on the server side these all are considered to be the same enum.
>> Maybe I should pass the same size everywhere even if one histogram bucket
>> will
>> always be empty?
>>
>>
>>
http://codereview.chromium.**org/7590002/<http://codereview.chromium.org/7590...
>>
>
>