UMA metrics added for HID detection dialog.

UMA metrics added for HID detection dialog.

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

[ygorshenin1, 2014-05-08 11:50:08]

https://codereview.chromium.org/276433003/diff/1/chrome/browser/ui/webui/chro...
File chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc
(right):

https://codereview.chromium.org/276433003/diff/1/chrome/browser/ui/webui/chro...
chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc:380:
base::UserMetricsAction("OOBE.HIDDetectionDialogShown"));
Consider to use HISTOGRAM_BOOLEAN.

https://codereview.chromium.org/276433003/diff/1/tools/metrics/actions/action...
File tools/metrics/actions/actions.xml (right):

https://codereview.chromium.org/276433003/diff/1/tools/metrics/actions/action...
tools/metrics/actions/actions.xml:7838: <owner>Please list the metric's owners.
Add more owner tags as needed.</owner>
Add yourself as an owner + add a short description.

https://codereview.chromium.org/276433003/diff/1/tools/metrics/histograms/his...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/276433003/diff/1/tools/metrics/histograms/his...
tools/metrics/histograms/histograms.xml:7390: + 
<owner>shess@chromium.org</owner>
Are you sure that shess@ is a correct owner to this histogram?

[merkulova, 2014-05-12 10:22:59]

+mnissler@ for browser_prefs.cc

It's discussable do we need boolean histogram recording HID-detection screen
shows as far as we have histogram recording number of these shows per OOBE
completion.

https://codereview.chromium.org/276433003/diff/1/chrome/browser/ui/webui/chro...
File chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc
(right):

https://codereview.chromium.org/276433003/diff/1/chrome/browser/ui/webui/chro...
chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc:380:
base::UserMetricsAction("OOBE.HIDDetectionDialogShown"));
On 2014/05/08 11:50:08, ygorshenin1 wrote:
> Consider to use HISTOGRAM_BOOLEAN.

Done.

https://codereview.chromium.org/276433003/diff/1/tools/metrics/actions/action...
File tools/metrics/actions/actions.xml (right):

https://codereview.chromium.org/276433003/diff/1/tools/metrics/actions/action...
tools/metrics/actions/actions.xml:7838: <owner>Please list the metric's owners.
Add more owner tags as needed.</owner>
On 2014/05/08 11:50:08, ygorshenin1 wrote:
> Add yourself as an owner + add a short description.

Removed with counter. Switching to bool histogram.

https://codereview.chromium.org/276433003/diff/1/tools/metrics/histograms/his...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/276433003/diff/1/tools/metrics/histograms/his...
tools/metrics/histograms/histograms.xml:7390: + 
<owner>shess@chromium.org</owner>
On 2014/05/08 11:50:08, ygorshenin1 wrote:
> Are you sure that shess@ is a correct owner to this histogram?

Well, he's clearly seen at annotation as author of this piece.

[Mattias Nissler (ping if slow), 2014-05-12 11:10:02]

The prefs stuff LGTM, but please also consider my comments on files not related
to prefs.

https://codereview.chromium.org/276433003/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/wizard_controller.cc (right):

https://codereview.chromium.org/276433003/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/wizard_controller.cc:715:
GetLocalState()->GetInteger(prefs::kTimesHIDDialogShown));
Would it make sense to clear the pref here? I suspect we don't need the value
ever again after OOBE, so it'd make sense to release the storage.

https://codereview.chromium.org/276433003/diff/20001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc
(right):

https://codereview.chromium.org/276433003/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc:111:
local_state->CommitPendingWrite();
This should not be necessary.

https://codereview.chromium.org/276433003/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc:195:
local_state->CommitPendingWrite();
not necessary.

https://codereview.chromium.org/276433003/diff/20001/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

https://codereview.chromium.org/276433003/diff/20001/chrome/common/pref_names...
chrome/common/pref_names.cc:2069: const char kTimesHIDDialogShown[] =
"HIDDialog.shown_how_many";
nit: Maybe shown_how_many_times or shown_how_often would be better names?

[merkulova, 2014-05-13 07:18:00]

Mattias, thanks for looking through  the full CL.

https://codereview.chromium.org/276433003/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/wizard_controller.cc (right):

https://codereview.chromium.org/276433003/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/wizard_controller.cc:715:
GetLocalState()->GetInteger(prefs::kTimesHIDDialogShown));
On 2014/05/12 11:10:02, Mattias Nissler wrote:
> Would it make sense to clear the pref here? I suspect we don't need the value
> ever again after OOBE, so it'd make sense to release the storage.

Done.

https://codereview.chromium.org/276433003/diff/20001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc
(right):

https://codereview.chromium.org/276433003/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc:111:
local_state->CommitPendingWrite();
On 2014/05/12 11:10:02, Mattias Nissler wrote:
> This should not be necessary.

Done.

https://codereview.chromium.org/276433003/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc:195:
local_state->CommitPendingWrite();
On 2014/05/12 11:10:02, Mattias Nissler wrote:
> not necessary.

Done.

https://codereview.chromium.org/276433003/diff/20001/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

https://codereview.chromium.org/276433003/diff/20001/chrome/common/pref_names...
chrome/common/pref_names.cc:2069: const char kTimesHIDDialogShown[] =
"HIDDialog.shown_how_many";
On 2014/05/12 11:10:02, Mattias Nissler wrote:
> nit: Maybe shown_how_many_times or shown_how_often would be better names?

Done.

[ygorshenin1, 2014-05-14 09:35:35]

https://codereview.chromium.org/276433003/diff/40001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc
(right):

https://codereview.chromium.org/276433003/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc:130: //
static
Move this method after OnInputDeviceRemoved() (follow the order in header file).

https://codereview.chromium.org/276433003/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc:192:
local_state->SetInteger(prefs::kTimesHIDDialogShown,
After discussion: could you please get rid of this "-1" logic? If possible, fix
this in this CL, otherwise add a TODO comment.

https://codereview.chromium.org/276433003/diff/40001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.h
(right):

https://codereview.chromium.org/276433003/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.h:186: enum
ContinueScenarioType {
As you're using this enum only in HIDDetectionScreenHandler could you please
move the enum declaration to the public part of HIDDetectionScreenHandler?

https://codereview.chromium.org/276433003/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.h:187: 
nit: delete blank line.

[merkulova, 2014-05-14 13:11:05]

+asvitkine@ for histograms.xml file

https://codereview.chromium.org/276433003/diff/40001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc
(right):

https://codereview.chromium.org/276433003/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc:130: //
static
On 2014/05/14 09:35:36, ygorshenin1 wrote:
> Move this method after OnInputDeviceRemoved() (follow the order in header
file).

Done.

https://codereview.chromium.org/276433003/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc:192:
local_state->SetInteger(prefs::kTimesHIDDialogShown,
On 2014/05/14 09:35:36, ygorshenin1 wrote:
> After discussion: could you please get rid of this "-1" logic? If possible,
fix
> this in this CL, otherwise add a TODO comment.

Done.

https://codereview.chromium.org/276433003/diff/40001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.h
(right):

https://codereview.chromium.org/276433003/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.h:186: enum
ContinueScenarioType {
On 2014/05/14 09:35:36, ygorshenin1 wrote:
> As you're using this enum only in HIDDetectionScreenHandler could you please
> move the enum declaration to the public part of HIDDetectionScreenHandler?

Done.

https://codereview.chromium.org/276433003/diff/40001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.h:187: 
On 2014/05/14 09:35:36, ygorshenin1 wrote:
> nit: delete blank line.

Done.

[Alexei Svitkine (slow), 2014-05-14 14:57:48]

https://codereview.chromium.org/276433003/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/reset_browsertest.cc (right):

https://codereview.chromium.org/276433003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/reset_browsertest.cc:79:
ASSERT_TRUE(JSExecuted("$('rHistogHistograHistogeset-button').click();"));
Is this change intentional?

https://codereview.chromium.org/276433003/diff/60001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc
(right):

https://codereview.chromium.org/276433003/diff/60001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc:164: int
scenario_type;
Nit: ContinueScenarioType instead of int.

https://codereview.chromium.org/276433003/diff/60001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc:360:
base::Unretained(this)));
Why not use weak_ptr_factory_.GetWeakPtr() instead of base::Unretained() here?
Same for UpdateDevices() below.

https://codereview.chromium.org/276433003/diff/60001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc:436:
HandleOnContinue();
Nit: prefer an early return and no else block.

https://codereview.chromium.org/276433003/diff/60001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc:438:
PrefService* const local_state = g_browser_process->local_state();
Nit: Chromium style doesn't usually use const in this manner. So just remove it.

https://codereview.chromium.org/276433003/diff/60001/tools/metrics/histograms...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/276433003/diff/60001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:7562: +    Whether HID detection dialog
was shown on OOBE. This metric is specific to
Maybe expand this to clarify exactly when it's logged? (e.g. "logged once per
OOBE when Continue is pressed" or something)

https://codereview.chromium.org/276433003/diff/60001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:7570: +    Records number of times the
dialog was shown by the time OOBE is completed.
Nit: Add "This metric is specific to ChromeOS."

[merkulova, 2014-05-22 12:02:32]

https://codereview.chromium.org/276433003/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/reset_browsertest.cc (right):

https://codereview.chromium.org/276433003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/reset_browsertest.cc:79:
ASSERT_TRUE(JSExecuted("$('rHistogHistograHistogeset-button').click();"));
On 2014/05/14 14:57:48, Alexei Svitkine wrote:
> Is this change intentional?
Oops, sorry

https://codereview.chromium.org/276433003/diff/60001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc
(right):

https://codereview.chromium.org/276433003/diff/60001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc:164: int
scenario_type;
On 2014/05/14 14:57:48, Alexei Svitkine wrote:
> Nit: ContinueScenarioType instead of int.

Done.

https://codereview.chromium.org/276433003/diff/60001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc:360:
base::Unretained(this)));
On 2014/05/14 14:57:48, Alexei Svitkine wrote:
> Why not use weak_ptr_factory_.GetWeakPtr() instead of base::Unretained() here?
> Same for UpdateDevices() below.

Done.

https://codereview.chromium.org/276433003/diff/60001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc:436:
HandleOnContinue();
On 2014/05/14 14:57:48, Alexei Svitkine wrote:
> Nit: prefer an early return and no else block.

Done.

https://codereview.chromium.org/276433003/diff/60001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.cc:438:
PrefService* const local_state = g_browser_process->local_state();
On 2014/05/14 14:57:48, Alexei Svitkine wrote:
> Nit: Chromium style doesn't usually use const in this manner. So just remove
it.

Done.

https://codereview.chromium.org/276433003/diff/60001/tools/metrics/histograms...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/276433003/diff/60001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:7562: +    Whether HID detection dialog
was shown on OOBE. This metric is specific to
On 2014/05/14 14:57:48, Alexei Svitkine wrote:
> Maybe expand this to clarify exactly when it's logged? (e.g. "logged once per
> OOBE when Continue is pressed" or something)

Done.

https://codereview.chromium.org/276433003/diff/60001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:7570: +    Records number of times the
dialog was shown by the time OOBE is completed.
On 2014/05/14 14:57:48, Alexei Svitkine wrote:
> Nit: Add "This metric is specific to ChromeOS."

Done.

[ygorshenin1, 2014-05-22 12:08:34]

lgtm

https://codereview.chromium.org/276433003/diff/70001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.h
(right):

https://codereview.chromium.org/276433003/diff/70001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.h:118: void
OnGetInputDevicesListFirstTime(
nit: add a short comment to this method.

[merkulova, 2014-05-22 12:17:29]

https://codereview.chromium.org/276433003/diff/70001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.h
(right):

https://codereview.chromium.org/276433003/diff/70001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/chromeos/login/hid_detection_screen_handler.h:118: void
OnGetInputDevicesListFirstTime(
On 2014/05/22 12:08:35, ygorshenin1 wrote:
> nit: add a short comment to this method.

Done.

[Alexei Svitkine (slow), 2014-05-22 13:18:16]

LGTM

[merkulova, 2014-05-23 12:29:05]

The CQ bit was checked by merkulova@chromium.org

[commit-bot: I haz the power, 2014-05-23 12:29:15]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/merkulova@chromium.org/276433003/90001

[commit-bot: I haz the power, 2014-05-23 14:56:29]

Change committed as 272526