Simplify some old avatar menu button code.

chrome/browser/ui/views/frame/browser_non_client_frame_view.cc

 // Copyright (c) 2012 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/frame/browser_non_client_frame_view.h"
 
 #include "build/build_config.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/profiles/avatar_menu.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/profiles/profile_avatar_icon_util.h"
 #include "chrome/browser/profiles/profile_manager.h"
 #include "chrome/browser/profiles/profiles_state.h"
 #include "chrome/browser/themes/theme_properties.h"
 #include "chrome/browser/ui/view_ids.h"
 #include "chrome/browser/ui/views/frame/browser_view.h"
 #include "chrome/browser/ui/views/frame/taskbar_decorator.h"
-#include "chrome/browser/ui/views/profiles/avatar_menu_button.h"
 #include "chrome/browser/ui/views/tabs/tab_strip.h"
 #include "components/signin/core/common/profile_management_switches.h"
 #include "grit/theme_resources.h"
 #include "third_party/skia/include/core/SkColor.h"
 #include "ui/base/material_design/material_design_controller.h"
 #include "ui/base/resource/resource_bundle.h"
 #include "ui/base/theme_provider.h"
 #include "ui/gfx/color_palette.h"
 #include "ui/gfx/image/image.h"
 #include "ui/gfx/paint_vector_icon.h"
@@ skipping 118 matching lines @@
 }
 
 gfx::ImageSkia BrowserNonClientFrameView::GetFrameImage() const {
   return GetFrameImage(ShouldPaintAsActive());
 }
 
 gfx::ImageSkia BrowserNonClientFrameView::GetFrameOverlayImage() const {
   return GetFrameOverlayImage(ShouldPaintAsActive());
 }
 
-void BrowserNonClientFrameView::UpdateOldAvatarButton() {
-  if (browser_view_->ShouldShowAvatar()) {
-    if (!avatar_button_) {
-      avatar_button_ = new AvatarMenuButton(browser_view_);
-      avatar_button_->set_id(VIEW_ID_AVATAR_BUTTON);
-      AddChildView(avatar_button_);
-      // Invalidate here because adding a child does not invalidate the layout.
-      InvalidateLayout();
-      frame_->GetRootView()->Layout();
-    }
-  } else if (avatar_button_) {
-    RemoveChildView(avatar_button_);
-    delete avatar_button_;
-    avatar_button_ = nullptr;
+void BrowserNonClientFrameView::UpdateProfileIndicatorIcon() {
+  if (!profile_indicator_) {
+    profile_indicator_ = new ProfileIndicatorIcon(browser_view_);
+    profile_indicator_->set_id(VIEW_ID_PROFILE_INDICATOR_ICON);
+    AddChildView(profile_indicator_);
+    // Invalidate here because adding a child does not invalidate the layout.
+    InvalidateLayout();
     frame_->GetRootView()->Layout();
   }
 
-  gfx::Image avatar;
-  gfx::Image taskbar_badge_avatar;
-  bool is_rectangle = false;
+  gfx::Image icon;
+  const Profile* profile = browser_view()->browser()->profile();
+  if (profile->GetProfileType() == Profile::INCOGNITO_PROFILE) {
+    icon = gfx::Image(GetOTRAvatarIcon());
+  } else {
+#if defined(OS_CHROMEOS)
+    bool unused;
+    AvatarMenu::GetImageForMenuButton(profile->GetPath(), &icon, &unused);

[Peter Kasting, 2016/05/17 03:51:42]

Nit: Does this need to be conditionally compiled in only for ChromeOS?  At a
glance it looks like it would link without the ifdef, in which case, why not
just do that?

[Evan Stade, 2016/05/17 18:13:08]

I wanted to add the NOTREACHED to document that this path shouldn't be hit
except on cros (which is otherwise not obvious).

[Peter Kasting, 2016/05/18 02:44:01]

Can we just have a comment that says "only reached on CrOS"?  I'm mainly trying
to avoid the ifdef, but I also kind of have an allergy to "if (foo) { ... } else
NOTREACHED" because it's technically kind of a violation of the "don't handle
DCHECK failure" policy.

[Evan Stade, 2016/05/20 22:04:42]

I personally don't have a problem with ifdefs. Sure, nested and/or complicated
ifdefs get hard to read, but I don't think this qualifies. I would prefer to
enforce a comment via NOTREACHED than to leave it up to the reader to notice. I
could make it

  } else {
#if !defined(OS_CHROMEOS)
    notreached
#endif
    dostuff
  }

if that made you feel better about not handling dchecks, but it seems a little
bit like going out of our way to avoid handling the dcheck (if the ifdef's
already there, why not use it).

[Peter Kasting, 2016/05/20 22:24:22]

How you have it is fine.

+#else
+    NOTREACHED();
+#endif
+  }
 
-  // Update the avatar button in the window frame and the taskbar overlay.
-  bool should_show_avatar_menu =
-      avatar_button_ || AvatarMenu::ShouldShowAvatarMenu();
-
-  if (!AvatarMenuButton::GetAvatarImages(this, should_show_avatar_menu, &avatar,
-                                         &taskbar_badge_avatar, &is_rectangle))
-    return;
-
-  // Disable the menu when we should not show the menu.
-  if (avatar_button_ && !AvatarMenu::ShouldShowAvatarMenu())
-    avatar_button_->SetEnabled(false);
-  if (avatar_button_)
-    avatar_button_->SetAvatarIcon(avatar, is_rectangle);
+  profile_indicator_->SetIcon(icon);
 }
 
 void BrowserNonClientFrameView::ViewHierarchyChanged(
     const ViewHierarchyChangedDetails& details) {
   if (details.is_add && details.child == this)
-    UpdateAvatar();
+    UpdateProfileIcons();
 }
 
 void BrowserNonClientFrameView::ActivationChanged(bool active) {
   if (ui::MaterialDesignController::IsModeMaterial()) {
     // On Windows, while deactivating the widget, this is called before the
     // active HWND has actually been changed.  Since we want the avatar state to
     // reflect that the window is inactive, we force NonClientFrameView to see
     // the "correct" state as an override.
     set_active_state_override(&active);
-    UpdateAvatar();
+    UpdateProfileIcons();
     set_active_state_override(nullptr);
 
     // Changing the activation state may change the toolbar top separator color
     // that's used as the stroke around tabs/the new tab button.
     browser_view_->tabstrip()->SchedulePaint();
   }
 
   // Changing the activation state may change the visible frame color.
   SchedulePaint();
 }
 
 void BrowserNonClientFrameView::OnProfileAdded(
     const base::FilePath& profile_path) {
   OnProfileAvatarChanged(profile_path);
 }
 
 void BrowserNonClientFrameView::OnProfileWasRemoved(
     const base::FilePath& profile_path,
     const base::string16& profile_name) {
   OnProfileAvatarChanged(profile_path);
 }
 
 void BrowserNonClientFrameView::OnProfileAvatarChanged(
     const base::FilePath& profile_path) {
   UpdateTaskbarDecoration();
-  UpdateAvatar();
+  UpdateProfileIcons();
 }
 
 const ui::ThemeProvider*
 BrowserNonClientFrameView::GetThemeProviderForProfile() const {
   // Because the frame's accessor reads the ThemeProvider from the profile and
   // not the widget, it can be called even before we're in a view hierarchy.
   return frame_->GetThemeProvider();
 }
 
 void BrowserNonClientFrameView::UpdateTaskbarDecoration() {
-  gfx::Image avatar;
-  gfx::Image taskbar_badge_avatar;
-  bool is_rectangle;
-  // Only need to update the taskbar overlay here.  If GetAvatarImages()
-  // returns false, don't bother trying to update the taskbar decoration since
-  // the returned images are not initialized.  This can happen if the user
-  // deletes the current profile.
-  if (AvatarMenuButton::GetAvatarImages(
-          this, AvatarMenu::ShouldShowAvatarMenu(), &avatar,
-          &taskbar_badge_avatar, &is_rectangle)) {
-    // For popups and panels which don't have the avatar button, we still
-    // need to draw the taskbar decoration. Even though we have an icon on the
-    // window's relaunch details, we draw over it because the user may have
-    // pinned the badge-less Chrome shortcut which will cause windows to ignore
-    // the relaunch details.
-    // TODO(calamity): ideally this should not be necessary but due to issues
-    // with the default shortcut being pinned, we add the runtime badge for
-    // safety. See crbug.com/313800.
-    bool show_decoration = AvatarMenu::ShouldShowAvatarMenu() &&
-        !browser_view_->browser()->profile()->IsGuestSession();
-    // In tests, make sure that the browser process and profile manager are
-    // valid before using.
-    if (g_browser_process && g_browser_process->profile_manager()) {
-      const ProfileAttributesStorage& storage =
-          g_browser_process->profile_manager()->GetProfileAttributesStorage();
-      show_decoration = show_decoration && storage.GetNumberOfProfiles() > 1;
-    }
-    chrome::DrawTaskbarDecoration(frame_->GetNativeWindow(),
-        show_decoration
-            ? (taskbar_badge_avatar.IsEmpty() ? &avatar : &taskbar_badge_avatar)
-            : nullptr);
+#if defined(OS_WIN)
+  if (browser_view()->browser()->profile()->IsGuestSession() ||
+      // In tests, make sure that the browser process and profile manager are
+      // valid before using.

[Peter Kasting, 2016/05/17 03:51:42]

Nit: Comment might be clearer as "Tests may not have a browser process or
profile manager, so null-check them before using."

[Evan Stade, 2016/05/17 18:13:08]

Done.

+      (g_browser_process && g_browser_process->profile_manager() &&
+       g_browser_process->profile_manager()
+               ->GetProfileAttributesStorage()
+               .GetNumberOfProfiles() <= 1)) {
+    chrome::DrawTaskbarDecoration(frame_->GetNativeWindow(), nullptr);

[Peter Kasting, 2016/05/17 03:51:42]

If you're going to make this whole block ifdefed for Windows only, I think we no
longer need the cross-platform declaration of this function and empty definition
of it.  It can be purely Windows-specific, and pulled in to this file via an
ifdefed #include.

[Evan Stade, 2016/05/17 18:13:08]

Done.

+    return;
   }
+
+  // For popups and panels which don't have the avatar button, we still
+  // need to draw the taskbar decoration. Even though we have an icon on the
+  // window's relaunch details, we draw over it because the user may have
+  // pinned the badge-less Chrome shortcut which will cause windows to ignore

[Peter Kasting, 2016/05/17 03:51:42]

Nit: While here: windows -> Windows ?

[Evan Stade, 2016/05/17 18:13:08]

Done.

+  // the relaunch details.
+  // TODO(calamity): ideally this should not be necessary but due to issues
+  // with the default shortcut being pinned, we add the runtime badge for
+  // safety. See crbug.com/313800.
+  gfx::Image decoration;
+  bool unused;

[Peter Kasting, 2016/05/17 03:51:42]

Nit: This is the second place in this file that doesn't want to use the third
arg.  Might be nice to allow this function to take nullptr there (maybe even as
a default argument).

[Evan Stade, 2016/05/17 18:13:08]

turns out it's not used anywhere any more. Removed altogether.

+  AvatarMenu::GetImageForMenuButton(
+      browser_view()->browser()->profile()->GetPath(), &decoration, &unused);
+  // This can happen if the user deletes the current profile.
+  if (decoration.IsEmpty())
+    return;
+  chrome::DrawTaskbarDecoration(frame_->GetNativeWindow(), &decoration);
+#endif
 }