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..06090923d0bbae1ef4c21ca3685eb339a5d43054 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,114 @@ 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)));
+
+  std::vector<ui::mojom::AcceleratorTransportPtr> accelerators_1;

[mfomitchev, 2016/11/29 17:37:27]

These three lines could be combined into one. This applies to all other uses of
AddAcceleratorHelper below.

[thanhph, 2016/11/29 19:06:41]

Done, really nice. I changed other files as well.

+  accelerators_1 =
+      ash::mus::AddAcceleratorHelper(accelerator_1, std::move(matcher));
+  EXPECT_TRUE(dispatcher.AddAccelerators(std::move(accelerators_1)));
 
   uint32_t accelerator_2 = 2;
   matcher = ui::CreateKeyMatcher(ui::mojom::KeyboardCode::N,
                                  ui::mojom::kEventFlagNone);
-  EXPECT_TRUE(dispatcher.AddAccelerator(accelerator_2, std::move(matcher)));
+
+  std::vector<ui::mojom::AcceleratorTransportPtr> accelerators_2;
+  accelerators_2 =
+      ash::mus::AddAcceleratorHelper(accelerator_2, std::move(matcher));
+  EXPECT_TRUE(dispatcher.AddAccelerators(std::move(accelerators_2)));
 
   // 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)));
+  std::vector<ui::mojom::AcceleratorTransportPtr> accelerators_3;
+  accelerators_3 =
+      ash::mus::AddAcceleratorHelper(accelerator_2, std::move(matcher));
+  EXPECT_FALSE(dispatcher.AddAccelerators(std::move(accelerators_3)));
 
   // 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)));
+  std::vector<ui::mojom::AcceleratorTransportPtr> accelerators_4;
+  accelerators_4 =
+      ash::mus::AddAcceleratorHelper(accelerator_2, std::move(matcher));
+  EXPECT_TRUE(dispatcher.AddAccelerators(std::move(accelerators_4)));
 
   // 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)));
+  std::vector<ui::mojom::AcceleratorTransportPtr> accelerators_5;
+  accelerators_5 =
+      ash::mus::AddAcceleratorHelper(accelerator_3, std::move(matcher));
+  EXPECT_FALSE(dispatcher.AddAccelerators(std::move(accelerators_5)));
 
   matcher = ui::CreateKeyMatcher(ui::mojom::KeyboardCode::T,
                                  ui::mojom::kEventFlagControlDown);
-  EXPECT_TRUE(dispatcher.AddAccelerator(accelerator_3, std::move(matcher)));
+  std::vector<ui::mojom::AcceleratorTransportPtr> accelerators_6;
+  accelerators_6 =
+      ash::mus::AddAcceleratorHelper(accelerator_3, std::move(matcher));
+  EXPECT_TRUE(dispatcher.AddAccelerators(std::move(accelerators_6)));
+}
+
+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);
+
+  std::vector<ui::mojom::AcceleratorTransportPtr> accelerators_1;

[mfomitchev, 2016/11/29 18:23:15]

We shouldn't create a vector just to take it's last element. Just add the
Accelerators one by one to the vector. Feel free to create a helper method to
avoid code duplication.

[thanhph, 2016/11/29 19:06:41]

I use one-liner to add the back of the newly created vector to vector
accelerators as below. Let me know if it's ok. I could also add the helper for
constructing single ui::mojom::AcceleratorTransportPtr as I did in previous cl.

accelerators.push_back(std::move(ui::AddAcceleratorHelper(accelerator_1,
std::move(matcher)).back()));

[mfomitchev, 2016/11/29 19:21:19]

I think adding a helper for constructing a single
ui::mojom::AcceleratorTransportPtr would be better.

[thanhph, 2016/11/29 20:36:45]

Done, this is cleaner. Thanks!

+  accelerators_1 =
+      ash::mus::AddAcceleratorHelper(accelerator_1, std::move(matcher));
+  accelerators.push_back(std::move(accelerators_1.back()));
+
+  uint32_t accelerator_2 = 2;
+  matcher = ui::CreateKeyMatcher(ui::mojom::KeyboardCode::N,
+                                 ui::mojom::kEventFlagNone);
+
+  std::vector<ui::mojom::AcceleratorTransportPtr> accelerators_2;
+  accelerators_2 =
+      ash::mus::AddAcceleratorHelper(accelerator_2, std::move(matcher));
+  accelerators.push_back(std::move(accelerators_2.back()));
+
+  uint32_t accelerator_3 = 3;
+  matcher = ui::CreateKeyMatcher(ui::mojom::KeyboardCode::T,
+                                 ui::mojom::kEventFlagNone);
+  std::vector<ui::mojom::AcceleratorTransportPtr> accelerators_3;
+  accelerators_3 =
+      ash::mus::AddAcceleratorHelper(accelerator_3, std::move(matcher));
+  accelerators.push_back(std::move(accelerators_3.back()));
+
+  // Adding all accelerators with the different ids should pass.
+  EXPECT_TRUE(dispatcher.AddAccelerators(std::move(accelerators)));
+
+  accelerators.clear();
+
+  matcher = ui::CreateKeyMatcher(ui::mojom::KeyboardCode::W,
+                                 ui::mojom::kEventFlagControlDown);
+  accelerators_1 =
+      ash::mus::AddAcceleratorHelper(accelerator_1, std::move(matcher));
+  accelerators.push_back(std::move(accelerators_1.back()));
+
+  matcher = ui::CreateKeyMatcher(ui::mojom::KeyboardCode::W,
+                                 ui::mojom::kEventFlagControlDown);
+  accelerators_2 =
+      ash::mus::AddAcceleratorHelper(accelerator_2, std::move(matcher));
+  accelerators.push_back(std::move(accelerators_2.back()));
+
+  matcher = ui::CreateKeyMatcher(ui::mojom::KeyboardCode::T,
+                                 ui::mojom::kEventFlagNone);
+  accelerators_3 =
+      ash::mus::AddAcceleratorHelper(accelerator_1, std::move(matcher));
+  accelerators.push_back(std::move(accelerators_3.back()));
+
+  // Adding accelerators with the duplicated id should fail.
+  EXPECT_FALSE(dispatcher.AddAccelerators(std::move(accelerators)));

[mfomitchev, 2016/11/29 18:23:15]

We should also test the case where some ids/matchers are duped and others aren't
and make sure FALSE is returned and also non-duped accelerators are added.

[thanhph, 2016/11/29 19:06:41]

accelerators_1 and accelerators_3 already have their ids duplicated here (whose
ids equal accelerator_1).

[mfomitchev, 2016/11/29 19:21:19]

All three ids are dups here, right?
We need a test where some are dups, and other aren't, and the test should also
confirm that non-dups are successfully added.

[thanhph, 2016/11/29 20:36:45]

Done, thanks!

 }
 
 TEST_F(EventDispatcherTest, EventMatching) {
@@ -423,7 +503,10 @@ 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));
+  std::vector<ui::mojom::AcceleratorTransportPtr> accelerators_1;
+  accelerators_1 =
+      ash::mus::AddAcceleratorHelper(accelerator_1, std::move(matcher));
+  dispatcher->AddAccelerators(std::move(accelerators_1));
 
   ui::KeyEvent key(ui::ET_KEY_PRESSED, ui::VKEY_W, ui::EF_CONTROL_DOWN);
   dispatcher->ProcessEvent(key, EventDispatcher::AcceleratorMatchPhase::ANY);
@@ -445,7 +528,10 @@ 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));
+  std::vector<ui::mojom::AcceleratorTransportPtr> accelerators_2;
+  accelerators_2 =
+      ash::mus::AddAcceleratorHelper(accelerator_2, std::move(matcher));
+  dispatcher->AddAccelerators(std::move(accelerators_2));
   dispatcher->ProcessEvent(key, EventDispatcher::AcceleratorMatchPhase::ANY);
   EXPECT_EQ(accelerator_2,
             event_dispatcher_delegate->GetAndClearLastAccelerator());
@@ -465,7 +551,11 @@ 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));
+  std::vector<ui::mojom::AcceleratorTransportPtr> accelerators_1;
+  accelerators_1 =
+      ash::mus::AddAcceleratorHelper(accelerator_1, std::move(matcher));
+
+  dispatcher->AddAccelerators(std::move(accelerators_1));
 
   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 +600,11 @@ 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));
+
+    std::vector<ui::mojom::AcceleratorTransportPtr> accelerators_1;
+    accelerators_1 = ash::mus::AddAcceleratorHelper(pre_id, std::move(matcher));
+
+    dispatcher->AddAccelerators(std::move(accelerators_1));
   }
 
   uint32_t post_id = 2;
@@ -518,7 +612,12 @@ 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));
+
+    std::vector<ui::mojom::AcceleratorTransportPtr> accelerators_2;
+    accelerators_2 =
+        ash::mus::AddAcceleratorHelper(post_id, std::move(matcher));
+
+    dispatcher->AddAccelerators(std::move(accelerators_2));
   }
 
   // Set focused window for EventDispatcher dispatches key events.