Remove ui::AcceleratorManagerDelegate

It was added in the early days of mustash to support the mus window
server. Now that we have WS2 / window service as a part of ash it isn't
needed anymore. It was only being used in tests.

Bug: 842365, 866526
Test: ui_base_unittests, ash_unittests, manually try accelerators
Change-Id: Ib156f42103ef21c6cdaf95183bf828d0959ad84e
Reviewed-on: https://chromium-review.googlesource.com/1225594Reviewed-by: Scott Violet <sky@chromium.org>
Commit-Queue: James Cook <jamescook@chromium.org>
Cr-Commit-Position: refs/heads/master@{#591370}

ash/accelerators/accelerator_controller.cc

@@ -1034,10 +1034,9 @@ void HandleTouchHudModeChange() {
 ////////////////////////////////////////////////////////////////////////////////
 // AcceleratorController, public:
 
-AcceleratorController::AcceleratorController(
-    ui::AcceleratorManagerDelegate* manager_delegate)
-    : accelerator_manager_(new ui::AcceleratorManager(manager_delegate)),
-      accelerator_history_(new ui::AcceleratorHistory) {
+AcceleratorController::AcceleratorController()
+    : accelerator_manager_(std::make_unique<ui::AcceleratorManager>()),
+      accelerator_history_(std::make_unique<ui::AcceleratorHistory>()) {
   Init();
 }
 

ash/accelerators/accelerator_controller.h

@@ -26,7 +26,6 @@
 
 namespace ui {
 class AcceleratorManager;
-class AcceleratorManagerDelegate;
 }
 
 namespace ash {
@@ -45,9 +44,7 @@ ASH_EXPORT extern const char kFullscreenMagnifierToggleAccelNotificationId[];
 class ASH_EXPORT AcceleratorController : public ui::AcceleratorTarget,
                                          public mojom::AcceleratorController {
  public:
-  // TODO(jamescook): Remove |manager_delegate|. https://crbug.com/842365
-  explicit AcceleratorController(
-      ui::AcceleratorManagerDelegate* manager_delegate);
+  AcceleratorController();
   ~AcceleratorController() override;
 
   // A list of possible ways in which an accelerator should be restricted before

ash/shell.cc

@@ -1104,7 +1104,7 @@ void Shell::Init(
   cursor_manager_->SetDisplay(
       display::Screen::GetScreen()->GetPrimaryDisplay());
 
-  accelerator_controller_ = std::make_unique<AcceleratorController>(nullptr);
+  accelerator_controller_ = std::make_unique<AcceleratorController>();
   voice_interaction_controller_ =
       std::make_unique<VoiceInteractionController>();
 

ui/base/BUILD.gn

@@ -345,7 +345,6 @@ jumbo_component("base") {
       "accelerators/accelerator_history.h",
       "accelerators/accelerator_manager.cc",
       "accelerators/accelerator_manager.h",
-      "accelerators/accelerator_manager_delegate.h",
       "base_window.cc",
       "base_window.h",
       "clipboard/clipboard.cc",

ui/base/accelerators/accelerator_manager.cc

@@ -8,15 +8,12 @@
 
 #include "base/logging.h"
 #include "base/stl_util.h"
-#include "ui/base/accelerators/accelerator_manager_delegate.h"
 
 namespace ui {
 
-AcceleratorManager::AcceleratorManager(AcceleratorManagerDelegate* delegate)
-    : delegate_(delegate) {}
+AcceleratorManager::AcceleratorManager() = default;
 
-AcceleratorManager::~AcceleratorManager() {
-}
+AcceleratorManager::~AcceleratorManager() = default;
 
 void AcceleratorManager::Register(
     const std::vector<ui::Accelerator>& accelerators,
@@ -24,14 +21,10 @@ void AcceleratorManager::Register(
     AcceleratorTarget* target) {
   DCHECK(target);
 
-  // Accelerators which haven't already been registered with any target.
-  std::vector<ui::Accelerator> new_accelerators;
-
   for (const ui::Accelerator& accelerator : accelerators) {
     AcceleratorTargetList& targets = accelerators_[accelerator].second;
     DCHECK(!base::ContainsValue(targets, target))
         << "Registering the same target multiple times";
-    const bool is_first_target_for_accelerator = targets.empty();
 
     // All priority accelerators go to the front of the line.
     if (priority == kHighPriority) {
@@ -50,11 +43,7 @@ void AcceleratorManager::Register(
       else
         targets.insert(++targets.begin(), target);
     }
-    if (is_first_target_for_accelerator)
-      new_accelerators.push_back(accelerator);
   }
-  if (delegate_ && !new_accelerators.empty())
-    delegate_->OnAcceleratorsRegistered(new_accelerators);
 }
 
 void AcceleratorManager::Unregister(const Accelerator& accelerator,
@@ -139,10 +128,7 @@ void AcceleratorManager::UnregisterImpl(AcceleratorMap::iterator map_iter,
   targets->remove(target);
   if (!targets->empty())
     return;
-  const ui::Accelerator accelerator = map_iter->first;
   accelerators_.erase(map_iter);
-  if (delegate_)
-    delegate_->OnAcceleratorUnregistered(accelerator);
 }
 
 }  // namespace ui

ui/base/accelerators/accelerator_manager.h

@@ -17,8 +17,6 @@
 
 namespace ui {
 
-class AcceleratorManagerDelegate;
-
 // AcceleratorManger handles processing of accelerators. A delegate may be
 // supplied which is notified as unique accelerators are added and removed.
 class UI_BASE_EXPORT AcceleratorManager {
@@ -28,7 +26,7 @@ class UI_BASE_EXPORT AcceleratorManager {
     kHighPriority,
   };
 
-  explicit AcceleratorManager(AcceleratorManagerDelegate* = nullptr);
+  AcceleratorManager();
   ~AcceleratorManager();
 
   // Register keyboard accelerators for the specified target. If multiple
@@ -93,7 +91,6 @@ class UI_BASE_EXPORT AcceleratorManager {
   void UnregisterImpl(AcceleratorMap::iterator map_iter,
                       AcceleratorTarget* target);
 
-  AcceleratorManagerDelegate* delegate_;
   AcceleratorMap accelerators_;
 
   DISALLOW_COPY_AND_ASSIGN(AcceleratorManager);

ui/base/accelerators/accelerator_manager_delegate.h

-// Copyright 2016 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.
-
-#ifndef UI_BASE_ACCELERATORS_ACCELERATOR_MANAGER_DELEGATE_H_
-#define UI_BASE_ACCELERATORS_ACCELERATOR_MANAGER_DELEGATE_H_
-
-#include "ui/base/ui_base_export.h"
-
-namespace ui {
-
-class Accelerator;
-
-// TODO: this should no longer be necessary, remove. https://crbug.com/842365
-class UI_BASE_EXPORT AcceleratorManagerDelegate {
- public:
-  // Called when new accelerators are registered. This is only called with
-  // newly registered accelerators. For example, if Register() is
-  // called with A and B, then OnAcceleratorsRegistered() is called with A and
-  // B. If Register() is subsequently called with A and C, then
-  // OnAcceleratorsRegistered() is only called with C, as A was already
-  // registered.
-  virtual void OnAcceleratorsRegistered(
-      const std::vector<ui::Accelerator>& accelerators) = 0;
-
-  // Called when there no more targets are registered for |accelerator|.
-  virtual void OnAcceleratorUnregistered(const Accelerator& accelerator) = 0;
-
- protected:
-  virtual ~AcceleratorManagerDelegate() {}
-};
-
-}  // namespace ui
-
-#endif  // UI_BASE_ACCELERATORS_ACCELERATOR_MANAGER_DELEGATE_H_

ui/base/accelerators/accelerator_manager_unittest.cc

@@ -4,19 +4,14 @@
 
 #include "ui/base/accelerators/accelerator_manager.h"
 
-#include <map>
-
-#include "base/compiler_specific.h"
 #include "base/macros.h"
 #include "testing/gtest/include/gtest/gtest.h"
-#include "ui/base/accelerators/accelerator_manager_delegate.h"
 #include "ui/base/accelerators/test_accelerator_target.h"
 #include "ui/events/event_constants.h"
 #include "ui/events/keycodes/keyboard_codes.h"
 
 namespace ui {
 namespace test {
-
 namespace {
 
 Accelerator GetAccelerator(KeyboardCode code, int mask) {
@@ -38,81 +33,24 @@ int BuildAcceleratorModifier(int id) {
   return result;
 }
 
-// AcceleratorManagerDelegate implementation that records calls to interface
-// using the following format.
-// . OnAcceleratorsRegistered() -> A list of "'Register ' + <id>" separated by
-// whitespaces.
-// . OnAcceleratorUnregistered() -> 'Unregister' + id
-// where the id is specified using SetIdForAccelerator().
-class TestAcceleratorManagerDelegate : public AcceleratorManagerDelegate {
- public:
-  TestAcceleratorManagerDelegate() {}
-  ~TestAcceleratorManagerDelegate() override {}
-
-  void SetIdForAccelerator(const Accelerator& accelerator,
-                           const std::string& id) {
-    accelerator_to_id_[accelerator] = id;
-  }
-
-  std::string GetAndClearCommands() {
-    std::string commands;
-    std::swap(commands, commands_);
-    return commands;
-  }
-
-  // AcceleratorManagerDelegate:
-  void OnAcceleratorsRegistered(
-      const std::vector<Accelerator>& accelerators) override {
-    for (const Accelerator& accelerator : accelerators) {
-      if (!commands_.empty())
-        commands_ += " ";
-      commands_ += "Register " + accelerator_to_id_[accelerator];
-    }
-  }
-  void OnAcceleratorUnregistered(const Accelerator& accelerator) override {
-    if (!commands_.empty())
-      commands_ += " ";
-    commands_ += "Unregister " + accelerator_to_id_[accelerator];
-  }
-
- private:
-  std::map<Accelerator, std::string> accelerator_to_id_;
-  std::string commands_;
-
-  DISALLOW_COPY_AND_ASSIGN(TestAcceleratorManagerDelegate);
-};
-
-}  // namespace
-
 class AcceleratorManagerTest : public testing::Test {
  public:
-  AcceleratorManagerTest() : manager_(&delegate_) {}
-  ~AcceleratorManagerTest() override {}
+  AcceleratorManagerTest() = default;
+  ~AcceleratorManagerTest() override = default;
 
  protected:
-  TestAcceleratorManagerDelegate delegate_;
   AcceleratorManager manager_;
 };
 
 TEST_F(AcceleratorManagerTest, Register) {
   TestAcceleratorTarget target;
   const Accelerator accelerator_a(VKEY_A, EF_NONE);
-  delegate_.SetIdForAccelerator(accelerator_a, "a");
-
   const Accelerator accelerator_b(VKEY_B, EF_NONE);
-  delegate_.SetIdForAccelerator(accelerator_b, "b");
-
   const Accelerator accelerator_c(VKEY_C, EF_NONE);
-  delegate_.SetIdForAccelerator(accelerator_c, "c");
-
   const Accelerator accelerator_d(VKEY_D, EF_NONE);
-  delegate_.SetIdForAccelerator(accelerator_d, "d");
-
   manager_.Register(
       {accelerator_a, accelerator_b, accelerator_c, accelerator_d},
       AcceleratorManager::kNormalPriority, &target);
-  EXPECT_EQ("Register a Register b Register c Register d",
-            delegate_.GetAndClearCommands());
 
   // The registered accelerators are processed.
   EXPECT_TRUE(manager_.Process(accelerator_a));
@@ -124,16 +62,12 @@ TEST_F(AcceleratorManagerTest, Register) {
 
 TEST_F(AcceleratorManagerTest, RegisterMultipleTarget) {
   const Accelerator accelerator_a(VKEY_A, EF_NONE);
-  delegate_.SetIdForAccelerator(accelerator_a, "a");
   TestAcceleratorTarget target1;
   manager_.Register({accelerator_a}, AcceleratorManager::kNormalPriority,
                     &target1);
-  EXPECT_EQ("Register a", delegate_.GetAndClearCommands());
   TestAcceleratorTarget target2;
   manager_.Register({accelerator_a}, AcceleratorManager::kNormalPriority,
                     &target2);
-  // Registering the same command shouldn't notify the delegate.
-  EXPECT_TRUE(delegate_.GetAndClearCommands().empty());
 
   // If multiple targets are registered with the same accelerator, the target
   // registered later processes the accelerator.
@@ -144,51 +78,37 @@ TEST_F(AcceleratorManagerTest, RegisterMultipleTarget) {
 
 TEST_F(AcceleratorManagerTest, Unregister) {
   const Accelerator accelerator_a(VKEY_A, EF_NONE);
-  delegate_.SetIdForAccelerator(accelerator_a, "a");
   TestAcceleratorTarget target;
   const Accelerator accelerator_b(VKEY_B, EF_NONE);
-  delegate_.SetIdForAccelerator(accelerator_b, "b");
   manager_.Register({accelerator_a, accelerator_b},
                     AcceleratorManager::kNormalPriority, &target);
-  EXPECT_EQ("Register a Register b", delegate_.GetAndClearCommands());
 
   // Unregistering a different accelerator does not affect the other
   // accelerator.
   manager_.Unregister(accelerator_b, &target);
-  EXPECT_EQ("Unregister b", delegate_.GetAndClearCommands());
   EXPECT_TRUE(manager_.Process(accelerator_a));
   EXPECT_EQ(1, target.accelerator_count());
 
   // The unregistered accelerator is no longer processed.
   target.ResetCounts();
   manager_.Unregister(accelerator_a, &target);
-  EXPECT_EQ("Unregister a", delegate_.GetAndClearCommands());
   EXPECT_FALSE(manager_.Process(accelerator_a));
   EXPECT_EQ(0, target.accelerator_count());
 }
 
 TEST_F(AcceleratorManagerTest, UnregisterAll) {
   const Accelerator accelerator_a(VKEY_A, EF_NONE);
-  delegate_.SetIdForAccelerator(accelerator_a, "a");
   TestAcceleratorTarget target1;
   const Accelerator accelerator_b(VKEY_B, EF_NONE);
-  delegate_.SetIdForAccelerator(accelerator_b, "b");
   manager_.Register({accelerator_a, accelerator_b},
                     AcceleratorManager::kNormalPriority, &target1);
+
   const Accelerator accelerator_c(VKEY_C, EF_NONE);
-  delegate_.SetIdForAccelerator(accelerator_c, "c");
   TestAcceleratorTarget target2;
   manager_.Register({accelerator_c}, AcceleratorManager::kNormalPriority,
                     &target2);
-  EXPECT_EQ("Register a Register b Register c",
-            delegate_.GetAndClearCommands());
+
   manager_.UnregisterAll(&target1);
-  {
-    const std::string commands = delegate_.GetAndClearCommands();
-    // Ordering is not guaranteed.
-    EXPECT_TRUE(commands == "Unregister a Unregister b" ||
-                commands == "Unregister b Unregister a");
-  }
 
   // All the accelerators registered for |target1| are no longer processed.
   EXPECT_FALSE(manager_.Process(accelerator_a));
@@ -242,21 +162,6 @@ TEST_F(AcceleratorManagerTest, Process) {
   }
 }
 
-// Verifies delegate is notifed correctly when unregistering and registering
-// with the same accelerator.
-TEST_F(AcceleratorManagerTest, Reregister) {
-  const Accelerator accelerator_a(VKEY_A, EF_NONE);
-  TestAcceleratorTarget target;
-  delegate_.SetIdForAccelerator(accelerator_a, "a");
-  manager_.Register({accelerator_a}, AcceleratorManager::kNormalPriority,
-                    &target);
-  EXPECT_EQ("Register a", delegate_.GetAndClearCommands());
-  manager_.UnregisterAll(&target);
-  EXPECT_EQ("Unregister a", delegate_.GetAndClearCommands());
-  manager_.Register({accelerator_a}, AcceleratorManager::kNormalPriority,
-                    &target);
-  EXPECT_EQ("Register a", delegate_.GetAndClearCommands());
-}
-
+}  // namespace
 }  // namespace test
 }  // namespace ui