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)));
+
+  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) {
   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));
+  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);
   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));
+  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());
 
   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));
+  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.
   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));
+
+    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;
   {
     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.
   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