Apply MD style to Linux avatar buttons.

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

Index: chrome/browser/ui/views/profiles/avatar_button.cc
diff --git a/chrome/browser/ui/views/profiles/avatar_button.cc b/chrome/browser/ui/views/profiles/avatar_button.cc
index 6b5ac736d3a02067e925a9bd2fac4370301d64fa..13bf372caec4f130adb59a3a3642aaf5cf883626 100644
--- a/chrome/browser/ui/views/profiles/avatar_button.cc
+++ b/chrome/browser/ui/views/profiles/avatar_button.cc
@@ -11,15 +11,20 @@
 #include "chrome/browser/profiles/profile_manager.h"
 #include "chrome/browser/profiles/profiles_state.h"
 #include "chrome/browser/signin/signin_manager_factory.h"
+#include "chrome/browser/themes/theme_properties.h"
 #include "chrome/browser/themes/theme_service.h"
 #include "chrome/browser/themes/theme_service_factory.h"
+#include "chrome/browser/ui/views/profiles/profile_chooser_view.h"
 #include "chrome/grit/theme_resources.h"
 #include "components/signin/core/browser/signin_manager.h"
 #include "ui/base/resource/resource_bundle.h"
+#include "ui/base/theme_provider.h"
+#include "ui/gfx/canvas.h"
 #include "ui/gfx/color_palette.h"
 #include "ui/gfx/paint_vector_icon.h"
 #include "ui/views/animation/flood_fill_ink_drop_ripple.h"
 #include "ui/views/animation/ink_drop_impl.h"
+#include "ui/views/animation/ink_drop_mask.h"
 #include "ui/views/controls/button/label_button_border.h"
 
 #if defined(OS_WIN)
@@ -29,12 +34,12 @@
 
 namespace {
 
-constexpr int kLeftRightInset = 8;
-constexpr int kTopInset = 2;
-constexpr int kBottomInset = 4;
+constexpr gfx::Insets kBorderInsets(2, 8, 4, 8);
 
 // TODO(emx): Calculate width based on caption button [http://crbug.com/716365]
-constexpr int kMdButtonMinWidth = 46;
+constexpr int kCondensibleButtonMinWidth = 46;
+// TODO(emx): Should this be calculated based on average character width?
+constexpr int kCondensibleButtonMaxWidth = 98;
 
 std::unique_ptr<views::Border> CreateThemedBorder(
     const int normal_image_set[],
@@ -50,16 +55,66 @@ std::unique_ptr<views::Border> CreateThemedBorder(
   border->SetPainter(false, views::Button::STATE_PRESSED,
                      views::Painter::CreateImageGridPainter(pushed_image_set));
 
-  border->set_insets(
-      gfx::Insets(kTopInset, kLeftRightInset, kBottomInset, kLeftRightInset));
+  border->set_insets(kBorderInsets);
 
   return std::move(border);
 }
 
-std::unique_ptr<views::Border> CreateWin10NativeBorder() {
-  return views::CreateEmptyBorder(kTopInset, kLeftRightInset, kBottomInset,
-                                  kLeftRightInset);
-}
+// This class draws the border (and background) of the avatar button for
+// "themed" browser windows, i.e. OpaqueBrowserFrameView. Currently it's only
+// used on Linux as the shape specifically matches the Linux caption buttons.
+// TODO(estade): make this look nice on Windows and use it there as well.
+class AvatarButtonThemedBorder : public views::Border {
+ public:
+  AvatarButtonThemedBorder() {}
+  ~AvatarButtonThemedBorder() override {}
+
+  void Paint(const views::View& view, gfx::Canvas* canvas) override {
+    // Start with an outer dark stroke.
+    cc::PaintFlags stroke_flags;
+    stroke_flags.setStyle(cc::PaintFlags::kStroke_Style);
+    stroke_flags.setColor(SkColorSetA(SK_ColorBLACK, 0x2B));

[Peter Kasting, 2017/05/15 21:51:27]

Nit: Where did this constant come from?  Is it just observed as what GTK does? 
Same with 0x3F below.

[Evan Stade, 2017/05/15 22:25:16]

They match the assets we use for the caption buttons on Linux (which are not in
themselves part of GTK but hard coded).

[Peter Kasting, 2017/05/15 23:41:02]

Maybe we should put a comment in saying "matches xyz, change this if you change
those"?  Maybe not worth it if you're hoping to do the work of expanding this
beyond just Linux soon.

[Evan Stade, 2017/05/16 23:50:13]

comment added

+    stroke_flags.setStrokeWidth(kStrokeWidth);
+    stroke_flags.setAntiAlias(true);
+    gfx::RectF stroke_bounds(view.GetLocalBounds());
+    stroke_bounds.Inset(gfx::InsetsF(0.5f));
+    canvas->DrawRoundRect(stroke_bounds, kCornerRadius, stroke_flags);
+
+    // There's a second, light stroke that matches the fill bounds.

[Peter Kasting, 2017/05/15 21:51:27]

I'm still concerned with computing these nested (stroke, stroke, ink drop flood
fill) curves by simply drawing inset round rects instead of subtracting the
actual stroke paths so the contours match exactly.  I think I wrote more
extensively about this on your previous version of this.

[Evan Stade, 2017/05/15 22:25:15]

Yea. Before you said you were worried about appearance at larger scale factors;
I tested at multiple DSFs up to 4x and it looked good. I don't know if there's a
way to do as you suggest for the ink drop.

[Peter Kasting, 2017/05/15 23:41:02]

Interesting.  I recall noticing similar artifacts back in the day on the omnibox
1 px border but maybe I was testing it locally on a really crazy scale factor.

It's theoretically more correct to do it the other way and doesn't seem too
hard, but it'd be so hard to notice that it's not worth me pushing for it unless
you're OCD like me and it would make you feel happier in principle :)
There probably is, but probably the easier route would be to assume a round rect
for the ink drop and carve it out of the interior of the stroke around it. 
Basically, start with the ink drop bounds as a filled round rect, expand the
round rect bounds out by 1 on each side twice, then turn those two expanded
rects into rings by subtracting the smaller rects.

The result would be rings whose outer perimeters match true round rects, but
whose inner perimeters are different.

+    stroke_bounds.Inset(gfx::InsetsF(kStrokeWidth));
+    stroke_flags.setColor(SkColorSetA(SK_ColorWHITE, 0x3F));
+    canvas->DrawRoundRect(stroke_bounds, kCornerRadius, stroke_flags);
+  }
+
+  gfx::Insets GetInsets() const override {
+    return kBorderStrokeInsets + gfx::Insets(0, 6);

[Peter Kasting, 2017/05/15 21:51:27]

Nit: Again, where does this 6 come from?  Can it be computed somehow?  Does it
relate to anything else in the UI code?

[Evan Stade, 2017/05/15 22:25:16]

I don't believe so. It was hardcoded before and it continues to be hardcoded. It
controls the amount of spacing between button text (or image) and border. Do you
have a suggestion for how to compute that? I could see it being some fraction of
the font size or something, but that fraction itself is a constant.

I also forgot to ask: have you checked the appearance of the default avatar icon
on Windows? I think it may be clipped at the top and/or bottom due to too much
border inset.

[Peter Kasting, 2017/05/15 23:41:02]

I'm thinking the spacing should match what we compute for labels-in-buttons
elsewhere in the UI, however that's done.

[Evan Stade, 2017/05/16 23:50:13]

I guess we're sorta matching this[1]. Changed.

[1]
https://cs.chromium.org/chromium/src/ui/views/controls/button/label_button_bo...

+  }
+
+  gfx::Size GetMinimumSize() const override {
+    return gfx::Size(GetInsets().width(), GetInsets().height());
+  }
+
+  static std::unique_ptr<views::InkDropMask> CreateInkDropMask(
+      const gfx::Size& size) {
+    return base::MakeUnique<views::RoundRectInkDropMask>(
+        size, kBorderStrokeInsets, kCornerRadius);
+  }
+
+ private:
+  static constexpr float kStrokeWidth = 1;
+
+  // Insets between view bounds and the interior of the strokes.
+  static constexpr gfx::Insets kBorderStrokeInsets{kStrokeWidth * 2};
+
+  // Corner radius of the roundrect.
+  static constexpr float kCornerRadius = 1;
+
+  DISALLOW_COPY_AND_ASSIGN(AvatarButtonThemedBorder);
+};
+
+constexpr float AvatarButtonThemedBorder::kStrokeWidth;

[Peter Kasting, 2017/05/15 21:51:27]

Nit: Does the compiler actually complain if you don't define these (i.e. they're
considered ODR-used)?

[Evan Stade, 2017/05/15 22:25:15]

The code compiles but won't link. There is some discussion at [1]

[1]
http://stackoverflow.com/questions/8016780/undefined-reference-to-static-cons...

[Peter Kasting, 2017/05/15 23:41:02]

Yeah, by "compiler" I really meant "linker".  Leave them, they're correct.

+constexpr gfx::Insets AvatarButtonThemedBorder::kBorderStrokeInsets;
+constexpr float AvatarButtonThemedBorder::kCornerRadius;
 
 }  // namespace
 
@@ -70,7 +125,7 @@ AvatarButton::AvatarButton(views::ButtonListener* listener,
       error_controller_(this, profile),
       profile_(profile),
       profile_observer_(this),
-      use_win10_native_button_(false) {
+      button_style_(button_style) {
   set_notify_action(CustomButton::NOTIFY_ON_PRESS);
   set_triggerable_event_flags(ui::EF_LEFT_MOUSE_BUTTON |
                               ui::EF_RIGHT_MOUSE_BUTTON);
@@ -89,55 +144,53 @@ AvatarButton::AvatarButton(views::ButtonListener* listener,
   SetFontList(
       label()->font_list().DeriveWithHeightUpperBound(kDisplayFontHeight));
 
-#if defined(OS_WIN)
-  // TODO(estade): Use MD button in other cases, too [http://crbug.com/591586]
-  if ((base::win::GetVersion() >= base::win::VERSION_WIN10) &&
-      ThemeServiceFactory::GetForProfile(profile)->UsingSystemTheme()) {
-    DCHECK_EQ(AvatarButtonStyle::NATIVE, button_style);
-    use_win10_native_button_ = true;
-  }
-#endif  // defined(OS_WIN)
+  bool apply_ink_drop = IsCondensible();
+#if defined(OS_LINUX)
+  DCHECK_EQ(AvatarButtonStyle::THEMED, button_style);
+  apply_ink_drop = true;
+#endif
 
-  if (use_win10_native_button_) {
-    constexpr int kMdButtonIconHeight = 16;
-    constexpr SkColor kMdButtonIconColor =
+  if (apply_ink_drop) {
+    constexpr int kIconSize = 16;
+    constexpr SkColor kIconColor =
         SkColorSetA(SK_ColorBLACK, static_cast<SkAlpha>(0.54 * 0xFF));
-    generic_avatar_ = gfx::CreateVectorIcon(
-        kAccountCircleIcon, kMdButtonIconHeight, kMdButtonIconColor);
-    SetBorder(CreateWin10NativeBorder());
+    generic_avatar_ =
+        gfx::CreateVectorIcon(kAccountCircleIcon, kIconSize, kIconColor);
 
     SetInkDropMode(InkDropMode::ON);
     set_has_ink_drop_action_on_click(true);
     SetFocusPainter(nullptr);
+#if defined(OS_LINUX)
+    set_ink_drop_base_color(SK_ColorWHITE);
+    SetBorder(base::MakeUnique<AvatarButtonThemedBorder>());
+#elif defined(OS_WIN)
+    DCHECK_EQ(AvatarButtonStyle::NATIVE, button_style);
     set_ink_drop_base_color(SK_ColorBLACK);
-  } else {
-    if (button_style == AvatarButtonStyle::THEMED) {
-      const int kNormalImageSet[] = IMAGE_GRID(IDR_AVATAR_THEMED_BUTTON_NORMAL);
-      const int kHoverImageSet[] = IMAGE_GRID(IDR_AVATAR_THEMED_BUTTON_HOVER);
-      const int kPressedImageSet[] =
-          IMAGE_GRID(IDR_AVATAR_THEMED_BUTTON_PRESSED);
-      SetButtonAvatar(IDR_AVATAR_THEMED_BUTTON_AVATAR);
-      SetBorder(CreateThemedBorder(kNormalImageSet, kHoverImageSet,
-                                   kPressedImageSet));
+    SetBorder(views::CreateEmptyBorder(kBorderInsets));
+#endif  // defined(OS_WIN)
+  } else if (button_style == AvatarButtonStyle::THEMED) {
+    const int kNormalImageSet[] = IMAGE_GRID(IDR_AVATAR_THEMED_BUTTON_NORMAL);
+    const int kHoverImageSet[] = IMAGE_GRID(IDR_AVATAR_THEMED_BUTTON_HOVER);
+    const int kPressedImageSet[] = IMAGE_GRID(IDR_AVATAR_THEMED_BUTTON_PRESSED);
+    SetButtonAvatar(IDR_AVATAR_THEMED_BUTTON_AVATAR);
+    SetBorder(
+        CreateThemedBorder(kNormalImageSet, kHoverImageSet, kPressedImageSet));
 #if defined(OS_WIN)
-    } else if (base::win::GetVersion() < base::win::VERSION_WIN8) {
-      const int kNormalImageSet[] = IMAGE_GRID(IDR_AVATAR_GLASS_BUTTON_NORMAL);
-      const int kHoverImageSet[] = IMAGE_GRID(IDR_AVATAR_GLASS_BUTTON_HOVER);
-      const int kPressedImageSet[] =
-          IMAGE_GRID(IDR_AVATAR_GLASS_BUTTON_PRESSED);
-      SetButtonAvatar(IDR_AVATAR_GLASS_BUTTON_AVATAR);
-      SetBorder(CreateThemedBorder(kNormalImageSet, kHoverImageSet,
-                                   kPressedImageSet));
+  } else if (base::win::GetVersion() < base::win::VERSION_WIN8) {
+    const int kNormalImageSet[] = IMAGE_GRID(IDR_AVATAR_GLASS_BUTTON_NORMAL);
+    const int kHoverImageSet[] = IMAGE_GRID(IDR_AVATAR_GLASS_BUTTON_HOVER);
+    const int kPressedImageSet[] = IMAGE_GRID(IDR_AVATAR_GLASS_BUTTON_PRESSED);
+    SetButtonAvatar(IDR_AVATAR_GLASS_BUTTON_AVATAR);
+    SetBorder(
+        CreateThemedBorder(kNormalImageSet, kHoverImageSet, kPressedImageSet));
 #endif
-    } else {
-      const int kNormalImageSet[] = IMAGE_GRID(IDR_AVATAR_NATIVE_BUTTON_NORMAL);
-      const int kHoverImageSet[] = IMAGE_GRID(IDR_AVATAR_NATIVE_BUTTON_HOVER);
-      const int kPressedImageSet[] =
-          IMAGE_GRID(IDR_AVATAR_NATIVE_BUTTON_PRESSED);
-      SetButtonAvatar(IDR_AVATAR_NATIVE_BUTTON_AVATAR);
-      SetBorder(CreateThemedBorder(kNormalImageSet, kHoverImageSet,
-                                   kPressedImageSet));
-    }
+  } else {
+    const int kNormalImageSet[] = IMAGE_GRID(IDR_AVATAR_NATIVE_BUTTON_NORMAL);
+    const int kHoverImageSet[] = IMAGE_GRID(IDR_AVATAR_NATIVE_BUTTON_HOVER);
+    const int kPressedImageSet[] = IMAGE_GRID(IDR_AVATAR_NATIVE_BUTTON_PRESSED);
+    SetButtonAvatar(IDR_AVATAR_NATIVE_BUTTON_AVATAR);
+    SetBorder(
+        CreateThemedBorder(kNormalImageSet, kHoverImageSet, kPressedImageSet));
   }
 
   Update();
@@ -158,11 +211,11 @@ void AvatarButton::OnGestureEvent(ui::GestureEvent* event) {
 }
 
 gfx::Size AvatarButton::GetMinimumSize() const {
-  if (use_win10_native_button_) {
+  if (IsCondensible()) {
     // Returns the size of the button when it is atop the tabstrip. Called by
     // GlassBrowserFrameView::LayoutProfileSwitcher().
     // TODO(emx): Calculate the height based on the top of the new tab button.
-    return gfx::Size(kMdButtonMinWidth, 20);
+    return gfx::Size(kCondensibleButtonMinWidth, 20);
   }
 
   return LabelButton::GetMinimumSize();
@@ -170,36 +223,54 @@ gfx::Size AvatarButton::GetMinimumSize() const {
 
 gfx::Size AvatarButton::GetPreferredSize() const {
   gfx::Size size = LabelButton::GetPreferredSize();
+  size.set_height(20);

[Peter Kasting, 2017/05/15 21:51:27]

Nit: Simpler:

  gfx::Size size(LabelButton::GetPreferredSize().width(), 20);

[Evan Stade, 2017/05/15 22:25:16]

Done.

 
-  if (use_win10_native_button_) {
-#if defined(OS_WIN)
+  if (IsCondensible()) {
     // Returns the normal size of the button (when it does not overlap the
     // tabstrip). Its height should match the caption button height.

[Peter Kasting, 2017/05/15 21:51:27]

Nit: This second sentence should probably turn into a TODO above where |size| is
computed, like:

  // TODO(emx): Compute height to match caption button height for other
  // platforms than Windows.

[Evan Stade, 2017/05/15 22:25:16]

todo added

     // The minimum width is the caption button width and the maximum is fixed
     // as per the spec in http://crbug.com/635699.

[Peter Kasting, 2017/05/15 21:51:28]

Nit: This thirds sentence, if it needs saying at all, probably should be up
where these two constants are defined.

[Evan Stade, 2017/05/15 22:25:16]

removed

-    // TODO(emx): Should this be calculated based on average character width?
-    constexpr int kMdButtonMaxWidth = 98;
-    size.set_width(
-        std::min(std::max(size.width(), kMdButtonMinWidth), kMdButtonMaxWidth));
+    size.set_width(std::min(std::max(size.width(), kCondensibleButtonMinWidth),
+                            kCondensibleButtonMaxWidth));
+#if defined(OS_WIN)
     size.set_height(MinimizeButtonMetrics::GetCaptionButtonHeightInDIPs());

[Peter Kasting, 2017/05/15 21:51:28]

Nit: This call shouldn't be inside IsCondensible(), I don't think, since the
fact that the avatar button height should match that of the caption buttons
doesn't depend on that (I think?).

I'd probably pull this #if out to where the "20" is used above, like:

// TODO...
#if OS_WIN
  const int height = MinimizeButtonMetrics::GetCaptionButtonHeightInDIPs();
#else
  constexpr int height = 20;
#endif
  gfx::Size size(LabelButton::GetPreferredSize().width(), height);

[Evan Stade, 2017/05/15 22:25:15]

I agree with this sentiment but changing that might create a behavioral
difference on themed Windows or versions <10, which might not be desirable. I'd
be worried to change that without having a way to test it. Seems like a task for
whoever takes up the TODO for programmatic drawing of this button for themed
Windows browsers.

 #endif
-  } else {
-    size.set_height(20);
   }
 
   return size;
 }
 
+std::unique_ptr<views::InkDropMask> AvatarButton::CreateInkDropMask() const {
+  if (button_style_ == AvatarButtonStyle::THEMED)
+    return AvatarButtonThemedBorder::CreateInkDropMask(size());
+  return LabelButton::CreateInkDropMask();

[Peter Kasting, 2017/05/15 21:51:28]

Nit: Could have single return with ?:, up to you

[Evan Stade, 2017/05/15 22:25:16]

Acknowledged.

+}
+
 std::unique_ptr<views::InkDropHighlight> AvatarButton::CreateInkDropHighlight()
     const {
-  auto center = gfx::RectF(GetLocalBounds()).CenterPoint();
+  if (button_style_ == AvatarButtonStyle::THEMED)
+    return LabelButton::CreateInkDropHighlight();
+
   auto ink_drop_highlight = base::MakeUnique<views::InkDropHighlight>(
-      size(), 0, center, GetInkDropBaseColor());
+      size(), 0, gfx::RectF(GetLocalBounds()).CenterPoint(),
+      GetInkDropBaseColor());
   constexpr float kInkDropHighlightOpacity = 0.08f;
   ink_drop_highlight->set_visible_opacity(kInkDropHighlightOpacity);
   return ink_drop_highlight;
 }
 
+void AvatarButton::NotifyClick(const ui::Event& event) {
+  LabelButton::NotifyClick(event);
+
+  views::Widget* bubble_widget = ProfileChooserView::GetCurrentBubbleWidget();
+  if (bubble_widget && !bubble_widget->HasObserver(this) &&
+      ink_drop_mode() == InkDropMode::ON) {
+    ProfileChooserView::GetCurrentBubbleWidget()->AddObserver(this);
+    AnimateInkDrop(views::InkDropState::ACTIVATED,
+                   ui::LocatedEvent::FromIfValid(&event));
+  }
+}
+
 bool AvatarButton::ShouldUseFloodFillInkDrop() const {
   return true;
 }
@@ -233,6 +304,11 @@ void AvatarButton::OnProfileSupervisedUserIdChanged(
     Update();
 }
 
+void AvatarButton::OnWidgetClosing(views::Widget* widget) {
+  if (ink_drop_mode() == InkDropMode::ON)
+    AnimateInkDrop(views::InkDropState::DEACTIVATED, nullptr);
+}
+
 void AvatarButton::Update() {
   ProfileAttributesStorage& storage =
       g_browser_process->profile_manager()->GetProfileAttributesStorage();
@@ -280,3 +356,12 @@ void AvatarButton::SetButtonAvatar(int avatar_idr) {
   ui::ResourceBundle* rb = &ui::ResourceBundle::GetSharedInstance();
   generic_avatar_ = *rb->GetImageNamed(avatar_idr).ToImageSkia();
 }
+
+bool AvatarButton::IsCondensible() const {
+#if defined(OS_WIN)

[Peter Kasting, 2017/05/15 21:51:27]

Nit: Can we preserve some kind of TODO here about using this button on
non-Windows platforms too, as the place you moved this from had?

[Evan Stade, 2017/05/15 22:25:16]

I considered that TODO to refer to the ink drop effects, not the condensibility.
I tried to get rid of references to MD in this patch as it's kind of ambiguous
here.

[Peter Kasting, 2017/05/15 23:41:02]

OK.  For reference, we want the condensibility on all platforms.

[Evan Stade, 2017/05/16 23:50:13]

added comment

+  return (base::win::GetVersion() >= base::win::VERSION_WIN10) &&
+         ThemeServiceFactory::GetForProfile(profile_)->UsingSystemTheme();
+#else
+  return false;
+#endif
+}