Added Ash.SystemMenu.DefaultView.VisibleItems histogram.

Added Ash.SystemMenu.DefaultView.VisibleRows histogram.

Added metrics to track which system menu rows are visible in order to
make informed decisions about feature usage.

BUG=620509
TEST=SystemTrayTest.*

Committed: https://crrev.com/5c85316c5787d77319c5c4fd15a1cb59dbee1fb0
Cr-Commit-Position: refs/heads/master@{#407562}

[bruthig, 2016-07-19 20:02:01]

The CQ bit was checked by bruthig@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-19 20:02:56]

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

[bruthig, 2016-07-19 20:03:00]

bruthig@chromium.org changed reviewers:
+ tdanderson@chromium.org

[bruthig, 2016-07-19 20:03:01]

Terry, can you take a first look?

[bruthig, 2016-07-19 20:14:29]

The CQ bit was unchecked by bruthig@chromium.org

[bruthig, 2016-07-19 20:15:08]

The CQ bit was checked by bruthig@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-19 20:16:12]

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

[tdanderson, 2016-07-19 21:58:40]

LGTM with comments below.

https://chromiumcodereview.appspot.com/2162153002/diff/20001/ash/common/syste...
File ash/common/system/chromeos/settings/tray_settings.cc (right):

https://chromiumcodereview.appspot.com/2162153002/diff/20001/ash/common/syste...
ash/common/system/chromeos/settings/tray_settings.cc:129: :
SystemTrayItem(system_tray, OS_SETTINGS), default_view_(NULL) {}
Just SETTINGS instead of OS_SETTINGS to match class name and remain consistent
with your naming of other enum values?

https://chromiumcodereview.appspot.com/2162153002/diff/20001/ash/common/syste...
File ash/common/system/date/tray_date.cc (right):

https://chromiumcodereview.appspot.com/2162153002/diff/20001/ash/common/syste...
ash/common/system/date/tray_date.cc:22: : SystemTrayItem(system_tray,
DATE_POWER_LOCK),
Just DATE?

https://chromiumcodereview.appspot.com/2162153002/diff/20001/ash/common/syste...
File ash/common/system/tray/system_tray_bubble.cc (right):

https://chromiumcodereview.appspot.com/2162153002/diff/20001/ash/common/syste...
ash/common/system/tray/system_tray_bubble.cc:283: DCHECK(false) << "Only the
BUBBLE_TYPE_DEFAULT BubbleType is supported.";
Not sure if we need a DCHECK or a message here.

https://chromiumcodereview.appspot.com/2162153002/diff/20001/ash/common/syste...
ash/common/system/tray/system_tray_bubble.cc:309: views::View* item_view = NULL;
nit: nullptr here and line 306

https://chromiumcodereview.appspot.com/2162153002/diff/20001/ash/common/syste...
ash/common/system/tray/system_tray_bubble.cc:326: 
nit: omit blank lines at 326 and 328

https://chromiumcodereview.appspot.com/2162153002/diff/20001/ash/common/syste...
File ash/common/system/tray/system_tray_bubble.h (right):

https://chromiumcodereview.appspot.com/2162153002/diff/20001/ash/common/syste...
ash/common/system/tray/system_tray_bubble.h:76: // Tracks the View's created in
the last call to CreateItemViews().
View's->views

https://chromiumcodereview.appspot.com/2162153002/diff/20001/ash/common/syste...
File ash/common/system/tray/system_tray_item.h (right):

https://chromiumcodereview.appspot.com/2162153002/diff/20001/ash/common/syste...
ash/common/system/tray/system_tray_item.h:27: // this enum and only insert items
before the COUNT item.
nit: awkward line break.

https://chromiumcodereview.appspot.com/2162153002/diff/20001/ash/common/syste...
ash/common/system/tray/system_tray_item.h:29: ACCESSIBILITY,         // 0
Consider making UNKNOWN/NONE the 0-element, and use that in unittests rather
than NETWORK.

https://chromiumcodereview.appspot.com/2162153002/diff/20001/ash/common/syste...
ash/common/system/tray/system_tray_item.h:60: ItemType type() const { return
item_type_; }
I'm a bit concerned that having a public type() in SystemTrayItem may lead to
future misuse (e.g., someone calling type() and performing a static_cast when
inappropriate). At the very least maybe we want to call this something like
type_for_uma(), and perhaps make it private (and friend the caller). WDYT?

https://chromiumcodereview.appspot.com/2162153002/diff/20001/ash/common/syste...
File ash/common/system/tray/tray_empty.h (left):

https://chromiumcodereview.appspot.com/2162153002/diff/20001/ash/common/syste...
ash/common/system/tray/tray_empty.h:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
Good riddance to dead code!

https://chromiumcodereview.appspot.com/2162153002/diff/20001/ash/common/syste...
File ash/common/system/update/tray_update.cc (right):

https://chromiumcodereview.appspot.com/2162153002/diff/20001/ash/common/syste...
ash/common/system/update/tray_update.cc:89: : TrayImageItem(system_tray,
IDR_AURA_UBER_TRAY_UPDATE, OS_UPDATE) {
Just UPDATE?

https://chromiumcodereview.appspot.com/2162153002/diff/20001/tools/metrics/hi...
File tools/metrics/histograms/histograms.xml (right):

https://chromiumcodereview.appspot.com/2162153002/diff/20001/tools/metrics/hi...
tools/metrics/histograms/histograms.xml:1412: +    The visible items displayed
in the system menu's default view. Recorded each
possibly more informative: "visible rows" rather than "visible items" ?

[commit-bot: I haz the power, 2016-07-19 21:59:27]

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

[commit-bot: I haz the power, 2016-07-19 21:59:28]

Dry run: This issue passed the CQ dry run.

[bruthig, 2016-07-20 18:01:17]

The CQ bit was checked by bruthig@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-20 18:02:10]

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

[bruthig, 2016-07-20 18:13:34]

Description was changed from

==========
Added Ash.SystemMenu.DefaultView.VisibleItems histogram.

Added metrics to track which system menu rows are visible in order to
make informed decisions about feature usage.

BUG=620509
TEST=manual
==========

to

==========
Added Ash.SystemMenu.DefaultView.VisibleRows histogram.

Added metrics to track which system menu rows are visible in order to
make informed decisions about feature usage.

BUG=620509
TEST=manual
==========

[bruthig, 2016-07-20 18:15:51]

bruthig@chromium.org changed reviewers:
+ jamescook@chromium.org

[bruthig, 2016-07-20 18:15:52]

jamescook@, can you review changes in:
- ash/*

rkaplow@, can you review changes in:
- histograms.xml
- ash/common/system/tray/system_tray_bubble.cc (histogram usage)

https://codereview.chromium.org/2162153002/diff/20001/ash/common/system/chrom...
File ash/common/system/chromeos/settings/tray_settings.cc (right):

https://codereview.chromium.org/2162153002/diff/20001/ash/common/system/chrom...
ash/common/system/chromeos/settings/tray_settings.cc:129: :
SystemTrayItem(system_tray, OS_SETTINGS), default_view_(NULL) {}
On 2016/07/19 21:58:39, tdanderson wrote:
> Just SETTINGS instead of OS_SETTINGS to match class name and remain consistent
> with your naming of other enum values?

Done.

https://codereview.chromium.org/2162153002/diff/20001/ash/common/system/date/...
File ash/common/system/date/tray_date.cc (right):

https://codereview.chromium.org/2162153002/diff/20001/ash/common/system/date/...
ash/common/system/date/tray_date.cc:22: : SystemTrayItem(system_tray,
DATE_POWER_LOCK),
On 2016/07/19 21:58:39, tdanderson wrote:
> Just DATE?

Done.

https://codereview.chromium.org/2162153002/diff/20001/ash/common/system/tray/...
File ash/common/system/tray/system_tray_bubble.cc (right):

https://codereview.chromium.org/2162153002/diff/20001/ash/common/system/tray/...
ash/common/system/tray/system_tray_bubble.cc:283: DCHECK(false) << "Only the
BUBBLE_TYPE_DEFAULT BubbleType is supported.";
On 2016/07/19 21:58:40, tdanderson wrote:
> Not sure if we need a DCHECK or a message here.

Removed.

https://codereview.chromium.org/2162153002/diff/20001/ash/common/system/tray/...
ash/common/system/tray/system_tray_bubble.cc:309: views::View* item_view = NULL;
On 2016/07/19 21:58:40, tdanderson wrote:
> nit: nullptr here and line 306

Done.

https://codereview.chromium.org/2162153002/diff/20001/ash/common/system/tray/...
ash/common/system/tray/system_tray_bubble.cc:326: 
On 2016/07/19 21:58:39, tdanderson wrote:
> nit: omit blank lines at 326 and 328

Done.

https://codereview.chromium.org/2162153002/diff/20001/ash/common/system/tray/...
File ash/common/system/tray/system_tray_bubble.h (right):

https://codereview.chromium.org/2162153002/diff/20001/ash/common/system/tray/...
ash/common/system/tray/system_tray_bubble.h:76: // Tracks the View's created in
the last call to CreateItemViews().
On 2016/07/19 21:58:40, tdanderson wrote:
> View's->views

Done.

https://codereview.chromium.org/2162153002/diff/20001/ash/common/system/tray/...
File ash/common/system/tray/system_tray_item.h (right):

https://codereview.chromium.org/2162153002/diff/20001/ash/common/system/tray/...
ash/common/system/tray/system_tray_item.h:27: // this enum and only insert items
before the COUNT item.
On 2016/07/19 21:58:40, tdanderson wrote:
> nit: awkward line break.

Done.

https://codereview.chromium.org/2162153002/diff/20001/ash/common/system/tray/...
ash/common/system/tray/system_tray_item.h:29: ACCESSIBILITY,         // 0
On 2016/07/19 21:58:40, tdanderson wrote:
> Consider making UNKNOWN/NONE the 0-element, and use that in unittests rather
> than NETWORK.

Added TEST at 0.

https://codereview.chromium.org/2162153002/diff/20001/ash/common/system/tray/...
ash/common/system/tray/system_tray_item.h:60: ItemType type() const { return
item_type_; }
On 2016/07/19 21:58:40, tdanderson wrote:
> I'm a bit concerned that having a public type() in SystemTrayItem may lead to
> future misuse (e.g., someone calling type() and performing a static_cast when
> inappropriate). At the very least maybe we want to call this something like
> type_for_uma(), and perhaps make it private (and friend the caller). WDYT?

Renamed ItemType as UmaType, moved uma_type() to private visibility and friended
SystemTrayBubble.

https://codereview.chromium.org/2162153002/diff/20001/ash/common/system/updat...
File ash/common/system/update/tray_update.cc (right):

https://codereview.chromium.org/2162153002/diff/20001/ash/common/system/updat...
ash/common/system/update/tray_update.cc:89: : TrayImageItem(system_tray,
IDR_AURA_UBER_TRAY_UPDATE, OS_UPDATE) {
On 2016/07/19 21:58:40, tdanderson wrote:
> Just UPDATE?

Done.

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

https://codereview.chromium.org/2162153002/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:1412: +    The visible items displayed
in the system menu's default view. Recorded each
On 2016/07/19 21:58:40, tdanderson wrote:
> possibly more informative: "visible rows" rather than "visible items" ?

Done.

[James Cook, 2016-07-20 20:13:50]

https://codereview.chromium.org/2162153002/diff/60001/ash/common/system/chrom...
File ash/common/system/chromeos/bluetooth/tray_bluetooth.cc (right):

https://codereview.chromium.org/2162153002/diff/60001/ash/common/system/chrom...
ash/common/system/chromeos/bluetooth/tray_bluetooth.cc:397: :
SystemTrayItem(system_tray, BLUETOOTH), default_(NULL), detailed_(NULL) {
nit: nullptr while you're here

https://codereview.chromium.org/2162153002/diff/60001/ash/common/system/chrom...
File ash/common/system/chromeos/enterprise/tray_enterprise.cc (right):

https://codereview.chromium.org/2162153002/diff/60001/ash/common/system/chrom...
ash/common/system/chromeos/enterprise/tray_enterprise.cc:19: :
SystemTrayItem(system_tray, ENTERPRISE), tray_view_(NULL) {
ditto

https://codereview.chromium.org/2162153002/diff/60001/ash/common/system/chrom...
File ash/common/system/chromeos/network/tray_vpn.cc (right):

https://codereview.chromium.org/2162153002/diff/60001/ash/common/system/chrom...
ash/common/system/chromeos/network/tray_vpn.cc:115: :
SystemTrayItem(system_tray, VPN), default_(NULL), detailed_(NULL) {
Ditto and throughout (if you touch a line that has NULL on it, convert it).
Lines that you don't otherwise touch are up to you.

https://codereview.chromium.org/2162153002/diff/60001/ash/common/system/tray/...
File ash/common/system/tray/system_tray_bubble.cc (right):

https://codereview.chromium.org/2162153002/diff/60001/ash/common/system/tray/...
ash/common/system/tray/system_tray_bubble.cc:283: if (bubble_type_ !=
BUBBLE_TYPE_DEFAULT) {
nit: no {}

https://codereview.chromium.org/2162153002/diff/60001/ash/common/system/tray/...
ash/common/system/tray/system_tray_bubble.cc:309: views::View* item_view =
nullptr;
item_view is a better name, thanks for changing it!

https://codereview.chromium.org/2162153002/diff/60001/ash/common/system/tray/...
ash/common/system/tray/system_tray_bubble.cc:336:
->SetBorder(views::Border::NullBorder());
It seems a bit wasteful to create the last 2 items with borders, then reset
those borders. Is there another way to do it? Just keep the approach in the old
code (and DCHECK that size() >= 2)? Or do you need single-item bubbles for
tests? (If so, comment that.)

https://codereview.chromium.org/2162153002/diff/60001/ash/common/system/tray/...
File ash/common/system/tray/system_tray_item.h (right):

https://codereview.chromium.org/2162153002/diff/60001/ash/common/system/tray/...
ash/common/system/tray/system_tray_item.h:26: // *** IMPORTANT *** - These
values are used for UMA metrics so do NOT
nit: This probably doesn't need to be "**** IMPORTANT". We have this warning
lots of places in the code. "NOTE" or something less dire would be fine. :-)

https://codereview.chromium.org/2162153002/diff/60001/ash/common/system/tray/...
ash/common/system/tray/system_tray_item.h:29: TEST,                  // 0 - Used
for test only.
I would either give these all a prefix (like UMA_TYPE_AUDIO or UMA_AUDIO) or
make this an enum class. I find the call sites to the constructor a little odd
when they pass a bare "AUDIO" param.

https://codereview.chromium.org/2162153002/diff/60001/ash/common/system/tray/...
ash/common/system/tray/system_tray_item.h:30: ACCESSIBILITY,         // 1
nit: Don't explicitly number the enum in comments. I would just put = 0 on the
test item and leave its comment, or put "= <num>" after each item.  We have
precedent for both:

https://cs.chromium.org/chromium/src/extensions/common/constants.h?q=used.*um...

https://cs.chromium.org/chromium/src/chrome/browser/plugins/plugin_finder.cc?...

https://codereview.chromium.org/2162153002/diff/60001/ash/common/system/tray/...
ash/common/system/tray/system_tray_item.h:148: UmaType uma_type() const { return
uma_type_; }
nit: If this is only used in test, eliminate and directly use |uma_type_|

https://codereview.chromium.org/2162153002/diff/60001/ash/common/system/tray/...
File ash/common/system/tray/system_tray_unittest.cc (right):

https://codereview.chromium.org/2162153002/diff/60001/ash/common/system/tray/...
ash/common/system/tray/system_tray_unittest.cc:45: const std::string
kVisibleRowsHistogramName =
I don't think we're allowed to have statically initialized variables with
non-POD-types. I would use const char foo[]

https://codereview.chromium.org/2162153002/diff/60001/ash/common/system/tray/...
ash/common/system/tray/system_tray_unittest.cc:170: }
Thanks for writing a nice test suite for this.

https://codereview.chromium.org/2162153002/diff/60001/ash/common/system/user/...
File ash/common/system/user/tray_user_separator.cc (right):

https://codereview.chromium.org/2162153002/diff/60001/ash/common/system/user/...
ash/common/system/user/tray_user_separator.cc:14: : SystemTrayItem(system_tray,
USER_SEPARATOR), separator_shown_(false) {}
Do we really need the separator count? If not, you could switch TEST to NONE and
use it here.

https://codereview.chromium.org/2162153002/diff/60001/ash/test/test_system_tr...
File ash/test/test_system_tray_item.cc (right):

https://codereview.chromium.org/2162153002/diff/60001/ash/test/test_system_tr...
ash/test/test_system_tray_item.cc:27: tray_view_ = nullptr;
nit: early return

https://codereview.chromium.org/2162153002/diff/60001/ash/test/test_system_tr...
ash/test/test_system_tray_item.cc:40: default_view_ = nullptr;
nit: early return

https://codereview.chromium.org/2162153002/diff/60001/ash/test/test_system_tr...
ash/test/test_system_tray_item.cc:52: detailed_view_ = nullptr;
ditto and below

https://codereview.chromium.org/2162153002/diff/60001/ash/test/test_system_tr...
ash/test/test_system_tray_item.cc:92: }  // namespace ash
Thanks for pulling this out into its own class. Much nicer.

https://codereview.chromium.org/2162153002/diff/60001/ash/test/test_system_tr...
File ash/test/test_system_tray_item.h (right):

https://codereview.chromium.org/2162153002/diff/60001/ash/test/test_system_tr...
ash/test/test_system_tray_item.h:16: explicit TestSystemTrayItem(bool
has_views);
nit: Don't pass |has_views| in the constructor. Make users call set_has_views().
This will make the calling code self-documenting.

https://codereview.chromium.org/2162153002/diff/60001/ash/test/test_system_tr...
ash/test/test_system_tray_item.h:17: 
~TestSystemTrayItem() override

[bruthig, 2016-07-20 21:23:25]

Description was changed from

==========
Added Ash.SystemMenu.DefaultView.VisibleRows histogram.

Added metrics to track which system menu rows are visible in order to
make informed decisions about feature usage.

BUG=620509
TEST=manual
==========

to

==========
Added Ash.SystemMenu.DefaultView.VisibleRows histogram.

Added metrics to track which system menu rows are visible in order to
make informed decisions about feature usage.

BUG=620509
TEST=SystemTrayTest.*
==========

[bruthig, 2016-07-21 14:34:57]

bruthig@chromium.org changed reviewers:
+ rkaplow@chromium.org

[bruthig, 2016-07-21 14:34:59]

jamescook@, can you please take another look?

rkaplow@, can you review changes in:
- histograms.xml
- ash/common/system/tray/system_tray_bubble.cc (histogram usage)

https://codereview.chromium.org/2162153002/diff/60001/ash/common/system/chrom...
File ash/common/system/chromeos/bluetooth/tray_bluetooth.cc (right):

https://codereview.chromium.org/2162153002/diff/60001/ash/common/system/chrom...
ash/common/system/chromeos/bluetooth/tray_bluetooth.cc:397: :
SystemTrayItem(system_tray, BLUETOOTH), default_(NULL), detailed_(NULL) {
On 2016/07/20 20:13:49, James Cook wrote:
> nit: nullptr while you're here

Done.

https://codereview.chromium.org/2162153002/diff/60001/ash/common/system/chrom...
File ash/common/system/chromeos/enterprise/tray_enterprise.cc (right):

https://codereview.chromium.org/2162153002/diff/60001/ash/common/system/chrom...
ash/common/system/chromeos/enterprise/tray_enterprise.cc:19: :
SystemTrayItem(system_tray, ENTERPRISE), tray_view_(NULL) {
On 2016/07/20 20:13:49, James Cook wrote:
> ditto

Done.

https://codereview.chromium.org/2162153002/diff/60001/ash/common/system/chrom...
File ash/common/system/chromeos/network/tray_vpn.cc (right):

https://codereview.chromium.org/2162153002/diff/60001/ash/common/system/chrom...
ash/common/system/chromeos/network/tray_vpn.cc:115: :
SystemTrayItem(system_tray, VPN), default_(NULL), detailed_(NULL) {
On 2016/07/20 20:13:49, James Cook wrote:
> Ditto and throughout (if you touch a line that has NULL on it, convert it).
> Lines that you don't otherwise touch are up to you.

Done.

https://codereview.chromium.org/2162153002/diff/60001/ash/common/system/tray/...
File ash/common/system/tray/system_tray_bubble.cc (right):

https://codereview.chromium.org/2162153002/diff/60001/ash/common/system/tray/...
ash/common/system/tray/system_tray_bubble.cc:283: if (bubble_type_ !=
BUBBLE_TYPE_DEFAULT) {
On 2016/07/20 20:13:49, James Cook wrote:
> nit: no {}

Done.

https://codereview.chromium.org/2162153002/diff/60001/ash/common/system/tray/...
ash/common/system/tray/system_tray_bubble.cc:336:
->SetBorder(views::Border::NullBorder());
On 2016/07/20 20:13:49, James Cook wrote:
> It seems a bit wasteful to create the last 2 items with borders, then reset
> those borders. Is there another way to do it? Just keep the approach in the
old
> code (and DCHECK that size() >= 2)? Or do you need single-item bubbles for
> tests? (If so, comment that.)

Done.

https://codereview.chromium.org/2162153002/diff/60001/ash/common/system/tray/...
File ash/common/system/tray/system_tray_item.h (right):

https://codereview.chromium.org/2162153002/diff/60001/ash/common/system/tray/...
ash/common/system/tray/system_tray_item.h:26: // *** IMPORTANT *** - These
values are used for UMA metrics so do NOT
On 2016/07/20 20:13:49, James Cook wrote:
> nit: This probably doesn't need to be "**** IMPORTANT". We have this warning
> lots of places in the code. "NOTE" or something less dire would be fine. :-)

Done.

https://codereview.chromium.org/2162153002/diff/60001/ash/common/system/tray/...
ash/common/system/tray/system_tray_item.h:29: TEST,                  // 0 - Used
for test only.
On 2016/07/20 20:13:49, James Cook wrote:
> I would either give these all a prefix (like UMA_TYPE_AUDIO or UMA_AUDIO) or
> make this an enum class. I find the call sites to the constructor a little odd
> when they pass a bare "AUDIO" param.

Done.

https://codereview.chromium.org/2162153002/diff/60001/ash/common/system/tray/...
ash/common/system/tray/system_tray_item.h:30: ACCESSIBILITY,         // 1
On 2016/07/20 20:13:49, James Cook wrote:
> nit: Don't explicitly number the enum in comments. I would just put = 0 on the
> test item and leave its comment, or put "= <num>" after each item.  We have
> precedent for both:
> 
>
https://cs.chromium.org/chromium/src/extensions/common/constants.h?q=used.*um...
> 
>
https://cs.chromium.org/chromium/src/chrome/browser/plugins/plugin_finder.cc?...

Done.

https://codereview.chromium.org/2162153002/diff/60001/ash/common/system/tray/...
ash/common/system/tray/system_tray_item.h:148: UmaType uma_type() const { return
uma_type_; }
On 2016/07/20 20:13:49, James Cook wrote:
> nit: If this is only used in test, eliminate and directly use |uma_type_|

This is used by the SystemTrayBubble class in production.

https://codereview.chromium.org/2162153002/diff/60001/ash/common/system/tray/...
File ash/common/system/tray/system_tray_unittest.cc (right):

https://codereview.chromium.org/2162153002/diff/60001/ash/common/system/tray/...
ash/common/system/tray/system_tray_unittest.cc:45: const std::string
kVisibleRowsHistogramName =
On 2016/07/20 20:13:49, James Cook wrote:
> I don't think we're allowed to have statically initialized variables with
> non-POD-types. I would use const char foo[]

Done.

https://codereview.chromium.org/2162153002/diff/60001/ash/common/system/user/...
File ash/common/system/user/tray_user_separator.cc (right):

https://codereview.chromium.org/2162153002/diff/60001/ash/common/system/user/...
ash/common/system/user/tray_user_separator.cc:14: : SystemTrayItem(system_tray,
USER_SEPARATOR), separator_shown_(false) {}
On 2016/07/20 20:13:49, James Cook wrote:
> Do we really need the separator count? If not, you could switch TEST to NONE
and
> use it here.

Added UMA_NOT_RECORDED and kept UMA_TEST because I actually want UMA_TEST to be
recorded to validate the other logic for deciding when to record or not.

https://codereview.chromium.org/2162153002/diff/60001/ash/test/test_system_tr...
File ash/test/test_system_tray_item.cc (right):

https://codereview.chromium.org/2162153002/diff/60001/ash/test/test_system_tr...
ash/test/test_system_tray_item.cc:52: detailed_view_ = nullptr;
On 2016/07/20 20:13:49, James Cook wrote:
> ditto and below

Done.

https://codereview.chromium.org/2162153002/diff/60001/ash/test/test_system_tr...
File ash/test/test_system_tray_item.h (right):

https://codereview.chromium.org/2162153002/diff/60001/ash/test/test_system_tr...
ash/test/test_system_tray_item.h:16: explicit TestSystemTrayItem(bool
has_views);
On 2016/07/20 20:13:50, James Cook wrote:
> nit: Don't pass |has_views| in the constructor. Make users call
set_has_views().
> This will make the calling code self-documenting.

Done.

https://codereview.chromium.org/2162153002/diff/60001/ash/test/test_system_tr...
ash/test/test_system_tray_item.h:17: 
On 2016/07/20 20:13:49, James Cook wrote:
> ~TestSystemTrayItem() override

Done.

[bruthig, 2016-07-21 14:35:15]

The CQ bit was checked by bruthig@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-21 14:35:31]

Dry run: 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-07-21 15:27:32]

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

[commit-bot: I haz the power, 2016-07-21 15:27:33]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[James Cook, 2016-07-21 16:14:00]

LGTM with nits

https://codereview.chromium.org/2162153002/diff/100001/ash/common/system/tray...
File ash/common/system/tray/system_tray_bubble.cc (right):

https://codereview.chromium.org/2162153002/diff/100001/ash/common/system/tray...
ash/common/system/tray/system_tray_bubble.cc:336: for (size_t i = 0; i <
item_containers.size() - 2; ++i)
nit: I would either use "int i" and cast the size to int, DCHECK_GE(size, 2), or
both. If it's unsigned and there are less than 2 items I think you get an
underflow and a long loop.

https://codereview.chromium.org/2162153002/diff/100001/ash/test/test_system_t...
File ash/test/test_system_tray_item.cc (right):

https://codereview.chromium.org/2162153002/diff/100001/ash/test/test_system_t...
ash/test/test_system_tray_item.cc:33: } else {
nit: no else due to early return

https://codereview.chromium.org/2162153002/diff/100001/ash/test/test_system_t...
File ash/test/test_system_tray_item.h (right):

https://codereview.chromium.org/2162153002/diff/100001/ash/test/test_system_t...
ash/test/test_system_tray_item.h:20: void set_has_views(bool has_views) {
has_views_ = has_views; }
nit: Document (either by constructor or class comment) that by default it has
views and they are visible.

[bruthig, 2016-07-21 17:24:25]

https://codereview.chromium.org/2162153002/diff/100001/ash/common/system/tray...
File ash/common/system/tray/system_tray_bubble.cc (right):

https://codereview.chromium.org/2162153002/diff/100001/ash/common/system/tray...
ash/common/system/tray/system_tray_bubble.cc:336: for (size_t i = 0; i <
item_containers.size() - 2; ++i)
On 2016/07/21 16:14:00, James Cook wrote:
> nit: I would either use "int i" and cast the size to int, DCHECK_GE(size, 2),
or
> both. If it's unsigned and there are less than 2 items I think you get an
> underflow and a long loop.

I'm not sure if you caught it first or the win bots ;)

WDYT of this change?

https://codereview.chromium.org/2162153002/diff/100001/ash/test/test_system_t...
File ash/test/test_system_tray_item.cc (right):

https://codereview.chromium.org/2162153002/diff/100001/ash/test/test_system_t...
ash/test/test_system_tray_item.cc:33: } else {
On 2016/07/21 16:14:00, James Cook wrote:
> nit: no else due to early return

Done.

https://codereview.chromium.org/2162153002/diff/100001/ash/test/test_system_t...
File ash/test/test_system_tray_item.h (right):

https://codereview.chromium.org/2162153002/diff/100001/ash/test/test_system_t...
ash/test/test_system_tray_item.h:20: void set_has_views(bool has_views) {
has_views_ = has_views; }
On 2016/07/21 16:14:00, James Cook wrote:
> nit: Document (either by constructor or class comment) that by default it has
> views and they are visible.

Done.

[James Cook, 2016-07-21 17:35:16]

still lgtm

https://codereview.chromium.org/2162153002/diff/140001/ash/common/system/tray...
File ash/common/system/tray/system_tray_bubble.cc (right):

https://codereview.chromium.org/2162153002/diff/140001/ash/common/system/tray...
ash/common/system/tray/system_tray_bubble.cc:336: for (size_t i = 0; i + 2 <
item_containers.size(); ++i)
Hrm. int last_item_with_border = static_cast<int>(size) - 2; then loop with an
int?

If that causes problems, this is OK, but I would put parentheses around (i + 2).

[rkaplow, 2016-07-21 18:34:25]

https://codereview.chromium.org/2162153002/diff/140001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2162153002/diff/140001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:1408: +<histogram
name="Ash.SystemMenu.DefaultView.VisibleRows" units="rows">
since this is an enum histogram, you should create an associated enum here so
you have the correct mapping

[bruthig, 2016-07-21 19:03:09]

The CQ bit was checked by bruthig@chromium.org to run a CQ dry run

[bruthig, 2016-07-21 19:03:22]

rkaplow@, can you PTAL?

https://codereview.chromium.org/2162153002/diff/140001/ash/common/system/tray...
File ash/common/system/tray/system_tray_bubble.cc (right):

https://codereview.chromium.org/2162153002/diff/140001/ash/common/system/tray...
ash/common/system/tray/system_tray_bubble.cc:336: for (size_t i = 0; i + 2 <
item_containers.size(); ++i)
On 2016/07/21 17:35:16, James Cook wrote:
> Hrm. int last_item_with_border = static_cast<int>(size) - 2; then loop with an
> int?
> 
> If that causes problems, this is OK, but I would put parentheses around (i +
2).

Done.

https://codereview.chromium.org/2162153002/diff/140001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2162153002/diff/140001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:1408: +<histogram
name="Ash.SystemMenu.DefaultView.VisibleRows" units="rows">
On 2016/07/21 18:34:25, rkaplow wrote:
> since this is an enum histogram, you should create an associated enum here so
> you have the correct mapping

Done.

[commit-bot: I haz the power, 2016-07-21 19:03:35]

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

[James Cook, 2016-07-21 19:04:50]

https://codereview.chromium.org/2162153002/diff/160001/ash/common/system/tray...
File ash/common/system/tray/system_tray_bubble.cc (right):

https://codereview.chromium.org/2162153002/diff/160001/ash/common/system/tray...
ash/common/system/tray/system_tray_bubble.cc:336: const int
last_item_with_border = static_cast<int>(item_containers.size());
Shouldn't there be a -2 in here somewhere?

[bruthig, 2016-07-21 19:15:09]

https://codereview.chromium.org/2162153002/diff/160001/ash/common/system/tray...
File ash/common/system/tray/system_tray_bubble.cc (right):

https://codereview.chromium.org/2162153002/diff/160001/ash/common/system/tray...
ash/common/system/tray/system_tray_bubble.cc:336: const int
last_item_with_border = static_cast<int>(item_containers.size());
On 2016/07/21 19:04:50, James Cook wrote:
> Shouldn't there be a -2 in here somewhere?

Whoops :(  I guess it's time for some caffeine...

[James Cook, 2016-07-21 21:06:29]

still lgtm

[rkaplow, 2016-07-25 17:55:24]

lgtm

https://codereview.chromium.org/2162153002/diff/180001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2162153002/diff/180001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:92730: +  <int value="26" label="COUNT -
Should not be recorded"/>
i don't think you really need to put the 26 label

[bruthig, 2016-07-25 18:02:47]

https://codereview.chromium.org/2162153002/diff/180001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2162153002/diff/180001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:92730: +  <int value="26" label="COUNT -
Should not be recorded"/>
On 2016/07/25 17:55:24, rkaplow wrote:
> i don't think you really need to put the 26 label

Removed.

[bruthig, 2016-07-25 18:02:55]

The CQ bit was checked by bruthig@chromium.org

[bruthig, 2016-07-25 18:02:56]

The patchset sent to the CQ was uploaded after l-g-t-m from
tdanderson@chromium.org, jamescook@chromium.org, rkaplow@chromium.org
Link to the patchset: https://codereview.chromium.org/2162153002/#ps200001
(title: "Removed the COUNT value from the SystemMenuDefaultViewRows enum.")

[commit-bot: I haz the power, 2016-07-25 18:03:16]

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-07-25 19:14:21]

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

[commit-bot: I haz the power, 2016-07-25 19:14:23]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[bruthig, 2016-07-25 19:23:13]

The CQ bit was checked by bruthig@chromium.org

[commit-bot: I haz the power, 2016-07-25 19:23:36]

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-07-25 20:23:41]

Description was changed from

==========
Added Ash.SystemMenu.DefaultView.VisibleRows histogram.

Added metrics to track which system menu rows are visible in order to
make informed decisions about feature usage.

BUG=620509
TEST=SystemTrayTest.*
==========

to

==========
Added Ash.SystemMenu.DefaultView.VisibleRows histogram.

Added metrics to track which system menu rows are visible in order to
make informed decisions about feature usage.

BUG=620509
TEST=SystemTrayTest.*
==========

[commit-bot: I haz the power, 2016-07-25 20:23:43]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2016-07-25 20:26:13]

Description was changed from

==========
Added Ash.SystemMenu.DefaultView.VisibleRows histogram.

Added metrics to track which system menu rows are visible in order to
make informed decisions about feature usage.

BUG=620509
TEST=SystemTrayTest.*
==========

to

==========
Added Ash.SystemMenu.DefaultView.VisibleRows histogram.

Added metrics to track which system menu rows are visible in order to
make informed decisions about feature usage.

BUG=620509
TEST=SystemTrayTest.*

Committed: https://crrev.com/5c85316c5787d77319c5c4fd15a1cb59dbee1fb0
Cr-Commit-Position: refs/heads/master@{#407562}
==========

[commit-bot: I haz the power, 2016-07-25 20:26:15]

Patchset 11 (id:??) landed as
https://crrev.com/5c85316c5787d77319c5c4fd15a1cb59dbee1fb0
Cr-Commit-Position: refs/heads/master@{#407562}