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}

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 @@ AppMenuIconController::Severity SeverityFromUpgradeLevel(
   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 @@ bool IsUnstableChannel() {
 
 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 @@ AppMenuIconController::AppMenuIconController(Profile* profile,
   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(

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 @@ class AppMenuIconController :
     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 @@ class AppMenuIconController :
   // 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 @@ class AppMenuIconController :
   // 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 @@ class AppMenuIconController :
 
   // 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);

chrome/browser/ui/toolbar/app_menu_icon_controller_unittest.cc

+// 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

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 @@ void ToolsMenuModel::Build(Browser* browser) {
 ////////////////////////////////////////////////////////////////////////////////
 // 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 @@ base::string16 AppMenuModel::GetLabelForCommandId(int command_id) const {
     case IDC_INSTALL_PWA:
       return GetInstallPWAAppMenuItemName(browser_).value();
     case IDC_UPGRADE_DIALOG:
+      DCHECK(browser_defaults::kShowUpgradeMenuItem);
       return GetUpgradeDialogMenuItemName();
     default:
       NOTREACHED();
@@ -298,8 +303,8 @@ base::string16 AppMenuModel::GetLabelForCommandId(int command_id) const {
 }
 
 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 @@ bool AppMenuModel::IsCommandIdVisible(int command_id) const {
 #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());

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 @@ class AppMenuModel : public ui::SimpleMenuModel,
   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 @@ class AppMenuModel : public ui::SimpleMenuModel,
   ui::AcceleratorProvider* provider_;  // weak
 
   Browser* const browser_;  // weak
+  AppMenuIconController* const app_menu_icon_controller_;
 
   std::unique_ptr<content::HostZoomMap::Subscription>
       browser_zoom_subscription_;

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 @@ class MenuError : public GlobalError {
   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 @@ class AppMenuModelTest : public BrowserWithTestWindowTest,
 // 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 @@ class TestAppMenuModel : public AppMenuModel {
 };
 
 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 @@ TEST_F(AppMenuModelTest, Basics) {
   // 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));

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(ToolbarView* toolbar_view)
 
 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 @@ void BrowserAppMenuButton::ShowMenu(bool for_drop) {
 
   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 @@ void BrowserAppMenuButton::UpdateIcon() {
   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 @@ void BrowserAppMenuButton::UpdateIcon() {
 
   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 =

chrome/browser/ui/views/toolbar/browser_app_menu_button.h

@@ -25,10 +25,12 @@ class BrowserAppMenuButton : public AppMenuButton,
   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 @@ class BrowserAppMenuButton : public AppMenuButton,
   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_;

chrome/browser/ui/views/toolbar/toolbar_view.cc

@@ -673,19 +673,20 @@ void ToolbarView::OnTouchUiChanged() {
 // 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:

chrome/browser/ui/views/toolbar/toolbar_view.h

@@ -190,8 +190,8 @@ class ToolbarView : public views::AccessiblePaneView,
   };
 
   // AppMenuIconController::Delegate:
-  void UpdateSeverity(AppMenuIconController::IconType type,
-                      AppMenuIconController::Severity severity) override;
+  void UpdateTypeAndSeverity(
+      AppMenuIconController::TypeAndSeverity type_and_severity) override;
 
   // ToolbarButtonProvider:
   BrowserActionsContainer* GetBrowserActionsContainer() override;

chrome/test/BUILD.gn

@@ -2801,6 +2801,7 @@ test("unit_tests") {
     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",
     ]
   }