[ash-md] Applied Material Design layout to TrayItemMore system menu rows.

ash/common/system/tray/tray_item_more.cc

Index: ash/common/system/tray/tray_item_more.cc
diff --git a/ash/common/system/tray/tray_item_more.cc b/ash/common/system/tray/tray_item_more.cc
index 681c08b58edc621c2402787500963048d5788abd..2854ed0d0b480b7f53c08ad037b9db4ff9670e13 100644
--- a/ash/common/system/tray/tray_item_more.cc
+++ b/ash/common/system/tray/tray_item_more.cc
@@ -5,10 +5,11 @@
 #include "ash/common/system/tray/tray_item_more.h"
 
 #include "ash/common/material_design/material_design_controller.h"
-#include "ash/common/system/tray/fixed_sized_image_view.h"
 #include "ash/common/system/tray/system_tray_item.h"
 #include "ash/common/system/tray/tray_constants.h"
 #include "ash/common/system/tray/tray_popup_item_style.h"
+#include "ash/common/system/tray/tray_popup_utils.h"
+#include "ash/common/system/tray/tri_view.h"
 #include "ash/resources/vector_icons/vector_icons.h"
 #include "base/memory/ptr_util.h"
 #include "grit/ash_resources.h"
@@ -19,6 +20,7 @@
 #include "ui/views/controls/image_view.h"
 #include "ui/views/controls/label.h"
 #include "ui/views/layout/box_layout.h"
+#include "ui/views/layout/fill_layout.h"
 
 namespace ash {
 
@@ -28,19 +30,19 @@ TrayItemMore::TrayItemMore(SystemTrayItem* owner, bool show_more)
       icon_(nullptr),
       label_(nullptr),
       more_(nullptr) {
-  SetLayoutManager(new views::BoxLayout(views::BoxLayout::kHorizontal,
-                                        kTrayPopupPaddingHorizontal, 0,
-                                        kTrayPopupPaddingBetweenItems));
+  TriView* tri_view = TrayPopupUtils::CreateDefaultRowView();
+  AddChildView(tri_view);
+  SetLayoutManager(new views::FillLayout);

[tdanderson, 2016/11/01 18:59:44]

optional: Consider adding something to the class-level documentation of TriView
to mention that a FillLayout should be used as the layout manager of TriView's
parent view.

[bruthig, 2016/11/01 23:00:43]

I'm torn on whether this should be documented or not.  In order for a child view
to be visible it must be allocated space via a layout.  I think it falls under
general domain knowledge within the View/LayoutManager framework.  I think the
requested docs are to specific to be added to the more general to TriView class.

 
-  icon_ = new FixedSizedImageView(0, GetTrayConstant(TRAY_POPUP_ITEM_HEIGHT));
-  AddChildView(icon_);
+  icon_ = TrayPopupUtils::CreateMainImageView();
+  tri_view->AddView(TriView::Container::START, icon_);
 
-  label_ = new views::Label;
+  label_ = TrayPopupUtils::CreateDefaultLabel();
   label_->SetHorizontalAlignment(gfx::ALIGN_LEFT);

[tdanderson, 2016/11/01 18:59:44]

Would it be appropriate to move line 41 into CreateDefaultLabel() ?

[bruthig, 2016/11/01 23:00:43]

Good call, done.

-  AddChildView(label_);
+  tri_view->AddView(TriView::Container::CENTER, label_);
 
   if (show_more) {
-    more_ = new views::ImageView;
+    more_ = TrayPopupUtils::CreateMoreImageView();
     more_->EnableCanvasFlippingForRTLUI(true);

[tdanderson, 2016/11/01 18:59:44]

Similar to above comment, should this be moved into CreateMoreImageView() ?

[bruthig, 2016/11/01 23:00:43]

Done.

     if (!MaterialDesignController::IsSystemTrayMenuMaterial()) {
       // The icon doesn't change in non-md.
@@ -48,7 +50,9 @@ TrayItemMore::TrayItemMore(SystemTrayItem* owner, bool show_more)
                           .GetImageNamed(IDR_AURA_UBER_TRAY_MORE)
                           .ToImageSkia());
     }
-    AddChildView(more_);
+    tri_view->AddView(TriView::Container::END, more_);
+  } else {
+    tri_view->SetContainerVisible(TriView::Container::END, false);

[tdanderson, 2016/11/01 18:59:44]

Interesting, I didn't realize this would have been necessary; presumably this
empty container affects the appearance of the layout? Have you considered
modifying TriView to make containers invisible whenever they are empty? (OK to
discuss / do as follow up to this CL).

I think manually setting empty containers to invisible would be an easy thing
for someone to forget.

[bruthig, 2016/11/01 23:00:43]

Setting the END container to hidden would allocate more space to the CENTER
container.  This may end up being desired for the END container but we don't
want to do this for the START container because that is how we ensure CENTER
container labels are left-aligned.  I am going to defer this change until a more
obvious pattern emerges.

   }
 }
 
@@ -94,29 +98,6 @@ bool TrayItemMore::PerformAction(const ui::Event& event) {
   return true;
 }
 
-void TrayItemMore::Layout() {
-  // Let the box-layout do the layout first. Then move the '>' arrow to right
-  // align.
-  views::View::Layout();
-
-  if (!show_more_)
-    return;
-
-  // Make sure the chevron always has the full size.
-  gfx::Size size = more_->GetPreferredSize();
-  gfx::Rect bounds(size);
-  bounds.set_x(width() - size.width() - kTrayPopupPaddingBetweenItems);
-  bounds.set_y((height() - size.height()) / 2);
-  more_->SetBoundsRect(bounds);
-
-  // Adjust the label's bounds in case it got cut off by |more_|.
-  if (label_->bounds().Intersects(more_->bounds())) {
-    gfx::Rect bounds = label_->bounds();
-    bounds.set_width(more_->x() - kTrayPopupPaddingBetweenItems - label_->x());

[tdanderson, 2016/11/01 18:59:44]

Double-checking since it could be easy to forget when only testing with short
strings: will TriView handle this case nicely?

[bruthig, 2016/11/01 23:00:43]

Yup it will. +1 for the TriView.

-    label_->SetBoundsRect(bounds);
-  }
-}
-
 void TrayItemMore::GetAccessibleState(ui::AXViewState* state) {
   ActionableView::GetAccessibleState(state);
   if (!accessible_name_.empty())