CrOS md user row adjustments

ash/common/system/user/user_view.cc

Index: ash/common/system/user/user_view.cc
diff --git a/ash/common/system/user/user_view.cc b/ash/common/system/user/user_view.cc
index 8290fb1bc1b7ca11e6e1448103a1f0c61a199651..d9353ff7737d5c70c969aa30746f65cafd771843 100644
--- a/ash/common/system/user/user_view.cc
+++ b/ash/common/system/user/user_view.cc
@@ -28,6 +28,7 @@
 #include "ash/common/wm_window.h"
 #include "ash/public/cpp/shell_window_ids.h"
 #include "ash/resources/vector_icons/vector_icons.h"
+#include "base/optional.h"
 #include "components/signin/core/account_id/account_id.h"
 #include "components/user_manager/user_info.h"
 #include "grit/ash_resources.h"
@@ -102,6 +103,49 @@ bool IsMultiProfileSupportedAndUserActive() {
          !WmShell::Get()->GetSessionStateDelegate()->IsUserSessionBlocked();
 }
 
+// Creates the view shown in the user switcher popup ("AddUserMenuOption").
+views::View* CreateAddUserView(
+    base::Optional<SessionStateDelegate::AddUserError> error) {
+  DCHECK(UseMd());
+  auto view = new views::View;
+  auto layout = new views::BoxLayout(
+      views::BoxLayout::kHorizontal, (kMenuButtonSize - kMenuIconSize) / 2,
+      kMenuSeparatorVerticalPadding, kTrayPopupPaddingBetweenItems);
+  layout->set_minimum_cross_axis_size(
+      error ? 56 : GetTrayConstant(TRAY_POPUP_ITEM_MIN_HEIGHT));

[tdanderson, 2016/11/16 23:08:08]

nit: can you derive this 56 from other constants or give it a name?

[Evan Stade, 2016/11/17 00:37:50]

part 1: I don't think so, it's basically just a magic number Sebastien came up
with AFAICT.

part 2: If you mean

const int kMinimumHeightForWhenTheresAnError = 56;
layout->set_minimum_cross_axis_size(
        error ? kMinimumHeightForWhenTheresAnError :
GetTrayConstant(TRAY_POPUP_ITEM_MIN_HEIGHT));

I don't think it adds legibility. Do you? Or did you have something else in
mind?

[tdanderson, 2016/11/17 01:40:15]

If it is truly just a magic number that cannot be given a meaningful name, then
I agree my suggestion will be counterproductive. My original suggestion should
have read "... or give it a meaningful name".

[Evan Stade, 2016/11/17 01:54:19]

Acknowledged.

+  view->SetLayoutManager(layout);
+  view->set_background(
+      views::Background::CreateSolidBackground(kBackgroundColor));
+
+  if (!error) {
+    auto icon = new views::ImageView();
+    icon->SetImage(
+        gfx::CreateVectorIcon(kSystemMenuNewUserIcon, kMenuIconColor));
+    view->AddChildView(icon);
+  }
+
+  int message_id = IDS_ASH_STATUS_TRAY_SIGN_IN_ANOTHER_ACCOUNT;
+  if (error) {
+    switch (*error) {
+      case SessionStateDelegate::ADD_USER_ERROR_NOT_ALLOWED_PRIMARY_USER:
+        message_id = IDS_ASH_STATUS_TRAY_MESSAGE_NOT_ALLOWED_PRIMARY_USER;
+        break;
+      case SessionStateDelegate::ADD_USER_ERROR_MAXIMUM_USERS_REACHED:
+        message_id = IDS_ASH_STATUS_TRAY_MESSAGE_CANNOT_ADD_USER;
+        break;
+      case SessionStateDelegate::ADD_USER_ERROR_OUT_OF_USERS:
+        message_id = IDS_ASH_STATUS_TRAY_MESSAGE_OUT_OF_USERS;
+        break;
+    }
+  }
+
+  auto command_label = new views::Label(l10n_util::GetStringUTF16(message_id));
+  command_label->SetHorizontalAlignment(gfx::ALIGN_LEFT);
+  command_label->SetMultiLine(true);
+  view->AddChildView(command_label);
+  return view;
+}
+
 class UserViewMouseWatcherHost : public views::MouseWatcherHost {
  public:
   explicit UserViewMouseWatcherHost(const gfx::Rect& screen_area)
@@ -180,6 +224,7 @@ class AddUserView : public views::View {
 
 AddUserView::AddUserView(ButtonFromView* owner)
     : add_user_(nullptr), owner_(owner), anchor_(nullptr) {
+  DCHECK(!UseMd());
   AddContent();
   owner_->ForceBorderVisible(true);
 }
@@ -202,28 +247,15 @@ void AddUserView::AddContent() {
   AddChildViewAt(add_user_, 0);
 
   // Add the icon which is also the anchor for messages.
-  if (UseMd()) {
-    views::ImageView* icon = new views::ImageView();
-    icon->SetImage(
-        gfx::CreateVectorIcon(kSystemMenuNewUserIcon, kMenuIconColor));
-    icon->SetBorder(views::CreateEmptyBorder(gfx::Insets(
-        (GetTrayConstant(TRAY_POPUP_ITEM_MAIN_IMAGE_CONTAINER_WIDTH) -
-         icon->GetPreferredSize().width()) /
-        2)));
-    anchor_ = icon;
-    add_user_->AddChildView(icon);
-  } else {
-    add_user_->SetBorder(
-        views::CreateEmptyBorder(0, kTrayUserTileHoverBorderInset, 0, 0));
-    RoundedImageView* icon =
-        new RoundedImageView(kTrayRoundedBorderRadius, true);
-    anchor_ = icon;
-    icon->SetImage(*ui::ResourceBundle::GetSharedInstance()
-                        .GetImageNamed(IDR_AURA_UBER_TRAY_ADD_MULTIPROFILE_USER)
-                        .ToImageSkia(),
-                   gfx::Size(kTrayItemSize, kTrayItemSize));
-    add_user_->AddChildView(icon);
-  }
+  add_user_->SetBorder(
+      views::CreateEmptyBorder(0, kTrayUserTileHoverBorderInset, 0, 0));
+  RoundedImageView* icon = new RoundedImageView(kTrayRoundedBorderRadius, true);
+  anchor_ = icon;
+  icon->SetImage(*ui::ResourceBundle::GetSharedInstance()
+                      .GetImageNamed(IDR_AURA_UBER_TRAY_ADD_MULTIPROFILE_USER)
+                      .ToImageSkia(),
+                 gfx::Size(kTrayItemSize, kTrayItemSize));
+  add_user_->AddChildView(icon);
 
   // Add the command text.
   views::Label* command_label = new views::Label(
@@ -232,6 +264,33 @@ void AddUserView::AddContent() {
   add_user_->AddChildView(command_label);
 }
 
+// This border reserves 4dp above and 8dp below and paints a horizontal
+// separator 3dp below the host view.
+class ActiveUserBorder : public views::Border {
+ public:
+  ActiveUserBorder() {}
+  ~ActiveUserBorder() override {}
+
+  // views::Border:
+  void Paint(const views::View& view, gfx::Canvas* canvas) override {
+    canvas->FillRect(
+        gfx::Rect(
+            0, view.height() - kMenuSeparatorVerticalPadding - kSeparatorWidth,
+            view.width(), kSeparatorWidth),
+        kHorizontalSeparatorColor);
+  }
+
+  gfx::Insets GetInsets() const override {
+    return gfx::Insets(kMenuSeparatorVerticalPadding, 0,
+                       kMenuSeparatorVerticalPadding * 2, 0);
+  }
+
+  gfx::Size GetMinimumSize() const override { return gfx::Size(); }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(ActiveUserBorder);
+};
+
 }  // namespace
 
 UserView::UserView(SystemTrayItem* owner, LoginStatus login, UserIndex index)
@@ -256,24 +315,15 @@ UserView::UserView(SystemTrayItem* owner, LoginStatus login, UserIndex index)
   AddUserCard(login);
 
   if (UseMd()) {
-    auto* layout = new views::BoxLayout(views::BoxLayout::kHorizontal, 0, 0, 0);
-    if (IsActiveUser()) {
-      layout->set_inside_border_insets(gfx::Insets(
-          0, kMenuExtraMarginFromLeftEdge, 0, kMenuExtraMarginFromLeftEdge));
-    }
-
+    auto* layout = new views::BoxLayout(views::BoxLayout::kHorizontal,
+                                        kMenuExtraMarginFromLeftEdge, 0, 0);
     SetLayoutManager(layout);
     layout->set_cross_axis_alignment(
         views::BoxLayout::CROSS_AXIS_ALIGNMENT_CENTER);
     layout->SetFlexForView(user_card_view_, 1);
 
-    if (IsActiveUser()) {
-      SetBorder(views::CreatePaddedBorder(
-          views::CreateSolidSidedBorder(0, 0, kSeparatorWidth, 0,
-                                        kSeparatorColor),
-          gfx::Insets(kMenuSeparatorVerticalPadding, 0,
-                      kMenuSeparatorVerticalPadding - kSeparatorWidth, 0)));
-    }
+    if (IsActiveUser())
+      SetBorder(base::MakeUnique<ActiveUserBorder>());
   }
 }
 
@@ -519,13 +569,6 @@ void UserView::AddUserCardMd(LoginStatus login) {
     is_user_card_button_ = true;
   }
   AddChildViewAt(user_card_view_, 0);
-  // Card for supervised user can consume more space than currently
-  // available. In that case we should increase system bubble's width.
-  // TODO(estade,sgabriel): do we need this?
-  if (login == LoginStatus::PUBLIC) {
-    owner_->system_tray()->GetSystemBubble()->bubble_view()->SetWidth(
-        GetPreferredSize().width());
-  }
 }
 
 void UserView::ToggleAddUserMenuOption() {
@@ -560,26 +603,24 @@ void UserView::ToggleAddUserMenuOption() {
   SessionStateDelegate::AddUserError add_user_error;
   add_user_enabled_ = delegate->CanAddUserToMultiProfile(&add_user_error);
 
-  AddUserView* add_user_view =
-      new AddUserView(static_cast<ButtonFromView*>(user_card_view_));
-  ButtonFromView* button = new ButtonFromView(
-      add_user_view, add_user_enabled_ ? this : nullptr,
-      !UseMd() && add_user_enabled_, gfx::Insets(UseMd() ? 0 : 1));
-  if (UseMd())
+  if (UseMd()) {
+    base::Optional<SessionStateDelegate::AddUserError> error;
+    if (!add_user_enabled_)
+      error = add_user_error;
+    ButtonFromView* button = new ButtonFromView(
+        CreateAddUserView(error), add_user_enabled_ ? this : nullptr, false,
+        gfx::Insets());
     button->set_ink_drop_insets(gfx::Insets(kTrayPopupInkDropInset));
-  button->SetAccessibleName(
-      l10n_util::GetStringUTF16(IDS_ASH_STATUS_TRAY_SIGN_IN_ANOTHER_ACCOUNT));
-  button->ForceBorderVisible(true);
+    button->SetAccessibleName(
+        l10n_util::GetStringUTF16(IDS_ASH_STATUS_TRAY_SIGN_IN_ANOTHER_ACCOUNT));
+    button->ForceBorderVisible(true);
 
-  if (UseMd()) {
     // Position the widget on top of the user card view (which is still in the
     // system menu). The top half of the widget will be transparent to allow
     // the active user to show through.
     gfx::Rect bounds = user_card_view_->GetBoundsInScreen();
     bounds.set_width(bounds.width() + kSeparatorWidth);
     int row_height = bounds.height();
-    bounds.set_height(row_height * 2);
-    add_menu_option_->SetBounds(bounds);
 
     views::View* container = new AddUserWidgetContents(
         base::Bind(&UserView::RemoveAddUserMenuOption, base::Unretained(this)));
@@ -590,50 +631,71 @@ void UserView::ToggleAddUserMenuOption() {
     container->SetLayoutManager(new views::FillLayout());
     container->AddChildView(button);
     add_menu_option_->SetContentsView(container);
+
+    bounds.set_height(container->GetPreferredSize().height());
+    add_menu_option_->SetBounds(bounds);
+
+    // Show the content.
+    add_menu_option_->SetAlwaysOnTop(true);
+    add_menu_option_->Show();
+
+    // We activate the entry automatically if invoked with focus.
+    if (add_user_enabled_ && user_card_view_->HasFocus()) {
+      button->GetFocusManager()->SetFocusedView(button);
+      user_card_view_->GetFocusManager()->SetFocusedView(button);
+    }
   } else {
+    AddUserView* add_user_view =
+        new AddUserView(static_cast<ButtonFromView*>(user_card_view_));
+    ButtonFromView* button =
+        new ButtonFromView(add_user_view, add_user_enabled_ ? this : nullptr,
+                           add_user_enabled_, gfx::Insets(1));
+    button->SetAccessibleName(
+        l10n_util::GetStringUTF16(IDS_ASH_STATUS_TRAY_SIGN_IN_ANOTHER_ACCOUNT));
+    button->ForceBorderVisible(true);
+
     add_menu_option_->SetOpacity(1.f);
     add_menu_option_->SetContentsView(button);
     // Position it below our user card.
     gfx::Rect bounds = user_card_view_->GetBoundsInScreen();
     bounds.set_y(bounds.y() + bounds.height());
     add_menu_option_->SetBounds(bounds);
-  }
 
-  // Show the content.
-  add_menu_option_->SetAlwaysOnTop(true);
-  add_menu_option_->Show();
+    // Show the content.
+    add_menu_option_->SetAlwaysOnTop(true);
+    add_menu_option_->Show();
 
-  if (add_user_enabled_) {
-    // We activate the entry automatically if invoked with focus.
-    if (user_card_view_->HasFocus()) {
-      button->GetFocusManager()->SetFocusedView(button);
-      user_card_view_->GetFocusManager()->SetFocusedView(button);
-    }
-  } else {
-    ui::ResourceBundle& bundle = ui::ResourceBundle::GetSharedInstance();
-    int message_id = 0;
-    switch (add_user_error) {
-      case SessionStateDelegate::ADD_USER_ERROR_NOT_ALLOWED_PRIMARY_USER:
-        message_id = IDS_ASH_STATUS_TRAY_MESSAGE_NOT_ALLOWED_PRIMARY_USER;
-        break;
-      case SessionStateDelegate::ADD_USER_ERROR_MAXIMUM_USERS_REACHED:
-        message_id = IDS_ASH_STATUS_TRAY_MESSAGE_CANNOT_ADD_USER;
-        break;
-      case SessionStateDelegate::ADD_USER_ERROR_OUT_OF_USERS:
-        message_id = IDS_ASH_STATUS_TRAY_MESSAGE_OUT_OF_USERS;
-        break;
-      default:
-        NOTREACHED() << "Unknown adding user error " << add_user_error;
+    if (add_user_enabled_) {
+      // We activate the entry automatically if invoked with focus.
+      if (user_card_view_->HasFocus()) {
+        button->GetFocusManager()->SetFocusedView(button);
+        user_card_view_->GetFocusManager()->SetFocusedView(button);
+      }
+    } else {
+      ui::ResourceBundle& bundle = ui::ResourceBundle::GetSharedInstance();
+      int message_id = 0;
+      switch (add_user_error) {
+        case SessionStateDelegate::ADD_USER_ERROR_NOT_ALLOWED_PRIMARY_USER:
+          message_id = IDS_ASH_STATUS_TRAY_MESSAGE_NOT_ALLOWED_PRIMARY_USER;
+          break;
+        case SessionStateDelegate::ADD_USER_ERROR_MAXIMUM_USERS_REACHED:
+          message_id = IDS_ASH_STATUS_TRAY_MESSAGE_CANNOT_ADD_USER;
+          break;
+        case SessionStateDelegate::ADD_USER_ERROR_OUT_OF_USERS:
+          message_id = IDS_ASH_STATUS_TRAY_MESSAGE_OUT_OF_USERS;
+          break;
+        default:
+          NOTREACHED() << "Unknown adding user error " << add_user_error;
+      }
+
+      popup_message_.reset(new PopupMessage(
+          bundle.GetLocalizedString(
+              IDS_ASH_STATUS_TRAY_CAPTION_CANNOT_ADD_USER),
+          bundle.GetLocalizedString(message_id), PopupMessage::ICON_WARNING,
+          add_user_view->anchor(), views::BubbleBorder::TOP_LEFT,
+          gfx::Size(parent()->bounds().width() - kPopupMessageOffset, 0),
+          2 * kPopupMessageOffset));
     }
-
-    popup_message_.reset(new PopupMessage(
-        bundle.GetLocalizedString(IDS_ASH_STATUS_TRAY_CAPTION_CANNOT_ADD_USER),
-        bundle.GetLocalizedString(message_id), PopupMessage::ICON_WARNING,
-        add_user_view->anchor(), views::BubbleBorder::TOP_LEFT,
-        gfx::Size(parent()->bounds().width() - kPopupMessageOffset, 0),
-        2 * kPopupMessageOffset));
-  }
-  if (!UseMd()) {
     // Find the screen area which encloses both elements and sets then a mouse
     // watcher which will close the "menu".
     gfx::Rect area = user_card_view_->GetBoundsInScreen();