cros: Ensure NOTIFICATION_CONTAINER is enabled

NOTIFICATION_CONTAINER is a container MenuItemView which is added and
removed to an app context menu as notifications for the app come and go.

The container is added to the tree of MenuItemViews by adding it to
the menu's SimpleMenuModel.

The bug is that the container MenuItemView for NOTIFICATION_CONTAINER
is not enabled when added. The root cause is that when the
NOTIFICATION_CONTAINER is added, it is not addd to |menu_items_|(same
name for both classes modified).

I originally tried to ensure that items added to the SimpleMenuModel
were also added to |menu_items_|, but when adding an item to
|menu_items_| in ash/public/cpp/menu_utils.h, we check
|delegate_|->IsCommandIdEnabled(), which requires that the item is in
|menu_items_|. This means that new items are disabled. This is because
|menu_items_| was designed to be built once, and not modified.

This fix works because we don't need to check
|menu_items_| anyways because NOTIFICATION_CONTAINER is always enabled.

Fixing |menu_items_| so that it holds all items that SimpleMenuModel
holds would be ideal, but its current behavior is only a problem in
the NOTIFICATION_CONTAINER edge case.

Bug: 857565
Change-Id: I1ec1d6db20b1ba45d0a772eb2ec7ab3ec0498c06
Reviewed-on: https://chromium-review.googlesource.com/1119057
Commit-Queue: Alex Newcomer <newcomer@chromium.org>
Reviewed-by: James Cook <jamescook@chromium.org>
Cr-Commit-Position: refs/heads/master@{#572215}

ash/app_list/BUILD.gn

@@ -193,6 +193,7 @@ test("app_list_unittests") {
     "pagination_model_unittest.cc",
     "test/run_all_unittests.cc",
     "views/app_list_main_view_unittest.cc",
+    "views/app_list_menu_model_adapter_unittest.cc",
     "views/app_list_view_unittest.cc",
     "views/apps_grid_view_unittest.cc",
     "views/folder_header_view_unittest.cc",

ash/app_list/views/app_list_menu_model_adapter.cc

@@ -6,6 +6,7 @@
 
 #include <utility>
 
+#include "ash/public/cpp/app_menu_constants.h"
 #include "ash/public/cpp/menu_utils.h"
 #include "base/metrics/histogram_macros.h"
 #include "ui/views/controls/menu/menu_runner.h"
@@ -92,6 +93,12 @@ bool AppListMenuModelAdapter::IsItemChecked(int id) const {
 }
 
 bool AppListMenuModelAdapter::IsCommandEnabled(int id) const {
+  // NOTIFICATION_CONTAINER is always enabled. It is added to this model by
+  // NotificationMenuController, but it is not added to |menu_items_|, so check
+  // for it first.
+  if (id == ash::NOTIFICATION_CONTAINER)
+    return true;
+
   return ash::menu_utils::GetMenuItemByCommandId(menu_items_, id)->enabled;
 }
 

ash/app_list/views/app_list_menu_model_adapter_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 "ash/app_list/views/app_list_menu_model_adapter.h"
+
+#include "ash/public/cpp/app_menu_constants.h"
+#include "testing/gtest/include/gtest/gtest.h"
+#include "ui/views/test/views_test_base.h"
+
+namespace app_list {
+
+namespace {
+
+class MockAppListMenuModelAdapterDelegate
+    : public AppListMenuModelAdapter::Delegate {
+ public:
+  MockAppListMenuModelAdapterDelegate() = default;
+  virtual ~MockAppListMenuModelAdapterDelegate() = default;
+
+  void ExecuteCommand(int command_id, int event_flags) override {}
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(MockAppListMenuModelAdapterDelegate);
+};
+
+}  // namespace
+
+class AppListMenuModelAdapterTest : public views::ViewsTestBase {
+ public:
+  AppListMenuModelAdapterTest() {}
+  ~AppListMenuModelAdapterTest() override = default;
+
+  void SetUp() override {
+    views::ViewsTestBase::SetUp();
+    mock_app_list_menu_model_adapter_delegate_ =
+        std::make_unique<MockAppListMenuModelAdapterDelegate>();
+    app_list_menu_model_adapter_ = std::make_unique<AppListMenuModelAdapter>(
+        "test-app-id", nullptr, ui::MenuSourceType::MENU_SOURCE_TYPE_LAST,
+        mock_app_list_menu_model_adapter_delegate_.get(),
+        AppListMenuModelAdapter::FULLSCREEN_APP_GRID, base::OnceClosure());
+  }
+
+  std::unique_ptr<AppListMenuModelAdapter> app_list_menu_model_adapter_;
+
+ private:
+  std::unique_ptr<MockAppListMenuModelAdapterDelegate>
+      mock_app_list_menu_model_adapter_delegate_;
+
+  DISALLOW_COPY_AND_ASSIGN(AppListMenuModelAdapterTest);
+};
+
+// Tests that NOTIFICATION_CONTAINER is enabled. This ensures that the
+// container is able to handle gesture events.
+TEST_F(AppListMenuModelAdapterTest, NotificationContainerEnabled) {
+  EXPECT_TRUE(app_list_menu_model_adapter_->IsCommandEnabled(
+      ash::NOTIFICATION_CONTAINER));
+}
+
+}  // namespace app_list

ash/shelf/shelf_context_menu_model.cc

@@ -8,6 +8,7 @@
 #include <string>
 #include <utility>
 
+#include "ash/public/cpp/app_menu_constants.h"
 #include "ash/public/cpp/ash_pref_names.h"
 #include "ash/public/cpp/menu_utils.h"
 #include "ash/public/cpp/shelf_item_delegate.h"
@@ -143,6 +144,12 @@ bool ShelfContextMenuModel::IsCommandIdChecked(int command_id) const {
 }
 
 bool ShelfContextMenuModel::IsCommandIdEnabled(int command_id) const {
+  // NOTIFICATION_CONTAINER is always enabled. It is added to this model by
+  // NotificationMenuController, but it is not added to |menu_items_|, so check
+  // for it first.
+  if (command_id == ash::NOTIFICATION_CONTAINER)
+    return true;
+
   return menu_utils::GetMenuItemByCommandId(menu_items_, command_id)->enabled;
 }
 

ash/shelf/shelf_context_menu_model_unittest.cc

@@ -4,6 +4,7 @@
 
 #include "ash/shelf/shelf_context_menu_model.h"
 
+#include "ash/public/cpp/app_menu_constants.h"
 #include "ash/public/cpp/config.h"
 #include "ash/public/cpp/shelf_item_delegate.h"
 #include "ash/session/test_session_controller_client.h"
@@ -318,5 +319,14 @@ TEST_F(ShelfContextMenuModelTest, ShelfContextMenuOptions) {
   EXPECT_EQ(3, menu.GetItemCount());
 }
 
+TEST_F(ShelfContextMenuModelTest, NotificationContainerEnabled) {
+  // Tests that NOTIFICATION_CONTAINER is enabled. This ensures that the
+  // container is able to handle gesture events.
+  ShelfContextMenuModel menu(MenuItemList(), nullptr, GetPrimaryDisplay().id());
+  menu.AddItem(NOTIFICATION_CONTAINER, base::string16());
+
+  EXPECT_TRUE(menu.IsCommandIdEnabled(NOTIFICATION_CONTAINER));
+}
+
 }  // namespace
 }  // namespace ash