[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 4d209a56ee1e9af7bda33876a4415957c03a3709..435e2a5202a7377651434f10ca67ae9c50dc31da 100644
--- a/ash/common/system/tray/tray_background_view.cc
+++ b/ash/common/system/tray/tray_background_view.cc
@@ -9,6 +9,7 @@
 #include "ash/common/shelf/wm_shelf.h"
 #include "ash/common/shelf/wm_shelf_util.h"
 #include "ash/common/shell_window_ids.h"
+#include "ash/common/system/status_area_widget.h"
 #include "ash/common/system/tray/system_tray.h"
 #include "ash/common/system/tray/tray_constants.h"
 #include "ash/common/system/tray/tray_event_filter.h"
@@ -28,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"
@@ -101,14 +103,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);
     }
   }
@@ -202,14 +204,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);
@@ -232,11 +226,13 @@ TrayBackgroundView::TrayBackgroundView(WmShelf* wm_shelf)
   tray_container_ = new TrayContainer(shelf_alignment_);
   SetContents(tray_container_);
   tray_event_filter_.reset(new TrayEventFilter);
+  is_separator_visible_ = false;

[James Cook, 2016/08/11 20:36:55]

can this be initialized in the member list above?

[yiyix, 2016/08/18 00:43:00]

Done.

 
   SetPaintToLayer(true);
   layer()->SetFillsBoundsOpaquely(false);
   // Start the tray items not visible, because visibility changes are animated.
   views::View::SetVisible(false);
+  CalculateAndSetTrayContainerBorder();
 }
 
 TrayBackgroundView::~TrayBackgroundView() {
@@ -308,6 +304,9 @@ void TrayBackgroundView::SetVisible(bool visible) {
     layer()->SetVisible(false);
     HideTransformation();
   }
+  // OnTrayVisibilityChanged will only be called if the visibility of the tray

[James Cook, 2016/08/11 20:36:55]

Is this still true? I don't understand what you're saying here.

[yiyix, 2016/08/18 00:43:00]

If the visibility is the same as before, then if statement in front of the
function returns true (if (visible == layer()->GetTargetVisibility())) so my
function will be called if the visibility is changed.

+  // item is changed.
+  wm_shelf_->GetStatusAreaWidget()->OnTrayVisibilityChanged(this);
 }
 
 const char* TrayBackgroundView::GetClassName() const {
@@ -362,9 +361,25 @@ 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;
+  }
+  // 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));
+}
+
 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
@@ -502,4 +517,33 @@ void TrayBackgroundView::UpdateShelfItemBackground(int alpha) {
   }
 }
 
+void TrayBackgroundView::SetSeparatorVisibility(bool is_shown) {
+  is_separator_visible_ = is_shown;
+}
+
+void TrayBackgroundView::OnPaint(gfx::Canvas* canvas) {
+  if (!MaterialDesignController::IsShelfMaterial() ||
+      shelf()->GetBackgroundType() ==
+          ShelfBackgroundType::SHELF_BACKGROUND_DEFAULT ||
+      !is_separator_visible_) {
+    return;
+  }
+
+  const int height = kTrayItemSize;
+  const int width = kSeparatorWidth;
+  const int x = kTrayItemSize + kHitRegionPadding + kHitRegionPadding;
+  const int y = (GetShelfConstant(SHELF_SIZE) - kTrayItemSize) / 2;
+  const float scale = canvas->UndoDeviceScaleFactor();

[James Cook, 2016/08/11 22:39:13]

Also, I still think this should go inside the scoped_canvas below. It affects
the scaling of the canvas, which should affect its matrix. Per SkCanvas.h, the
canvas->save() call inside ScopedCanvas should preserve the matrix.

https://cs.chromium.org/chromium/src/third_party/skia/include/core/SkCanvas.h...

Also, the example you cite puts the UndoDeviceScaleFactor inside the
ScopedCanvas.

https://cs.chromium.org/chromium/src/ui/views/painter.cc?sq=package:chromium&...

Maybe I'm wrong? I'm not super-familiar with SkCanvas.

[yiyix, 2016/08/18 00:43:00]

Do you mean change the order of const float scale =
canvas->UndoDeviceScaleFactor(); and gfx::ScopedCanvas scoped_canvas(canvas); so
everything happens in the saved canvas? I can do that. I am not familiar with
this part of code too.

+  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);
+}
+
 }  // namespace ash