Added histograms for tracking new user's priority prefs sync time and results.

Added histograms for tracking new user's priority prefs sync time and results.

BUG=392232
Committed: https://crrev.com/698ac976fa47efabc5e9396309de007a6616f2ee
Cr-Commit-Position: refs/heads/master@{#330235}

[dzhioev (left Google), 2015-05-15 08:20:59]

dzhioev@chromium.org changed reviewers:
+ isherman@chromium.org, nkostylev@chromium.org

[dzhioev (left Google), 2015-05-15 08:21:00]

Hello, please review.

isherman@: histograms.xml
nkostylev@: user_images_screen.*

Thanks

[Nikita (slow), 2015-05-15 16:23:31]

lgtm

[Ilya Sherman, 2015-05-15 17:36:40]

https://codereview.chromium.org/1130603006/diff/1/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/screens/user_image_screen.cc (right):

https://codereview.chromium.org/1130603006/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/screens/user_image_screen.cc:353:
static_cast<int>(timed_out), 2);
nit: Maybe write this as below?  I'm not too happy about the hardcoded 2 -- if
the SyncResult enum were ever expanded, this code path would silently break.

UMA_HISTOGRAM_BOOLEAN("Login.NewUserPriorityPrefsSyncResult",
                      timed_out == SyncResult::TIMED_OUT);

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

https://codereview.chromium.org/1130603006/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:14289: +    limit, and how many times it
didn't.
nit: Suggested rephrasing: "Records whether a new user's priority prefs sync
timed out."

https://codereview.chromium.org/1130603006/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:14297: +    Can't exceed a limit of 10s.
nit: Suggested rephrasing for the second sentence: "The operation will time out
after 10s."  Or, if that's not true, and you're simply reflecting the histogram
bound, then maybe it would be better to change the histogram bound to be equal
to the timeout?

[dzhioev (left Google), 2015-05-15 20:36:50]

https://codereview.chromium.org/1130603006/diff/1/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/screens/user_image_screen.cc (right):

https://codereview.chromium.org/1130603006/diff/1/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/screens/user_image_screen.cc:353:
static_cast<int>(timed_out), 2);
On 2015/05/15 17:36:40, Ilya Sherman wrote:
> nit: Maybe write this as below?  I'm not too happy about the hardcoded 2 -- if
> the SyncResult enum were ever expanded, this code path would silently break.
> 
> UMA_HISTOGRAM_BOOLEAN("Login.NewUserPriorityPrefsSyncResult",
>                       timed_out == SyncResult::TIMED_OUT);

I've added |COUNT| element to the enum, and use it here instead of hardcoded
|2|.

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

https://codereview.chromium.org/1130603006/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:14289: +    limit, and how many times it
didn't.
On 2015/05/15 17:36:40, Ilya Sherman wrote:
> nit: Suggested rephrasing: "Records whether a new user's priority prefs sync
> timed out."

Done.

https://codereview.chromium.org/1130603006/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:14297: +    Can't exceed a limit of 10s.
On 2015/05/15 17:36:40, Ilya Sherman wrote:
> nit: Suggested rephrasing for the second sentence: "The operation will time
out
> after 10s."
Done.
> Or, if that's not true, and you're simply reflecting the histogram
> bound, then maybe it would be better to change the histogram bound to be equal
> to the timeout?
No, it's fine, we actually have a 10s limit.

[Ilya Sherman, 2015-05-15 20:41:32]

LGTM

https://codereview.chromium.org/1130603006/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/screens/user_image_screen.cc (right):

https://codereview.chromium.org/1130603006/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screens/user_image_screen.cc:354:
static_cast<int>(SyncResult::COUNT));
nit: No need for the static_casts.

https://codereview.chromium.org/1130603006/diff/20001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1130603006/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:58693: +  <int value="0"
label="Succeed"/>
nit: Succeeded or Success

[dzhioev (left Google), 2015-05-15 21:14:43]

https://codereview.chromium.org/1130603006/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/screens/user_image_screen.cc (right):

https://codereview.chromium.org/1130603006/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screens/user_image_screen.cc:354:
static_cast<int>(SyncResult::COUNT));
On 2015/05/15 20:41:31, Ilya Sherman wrote:
> nit: No need for the static_casts.

static_cast is needed here, because the enum is a C++11 strongly typed enum
(enum class).

https://codereview.chromium.org/1130603006/diff/20001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1130603006/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:58693: +  <int value="0"
label="Succeed"/>
On 2015/05/15 20:41:32, Ilya Sherman wrote:
> nit: Succeeded or Success

Done.

[dzhioev (left Google), 2015-05-15 21:15:02]

The CQ bit was checked by dzhioev@chromium.org

[dzhioev (left Google), 2015-05-15 21:15:03]

The patchset sent to the CQ was uploaded after l-g-t-m from
nkostylev@chromium.org, isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/1130603006/#ps40001
(title: "nits")

[commit-bot: I haz the power, 2015-05-15 21:15:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1130603006/40001

[Ilya Sherman, 2015-05-15 21:34:55]

(Still LGTM)

https://codereview.chromium.org/1130603006/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/screens/user_image_screen.cc (right):

https://codereview.chromium.org/1130603006/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/login/screens/user_image_screen.cc:354:
static_cast<int>(SyncResult::COUNT));
On 2015/05/15 21:14:42, dzhioev wrote:
> On 2015/05/15 20:41:31, Ilya Sherman wrote:
> > nit: No need for the static_casts.
> 
> static_cast is needed here, because the enum is a C++11 strongly typed enum
> (enum class).

Ah, bummer.  I'm generally a fan of strongly typed enums, but it's too bad that
they don't play very nicely with the UMA_HISTOGRAM_ENUMERATION macro.  I guess
we'd need templates to fix that?

[dzhioev (left Google), 2015-05-15 23:06:16]

On 2015/05/15 21:34:55, Ilya Sherman wrote:
> (Still LGTM)
> 
>
https://codereview.chromium.org/1130603006/diff/20001/chrome/browser/chromeos...
> File chrome/browser/chromeos/login/screens/user_image_screen.cc (right):
> 
>
https://codereview.chromium.org/1130603006/diff/20001/chrome/browser/chromeos...
> chrome/browser/chromeos/login/screens/user_image_screen.cc:354:
> static_cast<int>(SyncResult::COUNT));
> On 2015/05/15 21:14:42, dzhioev wrote:
> > On 2015/05/15 20:41:31, Ilya Sherman wrote:
> > > nit: No need for the static_casts.
> > 
> > static_cast is needed here, because the enum is a C++11 strongly typed enum
> > (enum class).
> 
> Ah, bummer.  I'm generally a fan of strongly typed enums, but it's too bad
that
> they don't play very nicely with the UMA_HISTOGRAM_ENUMERATION macro.  I guess
> we'd need templates to fix that?

Probably. But detecting an enum is not a trivial task without library support
(C++11 library has http://en.cppreference.com/w/cpp/types/is_enum ).

[commit-bot: I haz the power, 2015-05-15 23:44:56]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-05-18 11:29:17]

Patchset 3 (id:??) landed as
https://crrev.com/698ac976fa47efabc5e9396309de007a6616f2ee
Cr-Commit-Position: refs/heads/master@{#330235}