WindowManagerClient::AddAccelerator() should take an array

services/ui/ws/event_dispatcher_unittest.cc

 // Copyright 2015 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 "services/ui/ws/event_dispatcher.h"
 
 #include <stddef.h>
 #include <stdint.h>
 
 #include <queue>
 
 #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"
 #include "services/ui/ws/server_window_compositor_frame_sink_manager_test_api.h"
 #include "services/ui/ws/test_server_window_delegate.h"
 #include "services/ui/ws/test_utils.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "ui/events/event.h"
 
 namespace ui {
@@ skipping 353 matching lines @@
 }
 
 TEST_F(EventDispatcherTest, AcceleratorBasic) {
   ClearSetup();
   TestEventDispatcherDelegate event_dispatcher_delegate(nullptr);
   EventDispatcher dispatcher(&event_dispatcher_delegate);
 
   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) {
   TestEventDispatcherDelegate* event_dispatcher_delegate =
       test_event_dispatcher_delegate();
   EventDispatcher* dispatcher = event_dispatcher();
 
   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);
   EXPECT_EQ(accelerator_1,
             event_dispatcher_delegate->GetAndClearLastAccelerator());
 
   // EF_NUM_LOCK_ON should be ignored since CreateKeyMatcher defaults to
   // ignoring.
   key = ui::KeyEvent(ui::ET_KEY_PRESSED, ui::VKEY_W,
                      ui::EF_CONTROL_DOWN | ui::EF_NUM_LOCK_ON);
   dispatcher->ProcessEvent(key, EventDispatcher::AcceleratorMatchPhase::ANY);
   EXPECT_EQ(accelerator_1,
             event_dispatcher_delegate->GetAndClearLastAccelerator());
 
   key = ui::KeyEvent(ui::ET_KEY_PRESSED, ui::VKEY_W, ui::EF_NONE);
   dispatcher->ProcessEvent(key, EventDispatcher::AcceleratorMatchPhase::ANY);
   EXPECT_EQ(0u, event_dispatcher_delegate->GetAndClearLastAccelerator());
 
   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());
 
   dispatcher->RemoveAccelerator(accelerator_2);
   dispatcher->ProcessEvent(key, EventDispatcher::AcceleratorMatchPhase::ANY);
   EXPECT_EQ(0u, event_dispatcher_delegate->GetAndClearLastAccelerator());
 }
 
 // Tests that a post-target accelerator is not triggered by ProcessEvent.
 TEST_F(EventDispatcherTest, PostTargetAccelerator) {
   TestEventDispatcherDelegate* event_dispatcher_delegate =
       test_event_dispatcher_delegate();
   EventDispatcher* dispatcher = event_dispatcher();
 
   mojom::EventMatcherPtr matcher = ui::CreateKeyMatcher(
       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.
   dispatcher->ProcessEvent(key, EventDispatcher::AcceleratorMatchPhase::ANY);
   EXPECT_EQ(accelerator_1,
             event_dispatcher_delegate->GetAndClearLastAccelerator());
   std::unique_ptr<DispatchedEventDetails> details =
       event_dispatcher_delegate->GetAndAdvanceDispatchedEventDetails();
   EXPECT_FALSE(details);
 
@@ skipping 24 matching lines @@
 TEST_F(EventDispatcherTest, ProcessPost) {
   TestEventDispatcherDelegate* event_dispatcher_delegate =
       test_event_dispatcher_delegate();
   EventDispatcher* dispatcher = event_dispatcher();
 
   uint32_t pre_id = 1;
   {
     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;
   {
     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.
   std::unique_ptr<ServerWindow> child = CreateChildWindow(WindowId(1, 3));
   event_dispatcher_delegate->SetFocusedWindowFromEventDispatcher(child.get());
 
   // Dispatch for ANY, which should trigger PRE and not call
   // DispatchInputEventToWindow().
   ui::KeyEvent key(ui::ET_KEY_PRESSED, ui::VKEY_W, ui::EF_CONTROL_DOWN);
   dispatcher->ProcessEvent(key, EventDispatcher::AcceleratorMatchPhase::ANY);
@@ skipping 1173 matching lines @@
   // The delegate can decide if it really wants to forward the event or not.
   EXPECT_EQ(child.get(),
             test_event_dispatcher_delegate()->lost_capture_window());
   EXPECT_EQ(child.get(), event_dispatcher()->capture_window());
   EXPECT_EQ(kClientAreaId, event_dispatcher()->capture_window_client_id());
 }
 
 }  // namespace test
 }  // namespace ws
 }  // namespace ui