Fix the icons for the app menu and the upgrade item in it when an update is available.
A regression was introduced in r581520 that caused the wrong icon and/or
the wrong coloring to be used in some circumstances. This CL fixes this
in two parts:
- AppMenuIconController may decide that the "annoyance level" from a
pending update is too low to bother the user. One bug introduced in
r581520 was that the controller still notified delegates that the
UPGRADE_NOTIFICATION icon type should be used in this case. Now, the
VERY_LOW annoyance level is ignore entirely for beta and stable
channels.
- AppMenuModel no longer bases its decision to include the upgrade menu
item directly on the UpgradeDetector. Rather, it now queries the
AppMenuIconController. This ensures that the same logic is used for
both the badging of the app menu and for the presence of the upgrade
item in the menu.
BUG=898958
Change-Id: I4b761844365968a24bc69030d711122fcaf16e28
Reviewed-on: https://chromium-review.googlesource.com/c/1312475
Commit-Queue: Greg Thompson <grt@chromium.org>
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Cr-Commit-Position: refs/heads/master@{#606842}diff --git a/chrome/browser/ui/toolbar/app_menu_icon_controller.cc b/chrome/browser/ui/toolbar/app_menu_icon_controller.cc
index 0ea3a47..96cdd30a 100644
--- a/chrome/browser/ui/toolbar/app_menu_icon_controller.cc
+++ b/chrome/browser/ui/toolbar/app_menu_icon_controller.cc
@@ -7,6 +7,7 @@
#include "base/logging.h"
#include "build/build_config.h"
#include "chrome/browser/chrome_notification_types.h"
+#include "chrome/browser/defaults.h"
#include "chrome/browser/ui/global_error/global_error_service.h"
#include "chrome/browser/ui/global_error/global_error_service_factory.h"
#include "chrome/browser/upgrade_detector/upgrade_detector.h"
@@ -55,17 +56,6 @@
return AppMenuIconController::Severity::NONE;
}
-// Returns true if we should show the upgrade recommended icon.
-bool ShouldShowUpgradeRecommended() {
-#if defined(OS_CHROMEOS)
- // In chromeos, the update recommendation is shown in the system tray. So it
- // should not be displayed in the app menu.
- return false;
-#else
- return UpgradeDetector::GetInstance()->notify_upgrade();
-#endif
-}
-
// Return true if the browser is updating on the dev or canary channels.
bool IsUnstableChannel() {
// Unbranded (Chromium) builds are on the UNKNOWN channel, so check explicitly
@@ -80,7 +70,14 @@
AppMenuIconController::AppMenuIconController(Profile* profile,
Delegate* delegate)
+ : AppMenuIconController(UpgradeDetector::GetInstance(), profile, delegate) {
+}
+
+AppMenuIconController::AppMenuIconController(UpgradeDetector* upgrade_detector,
+ Profile* profile,
+ Delegate* delegate)
: is_unstable_channel_(IsUnstableChannel()),
+ upgrade_detector_(upgrade_detector),
profile_(profile),
delegate_(delegate) {
DCHECK(profile_);
@@ -89,31 +86,39 @@
registrar_.Add(this, chrome::NOTIFICATION_GLOBAL_ERRORS_CHANGED,
content::Source<Profile>(profile_));
- UpgradeDetector::GetInstance()->AddObserver(this);
+ upgrade_detector_->AddObserver(this);
}
AppMenuIconController::~AppMenuIconController() {
- UpgradeDetector::GetInstance()->RemoveObserver(this);
+ upgrade_detector_->RemoveObserver(this);
}
void AppMenuIconController::UpdateDelegate() {
- if (ShouldShowUpgradeRecommended()) {
+ delegate_->UpdateTypeAndSeverity(GetTypeAndSeverity());
+}
+
+AppMenuIconController::TypeAndSeverity
+AppMenuIconController::GetTypeAndSeverity() const {
+ if (browser_defaults::kShowUpgradeMenuItem &&
+ upgrade_detector_->notify_upgrade()) {
UpgradeDetector::UpgradeNotificationAnnoyanceLevel level =
- UpgradeDetector::GetInstance()->upgrade_notification_stage();
+ upgrade_detector_->upgrade_notification_stage();
+ // The severity may be NONE even if the detector has been notified of an
+ // update. This can happen for beta and stable channels once the VERY_LOW
+ // annoyance level is reached.
auto severity = SeverityFromUpgradeLevel(is_unstable_channel_, level);
- delegate_->UpdateSeverity(IconType::UPGRADE_NOTIFICATION, severity);
- return;
+ if (severity != Severity::NONE)
+ return {IconType::UPGRADE_NOTIFICATION, severity};
}
if (GlobalErrorServiceFactory::GetForProfile(profile_)
->GetHighestSeverityGlobalErrorWithAppMenuItem()) {
// If you change the severity here, make sure to also change the menu icon
// and the bubble icon.
- delegate_->UpdateSeverity(IconType::GLOBAL_ERROR, Severity::MEDIUM);
- return;
+ return {IconType::GLOBAL_ERROR, Severity::MEDIUM};
}
- delegate_->UpdateSeverity(IconType::NONE, Severity::NONE);
+ return {IconType::NONE, Severity::NONE};
}
void AppMenuIconController::Observe(
diff --git a/chrome/browser/ui/toolbar/app_menu_icon_controller.h b/chrome/browser/ui/toolbar/app_menu_icon_controller.h
index 5ad267c3..8d0cbf4 100644
--- a/chrome/browser/ui/toolbar/app_menu_icon_controller.h
+++ b/chrome/browser/ui/toolbar/app_menu_icon_controller.h
@@ -5,6 +5,8 @@
#ifndef CHROME_BROWSER_UI_TOOLBAR_APP_MENU_ICON_CONTROLLER_H_
#define CHROME_BROWSER_UI_TOOLBAR_APP_MENU_ICON_CONTROLLER_H_
+#include <stdint.h>
+
#include "base/macros.h"
#include "build/build_config.h"
#include "chrome/browser/upgrade_detector/upgrade_observer.h"
@@ -14,6 +16,7 @@
#include "content/public/browser/notification_service.h"
class Profile;
+class UpgradeDetector;
// AppMenuIconController encapsulates the logic for badging the app menu icon
// as a result of various events - such as available updates, errors, etc.
@@ -21,25 +24,30 @@
public content::NotificationObserver,
public UpgradeObserver {
public:
- enum class IconType {
+ enum class IconType : uint32_t {
NONE,
UPGRADE_NOTIFICATION,
GLOBAL_ERROR,
};
- enum class Severity {
+ enum class Severity : uint32_t {
NONE,
LOW,
MEDIUM,
HIGH,
};
+ // The app menu icon's type and severity.
+ struct TypeAndSeverity {
+ IconType type;
+ Severity severity;
+ };
+
// Delegate interface for receiving icon update notifications.
class Delegate {
public:
- // Notifies the UI to update the icon to have the specified |severity|, as
- // well as specifying whether it should |animate|. The |type| parameter
- // specifies the type of change (i.e. the source of the notification).
- virtual void UpdateSeverity(IconType type, Severity severity) = 0;
+ // Notifies the UI to update the icon to have the specified
+ // |type_and_severity|.
+ virtual void UpdateTypeAndSeverity(TypeAndSeverity type_and_severity) = 0;
protected:
virtual ~Delegate() {}
@@ -48,6 +56,9 @@
// Creates an instance of this class for the given |profile| that will notify
// |delegate| of updates.
AppMenuIconController(Profile* profile, Delegate* delegate);
+ AppMenuIconController(UpgradeDetector* upgrade_detector,
+ Profile* profile,
+ Delegate* delegate);
~AppMenuIconController() override;
// Forces an update of the UI based on the current state of the world. This
@@ -56,6 +67,9 @@
// delegate.
void UpdateDelegate();
+ // Returns the icon type and severity based on the current state.
+ TypeAndSeverity GetTypeAndSeverity() const;
+
private:
// content::NotificationObserver:
void Observe(int type,
@@ -67,8 +81,9 @@
// True for desktop Chrome on dev and canary channels.
const bool is_unstable_channel_;
- Profile* profile_;
- Delegate* delegate_;
+ UpgradeDetector* const upgrade_detector_;
+ Profile* const profile_;
+ Delegate* const delegate_;
content::NotificationRegistrar registrar_;
DISALLOW_COPY_AND_ASSIGN(AppMenuIconController);
diff --git a/chrome/browser/ui/toolbar/app_menu_icon_controller_unittest.cc b/chrome/browser/ui/toolbar/app_menu_icon_controller_unittest.cc
new file mode 100644
index 0000000..e9331e7
--- /dev/null
+++ b/chrome/browser/ui/toolbar/app_menu_icon_controller_unittest.cc
@@ -0,0 +1,187 @@
+// Copyright 2018 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/toolbar/app_menu_icon_controller.h"
+
+#include "base/macros.h"
+#include "base/time/default_tick_clock.h"
+#include "base/time/time.h"
+#include "build/build_config.h"
+#include "chrome/browser/defaults.h"
+#include "chrome/browser/upgrade_detector/upgrade_detector.h"
+#include "chrome/test/base/testing_profile.h"
+#include "content/public/test/test_browser_thread_bundle.h"
+#include "testing/gmock/include/gmock/gmock.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+#if defined(OS_WIN)
+#include "chrome/install_static/install_modes.h"
+#include "chrome/install_static/test/scoped_install_details.h"
+#endif
+
+namespace {
+
+class MockAppMenuIconControllerDelegate
+ : public AppMenuIconController::Delegate {
+ public:
+ MOCK_METHOD1(UpdateTypeAndSeverity,
+ void(AppMenuIconController::TypeAndSeverity type_and_severity));
+};
+
+// A fake upgrade detector that can broadcast an annoyance level change to its
+// observers.
+class FakeUpgradeDetector : public UpgradeDetector {
+ public:
+ FakeUpgradeDetector()
+ : UpgradeDetector(base::DefaultTickClock::GetInstance()) {}
+
+ void BroadcastLevel(UpgradeNotificationAnnoyanceLevel level) {
+ set_upgrade_notification_stage(level);
+ NotifyUpgrade();
+ }
+
+ // UpgradeDetector:
+ base::TimeDelta GetHighAnnoyanceLevelDelta() override;
+ base::TimeTicks GetHighAnnoyanceDeadline() override;
+
+ private:
+ // UpgradeDetector:
+ void OnRelaunchNotificationPeriodPrefChanged() override;
+
+ DISALLOW_COPY_AND_ASSIGN(FakeUpgradeDetector);
+};
+
+base::TimeDelta FakeUpgradeDetector::GetHighAnnoyanceLevelDelta() {
+ // This value is not important for this test.
+ return base::TimeDelta();
+}
+
+base::TimeTicks FakeUpgradeDetector::GetHighAnnoyanceDeadline() {
+ // This value is not important for this test.
+ return base::TimeTicks();
+}
+
+void FakeUpgradeDetector::OnRelaunchNotificationPeriodPrefChanged() {}
+
+} // namespace
+
+bool operator==(const AppMenuIconController::TypeAndSeverity& a,
+ const AppMenuIconController::TypeAndSeverity& b) {
+ return a.type == b.type && a.severity == b.severity;
+}
+
+// A test parameterized on an install mode index. For Google Chrome builds on
+// Windows, this allows the test to run for each of the supported side-by-side
+// channels. For Chromium builds, there is only the one channel. For non-Win
+// builds, there does not appear to be an easy way to run the test as if it were
+// a different channel.
+class AppMenuIconControllerTest : public ::testing::TestWithParam<int> {
+ protected:
+ AppMenuIconControllerTest()
+#if defined(OS_WIN)
+ : install_details_(false, GetParam())
+#endif
+ {
+ }
+ UpgradeDetector* upgrade_detector() { return &upgrade_detector_; }
+ Profile* profile() { return &profile_; }
+
+ // Returns true if the test is apparently running as an unstable channel.
+ bool IsUnstableChannel() {
+#if !defined(GOOGLE_CHROME_BUILD)
+ // Dev and canary channels are specific to Google Chrome branding.
+ return false;
+#elif defined(OS_WIN)
+ // Windows supports specifying the channel via ScopedInstallDetails.
+ return GetParam() >= install_static::DEV_INDEX;
+#else
+ // Non-Windows platforms don't have a way to specify the channel; see
+ // https://crbug.com/903798.
+ return false;
+#endif
+ }
+
+ // Broadcasts a change to |level| to the UpgradeDetector's observers.
+ void BroadcastLevel(
+ UpgradeDetector::UpgradeNotificationAnnoyanceLevel level) {
+ upgrade_detector_.BroadcastLevel(level);
+ }
+
+ private:
+#if defined(OS_WIN)
+ install_static::ScopedInstallDetails install_details_;
+#endif
+ FakeUpgradeDetector upgrade_detector_;
+ content::TestBrowserThreadBundle thread_bundle_;
+ TestingProfile profile_;
+
+ DISALLOW_COPY_AND_ASSIGN(AppMenuIconControllerTest);
+};
+
+// Tests that the controller's delegate is notified with the proper icon type
+// and severity when an upgrade is detected.
+TEST_P(AppMenuIconControllerTest, UpgradeNotification) {
+ ::testing::StrictMock<MockAppMenuIconControllerDelegate> mock_delegate;
+
+ AppMenuIconController controller(upgrade_detector(), profile(),
+ &mock_delegate);
+
+ ::testing::InSequence sequence;
+
+ if (!browser_defaults::kShowUpgradeMenuItem) {
+ // Chrome OS doesn't change the icon.
+ EXPECT_CALL(mock_delegate,
+ UpdateTypeAndSeverity(AppMenuIconController::TypeAndSeverity{
+ AppMenuIconController::IconType::NONE,
+ AppMenuIconController::Severity::NONE}))
+ .Times(4);
+ } else if (IsUnstableChannel()) {
+ // For dev and canary channels, the upgrade notification should be sent out
+ // at a low level for every annoyance level.
+ EXPECT_CALL(mock_delegate,
+ UpdateTypeAndSeverity(AppMenuIconController::TypeAndSeverity{
+ AppMenuIconController::IconType::UPGRADE_NOTIFICATION,
+ AppMenuIconController::Severity::LOW}))
+ .Times(4);
+ } else {
+ // For stable and beta channels, the "none" type and severity should be sent
+ // for the "very low" annoyance level, and the ordinary corresponding
+ // severity for each other annoyance level.
+ EXPECT_CALL(mock_delegate,
+ UpdateTypeAndSeverity(AppMenuIconController::TypeAndSeverity{
+ AppMenuIconController::IconType::NONE,
+ AppMenuIconController::Severity::NONE}));
+ EXPECT_CALL(mock_delegate,
+ UpdateTypeAndSeverity(AppMenuIconController::TypeAndSeverity{
+ AppMenuIconController::IconType::UPGRADE_NOTIFICATION,
+ AppMenuIconController::Severity::LOW}));
+ EXPECT_CALL(mock_delegate,
+ UpdateTypeAndSeverity(AppMenuIconController::TypeAndSeverity{
+ AppMenuIconController::IconType::UPGRADE_NOTIFICATION,
+ AppMenuIconController::Severity::MEDIUM}));
+ EXPECT_CALL(mock_delegate,
+ UpdateTypeAndSeverity(AppMenuIconController::TypeAndSeverity{
+ AppMenuIconController::IconType::UPGRADE_NOTIFICATION,
+ AppMenuIconController::Severity::HIGH}));
+ }
+ EXPECT_CALL(mock_delegate,
+ UpdateTypeAndSeverity(AppMenuIconController::TypeAndSeverity{
+ AppMenuIconController::IconType::NONE,
+ AppMenuIconController::Severity::NONE}));
+
+ BroadcastLevel(UpgradeDetector::UPGRADE_ANNOYANCE_VERY_LOW);
+ BroadcastLevel(UpgradeDetector::UPGRADE_ANNOYANCE_LOW);
+ BroadcastLevel(UpgradeDetector::UPGRADE_ANNOYANCE_ELEVATED);
+ BroadcastLevel(UpgradeDetector::UPGRADE_ANNOYANCE_HIGH);
+ BroadcastLevel(UpgradeDetector::UPGRADE_ANNOYANCE_NONE);
+}
+
+#if defined(OS_WIN)
+INSTANTIATE_TEST_CASE_P(
+ ,
+ AppMenuIconControllerTest,
+ ::testing::Range(0, static_cast<int>(install_static::NUM_INSTALL_MODES)));
+#else
+INSTANTIATE_TEST_CASE_P(, AppMenuIconControllerTest, ::testing::Values(0));
+#endif
diff --git a/chrome/browser/ui/toolbar/app_menu_model.cc b/chrome/browser/ui/toolbar/app_menu_model.cc
index 380a008..a2eebeaf 100644
--- a/chrome/browser/ui/toolbar/app_menu_model.cc
+++ b/chrome/browser/ui/toolbar/app_menu_model.cc
@@ -37,6 +37,7 @@
#include "chrome/browser/ui/global_error/global_error_service_factory.h"
#include "chrome/browser/ui/managed_ui.h"
#include "chrome/browser/ui/tabs/tab_strip_model.h"
+#include "chrome/browser/ui/toolbar/app_menu_icon_controller.h"
#include "chrome/browser/ui/toolbar/bookmark_sub_menu_model.h"
#include "chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.h"
#include "chrome/browser/ui/toolbar/toolbar_actions_bar.h"
@@ -228,11 +229,14 @@
////////////////////////////////////////////////////////////////////////////////
// AppMenuModel
-AppMenuModel::AppMenuModel(ui::AcceleratorProvider* provider, Browser* browser)
+AppMenuModel::AppMenuModel(ui::AcceleratorProvider* provider,
+ Browser* browser,
+ AppMenuIconController* app_menu_icon_controller)
: ui::SimpleMenuModel(this),
uma_action_recorded_(false),
provider_(provider),
- browser_(browser) {}
+ browser_(browser),
+ app_menu_icon_controller_(app_menu_icon_controller) {}
AppMenuModel::~AppMenuModel() {
if (browser_) // Null in Cocoa tests.
@@ -290,6 +294,7 @@
case IDC_INSTALL_PWA:
return GetInstallPWAAppMenuItemName(browser_).value();
case IDC_UPGRADE_DIALOG:
+ DCHECK(browser_defaults::kShowUpgradeMenuItem);
return GetUpgradeDialogMenuItemName();
default:
NOTREACHED();
@@ -298,8 +303,8 @@
}
bool AppMenuModel::GetIconForCommandId(int command_id, gfx::Image* icon) const {
- if (command_id == IDC_UPGRADE_DIALOG &&
- UpgradeDetector::GetInstance()->notify_upgrade()) {
+ if (command_id == IDC_UPGRADE_DIALOG) {
+ DCHECK(browser_defaults::kShowUpgradeMenuItem);
*icon = UpgradeDetector::GetInstance()->GetIcon();
return true;
}
@@ -653,9 +658,12 @@
#endif
case IDC_PIN_TO_START_SCREEN:
return false;
- case IDC_UPGRADE_DIALOG:
- return browser_defaults::kShowUpgradeMenuItem &&
- UpgradeDetector::GetInstance()->notify_upgrade();
+ case IDC_UPGRADE_DIALOG: {
+ if (!browser_defaults::kShowUpgradeMenuItem || !app_menu_icon_controller_)
+ return false;
+ return app_menu_icon_controller_->GetTypeAndSeverity().type ==
+ AppMenuIconController::IconType::UPGRADE_NOTIFICATION;
+ }
#if !defined(OS_LINUX) || defined(USE_AURA)
case IDC_BOOKMARK_PAGE:
return !chrome::ShouldRemoveBookmarkThisPageUI(browser_->profile());
diff --git a/chrome/browser/ui/toolbar/app_menu_model.h b/chrome/browser/ui/toolbar/app_menu_model.h
index 6ddd0be..6d06271 100644
--- a/chrome/browser/ui/toolbar/app_menu_model.h
+++ b/chrome/browser/ui/toolbar/app_menu_model.h
@@ -19,6 +19,7 @@
#include "ui/base/models/button_menu_item_model.h"
#include "ui/base/models/simple_menu_model.h"
+class AppMenuIconController;
class BookmarkSubMenuModel;
class Browser;
class RecentTabsSubMenuModel;
@@ -114,8 +115,12 @@
static const int kMaxRecentTabsCommandId = 1200;
// Creates an app menu model for the given browser. Init() must be called
- // before passing this to an AppMenu.
- AppMenuModel(ui::AcceleratorProvider* provider, Browser* browser);
+ // before passing this to an AppMenu. |app_menu_icon_controller|, if provided,
+ // is used to decide whether or not to include an item for opening the upgrade
+ // dialog.
+ AppMenuModel(ui::AcceleratorProvider* provider,
+ Browser* browser,
+ AppMenuIconController* app_menu_icon_controller = nullptr);
~AppMenuModel() override;
// Runs Build() and registers observers.
@@ -224,6 +229,7 @@
ui::AcceleratorProvider* provider_; // weak
Browser* const browser_; // weak
+ AppMenuIconController* const app_menu_icon_controller_;
std::unique_ptr<content::HostZoomMap::Subscription>
browser_zoom_subscription_;
diff --git a/chrome/browser/ui/toolbar/app_menu_model_unittest.cc b/chrome/browser/ui/toolbar/app_menu_model_unittest.cc
index 90cac5b..27d9b7f 100644
--- a/chrome/browser/ui/toolbar/app_menu_model_unittest.cc
+++ b/chrome/browser/ui/toolbar/app_menu_model_unittest.cc
@@ -15,6 +15,7 @@
#include "chrome/browser/ui/global_error/global_error_service.h"
#include "chrome/browser/ui/global_error/global_error_service_factory.h"
#include "chrome/browser/ui/tabs/tab_strip_model.h"
+#include "chrome/browser/ui/toolbar/app_menu_icon_controller.h"
#include "chrome/browser/upgrade_detector/upgrade_detector.h"
#include "chrome/test/base/browser_with_test_window_test.h"
#include "chrome/test/base/menu_model_test.h"
@@ -52,6 +53,15 @@
DISALLOW_COPY_AND_ASSIGN(MenuError);
};
+class FakeIconDelegate : public AppMenuIconController::Delegate {
+ public:
+ FakeIconDelegate() = default;
+
+ // AppMenuIconController::Delegate:
+ void UpdateTypeAndSeverity(
+ AppMenuIconController::TypeAndSeverity type_and_severity) override {}
+};
+
} // namespace
class AppMenuModelTest : public BrowserWithTestWindowTest,
@@ -75,8 +85,10 @@
// not derived from SimpleMenuModel.
class TestAppMenuModel : public AppMenuModel {
public:
- TestAppMenuModel(ui::AcceleratorProvider* provider, Browser* browser)
- : AppMenuModel(provider, browser),
+ TestAppMenuModel(ui::AcceleratorProvider* provider,
+ Browser* browser,
+ AppMenuIconController* app_menu_icon_controller)
+ : AppMenuModel(provider, browser, app_menu_icon_controller),
execute_count_(0),
checked_count_(0),
enable_count_(0) {}
@@ -104,7 +116,18 @@
};
TEST_F(AppMenuModelTest, Basics) {
- TestAppMenuModel model(this, browser());
+ // Simulate that an update is available to ensure that the menu includes the
+ // upgrade item for platforms that support it.
+ UpgradeDetector* detector = UpgradeDetector::GetInstance();
+ detector->set_upgrade_notification_stage(
+ UpgradeDetector::UPGRADE_ANNOYANCE_LOW);
+ detector->NotifyUpgrade();
+ EXPECT_TRUE(detector->notify_upgrade());
+
+ FakeIconDelegate fake_delegate;
+ AppMenuIconController app_menu_icon_controller(browser()->profile(),
+ &fake_delegate);
+ TestAppMenuModel model(this, browser(), &app_menu_icon_controller);
model.Init();
int itemCount = model.GetItemCount();
@@ -112,20 +135,20 @@
// the exact number.
EXPECT_GT(itemCount, 10);
- UpgradeDetector* detector = UpgradeDetector::GetInstance();
- detector->set_upgrade_notification_stage(
- UpgradeDetector::UPGRADE_ANNOYANCE_LOW);
- detector->NotifyUpgrade();
- EXPECT_TRUE(detector->notify_upgrade());
+ // Verify that the upgrade item is visible if supported.
EXPECT_EQ(browser_defaults::kShowUpgradeMenuItem,
model.IsCommandIdVisible(IDC_UPGRADE_DIALOG));
// Execute a couple of the items and make sure it gets back to our delegate.
// We can't use CountEnabledExecutable() here because the encoding menu's
// delegate is internal, it doesn't use the one we pass in.
- // Note: The new menu has a spacing separator at the first slot.
- model.ActivatedAt(1);
- EXPECT_TRUE(model.IsEnabledAt(1));
+ // Note: the second item in the menu may be a separator if the browser
+ // supports showing upgrade status in the app menu.
+ int item_index = 1;
+ if (model.GetTypeAt(item_index) == ui::MenuModel::TYPE_SEPARATOR)
+ ++item_index;
+ model.ActivatedAt(item_index);
+ EXPECT_TRUE(model.IsEnabledAt(item_index));
// Make sure to use the index that is not separator in all configurations.
model.ActivatedAt(itemCount - 1);
EXPECT_TRUE(model.IsEnabledAt(itemCount - 1));
diff --git a/chrome/browser/ui/views/toolbar/browser_app_menu_button.cc b/chrome/browser/ui/views/toolbar/browser_app_menu_button.cc
index d993e5ca..0e27ae5 100644
--- a/chrome/browser/ui/views/toolbar/browser_app_menu_button.cc
+++ b/chrome/browser/ui/views/toolbar/browser_app_menu_button.cc
@@ -45,7 +45,10 @@
bool BrowserAppMenuButton::g_open_app_immediately_for_testing = false;
BrowserAppMenuButton::BrowserAppMenuButton(ToolbarView* toolbar_view)
- : AppMenuButton(toolbar_view), toolbar_view_(toolbar_view) {
+ : AppMenuButton(toolbar_view),
+ type_and_severity_{AppMenuIconController::IconType::NONE,
+ AppMenuIconController::Severity::NONE},
+ toolbar_view_(toolbar_view) {
SetInkDropMode(InkDropMode::ON);
SetFocusPainter(nullptr);
SetHorizontalAlignment(gfx::ALIGN_CENTER);
@@ -58,14 +61,12 @@
BrowserAppMenuButton::~BrowserAppMenuButton() {}
-void BrowserAppMenuButton::SetSeverity(
- AppMenuIconController::IconType type,
- AppMenuIconController::Severity severity) {
- type_ = type;
- severity_ = severity;
+void BrowserAppMenuButton::SetTypeAndSeverity(
+ AppMenuIconController::TypeAndSeverity type_and_severity) {
+ type_and_severity_ = type_and_severity;
SetTooltipText(
- severity_ == AppMenuIconController::Severity::NONE
+ type_and_severity_.severity == AppMenuIconController::Severity::NONE
? l10n_util::GetStringUTF16(IDS_APPMENU_TOOLTIP)
: l10n_util::GetStringUTF16(IDS_APPMENU_TOOLTIP_UPDATE_AVAILABLE));
UpdateIcon();
@@ -93,8 +94,10 @@
Browser* browser = toolbar_view_->browser();
- InitMenu(std::make_unique<AppMenuModel>(toolbar_view_, browser), browser,
- for_drop ? AppMenu::FOR_DROP : AppMenu::NO_FLAGS);
+ InitMenu(
+ std::make_unique<AppMenuModel>(toolbar_view_, browser,
+ toolbar_view_->app_menu_icon_controller()),
+ browser, for_drop ? AppMenu::FOR_DROP : AppMenu::NO_FLAGS);
base::TimeTicks menu_open_time = base::TimeTicks::Now();
menu()->RunMenu(this);
@@ -116,7 +119,7 @@
SkColor severity_color = gfx::kPlaceholderColor;
const ui::NativeTheme* native_theme = GetNativeTheme();
- switch (severity_) {
+ switch (type_and_severity_.severity) {
case AppMenuIconController::Severity::NONE:
severity_color = GetThemeProvider()->GetColor(
ThemeProperties::COLOR_TOOLBAR_BUTTON_ICON);
@@ -137,10 +140,11 @@
const bool touch_ui = ui::MaterialDesignController::touch_ui();
const gfx::VectorIcon* icon_id = nullptr;
- switch (type_) {
+ switch (type_and_severity_.type) {
case AppMenuIconController::IconType::NONE:
icon_id = touch_ui ? &kBrowserToolsTouchIcon : &kBrowserToolsIcon;
- DCHECK_EQ(AppMenuIconController::Severity::NONE, severity_);
+ DCHECK_EQ(AppMenuIconController::Severity::NONE,
+ type_and_severity_.severity);
break;
case AppMenuIconController::IconType::UPGRADE_NOTIFICATION:
icon_id =
diff --git a/chrome/browser/ui/views/toolbar/browser_app_menu_button.h b/chrome/browser/ui/views/toolbar/browser_app_menu_button.h
index ff631cc..d7923f3 100644
--- a/chrome/browser/ui/views/toolbar/browser_app_menu_button.h
+++ b/chrome/browser/ui/views/toolbar/browser_app_menu_button.h
@@ -25,10 +25,12 @@
explicit BrowserAppMenuButton(ToolbarView* toolbar_view);
~BrowserAppMenuButton() override;
- void SetSeverity(AppMenuIconController::IconType type,
- AppMenuIconController::Severity severity);
+ void SetTypeAndSeverity(
+ AppMenuIconController::TypeAndSeverity type_and_severity);
- AppMenuIconController::Severity severity() { return severity_; }
+ AppMenuIconController::Severity severity() {
+ return type_and_severity_.severity;
+ }
// Shows the app menu. |for_drop| indicates whether the menu is opened for a
// drag-and-drop operation.
@@ -76,9 +78,7 @@
std::unique_ptr<views::InkDropHighlight> CreateInkDropHighlight()
const override;
- AppMenuIconController::Severity severity_ =
- AppMenuIconController::Severity::NONE;
- AppMenuIconController::IconType type_ = AppMenuIconController::IconType::NONE;
+ AppMenuIconController::TypeAndSeverity type_and_severity_;
// Our owning toolbar view.
ToolbarView* const toolbar_view_;
diff --git a/chrome/browser/ui/views/toolbar/toolbar_view.cc b/chrome/browser/ui/views/toolbar/toolbar_view.cc
index 3a07ec4..cb7262f 100644
--- a/chrome/browser/ui/views/toolbar/toolbar_view.cc
+++ b/chrome/browser/ui/views/toolbar/toolbar_view.cc
@@ -673,19 +673,20 @@
// ToolbarView, private:
// AppMenuIconController::Delegate:
-void ToolbarView::UpdateSeverity(AppMenuIconController::IconType type,
- AppMenuIconController::Severity severity) {
+void ToolbarView::UpdateTypeAndSeverity(
+ AppMenuIconController::TypeAndSeverity type_and_severity) {
// There's no app menu in tabless windows.
if (!app_menu_button_)
return;
base::string16 accname_app = l10n_util::GetStringUTF16(IDS_ACCNAME_APP);
- if (type == AppMenuIconController::IconType::UPGRADE_NOTIFICATION) {
+ if (type_and_severity.type ==
+ AppMenuIconController::IconType::UPGRADE_NOTIFICATION) {
accname_app = l10n_util::GetStringFUTF16(
IDS_ACCNAME_APP_UPGRADE_RECOMMENDED, accname_app);
}
app_menu_button_->SetAccessibleName(accname_app);
- app_menu_button_->SetSeverity(type, severity);
+ app_menu_button_->SetTypeAndSeverity(type_and_severity);
}
// ToolbarButtonProvider:
diff --git a/chrome/browser/ui/views/toolbar/toolbar_view.h b/chrome/browser/ui/views/toolbar/toolbar_view.h
index 03f771fa..16c0dac 100644
--- a/chrome/browser/ui/views/toolbar/toolbar_view.h
+++ b/chrome/browser/ui/views/toolbar/toolbar_view.h
@@ -190,8 +190,8 @@
};
// AppMenuIconController::Delegate:
- void UpdateSeverity(AppMenuIconController::IconType type,
- AppMenuIconController::Severity severity) override;
+ void UpdateTypeAndSeverity(
+ AppMenuIconController::TypeAndSeverity type_and_severity) override;
// ToolbarButtonProvider:
BrowserActionsContainer* GetBrowserActionsContainer() override;
diff --git a/chrome/test/BUILD.gn b/chrome/test/BUILD.gn
index f84c2b5..eedb223 100644
--- a/chrome/test/BUILD.gn
+++ b/chrome/test/BUILD.gn
@@ -2801,6 +2801,7 @@
sources += [
"../browser/component_updater/crl_set_component_installer_unittest.cc",
"../browser/ui/hats/hats_unittest.cc",
+ "../browser/ui/toolbar/app_menu_icon_controller_unittest.cc",
]
}
