[Chrome OS MD] Draw a 1px separator between 2 tray items

ash/common/system/tray/tray_background_view.cc

Index: ash/common/system/tray/tray_background_view.cc
diff --git a/ash/common/system/tray/tray_background_view.cc b/ash/common/system/tray/tray_background_view.cc
index 569e939369e26c480a368488596996bd1150bdf7..d47fca9a611b03da7404b07f9ca7e7a7d7c7ea1b 100644
--- a/ash/common/system/tray/tray_background_view.cc
+++ b/ash/common/system/tray/tray_background_view.cc
@@ -29,6 +29,7 @@
 #include "ui/gfx/image/image_skia.h"
 #include "ui/gfx/image/image_skia_operations.h"
 #include "ui/gfx/nine_image_painter.h"
+#include "ui/gfx/scoped_canvas.h"
 #include "ui/gfx/skia_util.h"
 #include "ui/gfx/transform.h"
 #include "ui/views/background.h"
@@ -109,14 +110,14 @@ class TrayBackground : public views::Background {
     SkPaint background_paint;
     background_paint.setFlags(SkPaint::kAntiAlias_Flag);
     background_paint.setColor(background_color);
-    canvas->DrawRoundRect(view->GetLocalBounds(), kTrayRoundedBorderRadius,
+    canvas->DrawRoundRect(view->GetContentsBounds(), kTrayRoundedBorderRadius,
                           background_paint);
 
     if (tray_background_view_->draw_background_as_active()) {
       SkPaint highlight_paint;
       highlight_paint.setFlags(SkPaint::kAntiAlias_Flag);
       highlight_paint.setColor(kShelfButtonActivatedHighlightColor);
-      canvas->DrawRoundRect(view->GetLocalBounds(), kTrayRoundedBorderRadius,
+      canvas->DrawRoundRect(view->GetContentsBounds(), kTrayRoundedBorderRadius,
                             highlight_paint);
     }
   }
@@ -208,14 +209,6 @@ void TrayBackgroundView::TrayContainer::UpdateLayout() {
       IsHorizontalAlignment(alignment_) ? views::BoxLayout::kHorizontal
                                         : views::BoxLayout::kVertical;
 
-  if (!ash::MaterialDesignController::IsShelfMaterial()) {
-    // Additional padding used to adjust the user-visible size of status tray
-    // dark background.
-    const int padding = 3;
-    SetBorder(
-        views::Border::CreateEmptyBorder(padding, padding, padding, padding));
-  }
-
   views::BoxLayout* layout = new views::BoxLayout(orientation, 0, 0, 0);
   layout->SetDefaultFlex(1);
   views::View::SetLayoutManager(layout);
@@ -243,6 +236,7 @@ TrayBackgroundView::TrayBackgroundView(WmShelf* wm_shelf)
   layer()->SetFillsBoundsOpaquely(false);
   // Start the tray items not visible, because visibility changes are animated.
   views::View::SetVisible(false);
+  CalculateAndSetTrayContainerBorder();
 }
 
 TrayBackgroundView::~TrayBackgroundView() {
@@ -377,9 +371,27 @@ void TrayBackgroundView::SetContentsBackground() {
 
 void TrayBackgroundView::SetShelfAlignment(ShelfAlignment alignment) {
   shelf_alignment_ = alignment;
+  CalculateAndSetTrayContainerBorder();
   tray_container_->SetAlignment(alignment);
 }
 
+void TrayBackgroundView::CalculateAndSetTrayContainerBorder() {
+  if (!MaterialDesignController::IsShelfMaterial()) {
+    tray_container()->SetBorder(views::Border::NullBorder());
+    return;
+  } else {

[varkha, 2016/07/14 21:34:48]

nit: no need for 'else' with early return above.

[yiyix, 2016/07/26 20:27:31]

Done.

+    if (IsHorizontalAlignment(shelf()->GetAlignment())) {
+      // Extend hit region horizontally when horizontally aligned.
+      tray_container()->SetBorder(views::Border::CreateEmptyBorder(gfx::Insets(
+          0, kHitRegionPadding, 0, kHitRegionPadding + kSeparatorWidth)));
+    } else {
+      // Extend hit region vertically when vertically aligned.
+      tray_container()->SetBorder(views::Border::CreateEmptyBorder(gfx::Insets(
+          kHitRegionPadding, 0, kHitRegionPadding + kSeparatorWidth, 0)));
+    }
+  }
+}
+
 void TrayBackgroundView::OnImplicitAnimationsCompleted() {
   // If there is another animation in the queue, the reverse animation was
   // triggered before the completion of animating to invisible. Do not turn off
@@ -510,4 +522,30 @@ void TrayBackgroundView::UpdateBubbleViewArrow(
   // Nothing to do here.
 }
 
+void TrayBackgroundView::DrawSeparator(gfx::Canvas* canvas) {
+  if (!(shelf()->GetBackgroundType() ==
+        ShelfBackgroundType::SHELF_BACKGROUND_DEFAULT) &&
+      MaterialDesignController::IsShelfMaterial()) {

[varkha, 2016/07/14 21:34:47]

How about

if (!MaterialDesignController::IsShelfMaterial() ||
    shelf()->GetBackgroundType() ==
ShelfBackgroundType::SHELF_BACKGROUND_DEFAULT) {
  return;
}

instead?

[yiyix, 2016/07/26 20:27:31]

It is better for readers to follow

+    const int x = GetTrayConstant(TRAY_ITEM_HEIGHT_LEGACY) + kHitRegionPadding +
+                  kHitRegionPadding;
+    const int y = (GetShelfConstant(SHELF_SIZE) -
+                   GetTrayConstant(TRAY_ITEM_HEIGHT_LEGACY)) /
+                  2;
+    const int width = kSeparatorWidth;
+    const int height = GetTrayConstant(TRAY_ITEM_HEIGHT_LEGACY);

[varkha, 2016/07/14 21:34:47]

If you move this up you can use |height| instead of calling
GetTrayConstant(TRAY_ITEM_HEIGHT_LEGACY).
I would move both width and height above together.

[yiyix, 2016/07/26 20:27:31]

Done.

+    const float scale = canvas->UndoDeviceScaleFactor();
+    SkPaint paint;
+    paint.setColor(kSeparatorColor);
+    paint.setAntiAlias(true);
+    const bool horizontal_shelf =
+        IsHorizontalAlignment(shelf()->GetAlignment());
+    gfx::Rect bounds = horizontal_shelf ? gfx::Rect(x, y, width, height)

[varkha, 2016/07/14 21:34:47]

nit: const.

[yiyix, 2016/07/26 20:27:31]

Done.

+                                        : gfx::Rect(y, x, height, width);
+    gfx::ScopedCanvas scoped_canvas(canvas);
+    gfx::RectF rect(gfx::ScaleRect(gfx::RectF(bounds), scale));
+    canvas->DrawLine(horizontal_shelf ? rect.top_right() : rect.bottom_left(),
+                     rect.bottom_right(), paint);
+  }
+}
+
 }  // namespace ash