sky	
3 hours, 41 minutes ago (2016-12-02 17:25:31 UTC) #110
On 2016/12/02 15:11:19, thanhph wrote:
>
https://codereview.chromium.org/2520093003/diff/460001/ash/mus/accelerators/a...
> File ash/mus/accelerators/accelerator_controller_registrar.cc (right):
> 
>
https://codereview.chromium.org/2520093003/diff/460001/ash/mus/accelerators/a...
> ash/mus/accelerators/accelerator_controller_registrar.cc:129:
> window_manager_->window_manager_client()->AddAccelerators(
> On 2016/12/02 00:11:16, sky wrote:
> > On 2016/12/01 23:32:41, thanhph wrote:
> > > On 2016/12/01 22:17:04, sky wrote:
> > > > Is this only the first change you're intending to do? As the way you
have
> it
> > > now
> > > > you only call add with a single vector, so that in effect this change
> makes
> > > the
> > > > call have more overhead.
> > > 
> > > To change this I need to change the callback function OnAcceleratorAdded
to
> > > handle multiple accelerators. I can create another bug which is to reduce
> > > overhead of registering accelerators. This seems relate to this one I'm
> > working
> > > on next https://bugs.chromium.org/p/chromium/issues/detail?id=632050.
> > 
> > I don't think that would help as registration adds accelerators one at a
time.
> > Instead I think you need a function to be called on
> > AcceleratorControllerRegistrar after AcceleratorController::Init has been
> > called. Something like OnAcceleratorsRegistered(). This function would then
> > iterate over the internal structure and make the request to register the
> > accelerators as a single array.
> > 
> > You should do this separately. I just wanted to make sure you were going to
> make
> > use of the function that takes an array.
> 
> Thanks Scott, I'll keep this in mind. Should I put this comment thread also in
> this bug https://bugs.chromium.org/p/chromium/issues/detail?id=632050 ? 

Sure. If you add 632050 to the BUG= of this cl when this is landed both bugs are
updated.

> For what I understand,  bug 63250  is to resolve what you mention here.
> Alternatively, we can merge these 2 bugs into bug 63249 and I can proceed with
> the refactor. Thanks!

I'm fine either way.

> 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/edbf3f7ee62a25e213036c80b3c28ff6311a0dab

commit edbf3f7ee62a25e213036c80b3c28ff6311a0dab
Author: thanhph <thanhph@chromium.org>
Date: Mon Jan 30 19:17:54 2017

Make mash register initial batch of accelerators in single shot.

Add OnAcceleratorsRegistered to register all accelerators during startup with single IPC.

BUG= 632050 

Review-Url: https://codereview.chromium.org/2586333003
Cr-Commit-Position: refs/heads/master@{#447030}

[modify] https://crrev.com/edbf3f7ee62a25e213036c80b3c28ff6311a0dab/ash/accelerators/accelerator_controller_unittest.cc
[modify] https://crrev.com/edbf3f7ee62a25e213036c80b3c28ff6311a0dab/ash/common/accelerators/accelerator_controller.cc
[modify] https://crrev.com/edbf3f7ee62a25e213036c80b3c28ff6311a0dab/ash/common/accelerators/accelerator_controller.h
[modify] https://crrev.com/edbf3f7ee62a25e213036c80b3c28ff6311a0dab/ash/mus/accelerators/accelerator_controller_registrar.cc
[modify] https://crrev.com/edbf3f7ee62a25e213036c80b3c28ff6311a0dab/ash/mus/accelerators/accelerator_controller_registrar.h
[modify] https://crrev.com/edbf3f7ee62a25e213036c80b3c28ff6311a0dab/ash/mus/accelerators/accelerator_controller_unittest.cc
[modify] https://crrev.com/edbf3f7ee62a25e213036c80b3c28ff6311a0dab/chrome/browser/extensions/global_shortcut_listener_chromeos.cc
[modify] https://crrev.com/edbf3f7ee62a25e213036c80b3c28ff6311a0dab/services/ui/ws/event_dispatcher.cc
[modify] https://crrev.com/edbf3f7ee62a25e213036c80b3c28ff6311a0dab/ui/base/accelerators/accelerator_manager.cc
[modify] https://crrev.com/edbf3f7ee62a25e213036c80b3c28ff6311a0dab/ui/base/accelerators/accelerator_manager.h
[modify] https://crrev.com/edbf3f7ee62a25e213036c80b3c28ff6311a0dab/ui/base/accelerators/accelerator_manager_delegate.h
[modify] https://crrev.com/edbf3f7ee62a25e213036c80b3c28ff6311a0dab/ui/base/accelerators/accelerator_manager_unittest.cc
[modify] https://crrev.com/edbf3f7ee62a25e213036c80b3c28ff6311a0dab/ui/views/focus/focus_manager.cc

Thanh, is this fixed now?

Yes, it is. I'll close this, thanks!