[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 34d04a6f3e4bd30cab675388a4a5d9e3ace9e234..818c3e29a7723db79007bd760ef3f3edf80d340b 100644
--- a/ash/common/system/status_area_widget.cc
+++ b/ash/common/system/status_area_widget.cc
@@ -4,6 +4,7 @@
 
 #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"
@@ -188,6 +189,23 @@ 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() {
@@ -232,4 +250,26 @@ void StatusAreaWidget::UpdateAfterLoginStatusChange(LoginStatus login_status) {
     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