Make mash register initial batch of accelerators in single shot.

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"
 #include "ash/common/wm_shell.h"
 #include "ash/mus/accelerators/accelerator_ids.h"
 #include "ash/mus/bridge/wm_window_mus.h"
 #include "ash/mus/window_manager.h"
 #include "base/logging.h"
 #include "services/ui/common/accelerator_util.h"
 #include "ui/base/accelerators/accelerator_history.h"
 
 namespace ash {
 namespace mus {
 namespace {
 
 // Callback from registering the accelerator.
 void OnAcceleratorAdded(const ui::Accelerator& accelerator, bool added) {
   // All our accelerators should be registered, so we expect |added| to be true.
   DCHECK(added) << "duplicate accelerator key_code=" << accelerator.key_code()

[mfomitchev, 2016/12/22 01:04:50]

This debug info may no longer be correct if called from OnAcceleratorsAdded,
since false is returned as long as at least one accelerator wasn't added. Not
sure if printing the entire array would be super helpful either. Are we doing
any debug prints (DVLOG or something like that) on the window server side? This
is where we know which accelerator failed to be added.

[mfomitchev, 2016/12/22 21:18:55]

Not sure if you missed this comment?

[thanhph, 2016/12/24 18:52:16]

Thanks for the reminder. I put the line 

DVLOG(2) << "Can't add accelerator " <<  iter->get()->id;

in here

https://cs.chromium.org/chromium/src/services/ui/ws/window_tree.cc?cl=GROK&gs...

                 << " type=" << accelerator.type()
                 << " modifiers=" << accelerator.modifiers();
 }
 
+// Callback from registering the accelerators.
+void OnAcceleratorsAdded(const std::vector<ui::Accelerator>& ui_accelerators,
+                         bool added) {
+  for (auto iter = ui_accelerators.begin(); iter != ui_accelerators.end();
+       ++iter)
+    OnAcceleratorAdded(*iter, added);
 }  // namespace
+}
 
 AcceleratorControllerRegistrar::AcceleratorControllerRegistrar(
     WindowManager* window_manager,
     uint16_t id_namespace)
     : window_manager_(window_manager),
       id_namespace_(id_namespace),
       next_id_(0),
       router_(new AcceleratorRouter) {
   window_manager_->AddAcceleratorHandler(id_namespace, this);
 }
@@ skipping 50 matching lines @@
                ? ui::mojom::EventResult::HANDLED
                : ui::mojom::EventResult::UNHANDLED;
   }
   DCHECK_EQ(GetAcceleratorLocalId(id), ids.post_id);
   // NOTE: for post return value doesn't really matter.
   return WmShell::Get()->accelerator_controller()->Process(accelerator)
              ? ui::mojom::EventResult::HANDLED
              : ui::mojom::EventResult::UNHANDLED;
 }
 
+void AcceleratorControllerRegistrar::OnAcceleratorsRegistered(
+    const std::vector<ui::Accelerator>& accelerators) {
+  accelerator_ptrs_.clear();

[mfomitchev, 2016/12/22 01:04:50]

I think this all could be done a bit cleaner:
Don't make |accelerator_ptrs_| into a class member. It can just be a local
variable. Add a function:
AddAcceleratorToVector(const ui::Accelerator& accelerator, const
std::vector<ui::mojom::AcceleratorPtr>& accelerator_vector)

Call this function from OnAcceleratorsAdded repeatedly passing it the vector
(which can be a local variable).

Also call this function from OnAcceleratorRegistered, passing it the empty
vector. Get rid of |add_accelerator_immediately|.

[thanhph, 2016/12/22 17:48:44]

If we get rid of |add_accelerator_immediately|, we can't distinguish function
call between OnAcceleratorRegistered and OnAcceleratorsRegistered. This means we
need to call window_manager_->window_manager_client()->AddAccelerators() each
time the function OnAcceleratorRegistered() ends to register 1 accelerator with
1 IPC. Since OnAcceleratorsRegistered() will call OnAcceleratorRegistered(),
ideally we only need one IPC call for adding entire accelerators in accelerator
vector, which |add_accelerator_immediately| will serve this purpose.

+  for (auto iter = accelerators.begin(); iter != accelerators.end(); ++iter)
+    OnAcceleratorRegistered(*iter, false);
+
+  window_manager_->window_manager_client()->AddAccelerators(
+      std::move(accelerator_ptrs_),
+      base::Bind(OnAcceleratorsAdded, accelerators));
+}
+
+// add_accelerator_immediately default set to true.
 void AcceleratorControllerRegistrar::OnAcceleratorRegistered(
-    const ui::Accelerator& accelerator) {
+    const ui::Accelerator& accelerator,
+    bool add_accelerator_immediately) {
   Ids ids;
   if (!GenerateIds(&ids)) {
     DVLOG(1) << "max number of accelerators registered, dropping request";
     return;
   }
   DCHECK_EQ(0u, accelerator_to_ids_.count(accelerator));
   accelerator_to_ids_[accelerator] = ids;
   DCHECK_EQ(accelerator_to_ids_.size() * 2, ids_.size());
 
   ui::mojom::EventMatcherPtr event_matcher = ui::CreateKeyMatcher(
       static_cast<ui::mojom::KeyboardCode>(accelerator.key_code()),
       accelerator.modifiers());
   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;
 
-  window_manager_->window_manager_client()->AddAccelerators(
-      ui::CreateAcceleratorVector(
-          ComputeAcceleratorId(id_namespace_, ids.pre_id),
-          std::move(event_matcher)),
-      base::Bind(OnAcceleratorAdded, accelerator));
+  if (add_accelerator_immediately) {
+    window_manager_->window_manager_client()->AddAccelerators(

[mfomitchev, 2016/12/22 01:04:50]

Can we create an array vector the accelerators we are adding here and add that?
This way we'd be doing one IPC instead of two.

[thanhph, 2016/12/22 20:36:01]

Done.

+        ui::CreateAcceleratorVector(
+            ComputeAcceleratorId(id_namespace_, ids.pre_id),
+            std::move(event_matcher)),
+        base::Bind(OnAcceleratorAdded, accelerator));
 
-  window_manager_->window_manager_client()->AddAccelerators(
-      ui::CreateAcceleratorVector(
-          ComputeAcceleratorId(id_namespace_, ids.post_id),
-          std::move(post_event_matcher)),
-      base::Bind(OnAcceleratorAdded, accelerator));
+    window_manager_->window_manager_client()->AddAccelerators(
+        ui::CreateAcceleratorVector(
+            ComputeAcceleratorId(id_namespace_, ids.post_id),
+            std::move(post_event_matcher)),
+        base::Bind(OnAcceleratorAdded, accelerator));
+  } else {
+    accelerator_ptrs_.push_back(
+        ui::CreateAccelerator(ComputeAcceleratorId(id_namespace_, ids.pre_id),
+                              std::move(event_matcher)));
+    accelerator_ptrs_.push_back(
+        ui::CreateAccelerator(ComputeAcceleratorId(id_namespace_, ids.post_id),
+                              std::move(post_event_matcher)));
+  }
 }
 
 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);
   ids_.erase(ids.post_id);
   DCHECK_EQ(accelerator_to_ids_.size() * 2, ids_.size());
   window_manager_->window_manager_client()->RemoveAccelerator(
       ComputeAcceleratorId(id_namespace_, ids.pre_id));
   window_manager_->window_manager_client()->RemoveAccelerator(
       ComputeAcceleratorId(id_namespace_, ids.post_id));
 }
 
+void AcceleratorControllerRegistrar::OnAcceleratorsUnregistered(
+    std::vector<ui::Accelerator>& accelerators) {
+  for (auto iter = accelerators.begin(); iter != accelerators.end(); ++iter) {
+    OnAcceleratorUnregistered(*iter);
+  }
+}
+
 bool AcceleratorControllerRegistrar::GenerateIds(Ids* ids) {
   if (ids_.size() + 2 >= std::numeric_limits<uint16_t>::max())
     return false;
   ids->pre_id = GetNextLocalAcceleratorId();
   ids->post_id = GetNextLocalAcceleratorId();
   return true;
 }
 
 uint16_t AcceleratorControllerRegistrar::GetNextLocalAcceleratorId() {
   DCHECK_LT(ids_.size(), std::numeric_limits<uint16_t>::max());
   // 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