Update avatar button to MD (part 1)

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

+// Copyright 2014 The Chromium Authors. All rights reserved.

[Peter Kasting, 2017/05/06 02:23:09]

Any way to get this to appear as a moved file w/diff instead of a brand new one?
 That would make the review easier... it would also make it look less weird that
you're adding a file with a 2014 copyright.

[emx, 2017/05/09 16:26:52]

I don't believe so. Git decides automatically whether a file is "moved" or not
and I cannot find a way to force it to recognise that it was. See
http://stackoverflow.com/questions/2314652/is-it-possible-to-move-rename-file...
and
https://git.wiki.kernel.org/index.php/GitFaq#Why_does_Git_not_.22track.22_ren...
Even "git log --follow --find-copies-harder
src\chrome\browser\ui\views\profiles\avatar_button.cc" does not find the rename.
I guess it still doesn't look hard enough!

[Peter Kasting, 2017/05/09 17:40:28]

In that case I'd suggest doing the file rename as a separate CL that this one
depends on, with no other changes (other than #include fixups or whatever), and
then making this CL have just the changes to the file content.

[emx, 2017/05/10 17:38:48]

OK, I've made it a separate CL.

+// 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/avatar_button.h"
+
+#include <utility>
+
+#include "chrome/app/vector_icons/vector_icons.h"
+#include "chrome/browser/browser_process.h"
+#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_service.h"
+#include "chrome/browser/themes/theme_service_factory.h"
+#include "chrome/grit/theme_resources.h"
+#include "components/signin/core/browser/signin_manager.h"
+#include "ui/base/resource/resource_bundle.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/controls/button/label_button_border.h"
+
+#if defined(OS_WIN)
+#include "base/win/windows_version.h"
+#endif
+
+namespace {
+
+const int kThemedButtonHeight = 20;

[Peter Kasting, 2017/05/06 02:23:08]

Nit: Prefer constexpr to const for compile-time constants.  Prefer specifying
constants in the functions that use them, as close to first use as possible, for
everything that's used in only one function.

[emx, 2017/05/09 16:26:52]

Done.

+
+const int kMdButtonMinWidth = 48;

[Peter Kasting, 2017/05/06 02:23:08]

Should these values be computed based on average character widths instead of
direct pixel values?  That seems like it might be more i18n-friendly.

[emx, 2017/05/09 16:26:52]

The minimum width should be the caption button width actually - I've now
implemented this the same way as the height in MinimizeButtonMetrics.

For the maximum height, I don't know if it would be better, but the spec gives a
fixed maximum width of 98 pixels (I had 96, actually - corrected it). I think
this should be a separate bug, if it's necessary.

[Peter Kasting, 2017/05/09 17:40:29]

Implementation-wise, the spec is a design suggestion, but the engineer
responsible for implementing is also supposed to be responsible for ensuring we
handle things like i18n and a11y correctly.  Since you're newly-implementing
this, there's no real reason in code to prefer 96 vs. some number of
ave-char-widths vs. some other solution.  So I'd ask the designer what the
design intent of 96 was -- does it match some other width?  Is it "wide enough
to show this many characters"?  Learning the design intent will inform what we
should implement here.

It's possible to punt this to a separate bug, but it will likely be more work in
the end and you'll be on the hook for doing said work, so it might be easiest to
just go ahead and resolve immediately.

[emx, 2017/05/10 17:38:49]

OK, let's wait for Alan to reply to the email.

[Peter Kasting, 2017/05/11 01:03:42]

If he doesn't get back with something quickly, I would go ahead and land as-is
and leave a TODO about this and file it separately.  While there is merit in the
argument that "resolving now might be easier", there's also merit in "don't hang
the CL up indefinitely".

+const int kMdButtonMaxWidth = 96;
+// Height of the "cozy" MD button that slides over the tabstrip

[Peter Kasting, 2017/05/06 02:23:09]

Nit: Can avoid the term "Cozy" and just talk about the height when the button
and tabstrip are colocated

[emx, 2017/05/09 16:26:52]

Done.

+// ("tall" MD button height is computed)
+const int kMdButtonCozyHeight = 20;
+const int kMdButtonIconHeight = 16;

[Peter Kasting, 2017/05/06 02:23:09]

This height differs slightly from the 20 DIP height used elsewhere with this
icon.  That concerns me that the icon will have been optimized for a slightly
different height, leading to appearance issues.

I would imagine we either way to use 20 here, use 16 elsewhere, or we may
actually want separate versions of the icon optimized for different sizes.

[emx, 2017/05/09 16:26:52]

The spec clearly says 16 and it looks fine to me. 20 would be a tight fit when
the button is over the tabstrip. It's possible with the button set to 22
(instead of 20), but that still leaves very little space above and below the
icon. Anyway, this is something for bettes@.

[Peter Kasting, 2017/05/09 17:40:29]

See comment earlier about specs being suggestions :)
Right, I'm willing to believe using 16 elsewhere, rather than 20 here, is
better.  My suspicion is that Alan didn't even know we have this 20-px-optimized
icon in the tree we're using, that we use in other places, and that learning
what those places are would give more context for the design decisions about
what both the sizes there and here should be.  Definitely share this and see
what he thinks.

[emx, 2017/05/10 17:38:48]

So if the spec is a suggestion, then who actually makes the final decision about
how the product should look and function?
I've replied to the email with a screenshot of both 16x16 and 20x20 icons.

[Peter Kasting, 2017/05/11 01:03:43]

It's collaborative, ultimately -- UI Review has final authority but doesn't
generally review everything, UI Design has the most design skill but generally
less context + is spread thin, and engineering has the most context and focus. 
So I would generally say that while the final _decision_ isn't always made by
the engineer doing the implementation, the primary responsibility to think
through everything about the UX belongs to the engineer.

In this case, you got tasked with a complicated set of changes that touch the
browser top chrome, which is a fully-custom surface with lots of history and
baggage, so you had a task that would normally be done by someone with far more
background in similar changes (generally, the Windows-specific UI team, which
would be people like me and bsep).  Asking you to spin up all this background
before landing your first project isn't fair of me.

Maybe the best takeaways are:
* Assume in future UI work (top chrome or not) that specs, existing practice,
etc., may not actually be correct
* Tackle complex projects in as small of pieces as possible (e.g. CLs that
implement a single behavior aspect at a time)
* Be quick to rope in OWNERS and folks experienced in the area

However, given the task set you, you've done admirably at implementing this
well, responding patiently to review feedback, and relaying stakeholder concerns
from different directions.  Bravo :)

[emx, 2017/05/11 12:56:10]

Thanks for the detailed explanation. I agree with your takeaways and was already
thinking that, for the next such change, I will start by finding a code reviewer
and ensuring that they agree with the spec first.

+const SkColor kMdButtonIconColor = SkColorSetA(SK_ColorBLACK, 0.54 * 0xFF);

[Peter Kasting, 2017/05/06 02:23:09]

These kinds of one-off constants (for opacity here and below) worry me.  We
should have consistent appearance and behavior for all UI elements across
Chrome, rather than defining things individually per-element.

For example, ash/system/tray/tray_constants.cc also uses 0.08f for highlight
opacity, but 0.06f for ripple opacity instead of 0.04.  This kind of subtle
difference seems unlikely to be intentional.

It also worries me that this is hardcoded to black, instead of computing a
contrasting color with the button background dynamically (see win 10 custom
caption button code).  It also seems different than the text color, which seems
unlikely to look good.

[emx, 2017/05/09 16:26:52]

The highlight opacity of 0.08 is in the spec. Whether it should be the same as
any other UI element is a question I cannot answer.

The ripple opacity of 0.04 I think was a misinterpretation of the spec on my
part. The spec says to "Follow Core UI single tap animation curve and visuals
(bookmark bar)". The bookmark button ripple opacity is the default 0.175 (it
doesn't call set_ink_drop_visible_opacity), so I've also removed that call.

The colour is also in the spec and bettes@ said in an email "#5a5a5a is
identical to #000 on grey, but #000 0.54a is preferred for its color blending
properties." Again, I really don't know anywhere near enough to answer what we
should do here.

[Peter Kasting, 2017/05/09 17:40:28]

The way I would start is to come back to Alan and ask where the 0.08 came from
to begin with, and how, if at all, it should relate to other UI elements in
Chrome and Ash.  You could note the place I brought up or other cases you find
where we explicitly set highlight opacity, along with the default opacity value,
and ask if we shouldn't be trying to make all these the same; and if not, what
sort of general rules we should follow to understand where variance should come
from.

The reason my replies sound like a broken record about "can't just implement the
spec, have to understand why and be consistent" is that Chrome has too many
surfaces for any one designer to keep in their head, so while they'll do the
best job they can locally, we engineers have the context in code to see when
something is implemented consistently versus as a one-off; and there's a heavy
push from Chrome leadership for consistency and coherency across all Chrome UI,
so it's the sort of thing we need to uphold in our implementations.  Think of
yourself as the Last Line Of Defense for making any given feature not only
locally good, but a unified part of the Chrome whole :)

+// Opacity of the ink drop on hover
+const float kInkDropHighlightOpacity = 0.08f;
+// Opacity of the ink drop on click, which is added to kInkDropHighlightOpacity
+const float kInkDropRippleOpacity = 0.04f;
+
+std::unique_ptr<views::Border> CreateThemedBorder(
+    const int normal_image_set[],
+    const int hot_image_set[],
+    const int pushed_image_set[]) {
+  std::unique_ptr<views::LabelButtonAssetBorder> border(
+      new views::LabelButtonAssetBorder(views::Button::STYLE_TEXTBUTTON));
+
+  border->SetPainter(false, views::Button::STATE_NORMAL,
+                     views::Painter::CreateImageGridPainter(normal_image_set));
+  border->SetPainter(false, views::Button::STATE_HOVERED,
+                     views::Painter::CreateImageGridPainter(hot_image_set));
+  border->SetPainter(false, views::Button::STATE_PRESSED,
+                     views::Painter::CreateImageGridPainter(pushed_image_set));
+
+  const int kLeftRightInset = 8;
+  const int kTopInset = 2;
+  const int kBottomInset = 4;

[Peter Kasting, 2017/05/06 02:23:09]

Where did these insets come from?  They, and the Win10 ones below, feel
completely magic.  Why do they look good?  How can we compute them dynamically
(e.g. by just horizontally centering the contents of the button)?

[emx, 2017/05/09 16:26:52]

I don't know - this is existing code and I haven't changed them.

[Peter Kasting, 2017/05/09 17:40:28]

OK.  We really shouldn't do this sort of thing, but it's out-of-scope for this
CL to fix it, so don't worry about it here for now.

+  border->set_insets(
+      gfx::Insets(kTopInset, kLeftRightInset, kBottomInset, kLeftRightInset));
+
+  return std::move(border);
+}
+
+std::unique_ptr<views::Border> CreateWin10NativeBorder() {

[Peter Kasting, 2017/05/06 02:23:09]

Is this really Win10-specific?  Or is this basically "the MD appearance" and
supposed to be used by other platforms too?

[emx, 2017/05/09 16:26:52]

I arrived at these insets by experimentation on Windows 10. These are the only
values I could find that look OK at all the listed scaling settings.

As for the name of the method - it's a border currently used only by the windows
10 button, which was called "Win10NativeAvatarButton" at Evan's request (before
being merged back into the AvatarButton class). I don't know whether this would
apply on other platforms. I suspect not, but we won't know for sure until MD is
actually implemented for other platforms.

[Peter Kasting, 2017/05/09 17:40:28]

Can you say more about "look OK"?  For example, what sorts of things break if
one uses the cross-platform insets, or just centers everything vertically?  Are
there screenshots?

As much as possible, we want to avoid magic hardcoded values, because it's not
clear where they come from and why they work, in particular as we change button
sizes, font sizes, layout alignments, etc. in the future.  The ideal is no
constants, and everything computed algorithmically (e.g. centering the text
horizontally, which will Just Work even if we make the button wider or
something).  Less ideal but still possible is to have as few constants as we
can, and then make it clear in comments what principles and properties one can
use to derive new values in the future as the UI changes.

Also, regarding things looking OK at different scales, if we find some values
work well at certain scales and not others, that might indicate problems
elsewhere in layout, e.g. we shouldn't fix by changing the constant but rather
by changing flooring to rounding somewhere, or some other similar tweak.  It's
impossible to be more specific without understanding what kind of badness you
were running into.

[emx, 2017/05/10 17:38:48]

Having tested the insets again, it actually looks good with the original insets
(top 2, bottom 4, left/right 8), so I'm now using them for both the old
("themed") and new ("native Win10") buttons. I think I was using the wrong size
icon before (18x18) and that required adjusting the insets.

[Peter Kasting, 2017/05/11 01:03:43]

Sounds good, but it doesn't look like those changes made it into the uploaded
patch?

[emx, 2017/05/11 12:56:10]

Yes, sorry, I replied to the comments, but didn't upload any fixes yesterday
(only rebased on the rename CL). Uploading today.

+  // These insets look OK at 100%, 125%, 150%, 175% and 200% scaling.
+  const int kLeftRightInset = 8;
+  const int kTopInset = 1;
+  const int kBottomInset = 3;
+  return views::CreateEmptyBorder(kTopInset, kLeftRightInset, kBottomInset,
+                                  kLeftRightInset);
+}
+
+}  // namespace
+
+AvatarButton::AvatarButton(views::ButtonListener* listener,
+                           AvatarButtonStyle button_style,
+                           Profile* profile)
+    : LabelButton(listener, base::string16()),
+      error_controller_(this, profile),
+      profile_(profile),
+      use_win10_native_button_(false) {
+  set_notify_action(CustomButton::NOTIFY_ON_PRESS);
+  set_triggerable_event_flags(ui::EF_LEFT_MOUSE_BUTTON |
+                              ui::EF_RIGHT_MOUSE_BUTTON);
+  set_animate_on_state_change(false);
+  SetEnabledTextColors(SK_ColorWHITE);
+  SetTextSubpixelRenderingEnabled(false);
+  SetHorizontalAlignment(gfx::ALIGN_CENTER);
+
+  g_browser_process->profile_manager()
+      ->GetProfileAttributesStorage()
+      .AddObserver(this);
+
+  // The largest text height that fits in the button. If the font list height
+  // is larger than this, it will be shrunk to match it.
+  // TODO(noms): Calculate this constant algorithmically from the button's size.
+  const int kDisplayFontHeight = 16;
+  SetFontList(
+      label()->font_list().DeriveWithHeightUpperBound(kDisplayFontHeight));
+
+#if defined(OS_WIN)
+  // TODO: use MD button in other cases, too [http://crbug.com/591586]

[Peter Kasting, 2017/05/06 02:23:09]

Nit: Which other cases?  All of them?  Only Windows, but for custom themes as
well?  And who owns this TODO?  You?  estade?

[emx, 2017/05/09 16:26:52]

I don't know which other cases. I added this TODO at your request, so maybe you
can tell me? Or the reader can just look at the linked bug for details.

I would assume the owner of this TODO is the owner of the bug it refers to,
which can change from time to time. (Currently it's estade.)

[Peter Kasting, 2017/05/09 17:40:28]

It seems like Evan is the likely person to do this and will hopefully be doing
it soon, so maybe just make the TODO owned by him.

Normally when adding TODOs we put in an owner, who's not necessarily the one
responsible for implementing the work, but is the person who would understand
what the TODO means and how it could be resolved.  Hence my suggestion to have
an owner.

We also normally try to make these as specific and actionable as possible (hence
my other questions), but it seems like if Evan will be doing this shortly that's
not a big deal.

[emx, 2017/05/11 12:56:10]

Makes sense, thanks for explaining. I didn't realise that the concept of "TODO
owner" is a bit different to "bug owner". I've put in estade.

+  if ((base::win::GetVersion() >= base::win::VERSION_WIN10) &&
+      ThemeServiceFactory::GetForProfile(profile)->UsingSystemTheme()) {
+    DCHECK_EQ(AvatarButtonStyle::NATIVE, button_style);
+    use_win10_native_button_ = true;

[Peter Kasting, 2017/05/06 02:23:08]

Nit: Again, is "win10 native button" really the right name for this, or is this
basically "use newer style button"?  I'm mostly allergic to things like
"win10_xxx" appearing in code that's not #ifdef OS_WIN.

[emx, 2017/05/09 16:26:52]

"Win10NativeAvatarButton" was the class name suggested by estade back when this
was a separate class. (I had "MaterialDesignAvatarButton" before.) I don't know
how much of this code he plans to re-use on Linux or themed Windows. Could you
discuss with him, please, and come up with a name you're both happy with?

[Peter Kasting, 2017/05/09 17:40:28]

In the interest of not blocking you, go ahead and leave this as-is for now, and
hopefully it will get updated by his changes.

+  }
+#endif  // defined(OS_WIN)
+
+  if (use_win10_native_button_) {
+    generic_avatar_ = gfx::CreateVectorIcon(
+        kAccountCircleIcon, kMdButtonIconHeight, kMdButtonIconColor);
+    SetBorder(CreateWin10NativeBorder());
+
+    SetInkDropMode(InkDropMode::ON);
+    set_has_ink_drop_action_on_click(true);
+    SetFocusPainter(nullptr);
+    set_ink_drop_base_color(SK_ColorBLACK);
+    set_ink_drop_visible_opacity(kInkDropRippleOpacity);
+  } 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));
+#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));
+    }
+  }
+
+  Update();
+  SchedulePaint();
+}
+
+AvatarButton::~AvatarButton() {
+  g_browser_process->profile_manager()
+      ->GetProfileAttributesStorage()
+      .RemoveObserver(this);

[Peter Kasting, 2017/05/06 02:23:08]

Nit: Can we use ScopedObserver to avoid this explicit remove?

[emx, 2017/05/09 16:26:52]

Done.

+}
+
+void AvatarButton::OnGestureEvent(ui::GestureEvent* event) {
+  // TODO(wjmaclean): The check for ET_GESTURE_LONG_PRESS is done here since
+  // no other UI button based on CustomButton appears to handle mouse
+  // right-click. If other cases are identified, it may make sense to move this
+  // check to CustomButton.
+  if (event->type() == ui::ET_GESTURE_LONG_PRESS)
+    NotifyClick(*event);
+  else
+    LabelButton::OnGestureEvent(event);
+}
+
+gfx::Size AvatarButton::GetMinimumSize() const {
+  if (use_win10_native_button_) {
+    // Returns the "cozy" size of the button. Called by
+    // GlassBrowserFrameView::LayoutProfileSwitcher() when it calculates that
+    // the button needs to slide over the tabstrip.
+    return gfx::Size(kMdButtonMinWidth, kMdButtonCozyHeight);
+  }
+
+  return LabelButton::GetMinimumSize();
+}
+
+gfx::Size AvatarButton::GetPreferredSize() const {
+  gfx::Size size = LabelButton::GetPreferredSize();
+
+  if (use_win10_native_button_) {
+    // Returns the "tall" (normal) size of the button. Its height should match
+    // the caption button height.
+    size.set_width(
+        std::min(std::max(size.width(), kMdButtonMinWidth), kMdButtonMaxWidth));

[Peter Kasting, 2017/05/06 02:23:09]

Nit: MathUtil::ClampToRange()?

[emx, 2017/05/09 16:26:52]

I tried that, but "git cl upload" failed:

** Presubmit ERRORS **
You added one or more #includes that violate checkdeps rules.
  chrome\browser\ui\views\profiles\avatar_button.cc
      Illegal include: "cc/base/math_util.h"
    Because of no rule applying.

[Peter Kasting, 2017/05/11 01:03:43]

Sadness.  I'm dealing with this separately, don't worry about it.

+    // TODO(emx): get the proper height here - see http://crrev/2833363002

[Peter Kasting, 2017/05/06 02:23:08]

You can mark this patchset as dependent on that one and then actually do this
here, now.

[emx, 2017/05/09 16:26:52]

That one has been committed now.

+    size.set_height(30);
+  } else {
+    size.set_height(kThemedButtonHeight);
+  }
+
+  return size;
+}
+
+bool AvatarButton::ShouldUseFloodFillInkDrop() const {
+  return true;
+}
+
+std::unique_ptr<views::InkDropHighlight> AvatarButton::CreateInkDropHighlight()
+    const {
+  auto center = gfx::RectF(GetLocalBounds()).CenterPoint();
+  auto ink_drop_highlight = base::MakeUnique<views::InkDropHighlight>(
+      size(), 0, center, GetInkDropBaseColor());
+  ink_drop_highlight->set_visible_opacity(kInkDropHighlightOpacity);
+  return ink_drop_highlight;
+}
+
+void AvatarButton::OnAvatarErrorChanged() {
+  Update();
+}
+
+void AvatarButton::OnProfileAdded(const base::FilePath& profile_path) {
+  Update();
+}
+
+void AvatarButton::OnProfileWasRemoved(const base::FilePath& profile_path,
+                                       const base::string16& profile_name) {
+  // If deleting the active profile, don't bother updating the avatar
+  // button, as the browser window is being closed anyway.
+  if (profile_->GetPath() != profile_path)
+    Update();
+}
+
+void AvatarButton::OnProfileNameChanged(
+    const base::FilePath& profile_path,
+    const base::string16& old_profile_name) {
+  if (profile_->GetPath() == profile_path)
+    Update();
+}
+
+void AvatarButton::OnProfileSupervisedUserIdChanged(
+    const base::FilePath& profile_path) {
+  if (profile_->GetPath() == profile_path)
+    Update();
+}
+
+void AvatarButton::Update() {
+  ProfileAttributesStorage& storage =
+      g_browser_process->profile_manager()->GetProfileAttributesStorage();
+
+  // If we have a single local profile, then use the generic avatar
+  // button instead of the profile name. Never use the generic button if
+  // the active profile is Guest.
+  const bool use_generic_button =
+      !profile_->IsGuestSession() && storage.GetNumberOfProfiles() == 1 &&
+      !SigninManagerFactory::GetForProfile(profile_)->IsAuthenticated();
+

[Peter Kasting, 2017/05/06 02:23:09]

Nit: Blank line not necessary.  Or, leave the function below inlined here.

[emx, 2017/05/09 16:26:52]

Inlined.

+  UpdateButton(use_generic_button);
+
+  PreferredSizeChanged();
+}
+
+void AvatarButton::UpdateButton(bool use_generic_button) {
+  SetText(use_generic_button
+              ? base::string16()
+              : profiles::GetAvatarButtonTextForProfile(profile_));
+
+  // If the button has no text, clear the text shadows to make sure the
+  // image is centered correctly.
+  SetTextShadows(
+      use_generic_button
+          ? gfx::ShadowValues()
+          : gfx::ShadowValues(
+                10, gfx::ShadowValue(gfx::Vector2d(), 2.0f, SK_ColorDKGRAY)));
+
+  // We want the button to resize if the new text is shorter.
+  SetMinSize(gfx::Size());
+
+  if (use_generic_button) {
+    SetImage(views::Button::STATE_NORMAL, generic_avatar_);
+  } else if (error_controller_.HasAvatarError()) {
+    SetImage(views::Button::STATE_NORMAL,
+             gfx::CreateVectorIcon(kSyncProblemIcon, 16, gfx::kGoogleRed700));
+  } else {
+    SetImage(views::Button::STATE_NORMAL, gfx::ImageSkia());
+  }
+
+  // If we are not using the generic button, then reset the spacing between
+  // the text and the possible authentication error icon.
+  const int kDefaultImageTextSpacing = 5;
+  SetImageLabelSpacing(use_generic_button ? 0 : kDefaultImageTextSpacing);
+}
+
+void AvatarButton::SetButtonAvatar(int avatar_idr) {
+  ui::ResourceBundle* rb = &ui::ResourceBundle::GetSharedInstance();
+  generic_avatar_ = *rb->GetImageNamed(avatar_idr).ToImageSkia();
+}