WindowManagerClient::AddAccelerator() should take an array

services/ui/ws/event_dispatcher_unittest.cc

Index: services/ui/ws/event_dispatcher_unittest.cc
diff --git a/services/ui/ws/event_dispatcher_unittest.cc b/services/ui/ws/event_dispatcher_unittest.cc
index e06b70ef6fc6d71a862e065b0c894fc5eeec9635..c8fa762a51613f450257121f7b32cffca688ff04 100644
--- a/services/ui/ws/event_dispatcher_unittest.cc
+++ b/services/ui/ws/event_dispatcher_unittest.cc
@@ -11,7 +11,7 @@
 
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
-#include "services/ui/common/event_matcher_util.h"
+#include "services/ui/common/accelerator_util.h"
 #include "services/ui/ws/accelerator.h"
 #include "services/ui/ws/event_dispatcher_delegate.h"
 #include "services/ui/ws/server_window.h"
@@ -385,34 +385,102 @@ TEST_F(EventDispatcherTest, AcceleratorBasic) {
   uint32_t accelerator_1 = 1;
   mojom::EventMatcherPtr matcher = ui::CreateKeyMatcher(
       ui::mojom::KeyboardCode::W, ui::mojom::kEventFlagControlDown);
-  EXPECT_TRUE(dispatcher.AddAccelerator(accelerator_1, std::move(matcher)));
+  EXPECT_TRUE(dispatcher.AddAccelerators(
+      CreateAcceleratorVector(accelerator_1, std::move(matcher))));
 
   uint32_t accelerator_2 = 2;
   matcher = ui::CreateKeyMatcher(ui::mojom::KeyboardCode::N,
                                  ui::mojom::kEventFlagNone);
-  EXPECT_TRUE(dispatcher.AddAccelerator(accelerator_2, std::move(matcher)));
+  EXPECT_TRUE(dispatcher.AddAccelerators(
+      CreateAcceleratorVector(accelerator_2, std::move(matcher))));
 
   // Attempting to add a new accelerator with the same id should fail.
   matcher = ui::CreateKeyMatcher(ui::mojom::KeyboardCode::T,
                                  ui::mojom::kEventFlagNone);
-  EXPECT_FALSE(dispatcher.AddAccelerator(accelerator_2, std::move(matcher)));
+  EXPECT_FALSE(dispatcher.AddAccelerators(
+      CreateAcceleratorVector(accelerator_2, std::move(matcher))));
 
   // Adding the accelerator with the same id should succeed once the existing
   // accelerator is removed.
   dispatcher.RemoveAccelerator(accelerator_2);
   matcher = ui::CreateKeyMatcher(ui::mojom::KeyboardCode::T,
                                  ui::mojom::kEventFlagNone);
-  EXPECT_TRUE(dispatcher.AddAccelerator(accelerator_2, std::move(matcher)));
+  EXPECT_TRUE(dispatcher.AddAccelerators(
+      CreateAcceleratorVector(accelerator_2, std::move(matcher))));
 
   // Attempting to add an accelerator with the same matcher should fail.
   uint32_t accelerator_3 = 3;
   matcher = ui::CreateKeyMatcher(ui::mojom::KeyboardCode::T,
                                  ui::mojom::kEventFlagNone);
-  EXPECT_FALSE(dispatcher.AddAccelerator(accelerator_3, std::move(matcher)));
+  EXPECT_FALSE(dispatcher.AddAccelerators(
+      CreateAcceleratorVector(accelerator_3, std::move(matcher))));
 
   matcher = ui::CreateKeyMatcher(ui::mojom::KeyboardCode::T,
                                  ui::mojom::kEventFlagControlDown);
-  EXPECT_TRUE(dispatcher.AddAccelerator(accelerator_3, std::move(matcher)));
+  EXPECT_TRUE(dispatcher.AddAccelerators(
+      CreateAcceleratorVector(accelerator_3, std::move(matcher))));
+}
+
+TEST_F(EventDispatcherTest, AddAccelerators) {
+  ClearSetup();
+  TestEventDispatcherDelegate event_dispatcher_delegate(nullptr);
+  EventDispatcher dispatcher(&event_dispatcher_delegate);
+  std::vector<ui::mojom::AcceleratorTransportPtr> accelerators;
+
+  uint32_t accelerator_1 = 1;
+  mojom::EventMatcherPtr matcher = ui::CreateKeyMatcher(
+      ui::mojom::KeyboardCode::W, ui::mojom::kEventFlagControlDown);
+  accelerators.push_back(
+      CreateAcceleratorTransport(accelerator_1, std::move(matcher)));
+
+  uint32_t accelerator_2 = 2;
+  matcher = ui::CreateKeyMatcher(ui::mojom::KeyboardCode::N,
+                                 ui::mojom::kEventFlagNone);
+  accelerators.push_back(
+      CreateAcceleratorTransport(accelerator_2, std::move(matcher)));
+
+  uint32_t accelerator_3 = 3;
+  matcher = ui::CreateKeyMatcher(ui::mojom::KeyboardCode::T,
+                                 ui::mojom::kEventFlagNone);
+  accelerators.push_back(
+      CreateAcceleratorTransport(accelerator_3, std::move(matcher)));
+
+  // Adding unique accelerators should pass.
+  EXPECT_TRUE(dispatcher.AddAccelerators(std::move(accelerators)));
+
+  accelerators.clear();
+
+  uint32_t accelerator_4 = 4;
+
+  matcher = ui::CreateKeyMatcher(ui::mojom::KeyboardCode::W,

[mfomitchev, 2016/11/29 23:27:41]

These are all dupes - they have the same matcher as accelerator_1

[thanhph, 2016/11/30 01:31:20]

Nice! The AcceleratorBasic test belongs to other owner so I leave it as it's for
now.

[mfomitchev, 2016/11/30 02:33:04]

I am not sure what you mean? This is AddAccelerators test case which you have
added.

[thanhph, 2016/11/30 14:24:51]

Yes I added the test case AddAccelerators. I meant the test case above
AcceleratorBasic that can be refactored like you suggest here but I'll leave it
as it's since it belongs to other owner.

[mfomitchev, 2016/11/30 17:58:40]

I am not suggesting a refactor. I am saying that
a. accelerators 4, 5, and 6 are all duplicates of 1, 2, and 3, because their
matchers are the same. Your comment below says "Adding accelerator_6 with the
same id accelerator_5 should fail.", but in fact all of the additions should
fail.

b. We still need a test that attempts to add a few unique and one duplicate
accelerator. The test should confirm that:
 - false is returned
 - the unique accelerators were added
 - (if the duped accelerator had a different id but the same matcher) the
accelerator id associated with the matcher didn't change.

[thanhph, 2016/11/30 23:35:13]

Done, thanks! I added the tests as you mentioned.

[mfomitchev, 2016/12/01 03:58:07]

I only see a test that adds a bunch of unique accelerators successfuly, and then
tests that add a single (duped) accelerator unsuccessfully. We need a test that
attempts to add a few accelerators at once, and some are added successfully,
while others aren't. E.g. two unique and one duped.

[thanhph, 2016/12/01 15:00:54]

I just added this case in the new cl. Thanks!

+                                 ui::mojom::kEventFlagControlDown);
+  accelerators.push_back(
+      CreateAcceleratorTransport(accelerator_4, std::move(matcher)));
+
+  uint32_t accelerator_5 = 5;
+  matcher = ui::CreateKeyMatcher(ui::mojom::KeyboardCode::W,
+                                 ui::mojom::kEventFlagControlDown);
+  accelerators.push_back(
+      CreateAcceleratorTransport(accelerator_5, std::move(matcher)));
+
+  uint32_t accelerator_6 = accelerator_5;
+  matcher = ui::CreateKeyMatcher(ui::mojom::KeyboardCode::T,
+                                 ui::mojom::kEventFlagNone);
+  accelerators.push_back(
+      CreateAcceleratorTransport(accelerator_6, std::move(matcher)));
+
+  // Adding accelerator_6 with the same id accelerator_5 should fail.
+  EXPECT_FALSE(dispatcher.AddAccelerators(std::move(accelerators)));
+
+  accelerators.clear();
+
+  matcher = ui::CreateKeyMatcher(ui::mojom::KeyboardCode::T,
+                                 ui::mojom::kEventFlagNone);
+  accelerators.push_back(
+      CreateAcceleratorTransport(accelerator_4, std::move(matcher)));
+
+  // Check if accelerator_4 is already added to the dispatcher.

[mfomitchev, 2016/11/29 23:27:41]

This isn't really a conclusive way to test that the accelerator was added
successfully. Instead use an approach similar to that in EventMatching with
triggering the accelerator and calling GetAndClearLastAccelerator() on the test
delegate for example.

[thanhph, 2016/11/30 01:31:20]

Done, thanks!!

+  EXPECT_FALSE(dispatcher.AddAccelerators(std::move(accelerators)));
 }
 
 TEST_F(EventDispatcherTest, EventMatching) {
@@ -423,7 +491,8 @@ TEST_F(EventDispatcherTest, EventMatching) {
   mojom::EventMatcherPtr matcher = ui::CreateKeyMatcher(
       ui::mojom::KeyboardCode::W, ui::mojom::kEventFlagControlDown);
   uint32_t accelerator_1 = 1;
-  dispatcher->AddAccelerator(accelerator_1, std::move(matcher));
+  dispatcher->AddAccelerators(
+      CreateAcceleratorVector(accelerator_1, std::move(matcher)));
 
   ui::KeyEvent key(ui::ET_KEY_PRESSED, ui::VKEY_W, ui::EF_CONTROL_DOWN);
   dispatcher->ProcessEvent(key, EventDispatcher::AcceleratorMatchPhase::ANY);
@@ -445,7 +514,8 @@ TEST_F(EventDispatcherTest, EventMatching) {
   uint32_t accelerator_2 = 2;
   matcher = ui::CreateKeyMatcher(ui::mojom::KeyboardCode::W,
                                  ui::mojom::kEventFlagNone);
-  dispatcher->AddAccelerator(accelerator_2, std::move(matcher));
+  dispatcher->AddAccelerators(
+      CreateAcceleratorVector(accelerator_2, std::move(matcher)));
   dispatcher->ProcessEvent(key, EventDispatcher::AcceleratorMatchPhase::ANY);
   EXPECT_EQ(accelerator_2,
             event_dispatcher_delegate->GetAndClearLastAccelerator());
@@ -465,7 +535,8 @@ TEST_F(EventDispatcherTest, PostTargetAccelerator) {
       ui::mojom::KeyboardCode::W, ui::mojom::kEventFlagControlDown);
   matcher->accelerator_phase = ui::mojom::AcceleratorPhase::POST_TARGET;
   uint32_t accelerator_1 = 1;
-  dispatcher->AddAccelerator(accelerator_1, std::move(matcher));
+  dispatcher->AddAccelerators(
+      CreateAcceleratorVector(accelerator_1, std::move(matcher)));
 
   ui::KeyEvent key(ui::ET_KEY_PRESSED, ui::VKEY_W, ui::EF_CONTROL_DOWN);
   // The post-target accelerator should be fired if there is no focused window.
@@ -510,7 +581,8 @@ TEST_F(EventDispatcherTest, ProcessPost) {
     mojom::EventMatcherPtr matcher = ui::CreateKeyMatcher(
         ui::mojom::KeyboardCode::W, ui::mojom::kEventFlagControlDown);
     matcher->accelerator_phase = ui::mojom::AcceleratorPhase::PRE_TARGET;
-    dispatcher->AddAccelerator(pre_id, std::move(matcher));
+    dispatcher->AddAccelerators(
+        CreateAcceleratorVector(pre_id, std::move(matcher)));
   }
 
   uint32_t post_id = 2;
@@ -518,7 +590,8 @@ TEST_F(EventDispatcherTest, ProcessPost) {
     mojom::EventMatcherPtr matcher = ui::CreateKeyMatcher(
         ui::mojom::KeyboardCode::W, ui::mojom::kEventFlagControlDown);
     matcher->accelerator_phase = ui::mojom::AcceleratorPhase::POST_TARGET;
-    dispatcher->AddAccelerator(post_id, std::move(matcher));
+    dispatcher->AddAccelerators(
+        CreateAcceleratorVector(post_id, std::move(matcher)));
   }
 
   // Set focused window for EventDispatcher dispatches key events.