Add UMA stats for user settings

Add UMA stats for user settings

BUG=370596
For c/b/resources:

R=asvitkine@chromium.org, michaelpg@chromium.org
TBR=xiyuan@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=278154

[michaelpg, 2014-06-13 23:15:44]

https://codereview.chromium.org/335953002/diff/20001/chrome/browser/resources...
File chrome/browser/resources/options/browser_options.js (right):

https://codereview.chromium.org/335953002/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/browser_options.js:105:
['Options_ShowAdvancedSettings']);
Worth noting that this count will report up to 2x as many "show advanced
settings" actions as there actually are because users can hit "hide settings"
after "show settings". The actual factor is probably closer to 1.01, but could
you use the test in 97 to check whether they're being shown or hidden?

We probably only care about when they're being shown.

https://codereview.chromium.org/335953002/diff/20001/chrome/browser/resources...
File chrome/browser/resources/options/chromeos/accounts_options.html (right):

https://codereview.chromium.org/335953002/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/chromeos/accounts_options.html:18:
type="checkbox">
nit: You could fit this and the ones below in 2-3 lines instead of 4. Looking at
browser_options.html there doesn't seem to be any consistent attribute ordering
being enforced.

https://codereview.chromium.org/335953002/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/chromeos/accounts_options.html:57:
type="checkbox" inverted_pref>
This is an inverted_pref, which changes the value reported but apparently
doesn't change the way the metric is reported (see
pref_ui.js:updatePrefFromState_ and CoreOptionsHandler::ProcessUserMetric).

"RestrictUsers" is the inverse of the pref name, "allowGuest", so checking this
box should trigger "Options_RestrictUsers_Enable" even though it sets allowGuest
to false. But in actuality, I think it will log "Options_RestrictUsers_Disable".

The easiest way to get around this would be to just rename this
"Options_AllowAllUsers" or something (because the name allowGuest isn't
accurate), so it also inverts the checkbox's meaning.

https://codereview.chromium.org/335953002/diff/20001/chrome/browser/resources...
File chrome/browser/resources/options/chromeos/accounts_options.js (right):

https://codereview.chromium.org/335953002/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/chromeos/accounts_options.js:121:
['Options_AddWhitelistedUser']);
Options_WhitelistedUser_Add? (parity with BookmarkBar_ContextMenu_Add or
KeywordEditor_AddKeyword)

https://codereview.chromium.org/335953002/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/chromeos/accounts_options.js:132:
['Options_RemoveWhitelistedUser']);
Options_WhitelistedUser_Remove? (parity with BookmarkBar_ContextMenu_Remove or
KeywordEditor_RemoveKeyword)

https://codereview.chromium.org/335953002/diff/20001/chrome/browser/resources...
File chrome/browser/resources/options/sync_section.html (right):

https://codereview.chromium.org/335953002/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/sync_section.html:37:
metric="Options_UsersEnableScreenLock"
Just "Options_UsersScreenLock" or "Options_ScreenLock" since _Enable/_Disable is
free.

https://codereview.chromium.org/335953002/diff/20001/chrome/browser/resources...
File chrome/browser/resources/sync_setup_overlay.html (right):

https://codereview.chromium.org/335953002/diff/20001/chrome/browser/resources...
chrome/browser/resources/sync_setup_overlay.html:36:
metric="Options_SyncSelectDatatypes">
nit: s/types/Types/

https://codereview.chromium.org/335953002/diff/20001/chrome/browser/resources...
File chrome/browser/resources/sync_setup_overlay.js (right):

https://codereview.chromium.org/335953002/diff/20001/chrome/browser/resources...
chrome/browser/resources/sync_setup_overlay.js:386:
['Options_SyncSelectDatatypes']);
nit: s/types/Types/

[stevenjb, 2014-06-16 21:58:11]

+asvitkine@

https://codereview.chromium.org/335953002/diff/20001/chrome/browser/resources...
File chrome/browser/resources/options/browser_options.js (right):

https://codereview.chromium.org/335953002/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/browser_options.js:105:
['Options_ShowAdvancedSettings']);
On 2014/06/13 23:15:43, Michael Giuffrida wrote:
> Worth noting that this count will report up to 2x as many "show advanced
> settings" actions as there actually are because users can hit "hide settings"
> after "show settings". The actual factor is probably closer to 1.01, but could
> you use the test in 97 to check whether they're being shown or hidden?
> 
> We probably only care about when they're being shown.

Done.

https://codereview.chromium.org/335953002/diff/20001/chrome/browser/resources...
File chrome/browser/resources/options/chromeos/accounts_options.html (right):

https://codereview.chromium.org/335953002/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/chromeos/accounts_options.html:18:
type="checkbox">
On 2014/06/13 23:15:43, Michael Giuffrida wrote:
> nit: You could fit this and the ones below in 2-3 lines instead of 4. Looking
at
> browser_options.html there doesn't seem to be any consistent attribute
ordering
> being enforced.
I moved type next to id for brevity but I prefer the order within the file being
consistent.

https://codereview.chromium.org/335953002/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/chromeos/accounts_options.html:57:
type="checkbox" inverted_pref>
On 2014/06/13 23:15:43, Michael Giuffrida wrote:
> This is an inverted_pref, which changes the value reported but apparently
> doesn't change the way the metric is reported (see
> pref_ui.js:updatePrefFromState_ and CoreOptionsHandler::ProcessUserMetric).
> 
> "RestrictUsers" is the inverse of the pref name, "allowGuest", so checking
this
> box should trigger "Options_RestrictUsers_Enable" even though it sets
allowGuest
> to false. But in actuality, I think it will log
"Options_RestrictUsers_Disable".
> 
> The easiest way to get around this would be to just rename this
> "Options_AllowAllUsers" or something (because the name allowGuest isn't
> accurate), so it also inverts the checkbox's meaning.

Done.

https://codereview.chromium.org/335953002/diff/20001/chrome/browser/resources...
File chrome/browser/resources/options/chromeos/accounts_options.js (right):

https://codereview.chromium.org/335953002/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/chromeos/accounts_options.js:121:
['Options_AddWhitelistedUser']);
On 2014/06/13 23:15:43, Michael Giuffrida wrote:
> Options_WhitelistedUser_Add? (parity with BookmarkBar_ContextMenu_Add or
> KeywordEditor_AddKeyword)

Done.

https://codereview.chromium.org/335953002/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/chromeos/accounts_options.js:132:
['Options_RemoveWhitelistedUser']);
On 2014/06/13 23:15:43, Michael Giuffrida wrote:
> Options_WhitelistedUser_Remove? (parity with BookmarkBar_ContextMenu_Remove or
> KeywordEditor_RemoveKeyword)

Done.

https://codereview.chromium.org/335953002/diff/20001/chrome/browser/resources...
File chrome/browser/resources/options/sync_section.html (right):

https://codereview.chromium.org/335953002/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/sync_section.html:37:
metric="Options_UsersEnableScreenLock"
On 2014/06/13 23:15:43, Michael Giuffrida wrote:
> Just "Options_UsersScreenLock" or "Options_ScreenLock" since _Enable/_Disable
is
> free.

Done.

https://codereview.chromium.org/335953002/diff/20001/chrome/browser/resources...
File chrome/browser/resources/sync_setup_overlay.html (right):

https://codereview.chromium.org/335953002/diff/20001/chrome/browser/resources...
chrome/browser/resources/sync_setup_overlay.html:36:
metric="Options_SyncSelectDatatypes">
On 2014/06/13 23:15:43, Michael Giuffrida wrote:
> nit: s/types/Types/

Done.

https://codereview.chromium.org/335953002/diff/20001/chrome/browser/resources...
File chrome/browser/resources/sync_setup_overlay.js (right):

https://codereview.chromium.org/335953002/diff/20001/chrome/browser/resources...
chrome/browser/resources/sync_setup_overlay.js:386:
['Options_SyncSelectDatatypes']);
On 2014/06/13 23:15:43, Michael Giuffrida wrote:
> nit: s/types/Types/

Done (and changed to ToggleSyncDataType)

[michaelpg, 2014-06-16 22:07:53]

lgtm

[Alexei Svitkine (slow), 2014-06-17 15:56:18]

lgtm

[stevenjb, 2014-06-17 17:02:28]

The CQ bit was checked by stevenjb@chromium.org

[stevenjb, 2014-06-17 17:04:00]

The CQ bit was unchecked by stevenjb@chromium.org

[commit-bot: I haz the power, 2014-06-17 17:04:13]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/stevenjb@chromium.org/335953002/40001

[stevenjb, 2014-06-17 22:02:06]

The CQ bit was checked by stevenjb@chromium.org

[commit-bot: I haz the power, 2014-06-17 22:03:13]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/stevenjb@chromium.org/335953002/60001

[commit-bot: I haz the power, 2014-06-18 09:08:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-18 09:08:09]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)

[stevenjb, 2014-06-18 20:03:41]

Committed patchset #5 manually as r278154 (presubmit successful).