UMA: How often are different network error screens encountered during OOBE

UMA: How often are different network error screens encountered during OOBE
BUG=365354
Committed: https://crrev.com/f406302bb6e9ab6a533d3ac39c39b39705d7d422
Cr-Commit-Position: refs/heads/master@{#296922}

Committed: https://crrev.com/ba10dbe7c6c48e99abc6f0467f1ed208319650bb
Cr-Commit-Position: refs/heads/master@{#300537}

[Roman Sorokin (ftl), 2014-08-20 13:13:15]

+ygorshenin

ygorshenin, please review

[ygorshenin1, 2014-08-26 14:33:14]

Sorry for the delay in response.

https://codereview.chromium.org/494633003/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/enrollment/auto_enrollment_check_screen.cc
(right):

https://codereview.chromium.org/494633003/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/enrollment/auto_enrollment_check_screen.cc:217:
histogram_helper_->OnErrorShow(error_screen->GetErrorState());
nit: I suggest you to replace ErrorScreen::GetErrorState() call by the
error_state, because if someone occasionally will move the line before the call
to ErrorScreen::SetErrorState(), incorrect error state will be reported.

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

https://codereview.chromium.org/494633003/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/error_screens_histogram_helper.cc:29: return "";
nit: replace "" by std::string().

https://codereview.chromium.org/494633003/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/error_screens_histogram_helper.cc:81:
UMA_HISTOGRAM_MEDIUM_TIMES(kOobeTimeSpentOnErrorScreensPrefix +
Am I right that if, for example, 30 seconds were spent on the offline error
message and after that 30 seconds were spent on the portal error message, then
60 seconds will be reported for OOBE.ErrorScreensTime.Portal?

https://codereview.chromium.org/494633003/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/error_screens_histogram_helper.h (right):

https://codereview.chromium.org/494633003/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/error_screens_histogram_helper.h:17: explicit
ErrorScreensHistogramHelper(const char* screen_name);
nit: replace const char* by const std::string&.

https://codereview.chromium.org/494633003/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/error_screens_histogram_helper.h:31: void
OnErrorHideTime(base::Time now);
nit: add a blank line between methods and fields.

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

https://codereview.chromium.org/494633003/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/error_screens_histogram_helper_unittest.cc:34: }
nit: could you please add a blank line after the closing curly brace?

[Roman Sorokin (ftl), 2014-09-04 13:26:48]

ygorshenin1, please review

https://codereview.chromium.org/494633003/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/enrollment/auto_enrollment_check_screen.cc
(right):

https://codereview.chromium.org/494633003/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/enrollment/auto_enrollment_check_screen.cc:217:
histogram_helper_->OnErrorShow(error_screen->GetErrorState());
On 2014/08/26 14:33:13, ygorshenin1 wrote:
> nit: I suggest you to replace ErrorScreen::GetErrorState() call by the
> error_state, because if someone occasionally will move the line before the
call
> to ErrorScreen::SetErrorState(), incorrect error state will be reported.

Done.

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

https://codereview.chromium.org/494633003/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/error_screens_histogram_helper.cc:29: return "";
On 2014/08/26 14:33:13, ygorshenin1 wrote:
> nit: replace "" by std::string().

Done.

https://codereview.chromium.org/494633003/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/error_screens_histogram_helper.cc:81:
UMA_HISTOGRAM_MEDIUM_TIMES(kOobeTimeSpentOnErrorScreensPrefix +
Yes, we decided with Nikita to report the last error screen only.
On 2014/08/26 14:33:14, ygorshenin1 wrote:
> Am I right that if, for example, 30 seconds were spent on the offline error
> message and after that 30 seconds were spent on the portal error message, then
> 60 seconds will be reported for OOBE.ErrorScreensTime.Portal?

https://codereview.chromium.org/494633003/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/error_screens_histogram_helper.h (right):

https://codereview.chromium.org/494633003/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/error_screens_histogram_helper.h:17: explicit
ErrorScreensHistogramHelper(const char* screen_name);
On 2014/08/26 14:33:14, ygorshenin1 wrote:
> nit: replace const char* by const std::string&.

Done.

https://codereview.chromium.org/494633003/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/error_screens_histogram_helper.h:31: void
OnErrorHideTime(base::Time now);
On 2014/08/26 14:33:14, ygorshenin1 wrote:
> nit: add a blank line between methods and fields.

Done.

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

https://codereview.chromium.org/494633003/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/error_screens_histogram_helper_unittest.cc:34: }
On 2014/08/26 14:33:14, ygorshenin1 wrote:
> nit: could you please add a blank line after the closing curly brace?

Done.

[ygorshenin1, 2014-09-04 15:13:03]

lgtm

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

https://codereview.chromium.org/494633003/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/error_screens_histogram_helper.cc:81:
UMA_HISTOGRAM_MEDIUM_TIMES(kOobeTimeSpentOnErrorScreensPrefix +
OK, could you please add an clarification to the histograms.xml as this meaning
is not-obvious?

On 2014/09/04 13:26:47, Roman Sorokin wrote:
> Yes, we decided with Nikita to report the last error screen only.
> On 2014/08/26 14:33:14, ygorshenin1 wrote:
> > Am I right that if, for example, 30 seconds were spent on the offline error
> > message and after that 30 seconds were spent on the portal error message,
then
> > 60 seconds will be reported for OOBE.ErrorScreensTime.Portal?
>

[Roman Sorokin (ftl), 2014-09-05 14:58:57]

The CQ bit was checked by rsorokin@chromium.org

[Roman Sorokin (ftl), 2014-09-05 14:58:58]

The CQ bit was unchecked by rsorokin@chromium.org

[Roman Sorokin (ftl), 2014-09-10 07:31:18]

rsorokin@chromium.org changed reviewers:
+ jwd@chromium.org

[Roman Sorokin (ftl), 2014-09-10 07:31:57]

+jwd@

jwd@, please review tools/metrics/histograms/histograms.xml

[jwd, 2014-09-10 15:58:18]

https://codereview.chromium.org/494633003/diff/120001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/error_screens_histogram_helper.cc (right):

https://codereview.chromium.org/494633003/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/login/error_screens_histogram_helper.cc:40:
base::HistogramBase* histogram = base::LinearHistogram::FactoryGet(
Can you add a comment saying this corresponds to the UMA_HISTOGRAM_ENUMERATION
macro.

https://codereview.chromium.org/494633003/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/login/error_screens_histogram_helper.cc:62:
base::HistogramBase* histogram = base::LinearHistogram::FactoryTimeGet(
Having a linear histogram seems a bit odd for recording time. An exponential is
usually preferred.

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

https://codereview.chromium.org/494633003/diff/120001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:20678: +<histogram
name="OOBE.ErrorScreensTime.Enrollment.AuthExtTimeout"
You can use the histograms_suffixes feature to make these definitions more
compact. The rest of the code would not need to change.

[Roman Sorokin (ftl), 2014-09-11 15:05:34]

jwd@, please review

https://codereview.chromium.org/494633003/diff/120001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/error_screens_histogram_helper.cc (right):

https://codereview.chromium.org/494633003/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/login/error_screens_histogram_helper.cc:40:
base::HistogramBase* histogram = base::LinearHistogram::FactoryGet(
On 2014/09/10 15:58:18, Jesse Doherty wrote:
> Can you add a comment saying this corresponds to the UMA_HISTOGRAM_ENUMERATION
> macro.

Done.

https://codereview.chromium.org/494633003/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/login/error_screens_histogram_helper.cc:62:
base::HistogramBase* histogram = base::LinearHistogram::FactoryTimeGet(
On 2014/09/10 15:58:18, Jesse Doherty wrote:
> Having a linear histogram seems a bit odd for recording time. An exponential
is
> usually preferred.

Done.

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

https://codereview.chromium.org/494633003/diff/120001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:20678: +<histogram
name="OOBE.ErrorScreensTime.Enrollment.AuthExtTimeout"
On 2014/09/10 15:58:18, Jesse Doherty wrote:
> You can use the histograms_suffixes feature to make these definitions more
> compact. The rest of the code would not need to change.

Done.

[jwd, 2014-09-11 19:18:55]

lgtm

[Roman Sorokin (ftl), 2014-09-22 08:49:51]

ygorshenin1@, can you please review
chrome/browser/chromeos/login/error_screens_histogram_helper.cc

[ygorshenin1, 2014-09-22 11:36:40]

LGTM with nits.

https://codereview.chromium.org/494633003/diff/140001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/error_screens_histogram_helper.cc (right):

https://codereview.chromium.org/494633003/diff/140001/chrome/browser/chromeos...
chrome/browser/chromeos/login/error_screens_histogram_helper.cc:51: static const
base::TimeDelta time_min = base::TimeDelta::FromMilliseconds(10);
nit: could you please move variable declarations before all method declarations?

https://codereview.chromium.org/494633003/diff/140001/chrome/browser/chromeos...
chrome/browser/chromeos/login/error_screens_histogram_helper.cc:51: static const
base::TimeDelta time_min = base::TimeDelta::FromMilliseconds(10);
nit: TimeDelta's and int are lightweight enough, so there're no need in static
storage specifier.

[Roman Sorokin (ftl), 2014-09-26 10:46:20]

https://codereview.chromium.org/494633003/diff/140001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/error_screens_histogram_helper.cc (right):

https://codereview.chromium.org/494633003/diff/140001/chrome/browser/chromeos...
chrome/browser/chromeos/login/error_screens_histogram_helper.cc:51: static const
base::TimeDelta time_min = base::TimeDelta::FromMilliseconds(10);
On 2014/09/22 11:36:40, ygorshenin1 wrote:
> nit: TimeDelta's and int are lightweight enough, so there're no need in static
> storage specifier.

Done.

https://codereview.chromium.org/494633003/diff/140001/chrome/browser/chromeos...
chrome/browser/chromeos/login/error_screens_histogram_helper.cc:51: static const
base::TimeDelta time_min = base::TimeDelta::FromMilliseconds(10);
On 2014/09/22 11:36:40, ygorshenin1 wrote:
> nit: could you please move variable declarations before all method
declarations?

Done.

[Roman Sorokin (ftl), 2014-09-26 10:46:26]

The CQ bit was checked by rsorokin@chromium.org

[commit-bot: I haz the power, 2014-09-26 10:46:35]

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

[commit-bot: I haz the power, 2014-09-26 10:49:27]

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

[commit-bot: I haz the power, 2014-09-26 10:49:28]

Try jobs failed on following builders:
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/61151)
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Roman Sorokin (ftl), 2014-09-26 11:17:15]

The CQ bit was checked by rsorokin@chromium.org

[commit-bot: I haz the power, 2014-09-26 11:17:32]

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

[commit-bot: I haz the power, 2014-09-26 12:18:14]

Committed patchset #10 (id:180001) as 3a6772ee438643033a8f80e43ac84cbde111c50e

[commit-bot: I haz the power, 2014-09-26 12:18:56]

Patchset 10 (id:??) landed as
https://crrev.com/f406302bb6e9ab6a533d3ac39c39b39705d7d422
Cr-Commit-Position: refs/heads/master@{#296922}

[Roman Sorokin (ftl), 2014-10-02 15:52:57]

rsorokin@chromium.org changed reviewers:
+ dzhioev@chromium.org

[Roman Sorokin (ftl), 2014-10-02 15:52:59]

+dzhioev@

dzhioev@, please review

[dzhioev (left Google), 2014-10-02 18:38:12]

Generally good. My only concern is that you put an instance of
ErrorScreensHistogramHelper
into every screen/handler where we use ErrorScreen, and then call methods of
ErrorScreensHistogramHelper
every time we show/hide ErrorScreen. I think it can be done in a much more
flexible way, that doesn't
require changes in specific screens at all.
For example, we can keep a single instance of ErrorScreensHistogramHelper inside
of ErrorScreen object, which knows
when it is shown and hidden. Let's discuss details tomorrow.

[dzhioev (left Google), 2014-10-02 18:38:29]

https://codereview.chromium.org/494633003/diff/200001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/error_screens_histogram_helper.h (right):

https://codereview.chromium.org/494633003/diff/200001/chrome/browser/chromeos...
chrome/browser/chromeos/login/error_screens_histogram_helper.h:36: base::Time
error_screen_start_time;
error_screen_start_time_

https://codereview.chromium.org/494633003/diff/200001/chrome/browser/chromeos...
chrome/browser/chromeos/login/error_screens_histogram_helper.h:37:
base::TimeDelta time_on_error_screens;
time_on_error_screens_

[Roman Sorokin (ftl), 2014-10-21 15:52:02]

The CQ bit was checked by rsorokin@chromium.org

[commit-bot: I haz the power, 2014-10-21 15:52:48]

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

[commit-bot: I haz the power, 2014-10-21 19:56:01]

Committed patchset #12 (id:220001)

[commit-bot: I haz the power, 2014-10-21 19:56:41]

Patchset 12 (id:??) landed as
https://crrev.com/ba10dbe7c6c48e99abc6f0467f1ed208319650bb
Cr-Commit-Position: refs/heads/master@{#300537}