Reflow of the profile items in desktop user menu

chrome/browser/ui/views/profiles/profile_chooser_view.cc

 // Copyright 2014 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 "chrome/browser/ui/views/profiles/profile_chooser_view.h"
 
 #include "base/macros.h"
 #include "base/metrics/user_metrics.h"
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/browser_process.h"
@@ skipping 65 matching lines @@
 #include "ui/views/layout/box_layout.h"
 #include "ui/views/layout/fill_layout.h"
 #include "ui/views/layout/grid_layout.h"
 #include "ui/views/layout/layout_constants.h"
 #include "ui/views/widget/widget.h"
 
 namespace {
 
 // Helpers --------------------------------------------------------------------
 
-const int kFixedMenuWidth = 250;
 const int kButtonHeight = 32;
 const int kPasswordCombinedFixedGaiaViewWidth = 360;
 const int kFixedGaiaViewWidth = 448;
 const int kFixedAccountRemovalViewWidth = 280;
 const int kFixedSwitchUserViewWidth = 320;
 const int kLargeImageSide = 88;
+const int kMediumImageSide = 40;
+
+const int kTextfieldLabelHorizontalSpacing = 4;
+const int kProfileHorizontalSpacing = 16;

[Evan Stade, 2016/06/13 23:14:41]

pretty sure you should use kMaterialMenuHorizontalEdgeMargin in place of this
--- looking at the redlines, I don't think it's a coincidence that they're both
16.

[Jane, 2016/06/14 14:47:21]

Done. You have a point; I didn't think this way before.

+
+// Vertical spacing between the edge of the material design user menu and the
+// top or bottom of the menu items.
+const int kMaterialMenuVerticalEdgeMargin = 16;
+// Horizontal spacing between the edge of the material design user menu and the
+// left or right of the menu items.
+const int kMaterialMenuHorizontalEdgeMargin = 16;
 
 const int kVerticalSpacing = 16;
 
 const int kTitleViewNativeWidgetOffset = 8;
 
 bool IsProfileChooser(profiles::BubbleViewMode mode) {
   return mode == profiles::BUBBLE_VIEW_MODE_PROFILE_CHOOSER ||
       mode == profiles::BUBBLE_VIEW_MODE_FAST_PROFILE_CHOOSER;
 }
 
+// DEPRECATED: New user menu components should use views::BoxLayout instead.
 // Creates a GridLayout with a single column. This ensures that all the child
 // views added get auto-expanded to fill the full width of the bubble.
 views::GridLayout* CreateSingleColumnLayout(views::View* view, int width) {
   views::GridLayout* layout = new views::GridLayout(view);
   view->SetLayoutManager(layout);
 
   views::ColumnSet* columns = layout->AddColumnSet(0);
   columns->AddColumn(views::GridLayout::FILL, views::GridLayout::FILL, 0,
                      views::GridLayout::FIXED, width, width);
   return layout;
@@ skipping 143 matching lines @@
   gfx::Rect bounds(title_view_->GetPreferredSize());
   title_view_->SetBoundsRect(bounds);
   bounds.Offset(kTitleViewNativeWidgetOffset, kTitleViewNativeWidgetOffset);
   title_widget_->SetBounds(bounds);
 }
 
 }  // namespace
 
 // RightAlignedIconLabelButton -------------------------------------------------
 
-// A custom LabelButton that has a centered text and right aligned icon.
+// A custom LabelButton that has a left-aligned text and right aligned icon.
+// For non-material-design user menu, it has centered text instead.
 class RightAlignedIconLabelButton : public views::LabelButton {
  public:
   RightAlignedIconLabelButton(views::ButtonListener* listener,
                               const base::string16& text)
       : views::LabelButton(listener, text) {
     SetHorizontalAlignment(gfx::ALIGN_RIGHT);
-    label()->SetHorizontalAlignment(gfx::ALIGN_CENTER);
+    label()->SetHorizontalAlignment(switches::IsMaterialDesignUserMenu()
+                                        ? gfx::ALIGN_LEFT
+                                        : gfx::ALIGN_CENTER);
   }
 
  protected:
   void Layout() override {
     views::LabelButton::Layout();
 
     // Keep the text centered and the icon right-aligned by stretching the label
     // to take up more of the content area and centering its contents.
     gfx::Rect content_bounds = GetContentsBounds();
     gfx::Rect label_bounds = label()->bounds();
@@ skipping 17 matching lines @@
 
 // A custom Image control that shows a "change" button when moused over.
 class EditableProfilePhoto : public views::LabelButton {
  public:
   EditableProfilePhoto(views::ButtonListener* listener,
                        const gfx::Image& icon,
                        bool is_editing_allowed,
                        const gfx::Rect& bounds)
       : views::LabelButton(listener, base::string16()),
         photo_overlay_(NULL) {
-    gfx::Image image = profiles::GetSizedAvatarIcon(
-        icon, true, kLargeImageSide, kLargeImageSide);
+    const int icon_image_side = switches::IsMaterialDesignUserMenu()
+                                    ? kMediumImageSide
+                                    : kLargeImageSide;
+    gfx::Image image = profiles::GetSizedAvatarIcon(icon, true, icon_image_side,
+                                                    icon_image_side);
     SetImage(views::LabelButton::STATE_NORMAL, *image.ToImageSkia());
     SetBorder(views::Border::NullBorder());
     SetBoundsRect(bounds);
 
     // Calculate the circular mask that will be used to display the photo.
     circular_mask_.addCircle(SkIntToScalar(bounds.width() / 2),
                              SkIntToScalar(bounds.height() / 2),
                              SkIntToScalar(bounds.width() / 2));
 
     if (!is_editing_allowed) {
       SetEnabled(false);
       return;
     }
 
     set_notify_enter_exit_on_child(true);
 
     // Photo overlay that appears when hovering over the button.
     photo_overlay_ = new views::ImageView();
 
     const SkColor kBackgroundColor = SkColorSetARGB(65, 255, 255, 255);
     photo_overlay_->set_background(
         views::Background::CreateSolidBackground(kBackgroundColor));
-    photo_overlay_->SetImage(gfx::CreateVectorIcon(
-        gfx::VectorIconId::PHOTO_CAMERA, 48u, SkColorSetRGB(0x33, 0x33, 0x33)));
+    photo_overlay_->SetImage(
+        gfx::CreateVectorIcon(gfx::VectorIconId::PHOTO_CAMERA,
+                              switches::IsMaterialDesignUserMenu() ? 22u : 48u,
+                              SkColorSetRGB(0x33, 0x33, 0x33)));
 
     photo_overlay_->SetSize(bounds.size());
     photo_overlay_->SetVisible(false);
     AddChildView(photo_overlay_);
   }
 
   void OnPaint(gfx::Canvas* canvas) override {
     // Display the profile picture as a circle.
     canvas->ClipPath(circular_mask_, true);
     views::LabelButton::OnPaint(canvas);
@@ skipping 48 matching lines @@
                       bool is_editing_allowed)
       : button_(nullptr), label_(nullptr), profile_name_textfield_(nullptr) {
     SetLayoutManager(
         new views::BoxLayout(views::BoxLayout::kVertical, 0, 0, 0));
 
     ui::ResourceBundle* rb = &ui::ResourceBundle::GetSharedInstance();
     const gfx::FontList& medium_font_list =
         rb->GetFontList(ui::ResourceBundle::MediumFont);
 
     if (!is_editing_allowed) {
       label_ = new views::Label(text);

[Evan Stade, 2016/06/13 23:14:40]

since you asked me to find other constants I don't like, I am looking more
closely at this file. Why is label_ a member var at all?

[Jane, 2016/06/14 14:47:21]

Done.
I appreciate the close attention that you are paying on this! 
Now that you pointed it out, I agree with you that label_ has no apparent reason
to be a member variable, but I wouldn't know why it was written this way - I
simply didn't think it was necessary for me to go through other parts of this
file that I'm not really touching and clean up existing code that other
reviewers have approved. Also as a matter of fact, this class in particular will
be completely removed once the new menu gets rolled out (because profile name
will not be editable).
But since this is a small change, I went in and made it a local variable.

       label_->SetBorder(views::Border::CreateEmptyBorder(2, 0, 2, 0));

[Evan Stade, 2016/06/13 23:14:41]

are these 2s distinct from the 2s below?

[Jane, 2016/06/14 14:47:21]

I assume so, but then again, I'm not touching any of the 2s myself. Are you
saying it's necessary for me to make a constant for this?

[Evan Stade, 2016/06/14 15:16:26]

I'm a bit perplexed by your stance on pre-existing code. When you add new code
that is related to pre-existing code, you often make changes or refactors to
share logic or constants, and this requires modifying said pre-existing code. So
yes, it is necessary to evaluate and adjust the code that was in this file prior
to this CL.

Unless I misunderstand you, you're also implying that if one reviewer approves,
all reviewers should approve. Sad to say, sometimes a reviewer misses something.
If someone could review code perfectly they could write it perfectly as well and
we'd probably not need a review process in the first place.

[Jane, 2016/06/14 21:39:57]

Done. Makes sense, I set 2 to be border_width.

       label_->SetFontList(medium_font_list);
+      if (switches::IsMaterialDesignUserMenu())
+        label_->SetHorizontalAlignment(gfx::ALIGN_LEFT);
       AddChildView(label_);
       return;
     }
 
     button_ = new RightAlignedIconLabelButton(this, text);
     button_->SetFontList(medium_font_list);
     // Show an "edit" pencil icon when hovering over. In the default state,
     // we need to create an empty placeholder of the correct size, so that
     // the text doesn't jump around when the hovered icon appears.
     // TODO(estade): revisit colors and press effect.
     const int kIconSize = 16;
     button_->SetImage(views::LabelButton::STATE_NORMAL,
                       CreateSquarePlaceholderImage(kIconSize));
     button_->SetImage(views::LabelButton::STATE_HOVERED,
                       gfx::CreateVectorIcon(
                           gfx::VectorIconId::MODE_EDIT, kIconSize,
                           SkColorSetRGB(0x33, 0x33, 0x33)));
     button_->SetImage(views::LabelButton::STATE_PRESSED,
                       gfx::CreateVectorIcon(
                           gfx::VectorIconId::MODE_EDIT, kIconSize,
                           SkColorSetRGB(0x20, 0x20, 0x20)));
-    // To center the text, we need to offest it by the width of the icon we
-    // are adding and its padding. We need to also add a small top/bottom
-    // padding to account for the textfield's border.
-    const int kIconTextLabelButtonSpacing = 5;
-    button_->SetBorder(views::Border::CreateEmptyBorder(
-        2, kIconSize + kIconTextLabelButtonSpacing, 2, 0));
+    // We need to add a left padding as well as a small top/bottom padding
+    // to the text to account for the textfield's border.
+    if (switches::IsMaterialDesignUserMenu()) {
+      button_->SetBorder(views::Border::CreateEmptyBorder(

[Evan Stade, 2016/06/13 23:14:40]

OK, I may have misunderstood the purpose of kTextfieldLabelHorizontalSpacing.
Why can't you call Textfield::GetInsets() for these values?

[Jane, 2016/06/14 14:47:21]

Done. Thanks, didn't know this before.
(Although this variable will be removed in the downstream CL too... I realize
that it would have probably been a better idea to consolidate them.)

+          2, kTextfieldLabelHorizontalSpacing, 2, 0));
+    } else {
+      const int kIconTextLabelButtonSpacing = 5;
+      button_->SetBorder(views::Border::CreateEmptyBorder(
+          2, kIconSize + kIconTextLabelButtonSpacing, 2, 0));
+    }
     AddChildView(button_);
 
     profile_name_textfield_ = new views::Textfield();
     // Textfield that overlaps the button.
     profile_name_textfield_->set_controller(controller);
     profile_name_textfield_->SetFontList(medium_font_list);
-    profile_name_textfield_->SetHorizontalAlignment(gfx::ALIGN_CENTER);
+    profile_name_textfield_->SetHorizontalAlignment(
+        switches::IsMaterialDesignUserMenu() ? gfx::ALIGN_LEFT
+                                             : gfx::ALIGN_CENTER);
     profile_name_textfield_->SetVisible(false);
     AddChildView(profile_name_textfield_);
   }
 
   views::Textfield* profile_name_textfield() {
     return profile_name_textfield_;
   }
 
   // Hide the editable textfield to show the profile name button instead.
   void ShowReadOnlyView() {
@@ skipping 106 matching lines @@
 
   views::ImageButton* back_button_;
   views::Label* title_label_;
 
   DISALLOW_COPY_AND_ASSIGN(TitleCard);
 };
 
 // ProfileBadge --------------------------------------------------------
 
 const size_t kProfileBadgeSize = 30;
+const size_t kSmallProfileBadgeSize = 24;
 const size_t kProfileBadgeWhitePadding = 2;
 
 // Draws a white circle, then a light blue circle, then a dark blue icon.
 class ProfileBadge : public gfx::CanvasImageSource {
  public:
   ProfileBadge(gfx::VectorIconId id, size_t icon_size)
-      : CanvasImageSource(gfx::Size(kProfileBadgeSize, kProfileBadgeSize),
+      : CanvasImageSource(gfx::Size(switches::IsMaterialDesignUserMenu()
+                                        ? kSmallProfileBadgeSize
+                                        : kProfileBadgeSize,
+                                    switches::IsMaterialDesignUserMenu()
+                                        ? kSmallProfileBadgeSize
+                                        : kProfileBadgeSize),
                           false),
         id_(id),
         icon_size_(icon_size) {}
 
   ~ProfileBadge() override {}
 
   // CanvasImageSource:
   void Draw(gfx::Canvas* canvas) override {
     const SkISize size = canvas->sk_canvas()->getBaseLayerSize();
     gfx::Rect bounds(0, 0, size.width(), size.height());
 
     SkPaint paint;
     paint.setAntiAlias(true);
     paint.setColor(SK_ColorWHITE);
     canvas->DrawCircle(bounds.CenterPoint(), size.width() / 2, paint);
 
     paint.setColor(SkColorSetRGB(0xAF, 0xD9, 0xFC));
     canvas->DrawCircle(bounds.CenterPoint(),
                        size.width() / 2 - kProfileBadgeWhitePadding, paint);
 
-    int offset = (kProfileBadgeSize - icon_size_) / 2;
+    int offset = ((switches::IsMaterialDesignUserMenu() ? kSmallProfileBadgeSize

[Evan Stade, 2016/06/13 23:14:40]

this ternary appears 3 times

[Jane, 2016/06/14 14:47:21]

I did realize this and I wanted to make it a const right at where
kSmallProfileBadgeSize and kProfileBadgeSize are defined, but I get segfaults by
calling the switches function there, so I thought maybe having them remain this
way would be easier. However, if you think it's necessary, I can make a static
function for this.

[Evan Stade, 2016/06/14 15:16:26]

I think that would be an improvement. The badge size constants need not be
defined separately from that function.

[Jane, 2016/06/14 21:39:56]

Done.

+                                                        : kProfileBadgeSize) -
+                  icon_size_) / 2;
     canvas->Translate(gfx::Vector2d(offset, offset));
     gfx::PaintVectorIcon(canvas, id_, icon_size_, SkColorSetRGB(0, 0x66, 0xff));
   }
 
  private:
   const gfx::VectorIconId id_;
   const size_t icon_size_;
 
   DISALLOW_COPY_AND_ASSIGN(ProfileBadge);
 };
 
 gfx::ImageSkia CreateBadgeForProfile(Profile* profile) {
   ProfileBadge* badge =
       profile->IsChild()
-          ? new ProfileBadge(gfx::VectorIconId::ACCOUNT_CHILD_INVERT, 26)
-          : new ProfileBadge(gfx::VectorIconId::SUPERVISOR_ACCOUNT, 20);
-
+          ? new ProfileBadge(gfx::VectorIconId::ACCOUNT_CHILD_INVERT,
+                             switches::IsMaterialDesignUserMenu() ? 21 : 26)
+          : new ProfileBadge(gfx::VectorIconId::SUPERVISOR_ACCOUNT,
+                             switches::IsMaterialDesignUserMenu() ? 16 : 20);
   return gfx::ImageSkia(badge, badge->size());
 }
 
 // ProfileChooserView ---------------------------------------------------------
 
 // static
 ProfileChooserView* ProfileChooserView::profile_bubble_ = nullptr;
 bool ProfileChooserView::close_on_deactivate_for_testing_ = true;
 
 // static
@@ skipping 34 matching lines @@
 bool ProfileChooserView::IsShowing() {
   return profile_bubble_ != NULL;
 }
 
 // static
 void ProfileChooserView::Hide() {
   if (IsShowing())
     profile_bubble_->GetWidget()->Close();
 }
 
+// static
+int ProfileChooserView::GetFixedMenuWidth() {
+  return switches::IsMaterialDesignUserMenu() ? 240 : 250;
+}
+
 ProfileChooserView::ProfileChooserView(views::View* anchor_view,
                                        Browser* browser,
                                        profiles::BubbleViewMode view_mode,
                                        profiles::TutorialMode tutorial_mode,
                                        signin::GAIAServiceType service_type,
                                        signin_metrics::AccessPoint access_point)
     : BubbleDialogDelegateView(anchor_view, views::BubbleBorder::TOP_RIGHT),
       browser_(browser),
       view_mode_(view_mode),
       tutorial_mode_(tutorial_mode),
@@ skipping 155 matching lines @@
       break;
     case profiles::BUBBLE_VIEW_MODE_SWITCH_USER:
       layout = CreateSingleColumnLayout(this, kFixedSwitchUserViewWidth);
       sub_view = CreateSwitchUserView();
       ProfileMetrics::LogProfileNewAvatarMenuNotYou(
           ProfileMetrics::PROFILE_AVATAR_MENU_NOT_YOU_VIEW);
       break;
     case profiles::BUBBLE_VIEW_MODE_ACCOUNT_MANAGEMENT:
     case profiles::BUBBLE_VIEW_MODE_PROFILE_CHOOSER:
     case profiles::BUBBLE_VIEW_MODE_FAST_PROFILE_CHOOSER:
-      layout = CreateSingleColumnLayout(this, kFixedMenuWidth);
+      layout = CreateSingleColumnLayout(this, GetFixedMenuWidth());
       sub_view = CreateProfileChooserView(avatar_menu);
       break;
   }
   // Clears tutorial mode for all non-profile-chooser views.
   if (view_mode_ != profiles::BUBBLE_VIEW_MODE_PROFILE_CHOOSER)
     tutorial_mode_ = profiles::TUTORIAL_MODE_NONE;
 
   layout->StartRow(1, 0);
   layout->AddView(sub_view);
   Layout();
@@ skipping 229 matching lines @@
   Indexes other_profiles;
   views::View* tutorial_view = NULL;
   views::View* current_profile_view = NULL;
   views::View* current_profile_accounts = NULL;
   views::View* option_buttons_view = NULL;
   for (size_t i = 0; i < avatar_menu->GetNumberOfItems(); ++i) {
     const AvatarMenu::Item& item = avatar_menu->GetItemAt(i);
     if (item.active) {
       option_buttons_view = CreateOptionsView(
           item.signed_in && profiles::IsLockAvailable(browser_->profile()));
-      current_profile_view = CreateCurrentProfileView(item, false);
+      current_profile_view =
+          switches::IsMaterialDesignUserMenu()
+              ? CreateMaterialDesignCurrentProfileView(item, false)
+              : CreateCurrentProfileView(item, false);
       if (IsProfileChooser(view_mode_)) {
         tutorial_view = CreateTutorialViewIfNeeded(item);
       } else {
         current_profile_accounts = CreateCurrentProfileAccountsView(item);
       }
     } else {
       other_profiles.push_back(i);
     }
   }
 
@@ skipping 16 matching lines @@
 
   if (!IsProfileChooser(view_mode_)) {
     DCHECK(current_profile_accounts);
     layout->StartRow(0, 0);
     layout->AddView(new views::Separator(views::Separator::HORIZONTAL));
     layout->StartRow(1, 0);
     layout->AddView(current_profile_accounts);
   }
 
   if (browser_->profile()->IsSupervised()) {
-    layout->StartRow(0, 0);
-    layout->AddView(new views::Separator(views::Separator::HORIZONTAL));
+    if (!switches::IsMaterialDesignUserMenu()) {
+      layout->StartRow(0, 0);
+      layout->AddView(new views::Separator(views::Separator::HORIZONTAL));
+    }
     layout->StartRow(1, 0);
     layout->AddView(CreateSupervisedUserDisclaimerView());
   }
 
   layout->StartRow(0, 0);
   layout->AddView(new views::Separator(views::Separator::HORIZONTAL));
 
   if (option_buttons_view) {
     layout->StartRow(0, 0);
     layout->AddView(option_buttons_view);
@@ skipping 11 matching lines @@
     }
   }
 
   layout->StartRow(1, 0);
   layout->AddView(CreateOtherProfilesView(other_profiles));
 }
 
 views::View* ProfileChooserView::CreateProfileChooserView(
     AvatarMenu* avatar_menu) {
   views::View* view = new views::View();
-  views::GridLayout* layout = CreateSingleColumnLayout(view, kFixedMenuWidth);
+  views::GridLayout* layout =
+      CreateSingleColumnLayout(view, GetFixedMenuWidth());
 
   if (view_mode_ == profiles::BUBBLE_VIEW_MODE_FAST_PROFILE_CHOOSER) {
     PopulateMinimalProfileChooserView(layout, avatar_menu);
     // The user is using right-click switching, no need to tell them about it.
     PrefService* local_state = g_browser_process->local_state();
     local_state->SetBoolean(
         prefs::kProfileAvatarRightClickTutorialDismissed, true);
   } else {
     PopulateCompleteProfileChooserView(layout, avatar_menu);
   }
@@ skipping 54 matching lines @@
     const base::string16& button_text,
     bool stack_button,
     views::Link** link,
     views::LabelButton** button,
     views::ImageButton** close_button) {
   tutorial_mode_ = tutorial_mode;
 
   views::View* view = new views::View();
   view->set_background(views::Background::CreateSolidBackground(
       profiles::kAvatarTutorialBackgroundColor));
-  views::GridLayout* layout = CreateSingleColumnLayout(view,
-      kFixedMenuWidth - 2 * views::kButtonHEdgeMarginNew);
+  views::GridLayout* layout = CreateSingleColumnLayout(
+      view, GetFixedMenuWidth() - 2 * views::kButtonHEdgeMarginNew);
   // Creates a second column set for buttons and links.
   views::ColumnSet* button_columns = layout->AddColumnSet(1);
   button_columns->AddColumn(views::GridLayout::LEADING,
       views::GridLayout::CENTER, 0, views::GridLayout::USE_PREF, 0, 0);
   button_columns->AddPaddingColumn(
       1, views::kUnrelatedControlHorizontalSpacing);
   button_columns->AddColumn(views::GridLayout::TRAILING,
       views::GridLayout::CENTER, 0, views::GridLayout::USE_PREF, 0, 0);
   layout->SetInsets(views::kButtonVEdgeMarginNew,
                     views::kButtonHEdgeMarginNew,
@@ skipping 81 matching lines @@
       layout->SkipColumns(1);
   }
 
   return view;
 }
 
 views::View* ProfileChooserView::CreateCurrentProfileView(
     const AvatarMenu::Item& avatar_item,
     bool is_guest) {
   views::View* view = new views::View();
-  int column_width = kFixedMenuWidth - 2 * views::kButtonHEdgeMarginNew;
+  int column_width = GetFixedMenuWidth() - 2 * views::kButtonHEdgeMarginNew;
   views::GridLayout* layout = CreateSingleColumnLayout(view, column_width);
   layout->SetInsets(views::kButtonVEdgeMarginNew,
                     views::kButtonHEdgeMarginNew,
                     views::kUnrelatedControlVerticalSpacing,
                     views::kButtonHEdgeMarginNew);
 
   // Profile icon, centered.
   int x_offset = (column_width - kLargeImageSide) / 2;
   current_profile_photo_ = new EditableProfilePhoto(
       this, avatar_item.icon, !is_guest,
@@ skipping 92 matching lines @@
       layout->StartRow(1, 0);
       layout->AddView(signin_current_profile_link_);
       content::RecordAction(
           base::UserMetricsAction("Signin_Impression_FromAvatarBubbleSignin"));
     }
   }
 
   return view;
 }
 
+views::View* ProfileChooserView::CreateMaterialDesignCurrentProfileView(
+    const AvatarMenu::Item& avatar_item,
+    bool is_guest) {
+  views::View* view = new views::View();
+  int column_width =

[Evan Stade, 2016/06/13 23:14:41]

are you sure the box layout can't grow wider than this?

[Jane, 2016/06/14 14:47:21]

I'm only using column_width as the width for profile_container but not the box
layout if that's what you are asking. 

[Evan Stade, 2016/06/14 15:16:26]

what sets the width of the box layout then?

[Jane, 2016/06/14 21:39:57]

I thought the width of the box layout would be determined by the width of its
children and its inside_border_horizontal_spacing?

[Evan Stade, 2016/06/16 21:56:34]

Well, that would set its preferred size. But in this case you're adding the box
layout to a grid layout, and that grid layout is probably (or should be) making
it fill a pre-set width, so I'm not entirely sure why this is necessary.

[Jane, 2016/06/17 19:52:23]

Done. Removed column_width.

+      GetFixedMenuWidth() - 2 * kMaterialMenuHorizontalEdgeMargin;
+  view->SetLayoutManager(new views::BoxLayout(
+      views::BoxLayout::kVertical,
+      kMaterialMenuHorizontalEdgeMargin,
+      views::kRelatedControlVerticalSpacing,
+      views::kRelatedControlVerticalSpacing));
+
+  // Profile picture, left-aligned.
+  const int profile_bar_height = 56;
+  current_profile_photo_ = new EditableProfilePhoto(
+      this, avatar_item.icon, !is_guest,
+      gfx::Rect(0, (profile_bar_height - kMediumImageSide) / 2,
+                kMediumImageSide, kMediumImageSide));

[Evan Stade, 2016/06/13 23:14:40]

Yyou're doing layout at construction time, which is always going to be more
complicated than using a layout manager or overriding Layout(). I don't think
you even need to specify a size here because LabelButton will naturally use the
image for sizing. 

give profile_container a horizontal box layout with inside border vertical
spacing of 8 and between child spacing of kMaterialMenuHorizontalEdgeMargin

[Jane, 2016/06/14 14:47:21]

This is a very good suggestion. I thought about using a box layout and tried
doing it, but I think this is a more complicated case: the profile photo might
have a badge attached to it, so another layout/parent view is needed to hold
them together; the avatar name and email address needs to in the same column
too; and EditableProfileName needs to be closer to the profile photo than
kMaterialMenuHorizontalEdgeMargin because the textfield insets need to be
accounted. If anything, maybe I should create two columns for this. However, I
really think it would be easier to make it happen in the downstream CL because
some of these components will be simplified or removed. Let me know if you think
this is ok for now and what I should do as next step.
Also, I assume you are talking about EditableProfilePhoto when you said I don't
need to specify a size for LabelButton. You make a point, but then again, I
don't plan to investigate on this pre-existing code that will be removed soon,
unless you deem it necessary.

[Evan Stade, 2016/06/14 15:16:26]

When I said "or overriding Layout()", I guess I should have specified that this
is the typical way of handling very custom layout. That said, I don't think it's
too difficult to add a couple container views or conditionally adjust insets or
borders.
why are you adding code that uses EditableProfilePhoto if you're about to remove
EditableProfilePhoto?

[Jane, 2016/06/14 21:39:57]

Done. I added a couple of containers to the view, does it look better now?
Oops sorry, I got this mixed up with the name class. So the reason for setting
the bound of the profile photo is mostly for the positioning variables. The
current user menu needs to position the photo at the right x-position while the
material design user menu needs to position the photo at the right y-position. 

[Evan Stade, 2016/06/16 21:56:33]

SizedContainers are just a different way of hardcoding layout.
the right y-position? I'm not sure I follow, but I still think this is awkward.
You should let the profile photo be sized according to its preferred size and
positioned by the layout of its parent view. If you're using a horizontal box
layout, for example, the width would automatically be set to the preferred width
while the vertical centering would be achieved with set_cross_axis_alignment

[Jane, 2016/06/17 19:52:23]

Point taken. See edits/comments in the newest patch.
I meant "the correct y-position".
This makes sense. I removed the last argument (bounds) for the constructor, and
set its size to be preferred size instead. However, I couldn't think of a way to
use box layout for profile_photo_container (see comment in the newer patch), so
I'm still kind of hardcoding the position of EditableProfilePhoto using SetY.
Let me know if you know of a cleaner way!

+  SizedContainer* profile_container =
+      new SizedContainer(gfx::Size(column_width, profile_bar_height));
+  profile_container->AddChildView(current_profile_photo_);
+
+  // Add supervised badge for supervised profile.
+  if (browser_->profile()->IsSupervised()) {
+    const int badge_spacing = 4;
+    views::ImageView* supervised_icon = new views::ImageView();
+    supervised_icon->SetImage(CreateBadgeForProfile(browser_->profile()));
+    gfx::Size preferred_size = supervised_icon->GetPreferredSize();
+    gfx::Rect parent_bounds = current_profile_photo_->bounds();
+    supervised_icon->SetBounds(
+        parent_bounds.right() - preferred_size.width() + badge_spacing,
+        parent_bounds.bottom() - preferred_size.height() + badge_spacing,
+        preferred_size.width(), preferred_size.height());
+    profile_container->AddChildView(supervised_icon);
+  }
+
+  // Profile name, left-aligned to the right of profile icon.
+  bool editing_allowed =
+      !is_guest && !browser_->profile()->IsLegacySupervised();
+  current_profile_name_ = new EditableProfileName(
+      this, profiles::GetAvatarNameForProfile(browser_->profile()->GetPath()),
+      editing_allowed);
+  gfx::Rect photo_bounds = current_profile_photo_->bounds();
+  const int textfield_left_margin =
+      kProfileHorizontalSpacing -
+      (editing_allowed ? kTextfieldLabelHorizontalSpacing : 0);
+  current_profile_name_->SetBounds(
+      photo_bounds.width() + textfield_left_margin,
+      views::kRelatedControlVerticalSpacing,
+      profile_container->GetPreferredSize().width() - photo_bounds.width() -
+          textfield_left_margin,
+      profile_container->GetPreferredSize().height() / 2 -
+          views::kRelatedControlVerticalSpacing);
+  profile_container->AddChildView(current_profile_name_);
+
+  // Center-align the avatar name if the profile is not signed in.
+  if (is_guest || !avatar_item.signed_in) {
+    current_profile_name_->SetY(
+        (profile_container->GetPreferredSize().height() -
+         current_profile_name_->height()) /
+        2);
+    view->AddChildView(profile_container);

[Evan Stade, 2016/06/13 23:14:40]

can't you do this unconditionally?

[Jane, 2016/06/14 14:47:21]

No, because more child views will be added to profile_container if the profile
is signed in. I can't do this all at the end either, because the guest view has
to be returned right below.

[Evan Stade, 2016/06/14 15:16:26]

Not sure I understand. Why can't you add child views to profile_container after
adding profile_container to view?

[Jane, 2016/06/14 21:39:56]

Done.
What I meant was more along the line of how the code would be easier to
understand if the view is only added after all its child views are in place -
that's the impression I got from existing code. But following your comment, I
moved this line outside.

+  }
+
+  if (is_guest)
+    return view;
+
+  // The available links depend on the type of profile that is active.
+  if (avatar_item.signed_in) {
+    // Calculate the position and size for links available for signed-in
+    // profiles.
+    const int x_coordinate = photo_bounds.width() + kProfileHorizontalSpacing;

[Evan Stade, 2016/06/13 23:14:40]

If profile_container had a layout manager this would be simpler.

[Jane, 2016/06/14 14:47:21]

Same as above.

+    const int y_coordinate = profile_container->GetPreferredSize().height() / 2;
+    const int width = profile_container->GetPreferredSize().width() -
+                      photo_bounds.width() - kProfileHorizontalSpacing;
+    const int height = profile_container->GetPreferredSize().height() / 2 -
+                       views::kRelatedControlVerticalSpacing;
+
+    if (switches::IsEnableAccountConsistency()) {
+      base::string16 link_title = l10n_util::GetStringUTF16(
+          IsProfileChooser(view_mode_)
+              ? IDS_PROFILES_PROFILE_MANAGE_ACCOUNTS_BUTTON
+              : IDS_PROFILES_PROFILE_HIDE_MANAGE_ACCOUNTS_BUTTON);
+      manage_accounts_link_ = CreateLink(link_title, this);
+      manage_accounts_link_->SetHorizontalAlignment(gfx::ALIGN_LEFT);
+      manage_accounts_link_->SetBounds(x_coordinate, y_coordinate, width,
+                                       height);
+      profile_container->AddChildView(manage_accounts_link_);
+    } else if (HasAuthError(browser_->profile())) {
+      // Badge the email address if there's an authentication error.
+      auth_error_email_button_ =
+          new RightAlignedIconLabelButton(this, avatar_item.username);
+      auth_error_email_button_->SetElideBehavior(gfx::ELIDE_EMAIL);
+      auth_error_email_button_->SetImage(
+          views::LabelButton::STATE_NORMAL,
+          gfx::CreateVectorIcon(gfx::VectorIconId::WARNING, 12,
+                                gfx::kChromeIconGrey));
+      auth_error_email_button_->SetFocusForPlatform();
+      auth_error_email_button_->set_request_focus_on_press(true);

[Evan Stade, 2016/06/13 23:14:40]

why do you want this non-default behavior

[Jane, 2016/06/14 14:47:21]

As mentioned before, this code is basically copied from the function above
CreateCurrentProfileView (e.g. this line corresponds to L1456), and this code
will be heavily modified as the user menu revamp project proceeds. I didn't take
a close look at why this was done and I assumed it was reasonable as it is from
existing code. As a matter of fact, the lines under this particular condition is
getting removed in the downstream CL. I don't plan to investigate on this unless
you think it's necessary.

[Evan Stade, 2016/06/14 15:16:26]

If there are large blocks of code you're about to delete, you should clearly
mark them as such with disclaimers like TODO(yourname): Delete this entire block
of code: crbug.com/mybug

Generally speaking I'm not a huge fan of copying code without analyzing whether
it still makes sense.

[Jane, 2016/06/14 21:39:57]

I just deleted this block instead, this is probably the easier solution.
I understand that I should analyze the code while copying it, but the intention
here was to break up a potentially huge CL in two, so that this CL deals more
with getting the same set of functions going for the new menu while making some
UI adjustments; the downstream CL that branches off this CL performs a more
comprehensive revamp. This piece of code is deleted right at the said downstream
CL. Again, I realize that it would have probably been a better idea to not split
up the CL actually.
Btw I can't own a bug.

+      gfx::Insets insets =
+          views::LabelButtonAssetBorder::GetDefaultInsetsForStyle(
+              views::Button::STYLE_TEXTBUTTON);
+      auth_error_email_button_->SetBorder(views::Border::CreateEmptyBorder(
+          insets.top(), insets.left(), insets.bottom(), insets.right()));
+      auth_error_email_button_->SetBounds(x_coordinate - insets.left(),
+                                          y_coordinate, width, height);
+      profile_container->AddChildView(auth_error_email_button_);
+    } else {
+      views::Label* email_label = new views::Label(avatar_item.username);
+      email_label->SetElideBehavior(gfx::ELIDE_EMAIL);
+      email_label->SetEnabled(false);
+      email_label->SetHorizontalAlignment(gfx::ALIGN_LEFT);
+      email_label->SetBounds(x_coordinate, y_coordinate, width, height);
+      profile_container->AddChildView(email_label);
+    }
+    view->AddChildView(profile_container);
+    return view;
+  }
+
+  SigninManagerBase* signin_manager = SigninManagerFactory::GetForProfile(
+      browser_->profile()->GetOriginalProfile());
+  if (signin_manager->IsSigninAllowed()) {
+    views::Label* promo =
+        new views::Label(l10n_util::GetStringUTF16(IDS_PROFILES_SIGNIN_PROMO));
+    promo->SetMultiLine(true);
+    promo->SetHorizontalAlignment(gfx::ALIGN_LEFT);
+    view->AddChildView(promo);
+
+    signin_current_profile_link_ =

[Evan Stade, 2016/06/13 23:14:40]

this is not really a link...

[Jane, 2016/06/14 14:47:21]

Done.
Same as above, but since this code will live, I modified its name everywhere.

+        views::MdTextButton::CreateSecondaryUiBlueButton(
+            this, l10n_util::GetStringFUTF16(
+                      IDS_SYNC_START_SYNC_BUTTON_LABEL,
+                      l10n_util::GetStringUTF16(IDS_SHORT_PRODUCT_NAME)));
+    signin_current_profile_link_->SetMinSize(gfx::Size(0, 36));

[Evan Stade, 2016/06/13 23:14:41]

here's a constant I don't understand the need for

[Jane, 2016/06/14 14:47:21]

This was supposed to define the height of the button. Do you think this line is
unnecessary?

[Evan Stade, 2016/06/14 15:16:26]

yes, i think the height should be the same as all the other buttons in chrome

[Jane, 2016/06/14 21:39:57]

I put it here because SetMinSize of LabelButton sets the minimum height to 33,
but the redlines specify 36. I will remove this line if you think it's safe to
ignore.

[Evan Stade, 2016/06/16 21:56:33]

Yes, I do think you can/should. For example, there are changes in flight to make
all MD buttons just 24px tall (a result of the UI leads providing feedback on
the current, taller buttons). Whatever we land on for the rest of Chrome, you'll
want the same here.

[Jane, 2016/06/17 19:52:23]

Done.

+    view->AddChildView(signin_current_profile_link_);
+    content::RecordAction(
+        base::UserMetricsAction("Signin_Impression_FromAvatarBubbleSignin"));
+    view->AddChildView(padding_view);

[Evan Stade, 2016/06/13 23:14:40]

if you need padding under the md text button, perhaps you can add an empty
border to |view|, or you can add an empty SizedContainer view

[Jane, 2016/06/14 14:47:21]

I tried adding:

    view->SetBorder(views::Border::CreateEmptyBorder(
        0, 0, views::kRelatedControlVerticalSpacing, 0));

...but no extra padding was shown. Why?
So now I'm using a SizedContainer of height 1. Would you say it's ok to have an
extra height of 1?

[Evan Stade, 2016/06/14 15:16:26]

I dunno, is there something else setting a border on view? I suggest
investigating because "why" is a good question and might have an interesting
answer.

[Jane, 2016/06/14 21:39:57]

Done. It works now.

+  }
+
+  return view;
+}
+
 views::View* ProfileChooserView::CreateGuestProfileView() {
   gfx::Image guest_icon =
       ui::ResourceBundle::GetSharedInstance().GetImageNamed(
           profiles::GetPlaceholderAvatarIconResourceID());
   AvatarMenu::Item guest_avatar_item(0, base::FilePath(), guest_icon);
   guest_avatar_item.active = true;
   guest_avatar_item.name = l10n_util::GetStringUTF16(
       IDS_PROFILES_GUEST_PROFILE_NAME);
   guest_avatar_item.signed_in = false;
 
-  return CreateCurrentProfileView(guest_avatar_item, true);
+  return switches::IsMaterialDesignUserMenu()
+             ? CreateMaterialDesignCurrentProfileView(guest_avatar_item, true)
+             : CreateCurrentProfileView(guest_avatar_item, true);
 }
 
 views::View* ProfileChooserView::CreateOtherProfilesView(
     const Indexes& avatars_to_show) {
   views::View* view = new views::View();
-  views::GridLayout* layout = CreateSingleColumnLayout(view, kFixedMenuWidth);
+  views::GridLayout* layout =
+      CreateSingleColumnLayout(view, GetFixedMenuWidth());
 
   for (size_t index : avatars_to_show) {
     const AvatarMenu::Item& item = avatar_menu_->GetItemAt(index);
     const int kSmallImageSide = 32;
 
     // Use the low-res, small default avatars in the fast user switcher, like
     // we do in the menu bar.
     gfx::Image item_icon;
     AvatarMenu::GetImageForMenuButton(item.profile_path, &item_icon);
 
@@ skipping 10 matching lines @@
     layout->AddView(new views::Separator(views::Separator::HORIZONTAL));
     layout->StartRow(1, 0);
     layout->AddView(button);
   }
 
   return view;
 }
 
 views::View* ProfileChooserView::CreateOptionsView(bool display_lock) {
   views::View* view = new views::View();
-  views::GridLayout* layout = CreateSingleColumnLayout(view, kFixedMenuWidth);
+  views::GridLayout* layout =
+      CreateSingleColumnLayout(view, GetFixedMenuWidth());
 
   base::string16 text = browser_->profile()->IsGuestSession() ?
       l10n_util::GetStringUTF16(IDS_PROFILES_EXIT_GUEST) :
       l10n_util::GetStringUTF16(IDS_PROFILES_SWITCH_USERS_BUTTON);
   if (!browser_->profile()->IsGuestSession()
       && switches::IsMaterialDesignUserMenu()) {
     text = l10n_util::GetStringUTF16(IDS_PROFILES_MANAGE_USERS_BUTTON);
   }
   const int kIconSize = 16;
 
@@ skipping 29 matching lines @@
                               gfx::kChromeIconGrey));
     layout->StartRow(1, 0);
     layout->AddView(lock_button_);
   }
   return view;
 }
 
 views::View* ProfileChooserView::CreateSupervisedUserDisclaimerView() {
   views::View* view = new views::View();
   views::GridLayout* layout = CreateSingleColumnLayout(
-      view, kFixedMenuWidth - 2 * views::kButtonHEdgeMarginNew);
-  layout->SetInsets(views::kRelatedControlVerticalSpacing,
-                    views::kButtonHEdgeMarginNew,
-                    views::kRelatedControlVerticalSpacing,
-                    views::kButtonHEdgeMarginNew);
+      view, GetFixedMenuWidth() - 2 * views::kButtonHEdgeMarginNew);
+  if (switches::IsMaterialDesignUserMenu()) {
+    layout->SetInsets(0,
+                      kMaterialMenuHorizontalEdgeMargin,
+                      kMaterialMenuVerticalEdgeMargin,
+                      kMaterialMenuHorizontalEdgeMargin);
+  } else {
+    layout->SetInsets(
+        views::kRelatedControlVerticalSpacing, views::kButtonHEdgeMarginNew,
+        views::kRelatedControlVerticalSpacing, views::kButtonHEdgeMarginNew);
+  }
+
   views::Label* disclaimer = new views::Label(
       avatar_menu_->GetSupervisedUserInformation());
   disclaimer->SetMultiLine(true);
   disclaimer->SetAllowCharacterBreak(true);
   disclaimer->SetHorizontalAlignment(gfx::ALIGN_LEFT);
   ui::ResourceBundle* rb = &ui::ResourceBundle::GetSharedInstance();
   disclaimer->SetFontList(rb->GetFontList(ui::ResourceBundle::SmallFont));
   layout->StartRow(1, 0);
   layout->AddView(disclaimer);
 
   return view;
 }
 
 views::View* ProfileChooserView::CreateCurrentProfileAccountsView(
     const AvatarMenu::Item& avatar_item) {
   DCHECK(avatar_item.signed_in);
   views::View* view = new views::View();
   view->set_background(views::Background::CreateSolidBackground(
       profiles::kAvatarBubbleAccountsBackgroundColor));
-  views::GridLayout* layout = CreateSingleColumnLayout(view, kFixedMenuWidth);
+  views::GridLayout* layout =
+      CreateSingleColumnLayout(view, GetFixedMenuWidth());
 
   Profile* profile = browser_->profile();
   std::string primary_account =
       SigninManagerFactory::GetForProfile(profile)->GetAuthenticatedAccountId();
   DCHECK(!primary_account.empty());
   std::vector<std::string>accounts =
       profiles::GetSecondaryAccountsForProfile(profile, primary_account);
 
   // Get state of authentication error, if any.
   std::string error_account_id = GetAuthErrorAccountId(profile);
 
   // The primary account should always be listed first.
   // TODO(rogerta): we still need to further differentiate the primary account
   // from the others in the UI, so more work is likely required here:
   // crbug.com/311124.
   CreateAccountButton(layout, primary_account, true,
-                      error_account_id == primary_account, kFixedMenuWidth);
+                      error_account_id == primary_account, GetFixedMenuWidth());
   for (size_t i = 0; i < accounts.size(); ++i)
     CreateAccountButton(layout, accounts[i], false,
-                        error_account_id == accounts[i], kFixedMenuWidth);
+                        error_account_id == accounts[i], GetFixedMenuWidth());
 
   if (!profile->IsSupervised()) {
     layout->AddPaddingRow(0, views::kRelatedControlVerticalSpacing);
 
     add_account_link_ = CreateLink(l10n_util::GetStringFUTF16(
         IDS_PROFILES_PROFILE_ADD_ACCOUNT_BUTTON, avatar_item.name), this);
     add_account_link_->SetBorder(views::Border::CreateEmptyBorder(
         0, views::kButtonVEdgeMarginNew,
         views::kRelatedControlVerticalSpacing, 0));
     layout->StartRow(1, 0);
@@ skipping 289 matching lines @@
       IncognitoModePrefs::GetAvailability(browser_->profile()->GetPrefs()) !=
           IncognitoModePrefs::DISABLED;
   return incognito_available && !browser_->profile()->IsGuestSession();
 }
 
 void ProfileChooserView::PostActionPerformed(
     ProfileMetrics::ProfileDesktopMenu action_performed) {
   ProfileMetrics::LogProfileDesktopMenu(action_performed, gaia_service_type_);
   gaia_service_type_ = signin::GAIA_SERVICE_TYPE_NONE;
 }