Refactor AddScrollListItem() in system menu detailed views

Refactor AddScrollListItem() in system menu detailed views

Most of system menu detailed views have a scrollable list of items with
very similar layouts. This CL adds a few functions in TrayDetailsView to
create such items.

Among different detailed views, network and VPN are not using these
common functions, yet, as they need to be cleaned up first. That will be
done in follow-up CLs.

BUG=686343
TEST=TrayDetailsViewTest.ScrollContentsTest and
     *TrayAccessibilityTest.CheckMarksOnDetailMenu* in ash_unittests

Review-Url: https://codereview.chromium.org/2831023003
Cr-Commit-Position: refs/heads/master@{#467147}
Committed: https://chromium.googlesource.com/chromium/src/+/0b6712fcc4a9d0cd07624db4b3fac5c67edb2bac

[mohsen, 2017-04-20 19:36:34]

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

[commit-bot: I haz the power, 2017-04-20 19:37:10]

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, 2017-04-20 21:00:10]

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

[commit-bot: I haz the power, 2017-04-20 21:00:11]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[mohsen, 2017-04-21 02:20:20]

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

[commit-bot: I haz the power, 2017-04-21 02:20:46]

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, 2017-04-21 02:58:20]

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

[commit-bot: I haz the power, 2017-04-21 02:58:21]

Dry run: This issue passed the CQ dry run.

[mohsen, 2017-04-21 03:53:02]

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

[commit-bot: I haz the power, 2017-04-21 03:53:39]

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

[mohsen, 2017-04-21 04:23:05]

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

[commit-bot: I haz the power, 2017-04-21 04:23:15]

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

[mohsen, 2017-04-21 04:23:40]

Description was changed from

==========
Refactor AddScrollListItem() in system menu detailed views

Most of system menu detailed views have a scrollable list of items with
very similar layouts. This CL adds a few functions in TrayDetailsView to
create such items.

Among different detailed views, network and VPN are not using these
common functions, yet, as they need to be cleaned up first. That will be
done in follow-up CLs.

BUG=686343
TEST=TrayDetailsViewTest.ScrollContentsTest in ash_unittests
==========

to

==========
Refactor AddScrollListItem() in system menu detailed views

Most of system menu detailed views have a scrollable list of items with
very similar layouts. This CL adds a few functions in TrayDetailsView to
create such items.

Among different detailed views, network and VPN are not using these
common functions, yet, as they need to be cleaned up first. That will be
done in follow-up CLs.

BUG=686343
TEST=TrayDetailsViewTest.ScrollContentsTest and
     *TrayAccessibilityTest.CheckMarksOnDetailMenu* in ash_unittests
==========

[mohsen, 2017-04-21 04:24:25]

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

[mohsen, 2017-04-21 04:24:26]

Please take a look...

[commit-bot: I haz the power, 2017-04-21 04:51:29]

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

[commit-bot: I haz the power, 2017-04-21 04:51:31]

Dry run: This issue passed the CQ dry run.

[tdanderson, 2017-04-24 15:43:40]

Hi Mohsen, thanks - I'm glad this code is being refactored! I have left a few
questions and comments below.

https://codereview.chromium.org/2831023003/diff/60001/ash/system/audio/audio_...
File ash/system/audio/audio_detailed_view.cc (left):

https://codereview.chromium.org/2831023003/diff/60001/ash/system/audio/audio_...
ash/system/audio/audio_detailed_view.cc:88: TriView* header =
TrayPopupUtils::CreateDefaultRowView();
Just wanted to double-check that this works as intended, since this function
calls CreateDefaultRowView() but its replacement (AddScrollListSubHeader()) is
calling CreateSubHeaderRowView().

https://codereview.chromium.org/2831023003/diff/60001/ash/system/audio/audio_...
File ash/system/audio/audio_detailed_view.cc (right):

https://codereview.chromium.org/2831023003/diff/60001/ash/system/audio/audio_...
ash/system/audio/audio_detailed_view.cc:77: TriView* header =
AddScrollListSubHeader(icon, text);
Consider passing in the text_id to AddScrollListSubHeader instead of the string
itself, which will eliminate the need for each call site to call
l10_util::GetStringUTF16(). This assumes that all of our subheaders have (and
will have) a pre-defined string id, which is probably the case. WDYT?

https://codereview.chromium.org/2831023003/diff/60001/ash/system/bluetooth/tr...
File ash/system/bluetooth/tray_bluetooth.cc (left):

https://codereview.chromium.org/2831023003/diff/60001/ash/system/bluetooth/tr...
ash/system/bluetooth/tray_bluetooth.cc:402: if (display_name)
I am slightly uneasy about removing this logic along with your changes to
UpdateClickedDevice(). Is it at all possible for a device's display name or type
to change after having been added to the list? If so then I think you may need
to keep some of this extra logic here.

Consider removing these changes to a small follow-up CL and have fukino@
(Bluetooth owner) take a look.

https://codereview.chromium.org/2831023003/diff/60001/ash/system/bluetooth/tr...
File ash/system/bluetooth/tray_bluetooth.cc (right):

https://codereview.chromium.org/2831023003/diff/60001/ash/system/bluetooth/tr...
ash/system/bluetooth/tray_bluetooth.cc:122: const base::string16 label =
l10n_util::GetStringUTF16(
Thanks for the extra little fixes such as this throughout the CL

https://codereview.chromium.org/2831023003/diff/60001/ash/system/tray/hover_h...
File ash/system/tray/hover_highlight_view.cc (right):

https://codereview.chromium.org/2831023003/diff/60001/ash/system/tray/hover_h...
ash/system/tray/hover_highlight_view.cc:58: void
HoverHighlightView::SetSubText(const base::string16& sub_text) {
As far as I can tell, right now there are no places where SetSubText() is being
called with an empty label. If so, I suggest removing that functionality from
SetSubText() for this CL (which means you'd also remove the
TriView::RemoveView() function in this CL too). Instead introduce the new
functionality in a subsequent CL when it is in use.

https://codereview.chromium.org/2831023003/diff/60001/ash/system/tray/hover_h...
File ash/system/tray/hover_highlight_view.h (right):

https://codereview.chromium.org/2831023003/diff/60001/ash/system/tray/hover_h...
ash/system/tray/hover_highlight_view.h:72: // main label should be present.
Empty string can be used to clear the sub
nit: instead say something like "|text_label_| is non-null / nonempty" (call out
the variable by name) instead of "the main label should be present"

https://codereview.chromium.org/2831023003/diff/60001/ash/system/tray/tray_de...
File ash/system/tray/tray_details_view.h (right):

https://codereview.chromium.org/2831023003/diff/60001/ash/system/tray/tray_de...
ash/system/tray/tray_details_view.h:73: // Adds an item to the scroll list.
|gfx::kNoneIcon| can be used if there is
nit: consider a rewording of first sentence to give a bit more information such
as "Creates a targetable row containing |icon| and |text| and adds it as a child
view of |scroll_content_|" or similar. Ditto for the other docs here.

https://codereview.chromium.org/2831023003/diff/60001/ash/system/tray/tray_de...
ash/system/tray/tray_details_view.h:81: bool checked);
nit: newline

https://codereview.chromium.org/2831023003/diff/60001/ash/system/tray/tray_de...
ash/system/tray/tray_details_view.h:84: TriView* AddScrollListSubHeader(const
gfx::VectorIcon& icon,
To simplify the call sites (not requiring kNoneIcon to be passed in for certain
cases), consider having both AddScrollListSubHeader(icon) and
AddScrollListSubHeader(icon,text) protected methods here. Ditto for
AddScrollListItem().

https://codereview.chromium.org/2831023003/diff/60001/ash/system/tray/tray_po...
File ash/system/tray/tray_popup_utils.cc (right):

https://codereview.chromium.org/2831023003/diff/60001/ash/system/tray/tray_po...
ash/system/tray/tray_popup_utils.cc:177: TriView*
TrayPopupUtils::CreateSubHeaderRowView(bool hide_start) {
I see you have made mention of this in your CL description, but maybe it is
worth adding a TODO for anyone who stumbles across this, along the lines of "Do
not call this directly, use TrayDetailsView::AddScrollListSubHeader() instead.
TODO(mohsen): Move this function out of TrayPopupUtils and into TrayDetailsView
once network and VPN use TrayDetailsView::AddScrollListSubHeader()" or similar.

https://codereview.chromium.org/2831023003/diff/60001/ash/system/tray/tray_po...
File ash/system/tray/tray_popup_utils.h (right):

https://codereview.chromium.org/2831023003/diff/60001/ash/system/tray/tray_po...
ash/system/tray/tray_popup_utils.h:60: // not visible, padding would be
increased on that side.
I'm not sure about the last sentence here. If START is not visible, don't you
have CENTER beginning left-aligned in the container?

https://codereview.chromium.org/2831023003/diff/60001/ash/system/tray/tray_po...
ash/system/tray/tray_popup_utils.h:65: static TriView*
CreateSubHeaderRowView(bool hide_start);
optional: I don't think this is called out explicitly in the style guide, and
maybe this is purely personal preference, but I feel that naming variables in a
'positive' rather than a 'negative' way usually makes things more readable. So
here, consider calling the parameter something like |start_visible|, and
swapping all call sites from true to false and vice-versa.

https://codereview.chromium.org/2831023003/diff/60001/ash/system/tray_accessi...
File ash/system/tray_accessibility.h (left):

https://codereview.chromium.org/2831023003/diff/60001/ash/system/tray_accessi...
ash/system/tray_accessibility.h:64: public ShellObserver {
nice catch

[mohsen, 2017-04-25 04:11:38]

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

[commit-bot: I haz the power, 2017-04-25 04:12:08]

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, 2017-04-25 04:15:49]

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

[commit-bot: I haz the power, 2017-04-25 04:15:50]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[mohsen, 2017-04-25 04:36:04]

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

[commit-bot: I haz the power, 2017-04-25 04:36:17]

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, 2017-04-25 05:05:05]

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

[commit-bot: I haz the power, 2017-04-25 05:05:06]

Dry run: This issue passed the CQ dry run.

[mohsen, 2017-04-25 05:10:02]

https://codereview.chromium.org/2831023003/diff/60001/ash/system/audio/audio_...
File ash/system/audio/audio_detailed_view.cc (left):

https://codereview.chromium.org/2831023003/diff/60001/ash/system/audio/audio_...
ash/system/audio/audio_detailed_view.cc:88: TriView* header =
TrayPopupUtils::CreateDefaultRowView();
On 2017/04/24 at 15:43:39, tdanderson wrote:
> Just wanted to double-check that this works as intended, since this function
calls CreateDefaultRowView() but its replacement (AddScrollListSubHeader()) is
calling CreateSubHeaderRowView().

Yes, different layout managers are installed with different paddings. However,
they produce very similar output for our purposes. I talked with bruthig@ and
decided that CreateSubHeaderRowView() should actually use CreateDefaultRoView()
instead of directly using CreateMultiTargetRowView() so that similar padding and
layout management is used for sub-header rows, too. Done. WDYT?

https://codereview.chromium.org/2831023003/diff/60001/ash/system/audio/audio_...
File ash/system/audio/audio_detailed_view.cc (right):

https://codereview.chromium.org/2831023003/diff/60001/ash/system/audio/audio_...
ash/system/audio/audio_detailed_view.cc:77: TriView* header =
AddScrollListSubHeader(icon, text);
On 2017/04/24 at 15:43:39, tdanderson wrote:
> Consider passing in the text_id to AddScrollListSubHeader instead of the
string itself, which will eliminate the need for each call site to call
l10_util::GetStringUTF16(). This assumes that all of our subheaders have (and
will have) a pre-defined string id, which is probably the case. WDYT?

Done. I wanted AddScrollListSubHeader() to be more consistent with
AddScrollListItem() which cannot take a resource id. But, your suggestion also
makes sense and I don't have any preference.

https://codereview.chromium.org/2831023003/diff/60001/ash/system/bluetooth/tr...
File ash/system/bluetooth/tray_bluetooth.cc (left):

https://codereview.chromium.org/2831023003/diff/60001/ash/system/bluetooth/tr...
ash/system/bluetooth/tray_bluetooth.cc:402: if (display_name)
On 2017/04/24 at 15:43:39, tdanderson wrote:
> I am slightly uneasy about removing this logic along with your changes to
UpdateClickedDevice(). Is it at all possible for a device's display name or type
to change after having been added to the list? If so then I think you may need
to keep some of this extra logic here.
> 
> Consider removing these changes to a small follow-up CL and have fukino@
(Bluetooth owner) take a look.

If a device name is changed, it would be updated in
BluetoothDetailedView::Update(). UpdateclickedDevice() is called when an item is
clicked and is only responsible for updating connecting/connected messages. I
think the only reason it also updates name and icon is that it uses
SetupConnectingItem() which sets up the whole row from scratch. Does this make
sense? If not, I can exclude changes in tray_bluetooth from this Cl as you
suggested.

https://codereview.chromium.org/2831023003/diff/60001/ash/system/tray/hover_h...
File ash/system/tray/hover_highlight_view.cc (right):

https://codereview.chromium.org/2831023003/diff/60001/ash/system/tray/hover_h...
ash/system/tray/hover_highlight_view.cc:58: void
HoverHighlightView::SetSubText(const base::string16& sub_text) {
On 2017/04/24 at 15:43:39, tdanderson wrote:
> As far as I can tell, right now there are no places where SetSubText() is
being called with an empty label. If so, I suggest removing that functionality
from SetSubText() for this CL (which means you'd also remove the
TriView::RemoveView() function in this CL too). Instead introduce the new
functionality in a subsequent CL when it is in use.

Done.

https://codereview.chromium.org/2831023003/diff/60001/ash/system/tray/hover_h...
File ash/system/tray/hover_highlight_view.h (right):

https://codereview.chromium.org/2831023003/diff/60001/ash/system/tray/hover_h...
ash/system/tray/hover_highlight_view.h:72: // main label should be present.
Empty string can be used to clear the sub
On 2017/04/24 at 15:43:39, tdanderson wrote:
> nit: instead say something like "|text_label_| is non-null / nonempty" (call
out the variable by name) instead of "the main label should be present"

Done.

https://codereview.chromium.org/2831023003/diff/60001/ash/system/tray/tray_de...
File ash/system/tray/tray_details_view.h (right):

https://codereview.chromium.org/2831023003/diff/60001/ash/system/tray/tray_de...
ash/system/tray/tray_details_view.h:73: // Adds an item to the scroll list.
|gfx::kNoneIcon| can be used if there is
On 2017/04/24 at 15:43:39, tdanderson wrote:
> nit: consider a rewording of first sentence to give a bit more information
such as "Creates a targetable row containing |icon| and |text| and adds it as a
child view of |scroll_content_|" or similar. Ditto for the other docs here.

Done.

https://codereview.chromium.org/2831023003/diff/60001/ash/system/tray/tray_de...
ash/system/tray/tray_details_view.h:81: bool checked);
On 2017/04/24 at 15:43:39, tdanderson wrote:
> nit: newline

Done.

https://codereview.chromium.org/2831023003/diff/60001/ash/system/tray/tray_de...
ash/system/tray/tray_details_view.h:84: TriView* AddScrollListSubHeader(const
gfx::VectorIcon& icon,
On 2017/04/24 at 15:43:39, tdanderson wrote:
> To simplify the call sites (not requiring kNoneIcon to be passed in for
certain cases), consider having both AddScrollListSubHeader(icon) and
AddScrollListSubHeader(icon,text) protected methods here. Ditto for
AddScrollListItem().

Done.

https://codereview.chromium.org/2831023003/diff/60001/ash/system/tray/tray_po...
File ash/system/tray/tray_popup_utils.cc (right):

https://codereview.chromium.org/2831023003/diff/60001/ash/system/tray/tray_po...
ash/system/tray/tray_popup_utils.cc:177: TriView*
TrayPopupUtils::CreateSubHeaderRowView(bool hide_start) {
On 2017/04/24 at 15:43:39, tdanderson wrote:
> I see you have made mention of this in your CL description, but maybe it is
worth adding a TODO for anyone who stumbles across this, along the lines of "Do
not call this directly, use TrayDetailsView::AddScrollListSubHeader() instead.
TODO(mohsen): Move this function out of TrayPopupUtils and into TrayDetailsView
once network and VPN use TrayDetailsView::AddScrollListSubHeader()" or similar.

Well, I was not thinking of deleting this. Thought it might still be useful for
places that want to create a sub-header row with similar layout in classes that
are not necessarily inheriting TrayDetailsView. But, probably the only usage for
this function would be in detailed views. So, done!

https://codereview.chromium.org/2831023003/diff/60001/ash/system/tray/tray_po...
File ash/system/tray/tray_popup_utils.h (right):

https://codereview.chromium.org/2831023003/diff/60001/ash/system/tray/tray_po...
ash/system/tray/tray_popup_utils.h:60: // not visible, padding would be
increased on that side.
On 2017/04/24 at 15:43:39, tdanderson wrote:
> I'm not sure about the last sentence here. If START is not visible, don't you
have CENTER beginning left-aligned in the container?

What I meant is that when START is not visible, we add extra padding to the left
of the container. Modified the comment to hopefully make it clearer.

https://codereview.chromium.org/2831023003/diff/60001/ash/system/tray/tray_po...
ash/system/tray/tray_popup_utils.h:65: static TriView*
CreateSubHeaderRowView(bool hide_start);
On 2017/04/24 at 15:43:39, tdanderson wrote:
> optional: I don't think this is called out explicitly in the style guide, and
maybe this is purely personal preference, but I feel that naming variables in a
'positive' rather than a 'negative' way usually makes things more readable. So
here, consider calling the parameter something like |start_visible|, and
swapping all call sites from true to false and vice-versa.

Makes sense. Done.

[tdanderson, 2017-04-25 14:32:48]

https://codereview.chromium.org/2831023003/diff/60001/ash/system/audio/audio_...
File ash/system/audio/audio_detailed_view.cc (left):

https://codereview.chromium.org/2831023003/diff/60001/ash/system/audio/audio_...
ash/system/audio/audio_detailed_view.cc:88: TriView* header =
TrayPopupUtils::CreateDefaultRowView();
On 2017/04/25 05:10:01, mohsen wrote:
> On 2017/04/24 at 15:43:39, tdanderson wrote:
> > Just wanted to double-check that this works as intended, since this function
> calls CreateDefaultRowView() but its replacement (AddScrollListSubHeader()) is
> calling CreateSubHeaderRowView().
> 
> Yes, different layout managers are installed with different paddings. However,
> they produce very similar output for our purposes. 

By 'very similar' do you mean that the implementation in Patch Set 4 will give
the incorrect layout for the audio subheader rows?

>I talked with bruthig@ and
> decided that CreateSubHeaderRowView() should actually use
CreateDefaultRoView()
> instead of directly using CreateMultiTargetRowView() so that similar padding
and
> layout management is used for sub-header rows, too. Done. WDYT?

Unless the code in patch set 4 actually gives the incorrect layout, I would
prefer for the time being to avoid calling CreateDefaultRowView() directly from
CreateSubHeaderRowView() like you have in patch set 6. If there is an
opportunity to share code between the two then by all means create a private
helper function, but I see it as a bit confusing to call a function designed for
use by default views directly from a function designed for use by detailed
views.

I am also happy to table this discussion for a follow-on CL, provided that
whatever you're landing in this CL will still follow design specs.

https://codereview.chromium.org/2831023003/diff/60001/ash/system/audio/audio_...
File ash/system/audio/audio_detailed_view.cc (right):

https://codereview.chromium.org/2831023003/diff/60001/ash/system/audio/audio_...
ash/system/audio/audio_detailed_view.cc:77: TriView* header =
AddScrollListSubHeader(icon, text);
On 2017/04/25 05:10:01, mohsen wrote:
> On 2017/04/24 at 15:43:39, tdanderson wrote:
> > Consider passing in the text_id to AddScrollListSubHeader instead of the
> string itself, which will eliminate the need for each call site to call
> l10_util::GetStringUTF16(). This assumes that all of our subheaders have (and
> will have) a pre-defined string id, which is probably the case. WDYT?
> 
> Done. I wanted AddScrollListSubHeader() to be more consistent with
> AddScrollListItem() which cannot take a resource id. But, your suggestion also
> makes sense and I don't have any preference.

Ah I see, I guess we could have gone either way with this.

https://codereview.chromium.org/2831023003/diff/60001/ash/system/bluetooth/tr...
File ash/system/bluetooth/tray_bluetooth.cc (left):

https://codereview.chromium.org/2831023003/diff/60001/ash/system/bluetooth/tr...
ash/system/bluetooth/tray_bluetooth.cc:402: if (display_name)
On 2017/04/25 05:10:01, mohsen wrote:
> On 2017/04/24 at 15:43:39, tdanderson wrote:
> > I am slightly uneasy about removing this logic along with your changes to
> UpdateClickedDevice(). Is it at all possible for a device's display name or
type
> to change after having been added to the list? If so then I think you may need
> to keep some of this extra logic here.
> > 
> > Consider removing these changes to a small follow-up CL and have fukino@
> (Bluetooth owner) take a look.
> 
> If a device name is changed, it would be updated in
> BluetoothDetailedView::Update(). UpdateclickedDevice() is called when an item
is
> clicked and is only responsible for updating connecting/connected messages. I
> think the only reason it also updates name and icon is that it uses
> SetupConnectingItem() which sets up the whole row from scratch. Does this make
> sense? If not, I can exclude changes in tray_bluetooth from this Cl as you
> suggested.

OK, that makes sense. I'd be ok landing as-is. But in the next patch set please
cc fukino@ just as an FYI to these changes.

https://codereview.chromium.org/2831023003/diff/100001/ash/system/tray/tray_d...
File ash/system/tray/tray_details_view.h (right):

https://codereview.chromium.org/2831023003/diff/100001/ash/system/tray/tray_d...
ash/system/tray/tray_details_view.h:81: HoverHighlightView*
AddScrollListCheckableItem(const gfx::VectorIcon& icon,
Sorry for the churn here, but how about instead overloading these functions? So
you'd have:

AddScrollListItem(icon, text);
AddScrollListItem(text); // note you currently don't have this.

AddScrollListCheckableItem(icon, text, checked);
AddScrollListCheckableItem(text, checked);

AddScrollListSubHeader(icon, text);
AddScrollListSubHeader(text);

WDYT?

https://codereview.chromium.org/2831023003/diff/100001/ash/system/tray/tray_d...
ash/system/tray/tray_details_view.h:97: // |gfx::kNoneIcon|.
nit: I suggest omitting mention of kNoneIcon in the documentation for all the
new functions you're adding here.

[mohsen, 2017-04-25 19:11:40]

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

[commit-bot: I haz the power, 2017-04-25 19:12:32]

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, 2017-04-25 19:17:43]

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

[commit-bot: I haz the power, 2017-04-25 19:17:44]

Dry run: Try jobs failed on following builders:
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[mohsen, 2017-04-25 19:23:48]

https://codereview.chromium.org/2831023003/diff/60001/ash/system/audio/audio_...
File ash/system/audio/audio_detailed_view.cc (left):

https://codereview.chromium.org/2831023003/diff/60001/ash/system/audio/audio_...
ash/system/audio/audio_detailed_view.cc:88: TriView* header =
TrayPopupUtils::CreateDefaultRowView();
On 2017/04/25 at 14:32:48, tdanderson wrote:
> On 2017/04/25 05:10:01, mohsen wrote:
> > On 2017/04/24 at 15:43:39, tdanderson wrote:
> > > Just wanted to double-check that this works as intended, since this
function
> > calls CreateDefaultRowView() but its replacement (AddScrollListSubHeader())
is
> > calling CreateSubHeaderRowView().
> > 
> > Yes, different layout managers are installed with different paddings.
However,
> > they produce very similar output for our purposes. 
> 
> By 'very similar' do you mean that the implementation in Patch Set 4 will give
the incorrect layout for the audio subheader rows?

Yes, the label padding was 4 pixels off.

> >I talked with bruthig@ and
> > decided that CreateSubHeaderRowView() should actually use
CreateDefaultRoView()
> > instead of directly using CreateMultiTargetRowView() so that similar padding
and
> > layout management is used for sub-header rows, too. Done. WDYT?
> 
> Unless the code in patch set 4 actually gives the incorrect layout, I would
prefer for the time being to avoid calling CreateDefaultRowView() directly from
CreateSubHeaderRowView() like you have in patch set 6. If there is an
opportunity to share code between the two then by all means create a private
helper function, but I see it as a bit confusing to call a function designed for
use by default views directly from a function designed for use by detailed
views.

I don't think CreateDefaultRowView() is designed only for default views. It is
also used for rows in detailed views, as far as I can tell. And it seems
reasonable to me to use it for sub-header rows as we want icons and labels in
sub-headers and regular rows be aligned properly.

> I am also happy to table this discussion for a follow-on CL, provided that
whatever you're landing in this CL will still follow design specs.

https://codereview.chromium.org/2831023003/diff/100001/ash/system/tray/tray_d...
File ash/system/tray/tray_details_view.h (right):

https://codereview.chromium.org/2831023003/diff/100001/ash/system/tray/tray_d...
ash/system/tray/tray_details_view.h:81: HoverHighlightView*
AddScrollListCheckableItem(const gfx::VectorIcon& icon,
On 2017/04/25 at 14:32:48, tdanderson wrote:
> Sorry for the churn here, but how about instead overloading these functions?
So you'd have:
> 
> AddScrollListItem(icon, text);
> AddScrollListItem(text); // note you currently don't have this.
> 
> AddScrollListCheckableItem(icon, text, checked);
> AddScrollListCheckableItem(text, checked);
> 
> AddScrollListSubHeader(icon, text);
> AddScrollListSubHeader(text);
> 
> WDYT?

I believe preference is not to overload functions.
 (https://google.github.io/styleguide/cppguide.html#Function_Overloading)

Of course, I can overload if you still prefer to, as the OWNER.

https://codereview.chromium.org/2831023003/diff/100001/ash/system/tray/tray_d...
ash/system/tray/tray_details_view.h:97: // |gfx::kNoneIcon|.
On 2017/04/25 at 14:32:48, tdanderson wrote:
> nit: I suggest omitting mention of kNoneIcon in the documentation for all the
new functions you're adding here.

Removed kNoneIcon from all these functions' comments.

[mohsen, 2017-04-25 19:26:12]

+cc fukino@ (FYI, changes in tray_bluetooth.cc)

[tdanderson, 2017-04-25 21:00:27]

lgtm with the comments addressed

https://codereview.chromium.org/2831023003/diff/60001/ash/system/audio/audio_...
File ash/system/audio/audio_detailed_view.cc (left):

https://codereview.chromium.org/2831023003/diff/60001/ash/system/audio/audio_...
ash/system/audio/audio_detailed_view.cc:88: TriView* header =
TrayPopupUtils::CreateDefaultRowView();
On 2017/04/25 19:23:47, mohsen wrote:
> On 2017/04/25 at 14:32:48, tdanderson wrote:
> > On 2017/04/25 05:10:01, mohsen wrote:
> > > On 2017/04/24 at 15:43:39, tdanderson wrote:
> > > > Just wanted to double-check that this works as intended, since this
> function
> > > calls CreateDefaultRowView() but its replacement
(AddScrollListSubHeader())
> is
> > > calling CreateSubHeaderRowView().
> > > 
> > > Yes, different layout managers are installed with different paddings.
> However,
> > > they produce very similar output for our purposes. 
> > 
> > By 'very similar' do you mean that the implementation in Patch Set 4 will
give
> the incorrect layout for the audio subheader rows?
> 
> Yes, the label padding was 4 pixels off.
> 
> > >I talked with bruthig@ and
> > > decided that CreateSubHeaderRowView() should actually use
> CreateDefaultRoView()
> > > instead of directly using CreateMultiTargetRowView() so that similar
padding
> and
> > > layout management is used for sub-header rows, too. Done. WDYT?
> > 
> > Unless the code in patch set 4 actually gives the incorrect layout, I would
> prefer for the time being to avoid calling CreateDefaultRowView() directly
from
> CreateSubHeaderRowView() like you have in patch set 6. If there is an
> opportunity to share code between the two then by all means create a private
> helper function, but I see it as a bit confusing to call a function designed
for
> use by default views directly from a function designed for use by detailed
> views.
> 
> I don't think CreateDefaultRowView() is designed only for default views. It is
> also used for rows in detailed views, as far as I can tell. And it seems
> reasonable to me to use it for sub-header rows as we want icons and labels in
> sub-headers and regular rows be aligned properly.

Yes you're right. I was reading the function as "create (default row) view"
rather than "create default (row view)". Doing this sgtm.


> 
> > I am also happy to table this discussion for a follow-on CL, provided that
> whatever you're landing in this CL will still follow design specs.

https://codereview.chromium.org/2831023003/diff/100001/ash/system/tray/tray_d...
File ash/system/tray/tray_details_view.h (right):

https://codereview.chromium.org/2831023003/diff/100001/ash/system/tray/tray_d...
ash/system/tray/tray_details_view.h:81: HoverHighlightView*
AddScrollListCheckableItem(const gfx::VectorIcon& icon,
On 2017/04/25 19:23:48, mohsen wrote:
> On 2017/04/25 at 14:32:48, tdanderson wrote:
> > Sorry for the churn here, but how about instead overloading these functions?
> So you'd have:
> > 
> > AddScrollListItem(icon, text);
> > AddScrollListItem(text); // note you currently don't have this.
> > 
> > AddScrollListCheckableItem(icon, text, checked);
> > AddScrollListCheckableItem(text, checked);
> > 
> > AddScrollListSubHeader(icon, text);
> > AddScrollListSubHeader(text);
> > 
> > WDYT?
> 
> I believe preference is not to overload functions.
>  (https://google.github.io/styleguide/cppguide.html#Function_Overloading)
> 
> Of course, I can overload if you still prefer to, as the OWNER.

As discussed offline, I think overloading is appropriate in this instance.

[mohsen, 2017-04-25 21:45:36]

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

[commit-bot: I haz the power, 2017-04-25 21:46:25]

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, 2017-04-25 21:50:24]

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

[commit-bot: I haz the power, 2017-04-25 21:50:25]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[mohsen, 2017-04-25 21:54:52]

https://codereview.chromium.org/2831023003/diff/100001/ash/system/tray/tray_d...
File ash/system/tray/tray_details_view.h (right):

https://codereview.chromium.org/2831023003/diff/100001/ash/system/tray/tray_d...
ash/system/tray/tray_details_view.h:81: HoverHighlightView*
AddScrollListCheckableItem(const gfx::VectorIcon& icon,
On 2017/04/25 at 21:00:27, tdanderson wrote:
> On 2017/04/25 19:23:48, mohsen wrote:
> > On 2017/04/25 at 14:32:48, tdanderson wrote:
> > > Sorry for the churn here, but how about instead overloading these
functions?
> > So you'd have:
> > > 
> > > AddScrollListItem(icon, text);
> > > AddScrollListItem(text); // note you currently don't have this.
> > > 
> > > AddScrollListCheckableItem(icon, text, checked);
> > > AddScrollListCheckableItem(text, checked);
> > > 
> > > AddScrollListSubHeader(icon, text);
> > > AddScrollListSubHeader(text);
> > > 
> > > WDYT?
> > 
> > I believe preference is not to overload functions.
> >  (https://google.github.io/styleguide/cppguide.html#Function_Overloading)
> > 
> > Of course, I can overload if you still prefer to, as the OWNER.
> 
> As discussed offline, I think overloading is appropriate in this instance.

Done.

[mohsen, 2017-04-25 22:00:21]

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

[commit-bot: I haz the power, 2017-04-25 22:01:09]

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, 2017-04-25 22:34:50]

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

[commit-bot: I haz the power, 2017-04-25 22:34:51]

Dry run: This issue passed the CQ dry run.

[mohsen, 2017-04-25 22:35:28]

The CQ bit was checked by mohsen@chromium.org

[mohsen, 2017-04-25 22:35:28]

The patchset sent to the CQ was uploaded after l-g-t-m from
tdanderson@chromium.org
Link to the patchset: https://codereview.chromium.org/2831023003/#ps160001
(title: "Rebased")

[commit-bot: I haz the power, 2017-04-25 22:36:26]

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-04-25 22:44:02]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1493159728371420,
"parent_rev": "5d2886cbcbb9afed530b745b88873940e369634e", "commit_rev":
"0b6712fcc4a9d0cd07624db4b3fac5c67edb2bac"}

[commit-bot: I haz the power, 2017-04-25 22:44:14]

Description was changed from

==========
Refactor AddScrollListItem() in system menu detailed views

Most of system menu detailed views have a scrollable list of items with
very similar layouts. This CL adds a few functions in TrayDetailsView to
create such items.

Among different detailed views, network and VPN are not using these
common functions, yet, as they need to be cleaned up first. That will be
done in follow-up CLs.

BUG=686343
TEST=TrayDetailsViewTest.ScrollContentsTest and
     *TrayAccessibilityTest.CheckMarksOnDetailMenu* in ash_unittests
==========

to

==========
Refactor AddScrollListItem() in system menu detailed views

Most of system menu detailed views have a scrollable list of items with
very similar layouts. This CL adds a few functions in TrayDetailsView to
create such items.

Among different detailed views, network and VPN are not using these
common functions, yet, as they need to be cleaned up first. That will be
done in follow-up CLs.

BUG=686343
TEST=TrayDetailsViewTest.ScrollContentsTest and
     *TrayAccessibilityTest.CheckMarksOnDetailMenu* in ash_unittests

Review-Url: https://codereview.chromium.org/2831023003
Cr-Commit-Position: refs/heads/master@{#467147}
Committed:
https://chromium.googlesource.com/chromium/src/+/0b6712fcc4a9d0cd07624db4b3fa...
==========

[commit-bot: I haz the power, 2017-04-25 22:44:16]

Committed patchset #9 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/0b6712fcc4a9d0cd07624db4b3fa...