[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 3e26c5069993d2872ef9ae0d0f4602002b223bb3..c063fa352f85de28226b2057ef6967f5d3cbee39 100644
--- a/ash/common/system/tray/tray_background_view.cc
+++ b/ash/common/system/tray/tray_background_view.cc
@@ -7,6 +7,7 @@
 #include "ash/common/material_design/material_design_controller.h"
 #include "ash/common/shelf/shelf_constants.h"
 #include "ash/common/shelf/wm_shelf.h"
+#include "ash/common/shelf/wm_shelf_observer.h"
 #include "ash/common/shelf/wm_shelf_util.h"
 #include "ash/common/shell_window_ids.h"
 #include "ash/common/system/tray/system_tray.h"
@@ -29,6 +30,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"
@@ -102,14 +104,14 @@ class TrayBackground : public views::Background {
     SkPaint background_paint;
     background_paint.setFlags(SkPaint::kAntiAlias_Flag);
     background_paint.setColor(SkColorSetA(kShelfBaseColor, alpha_));
-    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);
     }
   }
@@ -203,14 +205,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);
@@ -220,7 +214,8 @@ void TrayBackgroundView::TrayContainer::UpdateLayout() {
 ////////////////////////////////////////////////////////////////////////////////
 // TrayBackgroundView
 
-TrayBackgroundView::TrayBackgroundView(WmShelf* wm_shelf)
+TrayBackgroundView::TrayBackgroundView(WmShelf* wm_shelf,
+                                       WmShelfObserver* wm_shelf_observer)
     : wm_shelf_(wm_shelf),
       tray_container_(NULL),
       shelf_alignment_(SHELF_ALIGNMENT_BOTTOM),
@@ -233,11 +228,14 @@ TrayBackgroundView::TrayBackgroundView(WmShelf* wm_shelf)
   tray_container_ = new TrayContainer(shelf_alignment_);
   SetContents(tray_container_);
   tray_event_filter_.reset(new TrayEventFilter);
+  wm_shelf_observer_ = wm_shelf_observer;

[James Cook, 2016/08/09 00:25:18]

nit: initialize in the member list above

Also, DCHECK(wm_shelf_observer_) since it must not be null (which makes the code
self-documenting)

[yiyix, 2016/08/11 01:23:19]

After removing the observer, this is not applicable.

+  is_separator_visible_ = false;
 
   SetPaintToLayer(true);
   layer()->SetFillsBoundsOpaquely(false);
   // Start the tray items not visible, because visibility changes are animated.
   views::View::SetVisible(false);
+  CalculateAndSetTrayContainerBorder();
 }
 
 TrayBackgroundView::~TrayBackgroundView() {
@@ -264,6 +262,9 @@ void TrayBackgroundView::InitializeBubbleAnimations(
 void TrayBackgroundView::SetVisible(bool visible) {
   if (visible == layer()->GetTargetVisibility())
     return;
+  // OnVisibilityChange will only be called if the visibility of the tray item
+  // is changed.
+  wm_shelf_observer_->OnVisibilityChange(this);

[James Cook, 2016/08/09 00:25:17]

so I think this should end with "Changing", since it's before the change

[yiyix, 2016/08/11 01:23:19]

After using index, the method is called after visibility change, so I updated it
to OnTrayVisibilityChanged.

 
   if (visible) {
     // The alignment of the shelf can change while the TrayBackgroundView is
@@ -365,9 +366,26 @@ 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;
+  }
+  if (IsHorizontalAlignment(shelf()->GetAlignment())) {

[James Cook, 2016/08/09 00:25:17]

optional nit: I'm about to land a CL that will let you call
wm_shelf_->IsHorizontalAlignment() instead of the utility function.

[yiyix, 2016/08/11 01:23:19]

Thank you for head up.

+    // Extend hit region horizontally when horizontally aligned.
+    tray_container()->SetBorder(views::Border::CreateEmptyBorder(gfx::Insets(
+        0, kHitRegionPadding, 0, kHitRegionPadding + kSeparatorWidth)));

[varkha, 2016/08/10 23:27:25]

Looks like you can simplify this similar to:

// Extend hit region horizontally or vertically depending on shelf alignment.
const gfx::Insets insets(IsHorizontalAlignment(shelf()->GetAlignment())
    ? gfx::Insets(0, kHitRegionPadding, 0, kHitRegionPadding + kSeparatorWidth)
    : gfx::Insets(kHitRegionPadding, 0, kHitRegionPadding + kSeparatorWidth, 0);
tray_container()->SetBorder(views::Border::CreateEmptyBorder(insets));

[yiyix, 2016/08/11 01:23:19]

Done.

+  } 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
@@ -505,4 +523,35 @@ void TrayBackgroundView::UpdateShelfItemBackground(int alpha) {
   }
 }
 
+void TrayBackgroundView::SetSeparatorVisibility(bool is_show) {

[James Cook, 2016/08/09 00:25:18]

nit: is_shown or just shown

[varkha, 2016/08/10 23:27:24]

... or just "visibility".

[yiyix, 2016/08/11 01:23:19]

Done.

+  if (is_separator_visible_ != is_show)

[James Cook, 2016/08/09 00:25:18]

I don't think you need this if()

[yiyix, 2016/08/11 01:23:19]

Done.

+    is_separator_visible_ = is_show;
+}
+
+void TrayBackgroundView::OnPaint(gfx::Canvas* canvas) {
+  if (!MaterialDesignController::IsShelfMaterial() ||
+      shelf()->GetBackgroundType() ==
+          ShelfBackgroundType::SHELF_BACKGROUND_DEFAULT)

[varkha, 2016/08/10 23:27:25]

nit - {} for multi-line condition.

[yiyix, 2016/08/11 01:23:19]

Done.

+    return;
+
+  if (is_separator_visible_) {

[James Cook, 2016/08/09 00:25:17]

nit: put !is_separator_visible_ into the early-exit above

[yiyix, 2016/08/11 01:23:19]

Done.

+    const int height = GetTrayConstant(TRAY_ITEM_HEIGHT_LEGACY);
+    const int width = kSeparatorWidth;
+    const int x = height + kHitRegionPadding + kHitRegionPadding;

[James Cook, 2016/08/09 00:25:18]

x is based on the height?

[yiyix, 2016/08/11 01:23:19]

The application icons have the same height and width, so I wanted to use
GetTrayConstant(TRAY_ITEM_HEIGHT_LEGACY) as the width to calculate the x
location of the separator. I can see how it is confusing to read now. I will use
GetTrayConstant(TRAY_ITEM_HEIGHT_LEGACY) instead of height for the readability. 

+    const int y = (GetShelfConstant(SHELF_SIZE) - height) / 2;
+    const float scale = canvas->UndoDeviceScaleFactor();

[James Cook, 2016/08/09 00:25:18]

I think you need to do this inside the scoped_canvas so you don't change the
state of |canvas| for code outside this function

[yiyix, 2016/08/11 01:23:19]

There is no drawing functions inside of ScopedCanvas. ScopeCanvas is there to
restore the Canvas if anything goes out of scope. I took this file as an example
to write this function.
https://cs.chromium.org/chromium/src/ui/views/painter.cc?sq=package:chromium&...

+    SkPaint paint;
+    paint.setColor(kSeparatorColor);
+    paint.setAntiAlias(true);
+    const bool horizontal_shelf =
+        IsHorizontalAlignment(shelf()->GetAlignment());
+    const gfx::Rect bounds = horizontal_shelf ? gfx::Rect(x, y, width, height)
+                                              : 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);
+  }

[James Cook, 2016/08/09 00:25:17]

I find this function a little hard to read. It seems odd to construct a bounds
rect just to draw a line from one point to another. Maybe it would be clearer if
it computed 2 points, or 1 point and x/y offsets?

[yiyix, 2016/08/11 01:23:19]

I was thinking that i need to construct a rect to draw the line, so it would be
easier if the line and the rect have the same size. 

+}
+
 }  // namespace ash