WindowManagerClient::AddAccelerator() should take an array

ash/mus/accelerators/accelerator_controller_registrar.cc

 // 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.
 
 #include "ash/mus/accelerators/accelerator_controller_registrar.h"
 
 #include <limits>
 
 #include "ash/common/accelerators/accelerator_controller.h"
 #include "ash/common/accelerators/accelerator_router.h"
@@ skipping 107 matching lines @@
   event_matcher->accelerator_phase = ui::mojom::AcceleratorPhase::PRE_TARGET;
   DCHECK(accelerator.type() == ui::ET_KEY_PRESSED ||
          accelerator.type() == ui::ET_KEY_RELEASED);
   event_matcher->type_matcher->type = accelerator.type() == ui::ET_KEY_PRESSED
                                           ? ui::mojom::EventType::KEY_PRESSED
                                           : ui::mojom::EventType::KEY_RELEASED;
 
   ui::mojom::EventMatcherPtr post_event_matcher = event_matcher.Clone();
   post_event_matcher->accelerator_phase =
       ui::mojom::AcceleratorPhase::POST_TARGET;
+  ui::mojom::AcceleratorEventPtr accelerator_pre_event(

[mfomitchev, 2016/11/22 22:44:16]

This doesn't make much sense. You are just creating an array containing a single
accelerator and still doing one mojo IPC per accelerator, just wrapping it in an
array.

[mfomitchev, 2016/11/22 23:07:13]

Nvm, I guess you have to do this b/c you removed the mojom method for
registering a single accelerator. I think it's reasonable. Just create a helper
method in the anonymous namespace (in this cc file) which creates a 
mojo::Array<ui::mojom::AcceleratorEventPtr> out of id and event matcher, so that
you don't duplicate the code. And please use consistent names.

[thanhph, 2016/11/24 16:10:12]

Thanks! I created a wrapper around single pair of (id, event_matcher). Let me
know how it goes. Should I copy this helper to event_dispatcher_unittest.cc
also?

+      ui::mojom::AcceleratorEvent::New());
+  accelerator_pre_event->id = ComputeAcceleratorId(id_namespace_, ids.pre_id);
+  accelerator_pre_event->event_matcher = std::move(event_matcher);
+
+  mojo::Array<ui::mojom::AcceleratorEventPtr> pre_multi_accelerators;
+  pre_multi_accelerators.push_back(std::move(accelerator_pre_event));
 
   window_manager_->window_manager_client()->AddAccelerator(
-      ComputeAcceleratorId(id_namespace_, ids.pre_id), std::move(event_matcher),
+      std::move(pre_multi_accelerators),
       base::Bind(OnAcceleratorAdded, accelerator));
+
+  ui::mojom::AcceleratorEventPtr accelerator_post_event(
+      ui::mojom::AcceleratorEvent::New());
+  accelerator_post_event->id = ComputeAcceleratorId(id_namespace_, ids.post_id);
+  accelerator_post_event->event_matcher = std::move(post_event_matcher);
+
+  mojo::Array<ui::mojom::AcceleratorEventPtr> post_multi_accelerators;
+  post_multi_accelerators.push_back(std::move(accelerator_post_event));
+
   window_manager_->window_manager_client()->AddAccelerator(
-      ComputeAcceleratorId(id_namespace_, ids.post_id),
-      std::move(post_event_matcher),
+      std::move(post_multi_accelerators),
       base::Bind(OnAcceleratorAdded, accelerator));
 }
 
 void AcceleratorControllerRegistrar::OnAcceleratorUnregistered(
     const ui::Accelerator& accelerator) {
   auto iter = accelerator_to_ids_.find(accelerator);
   DCHECK(iter != accelerator_to_ids_.end());
   Ids ids = iter->second;
   accelerator_to_ids_.erase(iter);
   ids_.erase(ids.pre_id);
@@ skipping 18 matching lines @@
   // Common case is we never wrap once, so this is typically cheap. Additionally
   // we expect there not to be too many accelerators.
   while (ids_.count(next_id_) > 0)
     ++next_id_;
   ids_.insert(next_id_);
   return next_id_++;
 }
 
 }  // namespace mus
 }  // namespace ash