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

ash/common/system/status_area_widget.cc

Index: ash/common/system/status_area_widget.cc
diff --git a/ash/common/system/status_area_widget.cc b/ash/common/system/status_area_widget.cc
index ada0972c044099b15bee9984ef61c38a2b378c6f..854bf8145f1bb6a004290a7d85515119467d808a 100644
--- a/ash/common/system/status_area_widget.cc
+++ b/ash/common/system/status_area_widget.cc
@@ -4,7 +4,9 @@
 
 #include "ash/common/system/status_area_widget.h"
 
+#include "ash/common/material_design/material_design_controller.h"
 #include "ash/common/shelf/wm_shelf.h"
+#include "ash/common/shelf/wm_shelf_observer.h"
 #include "ash/common/shell_window_ids.h"
 #include "ash/common/system/overview/overview_button_tray.h"
 #include "ash/common/system/status_area_widget_delegate.h"
@@ -167,41 +169,41 @@ void StatusAreaWidget::UpdateShelfItemBackground(int alpha) {
 }
 
 void StatusAreaWidget::AddSystemTray() {
-  system_tray_ = new SystemTray(wm_shelf_);
+  system_tray_ = new SystemTray(wm_shelf_, this);
   status_area_widget_delegate_->AddTray(system_tray_);
 }
 
 void StatusAreaWidget::AddWebNotificationTray() {
   DCHECK(system_tray_);
   web_notification_tray_ = new WebNotificationTray(
-      wm_shelf_, WmLookup::Get()->GetWindowForWidget(this), system_tray_);
+      wm_shelf_, WmLookup::Get()->GetWindowForWidget(this), system_tray_, this);
   status_area_widget_delegate_->AddTray(web_notification_tray_);
 }
 
 #if defined(OS_CHROMEOS)
 void StatusAreaWidget::AddLogoutButtonTray() {
-  logout_button_tray_ = new LogoutButtonTray(wm_shelf_);
+  logout_button_tray_ = new LogoutButtonTray(wm_shelf_, this);
   status_area_widget_delegate_->AddTray(logout_button_tray_);
 }
 
 void StatusAreaWidget::AddPaletteTray() {
-  palette_tray_ = new PaletteTray(wm_shelf_);
+  palette_tray_ = new PaletteTray(wm_shelf_, this);
   status_area_widget_delegate_->AddTray(palette_tray_);
 }
 
 void StatusAreaWidget::AddVirtualKeyboardTray() {
-  virtual_keyboard_tray_ = new VirtualKeyboardTray(wm_shelf_);
+  virtual_keyboard_tray_ = new VirtualKeyboardTray(wm_shelf_, this);
   status_area_widget_delegate_->AddTray(virtual_keyboard_tray_);
 }
 
 void StatusAreaWidget::AddImeMenuTray() {
-  ime_menu_tray_ = new ImeMenuTray(wm_shelf_);
+  ime_menu_tray_ = new ImeMenuTray(wm_shelf_, this);
   status_area_widget_delegate_->AddTray(ime_menu_tray_);
 }
 #endif
 
 void StatusAreaWidget::AddOverviewButtonTray() {
-  overview_button_tray_ = new OverviewButtonTray(wm_shelf_);
+  overview_button_tray_ = new OverviewButtonTray(wm_shelf_, this);
   status_area_widget_delegate_->AddTray(overview_button_tray_);
 }
 
@@ -242,4 +244,26 @@ void StatusAreaWidget::UpdateAfterLoginStatusChange(LoginStatus login_status) {
     overview_button_tray_->UpdateAfterLoginStatusChange(login_status);
 }
 
+void StatusAreaWidget::OnVisibilityChange(TrayBackgroundView* tray) {
+  if (ash::MaterialDesignController::IsShelfMaterial()) {

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

nit: early exit to avoid indenting below?

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

Done.

+#if defined(OS_CHROMEOS)
+    if (tray == virtual_keyboard_tray_) {
+      if (logout_button_tray_->visible()) {
+        tray->SetSeparatorVisibility(false);

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

nit: collapse into a single method invocation, i.e.
tray->SetSeparatorVisibility(!logout_button_tray_->visible());

This could avoid code bloat.

+      } else {
+        tray->SetSeparatorVisibility(true);
+      }

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

nit: I would do something like:
const bool show_separator = !logout_button_tray_->visible();
tray->SetSeparatorVisibility(show_separator);

also below

This also gives you a place to put comments about why you are showing/hiding it

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

Done.

+

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

nit: no blank line

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

Done.

+    } else if (tray == ime_menu_tray_) {
+      if (logout_button_tray_->visible() &&
+          !virtual_keyboard_tray_->visible()) {
+        tray->SetSeparatorVisibility(false);

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

nit: collapse into a single method invocation, i.e.
tray->SetSeparatorVisibility(!logout_button_tray_->visible() ||
virtual_keyboard_tray_->visible())

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

This function is re-write with indices. I will keep in mind how to simplify my
bool evaluations. Thanks

+      } else {
+        tray->SetSeparatorVisibility(true);
+      }
+    }

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

Is there a way to do this based on getting the tray item by index, rather than
hard-coding the knowledge about the order of the items into this function? You
need to hard-code knowledge about the logout button, since it is bright red, but
could you figure out information about its neighbor based on the index?

I'm worried this will break if someone reorders the items, and they won't notice
because the logout button is not commonly seen by developers.

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

I totally agree. I don't know anything about the logout button before starting
this task. I re-write this function based on the index.

+#endif
+  }
+}
+
 }  // namespace ash