CrOS md user row adjustments

ash/common/system/user/user_card_view.cc

Index: ash/common/system/user/user_card_view.cc
diff --git a/ash/common/system/user/user_card_view.cc b/ash/common/system/user/user_card_view.cc
index 836a1cacc1e3544a4b3cb2fb92e428f90eee8ee5..e56710599f6d363d5594f13527a79aa26e4c8538 100644
--- a/ash/common/system/user/user_card_view.cc
+++ b/ash/common/system/user/user_card_view.cc
@@ -14,6 +14,7 @@
 #include "ash/common/system/tray/system_tray_delegate.h"
 #include "ash/common/system/tray/system_tray_notifier.h"
 #include "ash/common/system/tray/tray_constants.h"
+#include "ash/common/system/tray/tray_popup_item_style.h"
 #include "ash/common/system/tray/tray_utils.h"
 #include "ash/common/system/user/rounded_image_view.h"
 #include "ash/common/wm_shell.h"
@@ -29,6 +30,7 @@
 #include "ui/accessibility/ax_node_data.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/base/resource/resource_bundle.h"
+#include "ui/gfx/canvas.h"
 #include "ui/gfx/geometry/insets.h"
 #include "ui/gfx/geometry/rect.h"
 #include "ui/gfx/geometry/size.h"
@@ -362,7 +364,8 @@ void PublicAccountUserDetails::CalculatePreferredSize(int max_allowed_width) {
 
 UserCardView::UserCardView(LoginStatus login_status,
                            int max_width,
-                           int user_index) {
+                           int user_index)
+    : user_index_(user_index) {
   auto layout = new views::BoxLayout(views::BoxLayout::kHorizontal, 0, 0,
                                      UseMd() ? kTrayPopupLabelHorizontalPadding
                                              : kTrayPopupPaddingBetweenItems);
@@ -372,12 +375,17 @@ UserCardView::UserCardView(LoginStatus login_status,
         GetTrayConstant(TRAY_POPUP_ITEM_MIN_HEIGHT));
     layout->set_cross_axis_alignment(
         views::BoxLayout::CROSS_AXIS_ALIGNMENT_CENTER);
+
+    if (user_index != 0) {

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

Consider sharing UserView::IsActiveUser() logic, perhaps moving that to a common
place / making static.

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

I don't like the idea of sharing such simple logic (and adding verbosity at
callsites), but I added is_active_user() to UserCardView. It's used in precisely
two places.

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

Thanks. One person's verbosity is perhaps another person's readability.

+      SetPaintToLayer(true);
+      layer()->SetOpacity(0.5f);
+    }
   }
 
   if (login_status == LoginStatus::PUBLIC) {
     AddPublicModeUserContent(max_width);
   } else {
-    AddUserContent(login_status, user_index);
+    AddUserContent(login_status);
   }
 }
 
@@ -399,16 +407,18 @@ void UserCardView::AddPublicModeUserContent(int max_width) {
   AddChildView(new PublicAccountUserDetails(details_max_width));
 }
 
-void UserCardView::AddUserContent(LoginStatus login_status, int user_index) {
-  views::View* avatar = CreateUserAvatarView(login_status, user_index);
+void UserCardView::AddUserContent(LoginStatus login_status) {
+  if (UseMd())
+    return AddUserContentMd(login_status);
+  views::View* avatar = CreateUserAvatarView(login_status, user_index_);
   AddChildView(avatar);
   views::Label* user_name = NULL;
   SessionStateDelegate* delegate = WmShell::Get()->GetSessionStateDelegate();
-  if (!user_index) {
+  if (!user_index_) {
     base::string16 user_name_string =
         login_status == LoginStatus::GUEST
             ? l10n_util::GetStringUTF16(IDS_ASH_STATUS_TRAY_GUEST_LABEL)
-            : delegate->GetUserInfo(user_index)->GetDisplayName();
+            : delegate->GetUserInfo(user_index_)->GetDisplayName();
     if (!user_name_string.empty()) {
       user_name = new views::Label(user_name_string);
       user_name->SetHorizontalAlignment(gfx::ALIGN_LEFT);
@@ -422,7 +432,7 @@ void UserCardView::AddUserContent(LoginStatus login_status, int user_index) {
         tray_delegate->IsUserSupervised()
             ? l10n_util::GetStringUTF16(IDS_ASH_STATUS_TRAY_SUPERVISED_LABEL)
             : base::UTF8ToUTF16(
-                  delegate->GetUserInfo(user_index)->GetDisplayEmail());
+                  delegate->GetUserInfo(user_index_)->GetDisplayEmail());
     if (!user_email_string.empty()) {
       user_email = new views::Label(user_email_string);
       user_email->SetFontList(
@@ -433,7 +443,7 @@ void UserCardView::AddUserContent(LoginStatus login_status, int user_index) {
   }
 
   // Adjust text properties dependent on if it is an active or inactive user.
-  if (user_index) {
+  if (user_index_) {
     // Fade the text of non active users to 50%.
     SkColor text_color = user_email->enabled_color();
     text_color = SkColorSetA(text_color, SkColorGetA(text_color) / 2);
@@ -456,7 +466,7 @@ void UserCardView::AddUserContent(LoginStatus login_status, int user_index) {
     if (user_email) {
 #if defined(OS_CHROMEOS)
       // Only non active user can have a media indicator.
-      MediaIndicator* media_indicator = new MediaIndicator(user_index);
+      MediaIndicator* media_indicator = new MediaIndicator(user_index_);
       views::View* email_indicator_view = new views::View;
       email_indicator_view->SetLayoutManager(new views::BoxLayout(
           views::BoxLayout::kHorizontal, 0, 0, kTrayPopupPaddingBetweenItems));
@@ -476,6 +486,83 @@ void UserCardView::AddUserContent(LoginStatus login_status, int user_index) {
   }
 }
 
+void UserCardView::AddUserContentMd(LoginStatus login_status) {

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

It seems the similarity between this and the non-MD counterpart is large enough
that it might make more sense to keep within one function and just add
if(use_md) {...} else {...} in ~4-5 places. I'll leave that up to you though.
(If you leave as is, it might be worthwhile including a note in the header that
the MD version of this function is basically a fork of the non-MD version).

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

It's way harder to sprinkle use_md around and not break anything and to have the
logic be easy to follow. It's way easier to untangle MD and non-MD when the time
comes to do so if you have forked.

Regarding the note in the header --- this uses a very common naming convention
for forked/alternate code and I have a hard time understanding how someone could
fail to notice this when it comes time to rip out pre-md code since
AddUserContent would become just a call-through to AddUserContentMd.

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

This all sounds very reasonable, and as I said, I leave it up to you with regard
to the style you decide (fork vs. inline is_md checks). By forking you
theoretically run the risk of the implementations drifting, but in reality that
shouldn't happen since you're the only one touching this code and we are close
to beta branch.

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

indeed, that is another consideration that I might take into account if we were
several milestones out.

+  AddChildView(CreateUserAvatarView(login_status, user_index_));
+  views::Label* user_name = nullptr;
+  SessionStateDelegate* delegate = WmShell::Get()->GetSessionStateDelegate();
+  if (!user_index_) {
+    base::string16 user_name_string =
+        login_status == LoginStatus::GUEST
+            ? l10n_util::GetStringUTF16(IDS_ASH_STATUS_TRAY_GUEST_LABEL)
+            : delegate->GetUserInfo(user_index_)->GetDisplayName();
+    if (!user_name_string.empty()) {
+      user_name = new views::Label(user_name_string);
+      user_name->SetHorizontalAlignment(gfx::ALIGN_LEFT);
+      TrayPopupItemStyle user_name_style(nullptr,
+                                         TrayPopupItemStyle::FontStyle::TITLE);
+      user_name_style.SetupLabel(user_name);
+    }
+  }
+
+  views::Label* user_email = NULL;
+  if (login_status != LoginStatus::GUEST) {
+    SystemTrayDelegate* tray_delegate = WmShell::Get()->system_tray_delegate();
+    base::string16 user_email_string =
+        tray_delegate->IsUserSupervised()
+            ? l10n_util::GetStringUTF16(IDS_ASH_STATUS_TRAY_SUPERVISED_LABEL)
+            : base::UTF8ToUTF16(
+                  delegate->GetUserInfo(user_index_)->GetDisplayEmail());
+    if (!user_email_string.empty()) {
+      user_email = new views::Label(user_email_string);
+      user_email->SetFontList(
+          ui::ResourceBundle::GetSharedInstance().GetFontList(
+              ui::ResourceBundle::SmallFont));
+      user_email->SetHorizontalAlignment(gfx::ALIGN_LEFT);
+      TrayPopupItemStyle user_email_style(
+          nullptr, TrayPopupItemStyle::FontStyle::CAPTION);
+      user_email_style.set_color_style(
+          TrayPopupItemStyle::ColorStyle::INACTIVE);
+      user_email_style.SetupLabel(user_email);
+    }
+  }
+
+  if (user_email && user_name) {
+    views::View* details = new views::View;
+    details->SetLayoutManager(
+        new views::BoxLayout(views::BoxLayout::kVertical, 0, 0, 0));
+    details->AddChildView(user_name);
+    details->AddChildView(user_email);
+    // The name and email have different font sizes. This border is designed
+    // to make both views take up equal space so the whitespace between them
+    // is centered on the vertical midpoint.
+    user_email->SetBorder(views::CreateEmptyBorder(
+        0, 0, user_name->GetPreferredSize().height() -
+                  user_email->GetPreferredSize().height(),
+        0));
+    AddChildView(details);
+  } else if (user_name) {
+    AddChildView(user_name);
+  } else if (user_email) {
+#if defined(OS_CHROMEOS)
+    // Only non active user can have a media indicator.
+    MediaIndicator* media_indicator = new MediaIndicator(user_index_);
+    views::View* email_indicator_view = new views::View;
+    email_indicator_view->SetLayoutManager(new views::BoxLayout(
+        views::BoxLayout::kHorizontal, 0, 0, kTrayPopupPaddingBetweenItems));
+    email_indicator_view->AddChildView(user_email);
+    email_indicator_view->AddChildView(media_indicator);
+
+    views::View* details = new views::View;
+    details->SetLayoutManager(new views::BoxLayout(
+        views::BoxLayout::kVertical, 0, kUserDetailsVerticalPadding, 0));
+    details->AddChildView(email_indicator_view);
+    details->AddChildView(media_indicator->GetMessageView());
+    AddChildView(details);
+#else
+    AddChildView(user_email);
+#endif
+  }
+}
 
 }  // namespace tray
 }  // namespace ash