arc: Provide UMA for OptIn flow.

arc: Provide UMA for OptIn flow.

We have set of different UMAs for ARC that we can use to analyze how
OptIn flow passed. However there was no metric that explicitly
defines how OptIn finished. This adds top level metric that shows
how OptIn has finished.

BUG=706564
TEST=Various modes for managed/unmanaged users. chrome://histograms
     shows expected results.

Review-Url: https://codereview.chromium.org/2785813002
Cr-Commit-Position: refs/heads/master@{#460892}
Committed: https://chromium.googlesource.com/chromium/src/+/8575dd76a6ceb99e0c87738d6ccc3db0c70d5176

[khmel, 2017-03-29 21:41:08]

khmel@chromium.org changed reviewers:
+ isherman@chromium.org, lhchavez@chromium.org

[khmel, 2017-03-29 21:41:09]

Hi Ilya and Luis,

PTAL

Thanks!

https://codereview.chromium.org/2785813002/diff/1/chrome/browser/chromeos/arc...
File chrome/browser/chromeos/arc/arc_session_manager.cc (right):

https://codereview.chromium.org/2785813002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_session_manager.cc:609:
UpdateOptInCancelUMA(OptInCancelReason::SESSION_BUSY);
We never recorded this issue. It would be nice to have this stats as well

[Ilya Sherman, 2017-03-29 21:57:44]

Metrics LGTM

[Luis Héctor Chávez, 2017-03-30 15:49:25]

sorry for the delay.

https://codereview.chromium.org/2785813002/diff/1/chrome/browser/chromeos/arc...
File chrome/browser/chromeos/arc/arc_optin_uma.h (right):

https://codereview.chromium.org/2785813002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_optin_uma.h:31: // Canceled by user.
The formatting for this enum looks different from the one in OptInFlowResult.
And different from OptInActionType too. Please stick to one.

https://codereview.chromium.org/2785813002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_optin_uma.h:133: // OptIn has been started.
nit: s/has been/has/

https://codereview.chromium.org/2785813002/diff/1/chrome/browser/chromeos/arc...
File chrome/browser/chromeos/arc/arc_session_manager.h (right):

https://codereview.chromium.org/2785813002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_session_manager.h:306: void
StartOptInFlowTracking();
Can we abstract this logic to stop giving too much responsibility to this class?

class ScopedOptInFlowTracker {
 public:
  ScopedOptInFlowTracker() { /* ... */ };
  ~ScopedOptInFlowTracker() { /* ... */ }
  TrackRetry() { retried_ = true; }
  TrackSuccess() { success_ = true; }
 private:
  bool retried_ = false;
  bool success_ = false;
}

then replace calls to StartOptInFlowTracking with scoped_opt_in_tracker_ =
base::MakeUnique<...>();

and StopOptInFlowTracking with scoped_opt_in_tracker_.reset();

[khmel, 2017-03-30 17:01:43]

Thank you Luis for comments!

PTAL updated version

https://codereview.chromium.org/2785813002/diff/1/chrome/browser/chromeos/arc...
File chrome/browser/chromeos/arc/arc_optin_uma.h (right):

https://codereview.chromium.org/2785813002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_optin_uma.h:31: // Canceled by user.
On 2017/03/30 15:49:25, Luis Héctor Chávez wrote:
> The formatting for this enum looks different from the one in OptInFlowResult.
> And different from OptInActionType too. Please stick to one.

Done.

https://codereview.chromium.org/2785813002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_optin_uma.h:133: // OptIn has been started.
On 2017/03/30 15:49:25, Luis Héctor Chávez wrote:
> nit: s/has been/has/

Done.

https://codereview.chromium.org/2785813002/diff/1/chrome/browser/chromeos/arc...
File chrome/browser/chromeos/arc/arc_session_manager.h (right):

https://codereview.chromium.org/2785813002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_session_manager.h:306: void
StartOptInFlowTracking();
On 2017/03/30 15:49:25, Luis Héctor Chávez wrote:
> Can we abstract this logic to stop giving too much responsibility to this
class?
> 
> class ScopedOptInFlowTracker {
>  public:
>   ScopedOptInFlowTracker() { /* ... */ };
>   ~ScopedOptInFlowTracker() { /* ... */ }
>   TrackRetry() { retried_ = true; }
>   TrackSuccess() { success_ = true; }
>  private:
>   bool retried_ = false;
>   bool success_ = false;
> }
> 
> then replace calls to StartOptInFlowTracking with scoped_opt_in_tracker_ =
> base::MakeUnique<...>();
> 
> and StopOptInFlowTracking with scoped_opt_in_tracker_.reset();

That nice, thanks

[Luis Héctor Chávez, 2017-03-30 17:07:21]

lgtm with a nit.

https://codereview.chromium.org/2785813002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/arc/arc_session_manager.cc (right):

https://codereview.chromium.org/2785813002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_session_manager.cc:80: class
ArcSessionManager::ScopedOptInFlowTracker {
Please add a small comment explaining this class and its usage.

[khmel, 2017-03-30 17:21:07]

Thank you!

https://codereview.chromium.org/2785813002/diff/20001/chrome/browser/chromeos...
File chrome/browser/chromeos/arc/arc_session_manager.cc (right):

https://codereview.chromium.org/2785813002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_session_manager.cc:80: class
ArcSessionManager::ScopedOptInFlowTracker {
On 2017/03/30 17:07:20, Luis Héctor Chávez wrote:
> Please add a small comment explaining this class and its usage.

Done.

[khmel, 2017-03-30 17:21:31]

The CQ bit was checked by khmel@chromium.org

[khmel, 2017-03-30 17:21:32]

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

[commit-bot: I haz the power, 2017-03-30 17:22:04]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-30 18:38:57]

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

[commit-bot: I haz the power, 2017-03-30 18:38:59]

Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[khmel, 2017-03-30 18:39:25]

The CQ bit was checked by khmel@chromium.org

[commit-bot: I haz the power, 2017-03-30 18:39:56]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-30 21:43:21]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1490899165181920,
"parent_rev": "9defbfb506f965590425064b6c38a072b96f6470", "commit_rev":
"8575dd76a6ceb99e0c87738d6ccc3db0c70d5176"}

[commit-bot: I haz the power, 2017-03-30 21:44:12]

Description was changed from

==========
arc: Provide UMA for OptIn flow.

We have set of different UMAs for ARC that we can use to analyze how
OptIn flow passed. However there was no metric that explicitly
defines how OptIn finished. This adds top level metric that shows
how OptIn has finished.

BUG=706564
TEST=Various modes for managed/unmanaged users. chrome://histograms
     shows expected results.
==========

to

==========
arc: Provide UMA for OptIn flow.

We have set of different UMAs for ARC that we can use to analyze how
OptIn flow passed. However there was no metric that explicitly
defines how OptIn finished. This adds top level metric that shows
how OptIn has finished.

BUG=706564
TEST=Various modes for managed/unmanaged users. chrome://histograms
     shows expected results.

Review-Url: https://codereview.chromium.org/2785813002
Cr-Commit-Position: refs/heads/master@{#460892}
Committed:
https://chromium.googlesource.com/chromium/src/+/8575dd76a6ceb99e0c87738d6ccc...
==========

[commit-bot: I haz the power, 2017-03-30 21:44:13]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/8575dd76a6ceb99e0c87738d6ccc...