From fd1a6fd38213817eb7d7e8d0df6c82fc9223ee0c Mon Sep 17 00:00:00 2001 From: estade Date: Thu, 3 Sep 2015 11:01:18 -0700 Subject: [PATCH] Switch global error menu icon to vectorized MD asset visible e.g. with the sync passphrase global error BUG=505953 Review URL: https://codereview.chromium.org/1289413003 Cr-Commit-Position: refs/heads/master@{#347188} --- .../recovery/recovery_install_global_error.cc | 5 +- .../recovery/recovery_install_global_error.h | 2 +- chrome/browser/signin/signin_ui_util.cc | 25 +++++--- chrome/browser/signin/signin_ui_util.h | 7 --- .../browser/ui/cocoa/toolbar/toolbar_controller.mm | 3 + chrome/browser/ui/global_error/global_error.cc | 21 +++++-- chrome/browser/ui/global_error/global_error.h | 4 +- chrome/browser/ui/libgtk2ui/native_theme_gtk2.cc | 1 + chrome/browser/ui/toolbar/wrench_menu_model.cc | 72 +--------------------- .../browser/ui/views/profiles/new_avatar_button.cc | 8 ++- ui/native_theme/common_theme.cc | 4 ++ ui/native_theme/native_theme.h | 1 + 12 files changed, 55 insertions(+), 98 deletions(-) diff --git a/chrome/browser/recovery/recovery_install_global_error.cc b/chrome/browser/recovery/recovery_install_global_error.cc index 243d586528f5..ce80798ef63d 100644 --- a/chrome/browser/recovery/recovery_install_global_error.cc +++ b/chrome/browser/recovery/recovery_install_global_error.cc @@ -65,8 +65,9 @@ base::string16 RecoveryInstallGlobalError::MenuItemLabel() { return l10n_util::GetStringUTF16(IDS_UPDATE_NOW); } -int RecoveryInstallGlobalError::MenuItemIconResourceID() { - return IDR_UPDATE_MENU_SEVERITY_HIGH; +gfx::Image RecoveryInstallGlobalError::MenuItemIcon() { + return ResourceBundle::GetSharedInstance().GetNativeImageNamed( + IDR_UPDATE_MENU_SEVERITY_HIGH); } void RecoveryInstallGlobalError::ExecuteMenuItem(Browser* browser) { diff --git a/chrome/browser/recovery/recovery_install_global_error.h b/chrome/browser/recovery/recovery_install_global_error.h index 20f64595e0b6..3e97f56f3fbe 100644 --- a/chrome/browser/recovery/recovery_install_global_error.h +++ b/chrome/browser/recovery/recovery_install_global_error.h @@ -30,7 +30,7 @@ class RecoveryInstallGlobalError : public GlobalErrorWithStandardBubble, bool HasMenuItem() override; int MenuItemCommandID() override; base::string16 MenuItemLabel() override; - int MenuItemIconResourceID() override; + gfx::Image MenuItemIcon() override; void ExecuteMenuItem(Browser* browser) override; bool HasBubbleView() override; bool HasShownBubbleView() override; diff --git a/chrome/browser/signin/signin_ui_util.cc b/chrome/browser/signin/signin_ui_util.cc index 563bef921a65..bab391d20493 100644 --- a/chrome/browser/signin/signin_ui_util.cc +++ b/chrome/browser/signin/signin_ui_util.cc @@ -28,21 +28,15 @@ #include "ui/gfx/font_list.h" #include "ui/gfx/text_elider.h" +namespace signin_ui_util { + namespace { + // Maximum width of a username - we trim emails that are wider than this so // the wrench menu doesn't get ridiculously wide. const int kUsernameMaxWidth = 200; -} // namespace - -namespace signin_ui_util { - -GlobalError* GetSignedInServiceError(Profile* profile) { - std::vector errors = GetSignedInServiceErrors(profile); - if (errors.empty()) - return NULL; - return errors[0]; -} +// Returns all errors reported by signed in services. std::vector GetSignedInServiceErrors(Profile* profile) { std::vector errors; // Chrome OS doesn't use SigninGlobalError or SyncGlobalError. Other platforms @@ -68,6 +62,17 @@ std::vector GetSignedInServiceErrors(Profile* profile) { return errors; } +// If a signed in service is reporting an error, returns the GlobalError +// object associated with that service, or NULL if no errors are reported. +GlobalError* GetSignedInServiceError(Profile* profile) { + std::vector errors = GetSignedInServiceErrors(profile); + if (errors.empty()) + return NULL; + return errors[0]; +} + +} // namespace + base::string16 GetSigninMenuLabel(Profile* profile) { GlobalError* error = signin_ui_util::GetSignedInServiceError(profile); if (error) diff --git a/chrome/browser/signin/signin_ui_util.h b/chrome/browser/signin/signin_ui_util.h index 943573801cea..34458dd092a7 100644 --- a/chrome/browser/signin/signin_ui_util.h +++ b/chrome/browser/signin/signin_ui_util.h @@ -20,13 +20,6 @@ namespace signin_ui_util { // The maximum number of times to show the welcome tutorial for an upgrade user. const int kUpgradeWelcomeTutorialShowMax = 1; -// If a signed in service is reporting an error, returns the GlobalError -// object associated with that service, or NULL if no errors are reported. -GlobalError* GetSignedInServiceError(Profile* profile); - -// Returns all errors reported by signed in services. -std::vector GetSignedInServiceErrors(Profile* profile); - // Returns the label that should be displayed in the signin menu (i.e. // "Sign in to Chromium", "Signin Error...", etc). base::string16 GetSigninMenuLabel(Profile* profile); diff --git a/chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm b/chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm index 5e9f500cf5d7..ae45ebd62716 100644 --- a/chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm +++ b/chrome/browser/ui/cocoa/toolbar/toolbar_controller.mm @@ -21,6 +21,7 @@ #include "chrome/browser/command_updater.h" #include "chrome/browser/profiles/profile.h" #include "chrome/browser/search/search.h" +#include "chrome/browser/sync/sync_global_error_factory.h" #include "chrome/browser/themes/theme_service.h" #include "chrome/browser/ui/browser.h" #include "chrome/browser/ui/browser_commands.h" @@ -246,6 +247,8 @@ class NotificationBridge : public WrenchMenuBadgeController::Delegate { profile:profile browser:browser nibFileNamed:@"Toolbar"])) { + // Start global error services now so we badge the menu correctly. + SyncGlobalErrorFactory::GetForProfile(profile); } return self; } diff --git a/chrome/browser/ui/global_error/global_error.cc b/chrome/browser/ui/global_error/global_error.cc index 4390baa7e991..7f464027dd8a 100644 --- a/chrome/browser/ui/global_error/global_error.cc +++ b/chrome/browser/ui/global_error/global_error.cc @@ -10,6 +10,13 @@ #include "ui/base/resource/resource_bundle.h" #include "ui/gfx/image/image.h" +#if !defined(OS_MACOSX) && !defined(OS_ANDROID) +#include "ui/gfx/paint_vector_icon.h" +#include "ui/gfx/vector_icons_public.h" +#include "ui/native_theme/common_theme.h" +#include "ui/native_theme/native_theme.h" +#endif + // GlobalError --------------------------------------------------------------- GlobalError::GlobalError() {} @@ -18,10 +25,16 @@ GlobalError::~GlobalError() {} GlobalError::Severity GlobalError::GetSeverity() { return SEVERITY_MEDIUM; } -int GlobalError::MenuItemIconResourceID() { - // If you change this make sure to also change the bubble icon and the wrench - // icon color. - return IDR_INPUT_ALERT_MENU; +gfx::Image GlobalError::MenuItemIcon() { +#if defined(OS_MACOSX) || defined(OS_ANDROID) + return ResourceBundle::GetSharedInstance().GetNativeImageNamed( + IDR_INPUT_ALERT_MENU); +#else + SkColor icon_color; + ui::CommonThemeGetSystemColor(ui::NativeTheme::kColorId_Amber, &icon_color); + return gfx::Image( + gfx::CreateVectorIcon(gfx::VectorIconId::WARNING, 18, icon_color)); +#endif } // GlobalErrorWithStandardBubble --------------------------------------------- diff --git a/chrome/browser/ui/global_error/global_error.h b/chrome/browser/ui/global_error/global_error.h index 1038177aa0e0..12055c570551 100644 --- a/chrome/browser/ui/global_error/global_error.h +++ b/chrome/browser/ui/global_error/global_error.h @@ -41,8 +41,8 @@ class GlobalError { virtual int MenuItemCommandID() = 0; // Returns the label for the menu item. virtual base::string16 MenuItemLabel() = 0; - // Returns the resource ID for the menu item icon. - virtual int MenuItemIconResourceID(); + // Returns the menu item icon. + virtual gfx::Image MenuItemIcon(); // Called when the user clicks on the menu item. virtual void ExecuteMenuItem(Browser* browser) = 0; diff --git a/chrome/browser/ui/libgtk2ui/native_theme_gtk2.cc b/chrome/browser/ui/libgtk2ui/native_theme_gtk2.cc index 91d220f01229..b7a61f170cda 100644 --- a/chrome/browser/ui/libgtk2ui/native_theme_gtk2.cc +++ b/chrome/browser/ui/libgtk2ui/native_theme_gtk2.cc @@ -417,6 +417,7 @@ GdkColor NativeThemeGtk2::GetSystemGdkColor(ColorId color_id) const { GetWindowStyle()->bg[GTK_STATE_NORMAL], 0xff / 2); } + case kColorId_Amber: case kColorId_ChromeIconGrey: case kColorId_GoogleBlue: case kColorId_NumColors: diff --git a/chrome/browser/ui/toolbar/wrench_menu_model.cc b/chrome/browser/ui/toolbar/wrench_menu_model.cc index 6f7068effc0b..625359402eed 100644 --- a/chrome/browser/ui/toolbar/wrench_menu_model.cc +++ b/chrome/browser/ui/toolbar/wrench_menu_model.cc @@ -366,8 +366,7 @@ bool WrenchMenuModel::IsItemForCommandIdDynamic(int command_id) const { #elif defined(OS_WIN) command_id == IDC_PIN_TO_START_SCREEN || #endif - command_id == IDC_UPGRADE_DIALOG || - (!switches::IsNewAvatarMenu() && command_id == IDC_SHOW_SIGNIN); + command_id == IDC_UPGRADE_DIALOG; } base::string16 WrenchMenuModel::GetLabelForCommandId(int command_id) const { @@ -397,10 +396,6 @@ base::string16 WrenchMenuModel::GetLabelForCommandId(int command_id) const { #endif case IDC_UPGRADE_DIALOG: return GetUpgradeDialogMenuItemName(); - case IDC_SHOW_SIGNIN: - DCHECK(!switches::IsNewAvatarMenu()); - return signin_ui_util::GetSigninMenuLabel( - browser_->profile()->GetOriginalProfile()); default: NOTREACHED(); return base::string16(); @@ -419,19 +414,6 @@ bool WrenchMenuModel::GetIconForCommandId(int command_id, } return false; } - case IDC_SHOW_SIGNIN: { - DCHECK(!switches::IsNewAvatarMenu()); - GlobalError* error = signin_ui_util::GetSignedInServiceError( - browser_->profile()->GetOriginalProfile()); - if (error) { - int icon_id = error->MenuItemIconResourceID(); - if (icon_id) { - *icon = rb.GetNativeImageNamed(icon_id); - return true; - } - } - return false; - } default: break; } @@ -446,16 +428,6 @@ void WrenchMenuModel::ExecuteCommand(int command_id, int event_flags) { return; } - if (!switches::IsNewAvatarMenu() && command_id == IDC_SHOW_SIGNIN) { - // If a custom error message is being shown, handle it. - GlobalError* error = signin_ui_util::GetSignedInServiceError( - browser_->profile()->GetOriginalProfile()); - if (error) { - error->ExecuteMenuItem(browser_); - return; - } - } - LogMenuMetrics(command_id); chrome::ExecuteCommand(browser_, command_id); } @@ -930,21 +902,6 @@ void WrenchMenuModel::Build() { CreateCutCopyPasteMenu(); AddItemWithStringId(IDC_OPTIONS, IDS_SETTINGS); -#if !defined(OS_CHROMEOS) - if (!switches::IsNewAvatarMenu()) { - // No "Sign in to Chromium..." menu item on ChromeOS. - SigninManager* signin = SigninManagerFactory::GetForProfile( - browser_->profile()->GetOriginalProfile()); - if (signin && signin->IsSigninAllowed() && - signin_ui_util::GetSignedInServiceErrors( - browser_->profile()->GetOriginalProfile()).empty()) { - AddItem(IDC_SHOW_SYNC_SETUP, - l10n_util::GetStringFUTF16( - IDS_SYNC_MENU_PRE_SYNCED_LABEL, - l10n_util::GetStringUTF16(IDS_SHORT_PRODUCT_NAME))); - } - } -#endif // The help submenu is only displayed on official Chrome builds. As the // 'About' item has been moved to this submenu, it's reinstated here for // Chromium builds. @@ -989,12 +946,6 @@ bool WrenchMenuModel::AddGlobalErrorMenuItems() { // window. This means that if a new error is added after the menu is built // it won't show in the existing wrench menu. To fix this we need to some // how update the menu if new errors are added. - ui::ResourceBundle& rb = ui::ResourceBundle::GetSharedInstance(); - // GetSignedInServiceErrors() can modify the global error list, so call it - // before iterating through that list below. - std::vector signin_errors; - signin_errors = signin_ui_util::GetSignedInServiceErrors( - browser_->profile()->GetOriginalProfile()); const GlobalErrorService::GlobalErrorList& errors = GlobalErrorServiceFactory::GetForProfile(browser_->profile())->errors(); bool menu_items_added = false; @@ -1003,26 +954,9 @@ bool WrenchMenuModel::AddGlobalErrorMenuItems() { GlobalError* error = *it; DCHECK(error); if (error->HasMenuItem()) { -#if !defined(OS_CHROMEOS) - // Don't add a signin error if it's already being displayed elsewhere. - if (std::find(signin_errors.begin(), signin_errors.end(), error) != - signin_errors.end()) { - MenuModel* model = this; - int index = 0; - if (MenuModel::GetModelAndIndexForCommandId( - IDC_SHOW_SIGNIN, &model, &index)) { - continue; - } - } -#endif - AddItem(error->MenuItemCommandID(), error->MenuItemLabel()); - int icon_id = error->MenuItemIconResourceID(); - if (icon_id) { - const gfx::Image& image = rb.GetNativeImageNamed(icon_id); - SetIcon(GetIndexOfCommandId(error->MenuItemCommandID()), - image); - } + SetIcon(GetIndexOfCommandId(error->MenuItemCommandID()), + error->MenuItemIcon()); menu_items_added = true; } } diff --git a/chrome/browser/ui/views/profiles/new_avatar_button.cc b/chrome/browser/ui/views/profiles/new_avatar_button.cc index 9b26ca398038..1e940c03e19a 100644 --- a/chrome/browser/ui/views/profiles/new_avatar_button.cc +++ b/chrome/browser/ui/views/profiles/new_avatar_button.cc @@ -16,6 +16,8 @@ #include "ui/gfx/geometry/vector2d.h" #include "ui/gfx/paint_vector_icon.h" #include "ui/gfx/vector_icons_public.h" +#include "ui/native_theme/common_theme.h" +#include "ui/native_theme/native_theme.h" #include "ui/views/border.h" #include "ui/views/controls/button/label_button_border.h" #include "ui/views/painter.h" @@ -208,10 +210,10 @@ void NewAvatarButton::Update() { if (use_generic_button) { SetImage(views::Button::STATE_NORMAL, generic_avatar_); } else if (has_auth_error_) { - // TODO(estade): revisit this color. + SkColor icon_color; + ui::CommonThemeGetSystemColor(ui::NativeTheme::kColorId_Amber, &icon_color); SetImage(views::Button::STATE_NORMAL, - gfx::CreateVectorIcon(gfx::VectorIconId::WARNING, 13, - SkColorSetRGB(0xFF, 0xC6, 0x1E))); + gfx::CreateVectorIcon(gfx::VectorIconId::WARNING, 13, icon_color)); } else { SetImage(views::Button::STATE_NORMAL, gfx::ImageSkia()); } diff --git a/ui/native_theme/common_theme.cc b/ui/native_theme/common_theme.cc index 462f7e9414cc..7d6456068141 100644 --- a/ui/native_theme/common_theme.cc +++ b/ui/native_theme/common_theme.cc @@ -40,6 +40,7 @@ const SkColor kBlueButtonPressedColor = SK_ColorWHITE; const SkColor kBlueButtonHoverColor = SK_ColorWHITE; const SkColor kBlueButtonShadowColor = SkColorSetRGB(0x53, 0x8C, 0xEA); // Material colors: +const SkColor kAmber = SkColorSetRGB(0xFF, 0xC1, 0x07); const SkColor kGoogleBlue = SkColorSetRGB(0x42, 0x85, 0xF4); const SkColor kChromeIconGrey = SkColorSetRGB(0x5A, 0x5A, 0x5A); // Material spinner/throbber: @@ -112,6 +113,9 @@ bool CommonThemeGetSystemColor(NativeTheme::ColorId color_id, SkColor* color) { *color = kBlueButtonShadowColor; break; // Material icons + case NativeTheme::kColorId_Amber: + *color = kAmber; + break; case NativeTheme::kColorId_ChromeIconGrey: *color = kChromeIconGrey; break; diff --git a/ui/native_theme/native_theme.h b/ui/native_theme/native_theme.h index 5cc5ea816cc9..1727424cba6b 100644 --- a/ui/native_theme/native_theme.h +++ b/ui/native_theme/native_theme.h @@ -325,6 +325,7 @@ class NATIVE_THEME_EXPORT NativeTheme { kColorId_ResultsTableNegativeHoveredText, kColorId_ResultsTableNegativeSelectedText, // For MD icons. + kColorId_Amber, kColorId_ChromeIconGrey, kColorId_GoogleBlue, // Colors for the material spinner (aka throbber). -- 2.11.4.GIT