Record UMA in ArcNavigationThrottle::OnDisambigDialogClosed

Record UMA in ArcNavigationThrottle::OnDisambigDialogClosed

BUG=623205
Committed: https://crrev.com/69365b5e8b1429e171f0a6fb786b343635db6267
Cr-Commit-Position: refs/heads/master@{#402411}

[Yusuke Sato, 2016-06-24 22:10:00]

yusukes@chromium.org changed reviewers:
+ cnwan@chromium.org, elijahtaylor@chromium.org

[Yusuke Sato, 2016-06-24 22:10:01]

cnwan, elijahtaylor: PTAL

[Chi-Ngai Wan, 2016-06-27 07:29:44]

lgtm

[Yusuke Sato, 2016-06-27 15:48:50]

yusukes@chromium.org changed reviewers:
+ lhchavez@chromium.org

[Yusuke Sato, 2016-06-27 15:48:50]

+Luis for c/b/chromeos/arc/
PTAL

[Luis Héctor Chávez, 2016-06-27 16:01:11]

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

https://codereview.chromium.org/2092253002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_navigation_throttle.cc:175: // fall through
nit: Can you run git cl format? I suspect that this should be aligned with the
line above (which also matches other "fall through" comments in
chrome/browser/chromeos)

https://codereview.chromium.org/2092253002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_navigation_throttle.cc:192: // the navigation in
Chrome. Otherwise store the preferred app (if any)
The "Otherwise ..." part of this comment does not apply anymore since that
appears before in the code. Please remove it or move it to where the code for
that part is.

https://codereview.chromium.org/2092253002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_navigation_throttle.cc:197: case
CloseReason::SIZE:
nit: default instead of an explicit case.

[Yusuke Sato, 2016-06-27 16:19:33]

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

https://codereview.chromium.org/2092253002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_navigation_throttle.cc:175: // fall through
On 2016/06/27 16:01:11, Luis Héctor Chávez wrote:
> nit: Can you run git cl format? I suspect that this should be aligned with the
> line above (which also matches other "fall through" comments in
> chrome/browser/chromeos)

git cl format did this actually.. and I don't like the formatting either. Added
{} to force the indent level.

https://codereview.chromium.org/2092253002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_navigation_throttle.cc:192: // the navigation in
Chrome. Otherwise store the preferred app (if any)
On 2016/06/27 16:01:11, Luis Héctor Chávez wrote:
> The "Otherwise ..." part of this comment does not apply anymore since that
> appears before in the code. Please remove it or move it to where the code for
> that part is.

Good catch, removed. The comment was added when |close_reason| was a boolean.
Now that it is an enum, the comment is probably redundant. Done.

https://codereview.chromium.org/2092253002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_navigation_throttle.cc:197: case
CloseReason::SIZE:
On 2016/06/27 16:01:11, Luis Héctor Chávez wrote:
> nit: default instead of an explicit case.

I tend to avoid having default: when using switch-case for enums because clang
automatically detects and warns missing case:s when new values are added to the
enum class. Using default: disables the compile time check which seems to be
more error prone. WDYT?

[Yusuke Sato, 2016-06-27 16:50:38]

yusukes@chromium.org changed reviewers:
+ isherman@chromium.org

[Yusuke Sato, 2016-06-27 16:50:38]

+isherman Please take a look at tools/metrics/histograms/histograms.xml.

[Luis Héctor Chávez, 2016-06-27 17:49:52]

c/b/c/arc lgtm

[Ilya Sherman, 2016-06-28 03:17:16]

https://codereview.chromium.org/2092253002/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/arc/arc_navigation_throttle.h (right):

https://codereview.chromium.org/2092253002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_navigation_throttle.h:30: // and need to be
synced with histograms.xml.
nit: Please also document that this enum should be treated as append-only.

https://codereview.chromium.org/2092253002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_navigation_throttle.h:36: REASON_ERROR = 4,
Optional nit: I'd omit the "REASON_" prefix since this is an enum class.

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

https://codereview.chromium.org/2092253002/diff/40001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:65221: +  <int value="4" label="Error"/>
nit: I'd recommend sorting "Error" to be the first item in the enum.  That way,
if you ever add items, it won't end up somewhere oddly in the middle.

[Yusuke Sato, 2016-06-28 05:04:23]

Thanks, please take another look.

https://codereview.chromium.org/2092253002/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/arc/arc_navigation_throttle.h (right):

https://codereview.chromium.org/2092253002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_navigation_throttle.h:30: // and need to be
synced with histograms.xml.
On 2016/06/28 03:17:15, Ilya Sherman (Away June 18-26) wrote:
> nit: Please also document that this enum should be treated as append-only.

Done.

https://codereview.chromium.org/2092253002/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/arc_navigation_throttle.h:36: REASON_ERROR = 4,
On 2016/06/28 03:17:15, Ilya Sherman (Away June 18-26) wrote:
> Optional nit: I'd omit the "REASON_" prefix since this is an enum class.

Done.

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

https://codereview.chromium.org/2092253002/diff/40001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:65221: +  <int value="4" label="Error"/>
On 2016/06/28 03:17:16, Ilya Sherman (Away June 18-26) wrote:
> nit: I'd recommend sorting "Error" to be the first item in the enum.  That
way,
> if you ever add items, it won't end up somewhere oddly in the middle.

Good idea. Done.

[Ilya Sherman, 2016-06-28 05:35:00]

LGTM, thanks

[Yusuke Sato, 2016-06-28 05:37:10]

The CQ bit was checked by yusukes@chromium.org

[Yusuke Sato, 2016-06-28 05:37:11]

The patchset sent to the CQ was uploaded after l-g-t-m from cnwan@chromium.org,
lhchavez@chromium.org
Link to the patchset: https://codereview.chromium.org/2092253002/#ps60001
(title: "address comments")

[commit-bot: I haz the power, 2016-06-28 05:37:25]

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, 2016-06-28 05:44:09]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-06-28 05:47:24]

Description was changed from

==========
Record UMA in ArcNavigationThrottle::OnDisambigDialogClosed

BUG=623205
==========

to

==========
Record UMA in ArcNavigationThrottle::OnDisambigDialogClosed

BUG=623205

Committed: https://crrev.com/69365b5e8b1429e171f0a6fb786b343635db6267
Cr-Commit-Position: refs/heads/master@{#402411}
==========

[commit-bot: I haz the power, 2016-06-28 05:47:25]

Patchset 4 (id:??) landed as
https://crrev.com/69365b5e8b1429e171f0a6fb786b343635db6267
Cr-Commit-Position: refs/heads/master@{#402411}