Skip to content

Commit

Permalink
Revert of Refactor the avatar button/icon class (patchset chromium#14
Browse files Browse the repository at this point in the history
…id:320001 of https://codereview.chromium.org/1009403002/)

Reason for revert:
Very likely has caused the latest flakiness of
BrowserNonClientFrameViewAshTest.AvatarDisplayOnTeleportedWindow
and
BrowserNonClientFrameViewAshTest.AvatarMenuButtonHitTestOnChromeOS

on
ChromeOS dbg:

https://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%20Tests%20%28dbg%29%281%29/builds/2069

Original issue's description:
> Refactor the avatar button/icon class.
> Such that they are based on the same base class.
>
> BUG=452524
>
> Committed: https://crrev.com/56078ce434955a67159b59303f6691adb55d7ca1
> Cr-Commit-Position: refs/heads/master@{#329249}

TBR=noms@chromium.org,msw@chromium.org,yiyaoliu@chromium.org
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=452524

Review URL: https://codereview.chromium.org/1139943002

Cr-Commit-Position: refs/heads/master@{#329394}
  • Loading branch information
pneubeck authored and Commit bot committed May 12, 2015
1 parent 3e966ca commit 73d41c0
Show file tree
Hide file tree
Showing 8 changed files with 88 additions and 155 deletions.
20 changes: 20 additions & 0 deletions chrome/browser/ui/views/frame/browser_non_client_frame_view.cc
Original file line number Diff line number Diff line change
Expand Up @@ -215,6 +215,26 @@ void BrowserNonClientFrameView::UpdateOldAvatarButton() {
avatar_button_ = nullptr;
frame_->GetRootView()->Layout();
}

gfx::Image avatar;
gfx::Image taskbar_badge_avatar;
bool is_rectangle = false;

// Update the avatar button in the window frame and the taskbar overlay.
bool should_show_avatar_menu =
avatar_button_ || AvatarMenu::ShouldShowAvatarMenu();

if (!AvatarMenuButton::GetAvatarImages(
browser_view_->browser()->profile(), 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);
}

void BrowserNonClientFrameView::UpdateNewAvatarButton(
Expand Down
60 changes: 0 additions & 60 deletions chrome/browser/ui/views/profiles/avatar_base_button.cc

This file was deleted.

42 changes: 0 additions & 42 deletions chrome/browser/ui/views/profiles/avatar_base_button.h

This file was deleted.

26 changes: 3 additions & 23 deletions chrome/browser/ui/views/profiles/avatar_menu_button.cc
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,8 @@ static inline int Round(double x) {
const char AvatarMenuButton::kViewClassName[] = "AvatarMenuButton";

AvatarMenuButton::AvatarMenuButton(Browser* browser, bool disabled)
: AvatarBaseButton(browser),
MenuButton(NULL, base::string16(), this, false),
: MenuButton(NULL, base::string16(), this, false),
browser_(browser),
disabled_(disabled),
is_rectangle_(false),
old_height_(0),
Expand All @@ -46,8 +46,6 @@ AvatarMenuButton::AvatarMenuButton(Browser* browser, bool disabled)

SetEventTargeter(
scoped_ptr<views::ViewTargeter>(new views::ViewTargeter(this)));

Update();
}

AvatarMenuButton::~AvatarMenuButton() {
Expand Down Expand Up @@ -146,24 +144,6 @@ bool AvatarMenuButton::GetAvatarImages(Profile* profile,
return true;
}

void AvatarMenuButton::Update() {
// The browser can be null in tests.
if (!browser())
return;

const bool should_show_avatar_menu = AvatarMenu::ShouldShowAvatarMenu();
SetEnabled(should_show_avatar_menu);

gfx::Image avatar;
gfx::Image taskbar_badge_avatar;
bool is_rectangle = false;
if (AvatarMenuButton::GetAvatarImages(
browser()->profile(), should_show_avatar_menu, &avatar,
&taskbar_badge_avatar, &is_rectangle)) {
SetAvatarIcon(avatar, is_rectangle);
}
}

// views::ViewTargeterDelegate:
bool AvatarMenuButton::DoesIntersectRect(const views::View* target,
const gfx::Rect& rect) const {
Expand All @@ -176,5 +156,5 @@ bool AvatarMenuButton::DoesIntersectRect(const views::View* target,
void AvatarMenuButton::OnMenuButtonClicked(views::View* source,
const gfx::Point& point) {
if (!disabled_)
chrome::ShowAvatarMenu(browser());
chrome::ShowAvatarMenu(browser_);
}
8 changes: 2 additions & 6 deletions chrome/browser/ui/views/profiles/avatar_menu_button.h
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@
#include <string>

#include "base/compiler_specific.h"
#include "chrome/browser/ui/views/profiles/avatar_base_button.h"
#include "ui/base/models/simple_menu_model.h"
#include "ui/views/controls/button/menu_button.h"
#include "ui/views/controls/button/menu_button_listener.h"
Expand All @@ -26,8 +25,7 @@ class Profile;
// A button used to show either the incognito avatar or the profile avatar.
// The button can optionally have a menu attached to it.

class AvatarMenuButton : public AvatarBaseButton,
public views::MenuButton,
class AvatarMenuButton : public views::MenuButton,
public views::MenuButtonListener,
public views::ViewTargeterDelegate {
public:
Expand Down Expand Up @@ -63,9 +61,6 @@ class AvatarMenuButton : public AvatarBaseButton,
gfx::Image* avatar,
gfx::Image* taskbar_badge_avatar,
bool* is_rectangle);
protected:
// AvatarBaseButton:
void Update() override;

private:
// views::ViewTargeterDelegate:
Expand All @@ -76,6 +71,7 @@ class AvatarMenuButton : public AvatarBaseButton,
void OnMenuButtonClicked(views::View* source,
const gfx::Point& point) override;

Browser* browser_;
bool disabled_;
scoped_ptr<ui::MenuModel> menu_model_;

Expand Down
60 changes: 45 additions & 15 deletions chrome/browser/ui/views/profiles/new_avatar_button.cc
Original file line number Diff line number Diff line change
Expand Up @@ -46,8 +46,8 @@ scoped_ptr<views::Border> CreateBorder(const int normal_image_set[],
NewAvatarButton::NewAvatarButton(views::ButtonListener* listener,
AvatarButtonStyle button_style,
Browser* browser)
: AvatarBaseButton(browser),
LabelButton(listener, base::string16()),
: LabelButton(listener, base::string16()),
browser_(browser),
has_auth_error_(false),
suppress_mouse_released_action_(false) {
set_triggerable_event_flags(
Expand Down Expand Up @@ -95,10 +95,12 @@ NewAvatarButton::NewAvatarButton(views::ButtonListener* listener,
*rb->GetImageNamed(IDR_AVATAR_GLASS_BUTTON_AVATAR).ToImageSkia();
}

g_browser_process->profile_manager()->GetProfileInfoCache().AddObserver(this);

// Subscribe to authentication error changes so that the avatar button can
// update itself. Note that guest mode profiles won't have a token service.
SigninErrorController* error =
profiles::GetSigninErrorController(browser->profile());
profiles::GetSigninErrorController(browser_->profile());
if (error) {
error->AddObserver(this);
OnErrorChanged(); // This calls Update().
Expand All @@ -109,8 +111,10 @@ NewAvatarButton::NewAvatarButton(views::ButtonListener* listener,
}

NewAvatarButton::~NewAvatarButton() {
g_browser_process->profile_manager()->
GetProfileInfoCache().RemoveObserver(this);
SigninErrorController* error =
profiles::GetSigninErrorController(browser()->profile());
profiles::GetSigninErrorController(browser_->profile());
if (error)
error->RemoveObserver(this);
}
Expand All @@ -128,19 +132,54 @@ void NewAvatarButton::OnMouseReleased(const ui::MouseEvent& event) {
LabelButton::OnMouseReleased(event);
}

void NewAvatarButton::OnProfileAdded(const base::FilePath& profile_path) {
Update();
}

void NewAvatarButton::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 (browser_->profile()->GetPath() != profile_path)
Update();
}

void NewAvatarButton::OnProfileNameChanged(
const base::FilePath& profile_path,
const base::string16& old_profile_name) {
if (browser_->profile()->GetPath() == profile_path)
Update();
}

void NewAvatarButton::OnProfileSupervisedUserIdChanged(
const base::FilePath& profile_path) {
if (browser_->profile()->GetPath() == profile_path)
Update();
}

void NewAvatarButton::OnErrorChanged() {
// If there is an error, show an warning icon.
const SigninErrorController* error =
profiles::GetSigninErrorController(browser_->profile());
has_auth_error_ = error && error->HasError();

Update();
}

void NewAvatarButton::Update() {
const ProfileInfoCache& cache =
g_browser_process->profile_manager()->GetProfileInfoCache();

// 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.
bool use_generic_button = (!browser()->profile()->IsGuestSession() &&
bool use_generic_button = (!browser_->profile()->IsGuestSession() &&
cache.GetNumberOfProfiles() == 1 &&
!cache.ProfileIsAuthenticatedAtIndex(0));

SetText(use_generic_button ? base::string16() :
profiles::GetAvatarButtonTextForProfile(browser()->profile()));
profiles::GetAvatarButtonTextForProfile(browser_->profile()));

// If the button has no text, clear the text shadows to make sure the
// image is centered correctly.
Expand Down Expand Up @@ -170,12 +209,3 @@ void NewAvatarButton::Update() {

PreferredSizeChanged();
}

void NewAvatarButton::OnErrorChanged() {
// If there is an error, show a warning.
const SigninErrorController* error =
profiles::GetSigninErrorController(browser()->profile());
has_auth_error_ = error && error->HasError();

Update();
}
25 changes: 18 additions & 7 deletions chrome/browser/ui/views/profiles/new_avatar_button.h
Original file line number Diff line number Diff line change
Expand Up @@ -5,15 +5,15 @@
#ifndef CHROME_BROWSER_UI_VIEWS_PROFILES_NEW_AVATAR_BUTTON_H_
#define CHROME_BROWSER_UI_VIEWS_PROFILES_NEW_AVATAR_BUTTON_H_

#include "chrome/browser/ui/views/profiles/avatar_base_button.h"
#include "chrome/browser/profiles/profile_info_cache_observer.h"
#include "components/signin/core/browser/signin_error_controller.h"
#include "ui/views/controls/button/label_button.h"

class Browser;

// Avatar button that displays the active profile's name in the caption area.
class NewAvatarButton : public AvatarBaseButton,
public views::LabelButton,
class NewAvatarButton : public views::LabelButton,
public ProfileInfoCacheObserver,
public SigninErrorController::Observer {
public:
// Different button styles that can be applied.
Expand All @@ -31,16 +31,27 @@ class NewAvatarButton : public AvatarBaseButton,
bool OnMousePressed(const ui::MouseEvent& event) override;
void OnMouseReleased(const ui::MouseEvent& event) override;

protected:
// AvatarBaseButton:
void Update() override;

private:
friend class ProfileChooserViewExtensionsTest;

// ProfileInfoCacheObserver:
void OnProfileAdded(const base::FilePath& profile_path) override;
void OnProfileWasRemoved(const base::FilePath& profile_path,
const base::string16& profile_name) override;
void OnProfileNameChanged(const base::FilePath& profile_path,
const base::string16& old_profile_name) override;
void OnProfileSupervisedUserIdChanged(
const base::FilePath& profile_path) override;

// SigninErrorController::Observer:
void OnErrorChanged() override;

// Called when the profile info cache has changed, which means we might
// have to update the icon/text of the button.
void Update();

Browser* browser_;

// Whether the signed in profile has an authentication error. Used to display
// an error icon next to the button text.
bool has_auth_error_;
Expand Down
2 changes: 0 additions & 2 deletions chrome/chrome_browser_ui.gypi
Original file line number Diff line number Diff line change
Expand Up @@ -2312,8 +2312,6 @@
'browser/ui/views/passwords/save_account_more_combobox_model.cc',
'browser/ui/views/passwords/save_account_more_combobox_model.h',
'browser/ui/views/process_singleton_dialog_linux.cc',
'browser/ui/views/profiles/avatar_base_button.cc',
'browser/ui/views/profiles/avatar_base_button.h',
'browser/ui/views/profiles/avatar_menu_bubble_view.cc',
'browser/ui/views/profiles/avatar_menu_bubble_view.h',
'browser/ui/views/profiles/avatar_menu_button.cc',
Expand Down

0 comments on commit 73d41c0

Please sign in to comment.