Sync Promo: Add more UMA metrics.

Sync Promo: Add more UMA metrics.

R=estade@chromium.org
BUG=103822
TEST=+1 action in SyncPromo.UserFlowAction and +2 new histograms.


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

[Evan Stade, 2011-11-15 17:45:19]

http://codereview.chromium.org/8528054/diff/1/chrome/browser/resources/sync_p...
File chrome/browser/resources/sync_promo.js (right):

http://codereview.chromium.org/8528054/diff/1/chrome/browser/resources/sync_p...
chrome/browser/resources/sync_promo.js:6: <include
src="options/options_page.js"/>
wtffff

http://codereview.chromium.org/8528054/diff/1/chrome/browser/resources/sync_p...
chrome/browser/resources/sync_promo.js:240: throbberStart_: null,
why are you complicating this by setting it to null? use 0

http://codereview.chromium.org/8528054/diff/1/chrome/browser/resources/sync_s...
File chrome/browser/resources/sync_setup_overlay.js (right):

http://codereview.chromium.org/8528054/diff/1/chrome/browser/resources/sync_s...
chrome/browser/resources/sync_setup_overlay.js:548: var visibleOrHidden =
visible ? "visible" : "hidden";
revert the changes to this function

http://codereview.chromium.org/8528054/diff/1/chrome/browser/ui/webui/metrics...
File chrome/browser/ui/webui/metrics_handler.h (right):

http://codereview.chromium.org/8528054/diff/1/chrome/browser/ui/webui/metrics...
chrome/browser/ui/webui/metrics_handler.h:36: // TODO(dbeam): Rename
recordInHistgram to something less broad and more
move this comment into a bug report and just link to it here.

http://codereview.chromium.org/8528054/diff/1/chrome/browser/ui/webui/sync_pr...
File chrome/browser/ui/webui/sync_promo_handler.cc (right):

http://codereview.chromium.org/8528054/diff/1/chrome/browser/ui/webui/sync_pr...
chrome/browser/ui/webui/sync_promo_handler.cc:216: // TODO(dbeam): Should this
be here or in a separate handler? This method was
agree it should be separate

http://codereview.chromium.org/8528054/diff/1/chrome/browser/ui/webui/sync_pr...
File chrome/browser/ui/webui/sync_promo_handler.h (right):

http://codereview.chromium.org/8528054/diff/1/chrome/browser/ui/webui/sync_pr...
chrome/browser/ui/webui/sync_promo_handler.h:45: // JavaScript handler to record
a single amount of time throbbers were shown.
comment no makey sense

http://codereview.chromium.org/8528054/diff/1/chrome/common/extensions/extens...
File chrome/common/extensions/extension_constants.h (right):

http://codereview.chromium.org/8528054/diff/1/chrome/common/extensions/extens...
chrome/common/extensions/extension_constants.h:457: // TODO(dbeam): Is this the
right place for these values?
nope

[Dan Beam, 2011-11-15 18:37:13]

http://codereview.chromium.org/8528054/diff/1/chrome/browser/resources/sync_p...
File chrome/browser/resources/sync_promo.js (right):

http://codereview.chromium.org/8528054/diff/1/chrome/browser/resources/sync_p...
chrome/browser/resources/sync_promo.js:6: <include
src="options/options_page.js"/>
On 2011/11/15 17:45:19, Evan Stade wrote:
> wtffff

Wasn't me.  Want me to move?

http://codereview.chromium.org/8528054/diff/1/chrome/browser/resources/sync_p...
chrome/browser/resources/sync_promo.js:240: throbberStart_: null,
On 2011/11/15 17:45:19, Evan Stade wrote:
> why are you complicating this by setting it to null? use 0

Done.

http://codereview.chromium.org/8528054/diff/1/chrome/browser/resources/sync_s...
File chrome/browser/resources/sync_setup_overlay.js (right):

http://codereview.chromium.org/8528054/diff/1/chrome/browser/resources/sync_s...
chrome/browser/resources/sync_setup_overlay.js:548: var visibleOrHidden =
visible ? "visible" : "hidden";
On 2011/11/15 17:45:19, Evan Stade wrote:
> revert the changes to this function

OK, I can, but why?  I removed the loop invariant code motion, changed the
indentation to match our style guide (I thought), and added documentation so I
could @inheritDoc from the subclass...

http://codereview.chromium.org/8528054/diff/1/chrome/browser/ui/webui/metrics...
File chrome/browser/ui/webui/metrics_handler.h (right):

http://codereview.chromium.org/8528054/diff/1/chrome/browser/ui/webui/metrics...
chrome/browser/ui/webui/metrics_handler.h:36: // TODO(dbeam): Rename
recordInHistgram to something less broad and more
On 2011/11/15 17:45:19, Evan Stade wrote:
> move this comment into a bug report and just link to it here.

Done. http://crbug.com/104338

http://codereview.chromium.org/8528054/diff/1/chrome/browser/ui/webui/sync_pr...
File chrome/browser/ui/webui/sync_promo_handler.cc (right):

http://codereview.chromium.org/8528054/diff/1/chrome/browser/ui/webui/sync_pr...
chrome/browser/ui/webui/sync_promo_handler.cc:216: // TODO(dbeam): Should this
be here or in a separate handler? This method was
On 2011/11/15 17:45:19, Evan Stade wrote:
> agree it should be separate

Done.

http://codereview.chromium.org/8528054/diff/1/chrome/browser/ui/webui/sync_pr...
File chrome/browser/ui/webui/sync_promo_handler.h (right):

http://codereview.chromium.org/8528054/diff/1/chrome/browser/ui/webui/sync_pr...
chrome/browser/ui/webui/sync_promo_handler.h:45: // JavaScript handler to record
a single amount of time throbbers were shown.
On 2011/11/15 17:45:19, Evan Stade wrote:
> comment no makey sense

JavaScript handler to record the amount of time throbbers were shown.?

http://codereview.chromium.org/8528054/diff/1/chrome/common/extensions/extens...
File chrome/common/extensions/extension_constants.h (right):

http://codereview.chromium.org/8528054/diff/1/chrome/common/extensions/extens...
chrome/common/extensions/extension_constants.h:457: // TODO(dbeam): Is this the
right place for these values?
On 2011/11/15 17:45:19, Evan Stade wrote:
> nope

Done. (moved to sync_promo_handler.h)

[Dan Beam, 2011-11-15 18:38:05]

http://codereview.chromium.org/8528054/diff/1/chrome/common/extensions/extens...
File chrome/common/extensions/extension_constants.h (right):

http://codereview.chromium.org/8528054/diff/1/chrome/common/extensions/extens...
chrome/common/extensions/extension_constants.h:457: // TODO(dbeam): Is this the
right place for these values?
On 2011/11/15 18:37:13, Dan Beam wrote:
> On 2011/11/15 17:45:19, Evan Stade wrote:
> > nope
> 
> Done. (moved to sync_promo_handler.h)

Sorry, sync_promo_handler.cc rather.

[Evan Stade, 2011-11-15 22:46:41]

http://codereview.chromium.org/8528054/diff/1/chrome/browser/resources/sync_p...
File chrome/browser/resources/sync_promo.js (right):

http://codereview.chromium.org/8528054/diff/1/chrome/browser/resources/sync_p...
chrome/browser/resources/sync_promo.js:6: <include
src="options/options_page.js"/>
On 2011/11/15 18:37:13, Dan Beam wrote:
> On 2011/11/15 17:45:19, Evan Stade wrote:
> > wtffff
> 
> Wasn't me.  Want me to move?

no

http://codereview.chromium.org/8528054/diff/1/chrome/browser/resources/sync_s...
File chrome/browser/resources/sync_setup_overlay.js (right):

http://codereview.chromium.org/8528054/diff/1/chrome/browser/resources/sync_s...
chrome/browser/resources/sync_setup_overlay.js:548: var visibleOrHidden =
visible ? "visible" : "hidden";
On 2011/11/15 18:37:13, Dan Beam wrote:
> On 2011/11/15 17:45:19, Evan Stade wrote:
> > revert the changes to this function
> 
> OK, I can, but why?  I removed the loop invariant code motion, changed the
> indentation to match our style guide (I thought), and added documentation so I
> could @inheritDoc from the subclass...

you can leave the docs and the indentation change. The rest of the changes are
performance related presumably but I think they are not worth the cost to
readability.

http://codereview.chromium.org/8528054/diff/1/chrome/browser/ui/webui/sync_pr...
File chrome/browser/ui/webui/sync_promo_handler.h (right):

http://codereview.chromium.org/8528054/diff/1/chrome/browser/ui/webui/sync_pr...
chrome/browser/ui/webui/sync_promo_handler.h:45: // JavaScript handler to record
a single amount of time throbbers were shown.
On 2011/11/15 18:37:13, Dan Beam wrote:
> On 2011/11/15 17:45:19, Evan Stade wrote:
> > comment no makey sense
> 
> JavaScript handler to record the amount of time throbbers were shown.?

to record the duration for which the throbber was visible during a (successful?
unsuccessful?) sign-in flow.

[Dan Beam, 2011-11-16 00:09:53]

http://codereview.chromium.org/8528054/diff/1/chrome/browser/resources/sync_s...
File chrome/browser/resources/sync_setup_overlay.js (right):

http://codereview.chromium.org/8528054/diff/1/chrome/browser/resources/sync_s...
chrome/browser/resources/sync_setup_overlay.js:548: var visibleOrHidden =
visible ? "visible" : "hidden";
On 2011/11/15 22:46:41, Evan Stade wrote:
> On 2011/11/15 18:37:13, Dan Beam wrote:
> > On 2011/11/15 17:45:19, Evan Stade wrote:
> > > revert the changes to this function
> > 
> > OK, I can, but why?  I removed the loop invariant code motion, changed the
> > indentation to match our style guide (I thought), and added documentation so
I
> > could @inheritDoc from the subclass...
> 
> you can leave the docs and the indentation change. The rest of the changes are
> performance related presumably but I think they are not worth the cost to
> readability.

Done.  Crankshaft should optimize the loop invariant code, i++ vs. i++, and
WebKit probably has a NodeList#length cache anyway, I'd guess...

http://codereview.chromium.org/8528054/diff/1/chrome/browser/ui/webui/sync_pr...
File chrome/browser/ui/webui/sync_promo_handler.h (right):

http://codereview.chromium.org/8528054/diff/1/chrome/browser/ui/webui/sync_pr...
chrome/browser/ui/webui/sync_promo_handler.h:45: // JavaScript handler to record
a single amount of time throbbers were shown.
On 2011/11/15 22:46:41, Evan Stade wrote:
> On 2011/11/15 18:37:13, Dan Beam wrote:
> > On 2011/11/15 17:45:19, Evan Stade wrote:
> > > comment no makey sense
> > 
> > JavaScript handler to record the amount of time throbbers were shown.?
> 
> to record the duration for which the throbber was visible during a
(successful?
> unsuccessful?) sign-in flow.

Done.

[Dan Beam, 2011-11-16 00:11:43]

> i++ vs. i++

d'oh, meant ++i vs. i++

[Evan Stade, 2011-11-16 00:53:16]

lgtm with offline bikeshed

[commit-bot: I haz the power, 2011-11-16 19:47:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dbeam@chromium.org/8528054/4018

[commit-bot: I haz the power, 2011-11-16 21:31:42]

Change committed as 110354