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

ash/common/system/status_area_widget.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #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"
 #include "ash/common/system/tray/system_tray.h"
 #include "ash/common/system/tray/system_tray_delegate.h"
 #include "ash/common/system/web_notification/web_notification_tray.h"
 #include "ash/common/wm_lookup.h"
 #include "ash/common/wm_root_window_controller.h"
 #include "ash/common/wm_shell.h"
 #include "ash/common/wm_window.h"
@@ skipping 142 matching lines @@
 #if defined(OS_CHROMEOS)
   virtual_keyboard_tray_->UpdateShelfItemBackground(alpha);
   logout_button_tray_->UpdateShelfItemBackground(alpha);
   ime_menu_tray_->UpdateShelfItemBackground(alpha);
   palette_tray_->UpdateShelfItemBackground(alpha);
 #endif
   overview_button_tray_->UpdateShelfItemBackground(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_);
 }
 
 void StatusAreaWidget::SetShelfAlignment(ShelfAlignment alignment) {
   status_area_widget_delegate_->set_alignment(alignment);
   if (system_tray_)
     system_tray_->SetShelfAlignment(alignment);
   if (web_notification_tray_)
     web_notification_tray_->SetShelfAlignment(alignment);
 #if defined(OS_CHROMEOS)
@@ skipping 20 matching lines @@
   if (web_notification_tray_)
     web_notification_tray_->UpdateAfterLoginStatusChange(login_status);
 #if defined(OS_CHROMEOS)
   if (logout_button_tray_)
     logout_button_tray_->UpdateAfterLoginStatusChange(login_status);
 #endif
   if (overview_button_tray_)
     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