[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/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"
@@ skipping 164 matching lines @@
 
 void StatusAreaWidget::AddVirtualKeyboardTray() {
   virtual_keyboard_tray_ = new VirtualKeyboardTray(wm_shelf_);
   status_area_widget_delegate_->AddTray(virtual_keyboard_tray_);
 }
 
 void StatusAreaWidget::AddImeMenuTray() {
   ime_menu_tray_ = new ImeMenuTray(wm_shelf_);
   status_area_widget_delegate_->AddTray(ime_menu_tray_);
 }
+
+bool StatusAreaWidget::IsNextVisibleTrayToLogout(TrayBackgroundView* tray) {
+  if (!logout_button_tray_->visible())
+    return false;
+  bool is_logout_found = false;
+  for (int c = 0; c < status_area_widget_delegate_->child_count(); c++) {
+    if (!is_logout_found) {
+      if (status_area_widget_delegate_->child_at(c) == logout_button_tray_) {
+        is_logout_found = true;
+      }
+    } else {
+      if (status_area_widget_delegate_->child_at(c)->visible())
+        return tray == status_area_widget_delegate_->child_at(c);
+    }
+  }

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

nit: I think this would be clearer if you did something like:

if (!logout->visible())
  return false;
int logout_button_index = delegate_->GetIndexOf(logout_button_tray_);
// Logout button should always exist.
DCHECK_NE(-1, logout_button_index);

Then loop through looking for the next visible child.

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

Yes, it makes the code easier to understand.

+  return false;
+}
 #endif
 
 void StatusAreaWidget::AddOverviewButtonTray() {
   overview_button_tray_ = new OverviewButtonTray(wm_shelf_);
   status_area_widget_delegate_->AddTray(overview_button_tray_);
 }
 
 void StatusAreaWidget::SetShelfAlignment(ShelfAlignment alignment) {
   status_area_widget_delegate_->set_alignment(alignment);
   if (system_tray_)
@@ skipping 24 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::OnTrayVisibilityChanged(TrayBackgroundView* tray) {

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

nit: move up to match function order in header

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

I moved up SetShelfAlignment, UpdateAfterLoginStatusChange and
OnTrayVisibilityChanged to match the header file.

+  if (!ash::MaterialDesignController::IsShelfMaterial()) {
+    return;
+  }
+
+  // No separator is required between |system_tray_| and |overview_button_tray_|
+  // and no separator is required for the right most tray item.
+  if (tray == overview_button_tray_ || tray == system_tray_) {
+    tray->SetSeparatorVisibility(false);
+  } else {

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

nit: early return above this line might make it easier to read with the
OS_CHROMEOS ifdef.

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

Done.

+// If |logout_button_tray_| is visible, then no leading separator is required

[varkha, 2016/08/11 20:01:03]

nit: indents.

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

Done.

+// because it is red and distinct on its own. If |logout_button_tray_| is not
+// visible, then separator should always be visible.
+#if defined(OS_CHROMEOS)

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

nit: move this above the comment

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

Done.

+    tray->SetSeparatorVisibility(!IsNextVisibleTrayToLogout(tray) &&
+                                 tray != logout_button_tray_);
+    return;
+#endif
+    tray->SetSeparatorVisibility(true);

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

maybe put this in an #else block?

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

Done.

+  }
+}
+
 }  // namespace ash