Adding in metrics for GAIA photo info. Also, fixing avatar logging to only log when the user chan...

Adding in metrics for GAIA photo info. Also, fixing avatar logging to only log when the user changes their photo. Currently it logs any time name OR avatar are updated. Now it should only do it for actual avatar changes.

BUG=93825, 106134
TEST=none

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

[rpetterson, 2011-12-03 01:07:22]

This is a fairly small change, but here are the reasons I added each reviewer:

sail - profiles
isherman - metrics
csilv - owners

[Ilya Sherman, 2011-12-03 02:31:13]

http://codereview.chromium.org/8790010/diff/6001/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile_metrics.cc (right):

http://codereview.chromium.org/8790010/diff/6001/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_metrics.cc:119: default:  // We should never
actually get here, but just in case
nit: Perhaps add a NOTREACHED() stmt here?

http://codereview.chromium.org/8790010/diff/6001/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile_metrics.h (right):

http://codereview.chromium.org/8790010/diff/6001/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_metrics.h:45: AVATAR_GAIA,               // 27
nit: I think the alignment is off-by-one for these comments relative to the
others above.

http://codereview.chromium.org/8790010/diff/6001/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_metrics.h:47: };
nit: Hmm, I thought this enum got moved to the .cc file.  Does it need to live
in the .h file?

http://codereview.chromium.org/8790010/diff/6001/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_metrics.h:92: GAIA_OPT_OUT,              // User
changed to not use GAIA photo as avatar
Hmm, is one of these the default, or are users somehow randomly assigned into
one group or the other?  If one is the default, these metrics might be a little
hard to interpret.  For example, let's say that you have 100 users, all of whom
start out using their GAIA photo as the avatar.  Then, 50% switch to a different
avatar.  Then 10% of those who switched switch back.  The way I'm imagining this
metric works, you'll see that 50 users opted out, and 5 user opted back in. 
From these numbers alone, you can't really recover the fact that 50% of your
users opted out initially -- which seems to me like the most interesting number.

LGTM if you're comfortable with this or have a different way to derive this
percentage, but just wanted to make sure that you have indeed thought about this
problem.

[rpetterson, 2011-12-05 20:27:14]

Waiting on @sail for profiles and @csilv for owners approval.

http://codereview.chromium.org/8790010/diff/6001/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile_metrics.cc (right):

http://codereview.chromium.org/8790010/diff/6001/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_metrics.cc:119: default:  // We should never
actually get here, but just in case
On 2011/12/03 02:31:13, Ilya Sherman wrote:
> nit: Perhaps add a NOTREACHED() stmt here?

Done.

http://codereview.chromium.org/8790010/diff/6001/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile_metrics.h (right):

http://codereview.chromium.org/8790010/diff/6001/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_metrics.h:45: AVATAR_GAIA,               // 27
On 2011/12/03 02:31:13, Ilya Sherman wrote:
> nit: I think the alignment is off-by-one for these comments relative to the
> others above.

Done.

http://codereview.chromium.org/8790010/diff/6001/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_metrics.h:47: };
On 2011/12/03 02:31:13, Ilya Sherman wrote:
> nit: Hmm, I thought this enum got moved to the .cc file.  Does it need to live
> in the .h file?

It was always in the .h. But it now has a new home in the .cc file. I moved the
call to log the GAIA Avatar selection into the SwitchGaia function so that it
was no longer needed outside the class.

http://codereview.chromium.org/8790010/diff/6001/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_metrics.h:92: GAIA_OPT_OUT,              // User
changed to not use GAIA photo as avatar
On 2011/12/03 02:31:13, Ilya Sherman wrote:
> Hmm, is one of these the default, or are users somehow randomly assigned into
> one group or the other?  If one is the default, these metrics might be a
little
> hard to interpret.  For example, let's say that you have 100 users, all of
whom
> start out using their GAIA photo as the avatar.  Then, 50% switch to a
different
> avatar.  Then 10% of those who switched switch back.  The way I'm imagining
this
> metric works, you'll see that 50 users opted out, and 5 user opted back in. 
> From these numbers alone, you can't really recover the fact that 50% of your
> users opted out initially -- which seems to me like the most interesting
number.
> 
> LGTM if you're comfortable with this or have a different way to derive this
> percentage, but just wanted to make sure that you have indeed thought about
this
> problem.

I understand what you're asking as I had the same discussion with Sailesh about
this. We're primarily interested in the numbers (and not the percentages) for
switching to avatar from GAIA but the other one is in there for sanity check.
That is, if we start to see equal numbers we know people are flipping back and
forth regularly.

[sail, 2011-12-05 20:35:24]

lgtm

[csilv, 2011-12-05 21:30:48]

lgtm

[commit-bot: I haz the power, 2011-12-05 21:44:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rlp@chromium.org/8790010/7001

[commit-bot: I haz the power, 2011-12-05 21:44:33]

Can't apply patch for file
chrome/browser/ui/webui/options/manage_profile_handler.cc.
While running patch -p0 --forward --force;
patching file chrome/browser/ui/webui/options/manage_profile_handler.cc
Hunk #1 FAILED at 174.
1 out of 1 hunk FAILED -- saving rejects to file
chrome/browser/ui/webui/options/manage_profile_handler.cc.rej

[commit-bot: I haz the power, 2011-12-05 23:12:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rlp@chromium.org/8790010/14001

[commit-bot: I haz the power, 2011-12-05 23:54:31]

Try job failure for 8790010-14001 (retry) on linux_rel for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2011-12-06 04:14:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rlp@chromium.org/8790010/12002

[commit-bot: I haz the power, 2011-12-06 06:26:32]

Change committed as 113128