Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2586333003:
Make mash register initial batch of accelerators in single shot. (Closed)
DescriptionMake 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}
Committed: https://chromium.googlesource.com/chromium/src/+/edbf3f7ee62a25e213036c80b3c28ff6311a0dab
Patch Set 1 #Patch Set 2 : Add unimplemented methods #
Total comments: 4
Patch Set 3 : Refactor OnAcceleratorRegistered #Patch Set 4 : Move default value of add_accelerator_immediately to header. #Patch Set 5 : Refactor ash unittest to be compatible with Register. #Patch Set 6 : Add const to function declaration. #Patch Set 7 : Add default variable to function declaration. #Patch Set 8 : Change register to use multiple accelerators in unit test. #Patch Set 9 : Fix vector initialization. #Patch Set 10 : Remove unimplemented place holder. #Patch Set 11 : Unit test result should expect both accelerators registered. #Patch Set 12 : Change register to use accelerator vector. #
Total comments: 29
Patch Set 13 : Remove temporary vector. Add comments. Add AddAcceleratorToVector to use in OnAcceleratorRegistered… #
Total comments: 14
Patch Set 14 : Add server log when adding accelerator vector. Fix/Refactor codes/comments/format. #
Total comments: 12
Patch Set 15 : Change comment/ Modify log sections/ Refactor code. #
Total comments: 4
Patch Set 16 : Rename Accelerators to AcceleratorList. #
Total comments: 6
Patch Set 17 : remove unnecessary std::vector. #
Total comments: 34
Patch Set 18 : Fix nits/names. #
Total comments: 6
Patch Set 19 : fix nits/names/logs. #
Total comments: 2
Patch Set 20 : Remove OnAcceleratorRegistered. Update comments. #
Total comments: 10
Patch Set 21 : Fix nits. Modify register test to register multiple accelerators. #Patch Set 22 : Pass by const & in AcceleratorManager::Register #
Total comments: 8
Patch Set 23 : fix nits/format and refactor AcceleratorControllerTest.Register. #
Total comments: 16
Patch Set 24 : Add RegisterAccelerator. Use const auto& for vector iteration. Fix names/comments. #
Total comments: 10
Patch Set 25 : Fix comments. Move inline function to .h #Messages
Total messages: 139 (100 generated)
The CQ bit was checked by thanhph@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Add OnAcceleratorsRegistered to register all at once accelerators during startup. BUG=632050 ========== to ========== Make mash register initial batch of accelerators in single shot. Add OnAcceleratorsRegistered to register all accelerators during startup with single IPC. BUG=632050 ==========
thanhph@chromium.org changed reviewers: + mfomitchev@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by thanhph@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
Patchset #2 (id:20001) has been deleted
The CQ bit was checked by thanhph@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #2 (id:40001) has been deleted
The CQ bit was checked by thanhph@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
https://codereview.chromium.org/2586333003/diff/60001/ash/common/accelerators... File ash/common/accelerators/accelerator_controller.cc (right): https://codereview.chromium.org/2586333003/diff/60001/ash/common/accelerators... ash/common/accelerators/accelerator_controller.cc:763: Register(accelerator, this); I am not sure what are you trying to do here. You are calling both Register() and Registers() which doesn't make much sense. I wouldn't add Registers() at all, because I don't think it's going to help much. I'd just call accelerator_manager_->Register() and pass it an array of accelerators along with target and priority (this implies we either overload the existing Register method in AcceleratorManager or rewrite it to always accept an array). Note that the target and priority are all the same, so it might make sense to take advantage of this: instead of passing an array of 3-element sets {accelerator, priority, target}, you can pass an array of accelerators, plus the target and the priority which these accelerators all share. https://codereview.chromium.org/2586333003/diff/60001/ash/mus/accelerators/ac... File ash/mus/accelerators/accelerator_controller_registrar.cc (right): https://codereview.chromium.org/2586333003/diff/60001/ash/mus/accelerators/ac... ash/mus/accelerators/accelerator_controller_registrar.cc:134: accelerator_ptrs_.push_back( We can't completely repurpose this method to just be a helper method for OnAcceleratorsRegistered - registering a single accelerator still needs to work by itself, and right now it doesn't - we are just putting accelerator in the array which doesn't get used until OnAcceleratorsRegistered is called.
The CQ bit was checked by thanhph@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by thanhph@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2586333003/diff/60001/ash/common/accelerators... File ash/common/accelerators/accelerator_controller.cc (right): https://codereview.chromium.org/2586333003/diff/60001/ash/common/accelerators... ash/common/accelerators/accelerator_controller.cc:763: Register(accelerator, this); On 2016/12/20 03:10:26, mfomitchev wrote: > I am not sure what are you trying to do here. You are calling both Register() > and Registers() which doesn't make much sense. > > I wouldn't add Registers() at all, because I don't think it's going to help > much. I'd just call accelerator_manager_->Register() and pass it an array of > accelerators along with target and priority (this implies we either overload the > existing Register method in AcceleratorManager or rewrite it to always accept an > array). Note that the target and priority are all the same, so it might make > sense to take advantage of this: instead of passing an array of 3-element sets > {accelerator, priority, target}, you can pass an array of accelerators, plus the > target and the priority which these accelerators all share. Thanks. I go with the approach "pass an array of accelerators, plus the target and the priority which these accelerators all share." https://codereview.chromium.org/2586333003/diff/60001/ash/mus/accelerators/ac... File ash/mus/accelerators/accelerator_controller_registrar.cc (right): https://codereview.chromium.org/2586333003/diff/60001/ash/mus/accelerators/ac... ash/mus/accelerators/accelerator_controller_registrar.cc:134: accelerator_ptrs_.push_back( On 2016/12/20 03:10:26, mfomitchev wrote: > We can't completely repurpose this method to just be a helper method for > OnAcceleratorsRegistered - registering a single accelerator still needs to work > by itself, and right now it doesn't - we are just putting accelerator in the > array which doesn't get used until OnAcceleratorsRegistered is called. Thanks, I added the window_manager_->window_manager_client()->AddAccelerators back. The function void AcceleratorControllerRegistrar::OnAcceleratorRegistered(const ui::Accelerator& accelerator) now will require another argument bool add_accelerator_immediately (default to true)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by thanhph@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by thanhph@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was unchecked by thanhph@chromium.org
The CQ bit was checked by thanhph@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by thanhph@chromium.org
The CQ bit was checked by thanhph@chromium.org to run a CQ dry run
The CQ bit was checked by thanhph@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by thanhph@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2586333003/diff/260001/ash/common/accelerator... File ash/common/accelerators/accelerator_controller.h (right): https://codereview.chromium.org/2586333003/diff/260001/ash/common/accelerator... ash/common/accelerators/accelerator_controller.h:67: Remove space, update comment. https://codereview.chromium.org/2586333003/diff/260001/ash/mus/accelerators/a... File ash/mus/accelerators/accelerator_controller_registrar.cc (right): https://codereview.chromium.org/2586333003/diff/260001/ash/mus/accelerators/a... ash/mus/accelerators/accelerator_controller_registrar.cc:26: DCHECK(added) << "duplicate accelerator key_code=" << accelerator.key_code() 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. https://codereview.chromium.org/2586333003/diff/260001/ash/mus/accelerators/a... ash/mus/accelerators/accelerator_controller_registrar.cc:111: accelerator_ptrs_.clear(); 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|. https://codereview.chromium.org/2586333003/diff/260001/ash/mus/accelerators/a... ash/mus/accelerators/accelerator_controller_registrar.cc:148: window_manager_->window_manager_client()->AddAccelerators( 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. https://codereview.chromium.org/2586333003/diff/260001/ash/mus/accelerators/a... File ash/mus/accelerators/accelerator_controller_registrar.h (right): https://codereview.chromium.org/2586333003/diff/260001/ash/mus/accelerators/a... ash/mus/accelerators/accelerator_controller_registrar.h:43: // default add_accelerator_immediately = true The comment doesn't add value - this is clear from signature. https://codereview.chromium.org/2586333003/diff/260001/ash/mus/accelerators/a... ash/mus/accelerators/accelerator_controller_registrar.h:51: void OnAcceleratorsUnregistered( Where is this actually called from? https://codereview.chromium.org/2586333003/diff/260001/chrome/browser/extensi... File chrome/browser/extensions/global_shortcut_listener_chromeos.cc (right): https://codereview.chromium.org/2586333003/diff/260001/chrome/browser/extensi... chrome/browser/extensions/global_shortcut_listener_chromeos.cc:51: const std::vector<ui::Accelerator> accelerators = {accelerator}; we don't need to create a local variable. https://codereview.chromium.org/2586333003/diff/260001/ui/base/accelerators/a... File ui/base/accelerators/accelerator_manager.cc (right): https://codereview.chromium.org/2586333003/diff/260001/ui/base/accelerators/a... ui/base/accelerators/accelerator_manager.cc:26: std::vector<ui::Accelerator> first_target_accelerators; Just call this |new_accelerators|? A comment explaining what it is would also help. https://codereview.chromium.org/2586333003/diff/260001/ui/base/accelerators/a... ui/base/accelerators/accelerator_manager.cc:45: // accelerator Reformat so that there's no gaps like this https://codereview.chromium.org/2586333003/diff/260001/ui/base/accelerators/a... File ui/base/accelerators/accelerator_manager.h (right): https://codereview.chromium.org/2586333003/diff/260001/ui/base/accelerators/a... ui/base/accelerators/accelerator_manager.h:34: // Register a keyboard accelerator for the specified target. If multiple Update comment https://codereview.chromium.org/2586333003/diff/260001/ui/base/accelerators/a... ui/base/accelerators/accelerator_manager.h:55: void Registers(std::vector<ui::Accelerator>& accelerators); Don't think we are using this? https://codereview.chromium.org/2586333003/diff/260001/ui/base/accelerators/a... File ui/base/accelerators/accelerator_manager_delegate.h (right): https://codereview.chromium.org/2586333003/diff/260001/ui/base/accelerators/a... ui/base/accelerators/accelerator_manager_delegate.h:26: virtual void OnAcceleratorsRegistered( comment https://codereview.chromium.org/2586333003/diff/260001/ui/views/focus/focus_m... File ui/views/focus/focus_manager.cc (right): https://codereview.chromium.org/2586333003/diff/260001/ui/views/focus/focus_m... ui/views/focus/focus_manager.cc:500: const std::vector<ui::Accelerator>& accelerators = {accelerator}; no need for a local variable.
https://codereview.chromium.org/2586333003/diff/260001/ash/mus/accelerators/a... File ash/mus/accelerators/accelerator_controller_registrar.cc (right): https://codereview.chromium.org/2586333003/diff/260001/ash/mus/accelerators/a... ash/mus/accelerators/accelerator_controller_registrar.cc:111: accelerator_ptrs_.clear(); On 2016/12/22 01:04:50, mfomitchev wrote: > 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|. 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.
On 2016/12/22 17:48:45, thanhph wrote: > https://codereview.chromium.org/2586333003/diff/260001/ash/mus/accelerators/a... > File ash/mus/accelerators/accelerator_controller_registrar.cc (right): > > https://codereview.chromium.org/2586333003/diff/260001/ash/mus/accelerators/a... > ash/mus/accelerators/accelerator_controller_registrar.cc:111: > accelerator_ptrs_.clear(); > On 2016/12/22 01:04:50, mfomitchev wrote: > > 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|. > > 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. You are ignoring the rest of my comment. With my suggestion, OnAcceleratorRegistered will no longer be called from OnAcceleratorsRegistered, so we don't need |add_accelerator_immediately| anymore. Instead OnAcceleratorRegistered and OnAcceleratorsRegistered will both call AddAcceleratorToVector, which will simply package accelerators in the vector without doing any IPCs. IPCs will happen separately from OnAcceleratorRegistered and OnAcceleratorsRegistered - I hope that won't result in too much duplicated code, otherwise we can create another helper method for IPC.
Thanks Mikhail. Please review my new cl! https://codereview.chromium.org/2586333003/diff/260001/ash/common/accelerator... File ash/common/accelerators/accelerator_controller.h (right): https://codereview.chromium.org/2586333003/diff/260001/ash/common/accelerator... ash/common/accelerators/accelerator_controller.h:67: On 2016/12/22 01:04:50, mfomitchev wrote: > Remove space, update comment. Done. https://codereview.chromium.org/2586333003/diff/260001/ash/mus/accelerators/a... File ash/mus/accelerators/accelerator_controller_registrar.cc (right): https://codereview.chromium.org/2586333003/diff/260001/ash/mus/accelerators/a... ash/mus/accelerators/accelerator_controller_registrar.cc:148: window_manager_->window_manager_client()->AddAccelerators( On 2016/12/22 01:04:50, mfomitchev wrote: > 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. Done. https://codereview.chromium.org/2586333003/diff/260001/ash/mus/accelerators/a... File ash/mus/accelerators/accelerator_controller_registrar.h (right): https://codereview.chromium.org/2586333003/diff/260001/ash/mus/accelerators/a... ash/mus/accelerators/accelerator_controller_registrar.h:43: // default add_accelerator_immediately = true On 2016/12/22 01:04:50, mfomitchev wrote: > The comment doesn't add value - this is clear from signature. Done. https://codereview.chromium.org/2586333003/diff/260001/ash/mus/accelerators/a... ash/mus/accelerators/accelerator_controller_registrar.h:51: void OnAcceleratorsUnregistered( On 2016/12/22 01:04:50, mfomitchev wrote: > Where is this actually called from? This is called in accelerator_manager_unittest.cc (line 99 for this patch). Test case AcceleratorManagerTest.UnregisterAll (in the same file) will fail if this function is not probably implemented. https://codereview.chromium.org/2586333003/diff/260001/chrome/browser/extensi... File chrome/browser/extensions/global_shortcut_listener_chromeos.cc (right): https://codereview.chromium.org/2586333003/diff/260001/chrome/browser/extensi... chrome/browser/extensions/global_shortcut_listener_chromeos.cc:51: const std::vector<ui::Accelerator> accelerators = {accelerator}; On 2016/12/22 01:04:50, mfomitchev wrote: > we don't need to create a local variable. Done. https://codereview.chromium.org/2586333003/diff/260001/ui/base/accelerators/a... File ui/base/accelerators/accelerator_manager.cc (right): https://codereview.chromium.org/2586333003/diff/260001/ui/base/accelerators/a... ui/base/accelerators/accelerator_manager.cc:26: std::vector<ui::Accelerator> first_target_accelerators; On 2016/12/22 01:04:50, mfomitchev wrote: > Just call this |new_accelerators|? A comment explaining what it is would also > help. Done. https://codereview.chromium.org/2586333003/diff/260001/ui/base/accelerators/a... ui/base/accelerators/accelerator_manager.cc:45: // accelerator On 2016/12/22 01:04:50, mfomitchev wrote: > Reformat so that there's no gaps like this Done. https://codereview.chromium.org/2586333003/diff/260001/ui/base/accelerators/a... File ui/base/accelerators/accelerator_manager.h (right): https://codereview.chromium.org/2586333003/diff/260001/ui/base/accelerators/a... ui/base/accelerators/accelerator_manager.h:34: // Register a keyboard accelerator for the specified target. If multiple On 2016/12/22 01:04:50, mfomitchev wrote: > Update comment Done. https://codereview.chromium.org/2586333003/diff/260001/ui/base/accelerators/a... ui/base/accelerators/accelerator_manager.h:55: void Registers(std::vector<ui::Accelerator>& accelerators); On 2016/12/22 01:04:50, mfomitchev wrote: > Don't think we are using this? Done, removed! https://codereview.chromium.org/2586333003/diff/260001/ui/base/accelerators/a... File ui/base/accelerators/accelerator_manager_delegate.h (right): https://codereview.chromium.org/2586333003/diff/260001/ui/base/accelerators/a... ui/base/accelerators/accelerator_manager_delegate.h:26: virtual void OnAcceleratorsRegistered( On 2016/12/22 01:04:50, mfomitchev wrote: > comment Done. https://codereview.chromium.org/2586333003/diff/260001/ui/views/focus/focus_m... File ui/views/focus/focus_manager.cc (right): https://codereview.chromium.org/2586333003/diff/260001/ui/views/focus/focus_m... ui/views/focus/focus_manager.cc:500: const std::vector<ui::Accelerator>& accelerators = {accelerator}; On 2016/12/22 01:04:50, mfomitchev wrote: > no need for a local variable. Done.
The CQ bit was checked by thanhph@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2586333003/diff/260001/ash/mus/accelerators/a... File ash/mus/accelerators/accelerator_controller_registrar.cc (right): https://codereview.chromium.org/2586333003/diff/260001/ash/mus/accelerators/a... ash/mus/accelerators/accelerator_controller_registrar.cc:26: DCHECK(added) << "duplicate accelerator key_code=" << accelerator.key_code() On 2016/12/22 01:04:50, mfomitchev wrote: > 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. Not sure if you missed this comment? https://codereview.chromium.org/2586333003/diff/260001/ash/mus/accelerators/a... File ash/mus/accelerators/accelerator_controller_registrar.h (right): https://codereview.chromium.org/2586333003/diff/260001/ash/mus/accelerators/a... ash/mus/accelerators/accelerator_controller_registrar.h:51: void OnAcceleratorsUnregistered( On 2016/12/22 20:36:01, thanhph wrote: > On 2016/12/22 01:04:50, mfomitchev wrote: > > Where is this actually called from? > > This is called in accelerator_manager_unittest.cc (line 99 for this patch). Test > case AcceleratorManagerTest.UnregisterAll (in the same file) will fail if this > function is not probably implemented. Line 99 is an implementation of OnAcceleratorsUnregistered from AcceleratorManagerDelegate interface. OnAcceleratorsUnregistered is never actually called in this CL as far as I can see. You could call it from AcceleratorManager::UnregisterAll if you want - then this would make sense. Personally, I'd just remove it as well - I don't think we are going to save much by adding it. https://codereview.chromium.org/2586333003/diff/280001/ash/common/accelerator... File ash/common/accelerators/accelerator_controller.h (right): https://codereview.chromium.org/2586333003/diff/280001/ash/common/accelerator... ash/common/accelerators/accelerator_controller.h:65: // multiple targets are registered for accelerators, a target registered If multiple targets are registered for any given accelerator, the target registered later has higher priority. https://codereview.chromium.org/2586333003/diff/280001/ash/mus/accelerators/a... File ash/mus/accelerators/accelerator_controller_registrar.h (right): https://codereview.chromium.org/2586333003/diff/280001/ash/mus/accelerators/a... ash/mus/accelerators/accelerator_controller_registrar.h:42: void AddAcceleratorToVector( private, add comment. https://codereview.chromium.org/2586333003/diff/280001/ash/mus/accelerators/a... ash/mus/accelerators/accelerator_controller_registrar.h:50: // Register multiple accelerators with single IPC. This is part of implementation of ui::AcceleratorManagerDelegate. The comment describing the behavior should be in ui::AcceleratorManagerDelegate, and it shouldn't be duplicated here. https://codereview.chromium.org/2586333003/diff/280001/ui/base/accelerators/a... File ui/base/accelerators/accelerator_manager.cc (right): https://codereview.chromium.org/2586333003/diff/280001/ui/base/accelerators/a... ui/base/accelerators/accelerator_manager.cc:26: // This vector only pick in |accelerators| each accelerator which appears for Accelerators which haven't already been registered with some target. https://codereview.chromium.org/2586333003/diff/280001/ui/base/accelerators/a... ui/base/accelerators/accelerator_manager.cc:29: std::vector<ui::Accelerator> first_target_accelerators; Repeat: Just call this |new_accelerators| https://codereview.chromium.org/2586333003/diff/280001/ui/base/accelerators/a... File ui/base/accelerators/accelerator_manager_delegate.h (right): https://codereview.chromium.org/2586333003/diff/280001/ui/base/accelerators/a... ui/base/accelerators/accelerator_manager_delegate.h:25: // Register all accelerators in |accelerators| vector with single IPC. This comment describes the behavior of implementation of OnAcceleratorsRegistered in AcceleratorControllerRegistrar. Instead it should be describing when OnAcceleratorsRegistered is called on the delegate (AcceleratorManagerDelegate). Look at the comment for OnAcceleratorRegistered and describe this one using a similar perspective. https://codereview.chromium.org/2586333003/diff/280001/ui/base/accelerators/a... File ui/base/accelerators/accelerator_manager_unittest.cc (right): https://codereview.chromium.org/2586333003/diff/280001/ui/base/accelerators/a... ui/base/accelerators/accelerator_manager_unittest.cc:104: // AcceleratorManagerDelegate: The comment should be preceeding the first method from the interface, and the methods should be in the same order as they are in the interface.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by thanhph@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks Mikhail. Please review my new cl. https://codereview.chromium.org/2586333003/diff/260001/ash/mus/accelerators/a... File ash/mus/accelerators/accelerator_controller_registrar.cc (right): https://codereview.chromium.org/2586333003/diff/260001/ash/mus/accelerators/a... ash/mus/accelerators/accelerator_controller_registrar.cc:26: DCHECK(added) << "duplicate accelerator key_code=" << accelerator.key_code() On 2016/12/22 21:18:55, mfomitchev wrote: > On 2016/12/22 01:04:50, mfomitchev wrote: > > 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. > > Not sure if you missed this comment? 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... https://codereview.chromium.org/2586333003/diff/260001/ash/mus/accelerators/a... File ash/mus/accelerators/accelerator_controller_registrar.h (right): https://codereview.chromium.org/2586333003/diff/260001/ash/mus/accelerators/a... ash/mus/accelerators/accelerator_controller_registrar.h:51: void OnAcceleratorsUnregistered( On 2016/12/22 21:18:55, mfomitchev wrote: > On 2016/12/22 20:36:01, thanhph wrote: > > On 2016/12/22 01:04:50, mfomitchev wrote: > > > Where is this actually called from? > > > > This is called in accelerator_manager_unittest.cc (line 99 for this patch). > Test > > case AcceleratorManagerTest.UnregisterAll (in the same file) will fail if this > > function is not probably implemented. > > Line 99 is an implementation of OnAcceleratorsUnregistered from > AcceleratorManagerDelegate interface. OnAcceleratorsUnregistered is never > actually called in this CL as far as I can see. You could call it from > AcceleratorManager::UnregisterAll if you want - then this would make sense. > Personally, I'd just remove it as well - I don't think we are going to save much > by adding it. Done, thanks! I've removed it for now because it seems, as you mention, no function calls it. Hope the bots don't complain. https://codereview.chromium.org/2586333003/diff/280001/ash/common/accelerator... File ash/common/accelerators/accelerator_controller.h (right): https://codereview.chromium.org/2586333003/diff/280001/ash/common/accelerator... ash/common/accelerators/accelerator_controller.h:65: // multiple targets are registered for accelerators, a target registered On 2016/12/22 21:18:55, mfomitchev wrote: > If multiple targets are registered for any given accelerator, the target > registered later has higher priority. Done. https://codereview.chromium.org/2586333003/diff/280001/ash/mus/accelerators/a... File ash/mus/accelerators/accelerator_controller_registrar.h (right): https://codereview.chromium.org/2586333003/diff/280001/ash/mus/accelerators/a... ash/mus/accelerators/accelerator_controller_registrar.h:42: void AddAcceleratorToVector( On 2016/12/22 21:18:55, mfomitchev wrote: > private, add comment. Done. https://codereview.chromium.org/2586333003/diff/280001/ash/mus/accelerators/a... ash/mus/accelerators/accelerator_controller_registrar.h:50: // Register multiple accelerators with single IPC. On 2016/12/22 21:18:55, mfomitchev wrote: > This is part of implementation of ui::AcceleratorManagerDelegate. The comment > describing the behavior should be in ui::AcceleratorManagerDelegate, and it > shouldn't be duplicated here. Done. https://codereview.chromium.org/2586333003/diff/280001/ui/base/accelerators/a... File ui/base/accelerators/accelerator_manager.cc (right): https://codereview.chromium.org/2586333003/diff/280001/ui/base/accelerators/a... ui/base/accelerators/accelerator_manager.cc:26: // This vector only pick in |accelerators| each accelerator which appears for On 2016/12/22 21:18:55, mfomitchev wrote: > Accelerators which haven't already been registered with some target. Done. https://codereview.chromium.org/2586333003/diff/280001/ui/base/accelerators/a... ui/base/accelerators/accelerator_manager.cc:29: std::vector<ui::Accelerator> first_target_accelerators; On 2016/12/22 21:18:55, mfomitchev wrote: > Repeat: Just call this |new_accelerators| Done. https://codereview.chromium.org/2586333003/diff/280001/ui/base/accelerators/a... File ui/base/accelerators/accelerator_manager_delegate.h (right): https://codereview.chromium.org/2586333003/diff/280001/ui/base/accelerators/a... ui/base/accelerators/accelerator_manager_delegate.h:25: // Register all accelerators in |accelerators| vector with single IPC. On 2016/12/22 21:18:55, mfomitchev wrote: > This comment describes the behavior of implementation of > OnAcceleratorsRegistered in AcceleratorControllerRegistrar. Instead it should be > describing when OnAcceleratorsRegistered is called on the delegate > (AcceleratorManagerDelegate). Look at the comment for OnAcceleratorRegistered > and describe this one using a similar perspective. Done. I updated it to: // Called the first time a target is registered for each accelerator in // |accelerators|. This is only called the first time a target is registered // for each unique accelerator. For example, if Register() is called twice // with the same accelerator, this is called only for the first call. https://codereview.chromium.org/2586333003/diff/280001/ui/base/accelerators/a... File ui/base/accelerators/accelerator_manager_unittest.cc (right): https://codereview.chromium.org/2586333003/diff/280001/ui/base/accelerators/a... ui/base/accelerators/accelerator_manager_unittest.cc:104: // AcceleratorManagerDelegate: On 2016/12/22 21:18:55, mfomitchev wrote: > The comment should be preceeding the first method from the interface, and the > methods should be in the same order as they are in the interface. Done, cool!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2586333003/diff/300001/ash/mus/accelerators/a... File ash/mus/accelerators/accelerator_controller_registrar.cc (right): https://codereview.chromium.org/2586333003/diff/300001/ash/mus/accelerators/a... ash/mus/accelerators/accelerator_controller_registrar.cc:34: for (auto iter = ui_accelerators.begin(); iter != ui_accelerators.end(); We still need the DCHECK here, even though we don't know which accelerator caused the failure. The logging you added on Mus side is simply logging and not a DCHECK (as it should be), so if we remove the DCHECK here, we lose error detection. https://codereview.chromium.org/2586333003/diff/300001/ash/mus/accelerators/a... ash/mus/accelerators/accelerator_controller_registrar.cc:121: ui::mojom::EventMatcherPtr event_matcher = ui::CreateKeyMatcher( Nit: rename to pre_event_matcher to be consistent? https://codereview.chromium.org/2586333003/diff/300001/ash/mus/accelerators/a... File ash/mus/accelerators/accelerator_controller_registrar.h (right): https://codereview.chromium.org/2586333003/diff/300001/ash/mus/accelerators/a... ash/mus/accelerators/accelerator_controller_registrar.h:42: // ui::AcceleratorManagerDelegate: Can this be moved to private? Or no? https://codereview.chromium.org/2586333003/diff/300001/ash/mus/accelerators/a... ash/mus/accelerators/accelerator_controller_registrar.h:50: // Add accelerator with newly generated id to accelerator vector. IMHO "newly generated id" is a bit ambiguous, because it is not clear what generates them - it's better to just say "generates the id". Also, this function adds two accelerators to the vector (pre and post) - a detail which is worth mentioning. Something like this would do: Creates a PRE_TARGET and POST_TARGET mojom accelerators for the provided |accelerator| and adds them to the provided |accelerator_vector|. https://codereview.chromium.org/2586333003/diff/300001/services/ui/ws/window_... File services/ui/ws/window_tree.cc (right): https://codereview.chromium.org/2586333003/diff/300001/services/ui/ws/window_... services/ui/ws/window_tree.cc:1764: DVLOG(1) << "Can't add accelerator " << iter->get()->id; This isn't detailed enough - compare this to the original error message in accelerator_controller_registrar.cc. Th "id" doesn't really help you debug as much as keybode, type, and modifiers. Also, can we move this to EventDispatcher::AddAccelerator and also log why it failed (duplicated id vs. duplicate matcher)? https://codereview.chromium.org/2586333003/diff/300001/ui/base/accelerators/a... File ui/base/accelerators/accelerator_manager.cc (right): https://codereview.chromium.org/2586333003/diff/300001/ui/base/accelerators/a... ui/base/accelerators/accelerator_manager.cc:27: // with any target. Same accelerator with different target won't be added Replace the entire comment with "Accelerators which haven't already been registered with some target." This is accurate and sufficient description of what that vector is supposed to contain. Right now you are just repeating the same thing twice. https://codereview.chromium.org/2586333003/diff/300001/ui/base/accelerators/a... File ui/base/accelerators/accelerator_manager_delegate.h (right): https://codereview.chromium.org/2586333003/diff/300001/ui/base/accelerators/a... ui/base/accelerators/accelerator_manager_delegate.h:25: // Called the first time a target is registered for each accelerator in I think this would be a bit more clear: Called when a number of accelerators are registered at once. An accelerator is only included in the |accelerators| vector the first time a target for it is registered.
mfomitchev@chromium.org changed reviewers: + sadrul@chromium.org
sadrul@chromium.org: Please review changes in
thanhph@: Please continue working with Sadrul on this while I am OOO
thanhph@google.com changed reviewers: + thanhph@google.com
Thanks Mikhail. Hi Sadrul, Please review my new cl. Thanks, Thanh. https://codereview.chromium.org/2586333003/diff/300001/ash/mus/accelerators/a... File ash/mus/accelerators/accelerator_controller_registrar.h (right): https://codereview.chromium.org/2586333003/diff/300001/ash/mus/accelerators/a... ash/mus/accelerators/accelerator_controller_registrar.h:42: // ui::AcceleratorManagerDelegate: On 2017/01/03 19:54:35, mfomitchev_OOO_until_Jan_19 wrote: > Can this be moved to private? Or no? Done, yes! I moved these 2 functions to private section. https://codereview.chromium.org/2586333003/diff/300001/ash/mus/accelerators/a... ash/mus/accelerators/accelerator_controller_registrar.h:50: // Add accelerator with newly generated id to accelerator vector. On 2017/01/03 19:54:35, mfomitchev_OOO_until_Jan_19 wrote: > IMHO "newly generated id" is a bit ambiguous, because it is not clear what > generates them - it's better to just say "generates the id". Also, this function > adds two accelerators to the vector (pre and post) - a detail which is worth > mentioning. > Something like this would do: > Creates a PRE_TARGET and POST_TARGET mojom accelerators for the provided > |accelerator| and adds them to the provided |accelerator_vector|. Done, thanks! Please review my new comment. https://codereview.chromium.org/2586333003/diff/300001/services/ui/ws/window_... File services/ui/ws/window_tree.cc (right): https://codereview.chromium.org/2586333003/diff/300001/services/ui/ws/window_... services/ui/ws/window_tree.cc:1764: DVLOG(1) << "Can't add accelerator " << iter->get()->id; On 2017/01/03 19:54:35, mfomitchev_OOO_until_Jan_19 wrote: > This isn't detailed enough - compare this to the original error message in > accelerator_controller_registrar.cc. Th "id" doesn't really help you debug as > much as keybode, type, and modifiers. > Also, can we move this to EventDispatcher::AddAccelerator and also log why it > failed (duplicated id vs. duplicate matcher)? Done. I reverted this code and put logs in EventDispatcher::AddAccelerator instead. Thanks! https://codereview.chromium.org/2586333003/diff/300001/ui/base/accelerators/a... File ui/base/accelerators/accelerator_manager.cc (right): https://codereview.chromium.org/2586333003/diff/300001/ui/base/accelerators/a... ui/base/accelerators/accelerator_manager.cc:27: // with any target. Same accelerator with different target won't be added On 2017/01/03 19:54:35, mfomitchev_OOO_until_Jan_19 wrote: > Replace the entire comment with "Accelerators which haven't already been > registered with some target." > > This is accurate and sufficient description of what that vector is supposed to > contain. Right now you are just repeating the same thing twice. Done. https://codereview.chromium.org/2586333003/diff/300001/ui/base/accelerators/a... File ui/base/accelerators/accelerator_manager_delegate.h (right): https://codereview.chromium.org/2586333003/diff/300001/ui/base/accelerators/a... ui/base/accelerators/accelerator_manager_delegate.h:25: // Called the first time a target is registered for each accelerator in On 2017/01/03 19:54:35, mfomitchev_OOO_until_Jan_19 wrote: > I think this would be a bit more clear: > > Called when a number of accelerators are registered at once. An accelerator is > only included in the |accelerators| vector the first time a target for it is > registered. I modified it a bit. Let me know if it's clear. Thanks!
The CQ bit was checked by thanhph@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2586333003/diff/320001/ash/mus/accelerators/a... File ash/mus/accelerators/accelerator_controller_registrar.cc (right): https://codereview.chromium.org/2586333003/diff/320001/ash/mus/accelerators/a... ash/mus/accelerators/accelerator_controller_registrar.cc:117: std::move(accelerator_vector), It looks like you send at most two accelerators at a time here, right? So we are not really using a single IPC to register all accelerators. For X accelerators registered in ash, we are still sending X IPC (each IPC carrying two accelerators). That is my understanding of this code. Is this correct? Because I don't think this is what we want.
https://codereview.chromium.org/2586333003/diff/320001/ash/mus/accelerators/a... File ash/mus/accelerators/accelerator_controller_registrar.cc (right): https://codereview.chromium.org/2586333003/diff/320001/ash/mus/accelerators/a... ash/mus/accelerators/accelerator_controller_registrar.cc:117: std::move(accelerator_vector), On 2017/01/11 17:55:13, sadrul wrote: > It looks like you send at most two accelerators at a time here, right? So we are > not really using a single IPC to register all accelerators. For X accelerators > registered in ash, we are still sending X IPC (each IPC carrying two > accelerators). That is my understanding of this code. Is this correct? Because I > don't think this is what we want. This current cl (hopefully) does what you mention here. This function OnAcceleratorRegistered is used for registering single accelerator only. For multiple accelerator registration, OnAcceleratorsRegistered will be used. This function call Register (https://cs.chromium.org/chromium/src/ash/common/accelerators/accelerator_cont... ) has been moved outside of the loop to register a whole vector instead of single accelerator in every loop.
https://codereview.chromium.org/2586333003/diff/320001/ash/mus/accelerators/a... File ash/mus/accelerators/accelerator_controller_registrar.h (right): https://codereview.chromium.org/2586333003/diff/320001/ash/mus/accelerators/a... ash/mus/accelerators/accelerator_controller_registrar.h:47: void OnAcceleratorsRegistered( Sadrul mentioned that it would be better to call this OnAcceleratorListRegistered to make it easier to differentiate from OnAcceleratorRegistered, which seems like a good idea. OnAcceleratorsRegistered should then be renamed accordingly.
The CQ bit was checked by thanhph@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2586333003/diff/320001/ash/mus/accelerators/a... File ash/mus/accelerators/accelerator_controller_registrar.h (right): https://codereview.chromium.org/2586333003/diff/320001/ash/mus/accelerators/a... ash/mus/accelerators/accelerator_controller_registrar.h:47: void OnAcceleratorsRegistered( On 2017/01/24 17:40:01, mfomitchev wrote: > Sadrul mentioned that it would be better to call this > OnAcceleratorListRegistered to make it easier to differentiate from > OnAcceleratorRegistered, which seems like a good idea. > > OnAcceleratorsRegistered should then be renamed accordingly. Done, thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
Can you also please take a look at the failing trybot? Is that related? https://codereview.chromium.org/2586333003/diff/340001/ash/mus/accelerators/a... File ash/mus/accelerators/accelerator_controller_registrar.h (right): https://codereview.chromium.org/2586333003/diff/340001/ash/mus/accelerators/a... ash/mus/accelerators/accelerator_controller_registrar.h:100: std::vector<ui::mojom::AcceleratorPtr> accelerator_ptrs_; remove https://codereview.chromium.org/2586333003/diff/340001/ash/mus/accelerators/a... File ash/mus/accelerators/accelerator_controller_unittest.cc (right): https://codereview.chromium.org/2586333003/diff/340001/ash/mus/accelerators/a... ash/mus/accelerators/accelerator_controller_unittest.cc:349: const std::vector<ui::Accelerator>& accelerators = {accelerator_a}; no need for the local variable when it's only used once. There's more cases like this below. https://codereview.chromium.org/2586333003/diff/340001/ash/mus/accelerators/a... ash/mus/accelerators/accelerator_controller_unittest.cc:360: const std::vector<ui::Accelerator>& accelerators = {accelerator_a}; Nit: I wouldn't declare a local var for this either. We do use it twice, but using {accelerator_a} instead works pretty well too. Same in other similar places.
On 2017/01/24 21:00:30, mfomitchev wrote: > Can you also please take a look at the failing trybot? Is that related? > > https://codereview.chromium.org/2586333003/diff/340001/ash/mus/accelerators/a... > File ash/mus/accelerators/accelerator_controller_registrar.h (right): > > https://codereview.chromium.org/2586333003/diff/340001/ash/mus/accelerators/a... > ash/mus/accelerators/accelerator_controller_registrar.h:100: > std::vector<ui::mojom::AcceleratorPtr> accelerator_ptrs_; > remove > > https://codereview.chromium.org/2586333003/diff/340001/ash/mus/accelerators/a... > File ash/mus/accelerators/accelerator_controller_unittest.cc (right): > > https://codereview.chromium.org/2586333003/diff/340001/ash/mus/accelerators/a... > ash/mus/accelerators/accelerator_controller_unittest.cc:349: const > std::vector<ui::Accelerator>& accelerators = {accelerator_a}; > no need for the local variable when it's only used once. There's more cases like > this below. > > https://codereview.chromium.org/2586333003/diff/340001/ash/mus/accelerators/a... > ash/mus/accelerators/accelerator_controller_unittest.cc:360: const > std::vector<ui::Accelerator>& accelerators = {accelerator_a}; > Nit: I wouldn't declare a local var for this either. We do use it twice, but > using {accelerator_a} instead works pretty well too. Same in other similar > places. I think these Anrdoid bots are failing because of changes in the bots. Other cls have the same errors.
https://codereview.chromium.org/2586333003/diff/340001/ash/mus/accelerators/a... File ash/mus/accelerators/accelerator_controller_registrar.h (right): https://codereview.chromium.org/2586333003/diff/340001/ash/mus/accelerators/a... ash/mus/accelerators/accelerator_controller_registrar.h:100: std::vector<ui::mojom::AcceleratorPtr> accelerator_ptrs_; On 2017/01/24 21:00:30, mfomitchev wrote: > remove good catch, thanks! https://codereview.chromium.org/2586333003/diff/340001/ash/mus/accelerators/a... File ash/mus/accelerators/accelerator_controller_unittest.cc (right): https://codereview.chromium.org/2586333003/diff/340001/ash/mus/accelerators/a... ash/mus/accelerators/accelerator_controller_unittest.cc:349: const std::vector<ui::Accelerator>& accelerators = {accelerator_a}; On 2017/01/24 21:00:30, mfomitchev wrote: > no need for the local variable when it's only used once. There's more cases like > this below. Done, thanks! https://codereview.chromium.org/2586333003/diff/340001/ash/mus/accelerators/a... ash/mus/accelerators/accelerator_controller_unittest.cc:360: const std::vector<ui::Accelerator>& accelerators = {accelerator_a}; On 2017/01/24 21:00:30, mfomitchev wrote: > Nit: I wouldn't declare a local var for this either. We do use it twice, but > using {accelerator_a} instead works pretty well too. Same in other similar > places. Done.
The CQ bit was checked by thanhph@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2586333003/diff/360001/ash/accelerators/accel... File ash/accelerators/accelerator_controller_unittest.cc (right): https://codereview.chromium.org/2586333003/diff/360001/ash/accelerators/accel... ash/accelerators/accelerator_controller_unittest.cc:349: const std::vector<ui::Accelerator>& accelerators = {accelerator_a}; nit: remove https://codereview.chromium.org/2586333003/diff/360001/ash/mus/accelerators/a... File ash/mus/accelerators/accelerator_controller_registrar.cc (right): https://codereview.chromium.org/2586333003/diff/360001/ash/mus/accelerators/a... ash/mus/accelerators/accelerator_controller_registrar.cc:32: const std::vector<ui::Accelerator>& ui_accelerators, For consistency: ui_accelerators -> accelerators https://codereview.chromium.org/2586333003/diff/360001/ash/mus/accelerators/a... ash/mus/accelerators/accelerator_controller_registrar.cc:36: OnAcceleratorAdded(*iter, added); This will result in misleading DCHECK errors: if there is at least one duplicated accelerator in |ui_accelerators|, |added| will be false, and then the DCHECK will alwasy be triggered on the first accelerator (which may not actually be duplicated). https://codereview.chromium.org/2586333003/diff/360001/ash/mus/accelerators/a... ash/mus/accelerators/accelerator_controller_registrar.cc:109: // add_accelerator_immediately default set to true. Is this leftover from previous revision? https://codereview.chromium.org/2586333003/diff/360001/ash/mus/accelerators/a... ash/mus/accelerators/accelerator_controller_registrar.cc:145: base::Bind(OnAcceleratorVectorAdded, accelerators)); Because AddAcceleratorToVector may abort when GenerateIds() fails (line 152), passing |accelerators| to OnAcceleratorVectorAdded() is not quite correct. (Granted, OnAcceleratorVectorAdded doesn't do much now, but this may change). Can we pass it accelerator_vector instead? Same applies to OnAcceleratorRegistered(). https://codereview.chromium.org/2586333003/diff/360001/ash/mus/accelerators/a... File ash/mus/accelerators/accelerator_controller_registrar.h (right): https://codereview.chromium.org/2586333003/diff/360001/ash/mus/accelerators/a... ash/mus/accelerators/accelerator_controller_registrar.h:46: Remove whitespace since OnAcceleratorVectorRegistered is part of ui::AcceleratorManagerDelegate implementation. https://codereview.chromium.org/2586333003/diff/360001/ash/mus/accelerators/a... ash/mus/accelerators/accelerator_controller_registrar.h:57: friend class AcceleratorControllerRegistrarTestApi; Friend declaration should be in the beginning of the private: section. https://codereview.chromium.org/2586333003/diff/360001/ash/mus/accelerators/a... File ash/mus/accelerators/accelerator_controller_unittest.cc (right): https://codereview.chromium.org/2586333003/diff/360001/ash/mus/accelerators/a... ash/mus/accelerators/accelerator_controller_unittest.cc:348: nit: we are adding a lot of empty lines in this file without a great reason. https://codereview.chromium.org/2586333003/diff/360001/services/ui/ws/event_d... File services/ui/ws/event_dispatcher.cc (right): https://codereview.chromium.org/2586333003/diff/360001/services/ui/ws/event_d... services/ui/ws/event_dispatcher.cc:258: DVLOG(1) << "duplicate accelerator. Accelerator id=" << accelerator->id() duplicate accelerator -> duplicate accelerator id https://codereview.chromium.org/2586333003/diff/360001/services/ui/ws/event_d... services/ui/ws/event_dispatcher.cc:263: DVLOG(1) << "duplicate matcher. Accelerator id=" << accelerator->id() duplicate matcher -> duplicate accelerator matcher https://codereview.chromium.org/2586333003/diff/360001/services/ui/ws/event_d... services/ui/ws/event_dispatcher.cc:263: DVLOG(1) << "duplicate matcher. Accelerator id=" << accelerator->id() Can we get rid of the duplicate code here? The only difference is the substring string in the error message, so lets define that substring via if/else, but construct the rest of the message via the common code? https://codereview.chromium.org/2586333003/diff/360001/ui/base/accelerators/a... File ui/base/accelerators/accelerator_manager_delegate.h (right): https://codereview.chromium.org/2586333003/diff/360001/ui/base/accelerators/a... ui/base/accelerators/accelerator_manager_delegate.h:20: virtual void OnAcceleratorRegistered(const Accelerator& accelerator) = 0; This doesn't actually seem to be called anywhere anymore? https://codereview.chromium.org/2586333003/diff/360001/ui/base/accelerators/a... File ui/base/accelerators/accelerator_manager_unittest.cc (right): https://codereview.chromium.org/2586333003/diff/360001/ui/base/accelerators/a... ui/base/accelerators/accelerator_manager_unittest.cc:135: const std::vector<ui::Accelerator>& accelerators = {accelerator_a}; remove https://codereview.chromium.org/2586333003/diff/360001/ui/base/accelerators/a... ui/base/accelerators/accelerator_manager_unittest.cc:148: const std::vector<ui::Accelerator>& accelerators = {accelerator_a}; remove https://codereview.chromium.org/2586333003/diff/360001/ui/base/accelerators/a... ui/base/accelerators/accelerator_manager_unittest.cc:173: const std::vector<ui::Accelerator>& accelerators = {accelerator_a, Remove here and all the cases below. In the future, it might be a good idea to review your CL on codereview.chromium.org after uploading but before mailing comments to catch obvious nits like this. If you wish you can also do it even before uploading using "git meld" or "git diff".
Patchset #18 (id:380001) has been deleted
Thanks Mikhail. Please review my new cl. https://codereview.chromium.org/2586333003/diff/360001/ash/accelerators/accel... File ash/accelerators/accelerator_controller_unittest.cc (right): https://codereview.chromium.org/2586333003/diff/360001/ash/accelerators/accel... ash/accelerators/accelerator_controller_unittest.cc:349: const std::vector<ui::Accelerator>& accelerators = {accelerator_a}; On 2017/01/24 22:56:02, mfomitchev wrote: > nit: remove Done. https://codereview.chromium.org/2586333003/diff/360001/ash/mus/accelerators/a... File ash/mus/accelerators/accelerator_controller_registrar.cc (right): https://codereview.chromium.org/2586333003/diff/360001/ash/mus/accelerators/a... ash/mus/accelerators/accelerator_controller_registrar.cc:32: const std::vector<ui::Accelerator>& ui_accelerators, On 2017/01/24 22:56:02, mfomitchev wrote: > For consistency: > ui_accelerators -> accelerators Done. https://codereview.chromium.org/2586333003/diff/360001/ash/mus/accelerators/a... ash/mus/accelerators/accelerator_controller_registrar.cc:36: OnAcceleratorAdded(*iter, added); On 2017/01/24 22:56:02, mfomitchev wrote: > This will result in misleading DCHECK errors: if there is at least one > duplicated accelerator in |ui_accelerators|, |added| will be false, and then the > DCHECK will alwasy be triggered on the first accelerator (which may not actually > be duplicated). Acknowledged. Since |added| is a variable and not array, this will happen. I change the log message to say the array contain duplicates and print out the accelerator list. https://codereview.chromium.org/2586333003/diff/360001/ash/mus/accelerators/a... ash/mus/accelerators/accelerator_controller_registrar.cc:109: // add_accelerator_immediately default set to true. On 2017/01/24 22:56:02, mfomitchev wrote: > Is this leftover from previous revision? Oops. I removed it. Thanks! https://codereview.chromium.org/2586333003/diff/360001/ash/mus/accelerators/a... ash/mus/accelerators/accelerator_controller_registrar.cc:145: base::Bind(OnAcceleratorVectorAdded, accelerators)); On 2017/01/24 22:56:02, mfomitchev wrote: > Because AddAcceleratorToVector may abort when GenerateIds() fails (line 152), > passing |accelerators| to OnAcceleratorVectorAdded() is not quite correct. > (Granted, OnAcceleratorVectorAdded doesn't do much now, but this may change). > Can we pass it accelerator_vector instead? Same applies to > OnAcceleratorRegistered(). I think accelerator_vector is already used in std::move(accelerator_vector) so I can't bind it again. I tried to put accelerator_vector in OnAcceleratorVectorAdded, but this requires more codes to extract key_code and modifiers. I think leaving this as it's is cleaner. The GenerateIds function below already has a log incase ID can't be generated. https://codereview.chromium.org/2586333003/diff/360001/ash/mus/accelerators/a... File ash/mus/accelerators/accelerator_controller_registrar.h (right): https://codereview.chromium.org/2586333003/diff/360001/ash/mus/accelerators/a... ash/mus/accelerators/accelerator_controller_registrar.h:46: On 2017/01/24 22:56:02, mfomitchev wrote: > Remove whitespace since OnAcceleratorVectorRegistered is part of > ui::AcceleratorManagerDelegate implementation. Done. https://codereview.chromium.org/2586333003/diff/360001/ash/mus/accelerators/a... ash/mus/accelerators/accelerator_controller_registrar.h:57: friend class AcceleratorControllerRegistrarTestApi; On 2017/01/24 22:56:02, mfomitchev wrote: > Friend declaration should be in the beginning of the private: section. Done. https://codereview.chromium.org/2586333003/diff/360001/ash/mus/accelerators/a... File ash/mus/accelerators/accelerator_controller_unittest.cc (right): https://codereview.chromium.org/2586333003/diff/360001/ash/mus/accelerators/a... ash/mus/accelerators/accelerator_controller_unittest.cc:348: On 2017/01/24 22:56:02, mfomitchev wrote: > nit: we are adding a lot of empty lines in this file without a great reason. Done. https://codereview.chromium.org/2586333003/diff/360001/services/ui/ws/event_d... File services/ui/ws/event_dispatcher.cc (right): https://codereview.chromium.org/2586333003/diff/360001/services/ui/ws/event_d... services/ui/ws/event_dispatcher.cc:258: DVLOG(1) << "duplicate accelerator. Accelerator id=" << accelerator->id() On 2017/01/24 22:56:03, mfomitchev wrote: > duplicate accelerator -> duplicate accelerator id Done. https://codereview.chromium.org/2586333003/diff/360001/services/ui/ws/event_d... services/ui/ws/event_dispatcher.cc:263: DVLOG(1) << "duplicate matcher. Accelerator id=" << accelerator->id() On 2017/01/24 22:56:03, mfomitchev wrote: > Can we get rid of the duplicate code here? The only difference is the substring > string in the error message, so lets define that substring via if/else, but > construct the rest of the message via the common code? Done. https://codereview.chromium.org/2586333003/diff/360001/services/ui/ws/event_d... services/ui/ws/event_dispatcher.cc:263: DVLOG(1) << "duplicate matcher. Accelerator id=" << accelerator->id() On 2017/01/24 22:56:03, mfomitchev wrote: > duplicate matcher -> duplicate accelerator matcher Done. https://codereview.chromium.org/2586333003/diff/360001/ui/base/accelerators/a... File ui/base/accelerators/accelerator_manager_delegate.h (right): https://codereview.chromium.org/2586333003/diff/360001/ui/base/accelerators/a... ui/base/accelerators/accelerator_manager_delegate.h:20: virtual void OnAcceleratorRegistered(const Accelerator& accelerator) = 0; On 2017/01/24 22:56:03, mfomitchev wrote: > This doesn't actually seem to be called anywhere anymore? This is used in accelerator_controller_registrar.cc (line 119) and in accelerator_manager_unittest.cc https://codereview.chromium.org/2586333003/diff/360001/ui/base/accelerators/a... File ui/base/accelerators/accelerator_manager_unittest.cc (right): https://codereview.chromium.org/2586333003/diff/360001/ui/base/accelerators/a... ui/base/accelerators/accelerator_manager_unittest.cc:135: const std::vector<ui::Accelerator>& accelerators = {accelerator_a}; On 2017/01/24 22:56:03, mfomitchev wrote: > remove Done. https://codereview.chromium.org/2586333003/diff/360001/ui/base/accelerators/a... ui/base/accelerators/accelerator_manager_unittest.cc:148: const std::vector<ui::Accelerator>& accelerators = {accelerator_a}; On 2017/01/24 22:56:03, mfomitchev wrote: > remove Done. https://codereview.chromium.org/2586333003/diff/360001/ui/base/accelerators/a... ui/base/accelerators/accelerator_manager_unittest.cc:173: const std::vector<ui::Accelerator>& accelerators = {accelerator_a, On 2017/01/24 22:56:03, mfomitchev wrote: > Remove here and all the cases below. > In the future, it might be a good idea to review your CL on > http://codereview.chromium.org after uploading but before mailing comments to catch > obvious nits like this. If you wish you can also do it even before uploading > using "git meld" or "git diff". Done, thanks! It's a nice GUI good tool indeed.
Patchset #18 (id:400001) has been deleted
https://codereview.chromium.org/2586333003/diff/360001/ash/mus/accelerators/a... File ash/mus/accelerators/accelerator_controller_registrar.cc (right): https://codereview.chromium.org/2586333003/diff/360001/ash/mus/accelerators/a... ash/mus/accelerators/accelerator_controller_registrar.cc:36: OnAcceleratorAdded(*iter, added); On 2017/01/25 20:12:37, thanhph1 wrote: > On 2017/01/24 22:56:02, mfomitchev wrote: > > This will result in misleading DCHECK errors: if there is at least one > > duplicated accelerator in |ui_accelerators|, |added| will be false, and then > the > > DCHECK will alwasy be triggered on the first accelerator (which may not > actually > > be duplicated). > > Acknowledged. Since |added| is a variable and not array, this will happen. I > change the log message to say the array contain duplicates and print out the > accelerator list. This was the reason I asked you to add logging in Window Server's EventDispatcher earlier. Considering we can get an accurate log message for a specific accelerator from there, I don't think dumping accelerator info for the entire array here is very practical. So I'd just have an error message saying something like "Unexpected accelerator registration failure.". https://codereview.chromium.org/2586333003/diff/360001/ui/base/accelerators/a... File ui/base/accelerators/accelerator_manager_delegate.h (right): https://codereview.chromium.org/2586333003/diff/360001/ui/base/accelerators/a... ui/base/accelerators/accelerator_manager_delegate.h:20: virtual void OnAcceleratorRegistered(const Accelerator& accelerator) = 0; On 2017/01/25 20:12:37, thanhph1 wrote: > On 2017/01/24 22:56:03, mfomitchev wrote: > > This doesn't actually seem to be called anywhere anymore? > > This is used in accelerator_controller_registrar.cc (line 119) and in > accelerator_manager_unittest.cc We don't call this method in accelerator_controller_registrar.cc - we override it. Same with the unit test. (Worth noting that even if the unit test did call the method - that wouldn't be very relevant - usually it would be due to the fact that the unit test is testing functionality we don't use. Either way, we shouldn't have test-only method in a public interface used by production code like this one). https://codereview.chromium.org/2586333003/diff/420001/services/ui/ws/event_d... File services/ui/ws/event_dispatcher.cc (right): https://codereview.chromium.org/2586333003/diff/420001/services/ui/ws/event_d... services/ui/ws/event_dispatcher.cc:255: bool is_duplicate; Nit: Move the declarations below the comment. https://codereview.chromium.org/2586333003/diff/420001/services/ui/ws/event_d... services/ui/ws/event_dispatcher.cc:261: error_message = "duplicate accelerator id."; Nit: Capitalize the first letter. Same for below error_msg. https://codereview.chromium.org/2586333003/diff/420001/services/ui/ws/event_d... services/ui/ws/event_dispatcher.cc:268: DVLOG(1) << error_message << " accelerator id=" << accelerator->id() Maybe use LOG(ERROR) instead of DVLOG(1) - we probably don't expect this to happen, and it is useful to have this in the log without running chrome with a special set of flags.
Thanks Mikhail. Please review my new cl. Thanh. https://codereview.chromium.org/2586333003/diff/360001/ui/base/accelerators/a... File ui/base/accelerators/accelerator_manager_delegate.h (right): https://codereview.chromium.org/2586333003/diff/360001/ui/base/accelerators/a... ui/base/accelerators/accelerator_manager_delegate.h:20: virtual void OnAcceleratorRegistered(const Accelerator& accelerator) = 0; On 2017/01/26 16:03:36, mfomitchev wrote: > On 2017/01/25 20:12:37, thanhph1 wrote: > > On 2017/01/24 22:56:03, mfomitchev wrote: > > > This doesn't actually seem to be called anywhere anymore? > > > > This is used in accelerator_controller_registrar.cc (line 119) and in > > accelerator_manager_unittest.cc > > We don't call this method in accelerator_controller_registrar.cc - we override > it. Same with the unit test. > (Worth noting that even if the unit test did call the method - that wouldn't be > very relevant - usually it would be due to the fact that the unit test is > testing functionality we don't use. Either way, we shouldn't have test-only > method in a public interface used by production code like this one). Cool, thanks! I removed this function. https://codereview.chromium.org/2586333003/diff/420001/services/ui/ws/event_d... File services/ui/ws/event_dispatcher.cc (right): https://codereview.chromium.org/2586333003/diff/420001/services/ui/ws/event_d... services/ui/ws/event_dispatcher.cc:255: bool is_duplicate; On 2017/01/26 16:03:36, mfomitchev wrote: > Nit: Move the declarations below the comment. Done. https://codereview.chromium.org/2586333003/diff/420001/services/ui/ws/event_d... services/ui/ws/event_dispatcher.cc:261: error_message = "duplicate accelerator id."; On 2017/01/26 16:03:36, mfomitchev wrote: > Nit: Capitalize the first letter. Same for below error_msg. Done. https://codereview.chromium.org/2586333003/diff/420001/services/ui/ws/event_d... services/ui/ws/event_dispatcher.cc:268: DVLOG(1) << error_message << " accelerator id=" << accelerator->id() On 2017/01/26 16:03:36, mfomitchev wrote: > Maybe use LOG(ERROR) instead of DVLOG(1) - we probably don't expect this to > happen, and it is useful to have this in the log without running chrome with a > special set of flags. Done.
https://codereview.chromium.org/2586333003/diff/440001/ash/mus/accelerators/a... File ash/mus/accelerators/accelerator_controller_registrar.h (right): https://codereview.chromium.org/2586333003/diff/440001/ash/mus/accelerators/a... ash/mus/accelerators/accelerator_controller_registrar.h:46: void OnAcceleratorRegistered(const ui::Accelerator& accelerator); There's no reason to have this method anymore now that we removed it from ui::AcceleratorManagerDelegate interface. Same for the implementation in the unit test.
Patchset #20 (id:460001) has been deleted
Patchset #20 (id:480001) has been deleted
https://codereview.chromium.org/2586333003/diff/440001/ash/mus/accelerators/a... File ash/mus/accelerators/accelerator_controller_registrar.h (right): https://codereview.chromium.org/2586333003/diff/440001/ash/mus/accelerators/a... ash/mus/accelerators/accelerator_controller_registrar.h:46: void OnAcceleratorRegistered(const ui::Accelerator& accelerator); On 2017/01/26 20:05:13, mfomitchev wrote: > There's no reason to have this method anymore now that we removed it from > ui::AcceleratorManagerDelegate interface. Same for the implementation in the > unit test. Done, thanks! I also updated unit test comment.
The CQ bit was checked by thanhph@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by thanhph@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2586333003/diff/360001/ash/mus/accelerators/a... File ash/mus/accelerators/accelerator_controller_registrar.cc (right): https://codereview.chromium.org/2586333003/diff/360001/ash/mus/accelerators/a... ash/mus/accelerators/accelerator_controller_registrar.cc:145: base::Bind(OnAcceleratorVectorAdded, accelerators)); On 2017/01/25 20:12:37, thanhph1 wrote: > On 2017/01/24 22:56:02, mfomitchev wrote: > > Because AddAcceleratorToVector may abort when GenerateIds() fails (line 152), > > passing |accelerators| to OnAcceleratorVectorAdded() is not quite correct. > > (Granted, OnAcceleratorVectorAdded doesn't do much now, but this may change). > > Can we pass it accelerator_vector instead? Same applies to > > OnAcceleratorRegistered(). > > I think accelerator_vector is already used in std::move(accelerator_vector) so I > can't bind it again. I tried to put accelerator_vector in > OnAcceleratorVectorAdded, but this requires more codes to extract key_code and > modifiers. I think leaving this as it's is cleaner. The GenerateIds function > below already has a log incase ID can't be generated. Ok. Then lets not pass accelerators vector to OnAcceleratorVectorAdded at all - it's not used anyway. https://codereview.chromium.org/2586333003/diff/500001/ash/mus/accelerators/a... File ash/mus/accelerators/accelerator_controller_registrar.cc (right): https://codereview.chromium.org/2586333003/diff/500001/ash/mus/accelerators/a... ash/mus/accelerators/accelerator_controller_registrar.cc:25: DCHECK(added) << "Unexpected accelerator vector registration failure."; Please add a comment similar to the one we had in OnAcceleratorAdded. https://codereview.chromium.org/2586333003/diff/500001/ash/mus/accelerators/a... ash/mus/accelerators/accelerator_controller_registrar.cc:131: DVLOG(1) << "max number of accelerators registered, dropping request"; Let's turn this into LOG(ERROR) - this seems like a pretty bad failure, which we should always log. https://codereview.chromium.org/2586333003/diff/500001/services/ui/ws/event_d... File services/ui/ws/event_dispatcher.cc (right): https://codereview.chromium.org/2586333003/diff/500001/services/ui/ws/event_d... services/ui/ws/event_dispatcher.cc:257: bool is_duplicate; Initialize this to false. https://codereview.chromium.org/2586333003/diff/500001/ui/base/accelerators/a... File ui/base/accelerators/accelerator_manager_delegate.h (right): https://codereview.chromium.org/2586333003/diff/500001/ui/base/accelerators/a... ui/base/accelerators/accelerator_manager_delegate.h:23: virtual void OnAcceleratorVectorRegistered( Sorry for back-and-forth on this, but now that we don't have OnAcceleratorRegistered, can we change the name back to OnAcceleratorsRegistered? That would be shorter and more consistent with the naming elsewhere. Also, can you please move it to before OnAcceleratorUnregistered? https://codereview.chromium.org/2586333003/diff/500001/ui/base/accelerators/a... File ui/base/accelerators/accelerator_manager_unittest.cc (right): https://codereview.chromium.org/2586333003/diff/500001/ui/base/accelerators/a... ui/base/accelerators/accelerator_manager_unittest.cc:75: // . OnAcceleratorVectorRegistered() -> 'Register a list of ' + ids This doesn't seem to match what's implemented in OnAcceleratorVectorRegistered. We should also either change the existing "Register" test to register multiple accelerators at once, or add a new test for that. Currently only "Unregister" tests adding multiple accelerators at once, which doesn't make a lot of sense.
Patchset #21 (id:520001) has been deleted
Thanks Mikhail. Please review my cl. https://codereview.chromium.org/2586333003/diff/500001/ash/mus/accelerators/a... File ash/mus/accelerators/accelerator_controller_registrar.cc (right): https://codereview.chromium.org/2586333003/diff/500001/ash/mus/accelerators/a... ash/mus/accelerators/accelerator_controller_registrar.cc:25: DCHECK(added) << "Unexpected accelerator vector registration failure."; On 2017/01/26 21:39:49, mfomitchev wrote: > Please add a comment similar to the one we had in OnAcceleratorAdded. Done. https://codereview.chromium.org/2586333003/diff/500001/ash/mus/accelerators/a... ash/mus/accelerators/accelerator_controller_registrar.cc:131: DVLOG(1) << "max number of accelerators registered, dropping request"; On 2017/01/26 21:39:49, mfomitchev wrote: > Let's turn this into LOG(ERROR) - this seems like a pretty bad failure, which we > should always log. Done. https://codereview.chromium.org/2586333003/diff/500001/services/ui/ws/event_d... File services/ui/ws/event_dispatcher.cc (right): https://codereview.chromium.org/2586333003/diff/500001/services/ui/ws/event_d... services/ui/ws/event_dispatcher.cc:257: bool is_duplicate; On 2017/01/26 21:39:49, mfomitchev wrote: > Initialize this to false. Done. https://codereview.chromium.org/2586333003/diff/500001/ui/base/accelerators/a... File ui/base/accelerators/accelerator_manager_delegate.h (right): https://codereview.chromium.org/2586333003/diff/500001/ui/base/accelerators/a... ui/base/accelerators/accelerator_manager_delegate.h:23: virtual void OnAcceleratorVectorRegistered( On 2017/01/26 21:39:49, mfomitchev wrote: > Sorry for back-and-forth on this, but now that we don't have > OnAcceleratorRegistered, can we change the name back to > OnAcceleratorsRegistered? That would be shorter and more consistent with the > naming elsewhere. Also, can you please move it to before > OnAcceleratorUnregistered? Done, ya this name is shorter. https://codereview.chromium.org/2586333003/diff/500001/ui/base/accelerators/a... File ui/base/accelerators/accelerator_manager_unittest.cc (right): https://codereview.chromium.org/2586333003/diff/500001/ui/base/accelerators/a... ui/base/accelerators/accelerator_manager_unittest.cc:75: // . OnAcceleratorVectorRegistered() -> 'Register a list of ' + ids On 2017/01/26 21:39:49, mfomitchev wrote: > This doesn't seem to match what's implemented in OnAcceleratorVectorRegistered. > > We should also either change the existing "Register" test to register multiple > accelerators at once, or add a new test for that. Currently only "Unregister" > tests adding multiple accelerators at once, which doesn't make a lot of sense. Done. I modified "Register" test to add a vector of 4 accelerators. The unregister accelerators is already done in "Unregister" test so I don't duplicate here.
The CQ bit was checked by thanhph@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks! LGTM with nits. https://codereview.chromium.org/2586333003/diff/560001/ash/common/accelerator... File ash/common/accelerators/accelerator_controller.cc (right): https://codereview.chromium.org/2586333003/diff/560001/ash/common/accelerator... ash/common/accelerators/accelerator_controller.cc:768: std::vector<ui::Accelerator> accelerators; nit: ui_accelerators for consistency https://codereview.chromium.org/2586333003/diff/560001/ash/mus/accelerators/a... File ash/mus/accelerators/accelerator_controller_registrar.cc (right): https://codereview.chromium.org/2586333003/diff/560001/ash/mus/accelerators/a... ash/mus/accelerators/accelerator_controller_registrar.cc:23: void OnAcceleratorVectorAdded(const std::vector<ui::Accelerator>& accelerators, Nit: rename OnAcceleratorsAdded for consistency. https://codereview.chromium.org/2586333003/diff/560001/ui/base/accelerators/a... File ui/base/accelerators/accelerator_manager_unittest.cc (right): https://codereview.chromium.org/2586333003/diff/560001/ui/base/accelerators/a... ui/base/accelerators/accelerator_manager_unittest.cc:74: // . OnAcceleratorsRegistered() -> A list of 'Register ' + id Nit: A list of "'Register ' + <id>" separated by whitespaces. https://codereview.chromium.org/2586333003/diff/560001/ui/base/accelerators/a... ui/base/accelerators/accelerator_manager_unittest.cc:129: TestTarget target; Would be good to similarly update the Register test in accelerator_controller_unittest.cc (both mus and non-mus versions).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by thanhph@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
thanhph@google.com changed reviewers: + sky@chromium.org - sadrul@chromium.org
Thanks Mikhail. Hi Scott, Please review my cl. Thanks, Thanh. https://codereview.chromium.org/2586333003/diff/560001/ash/common/accelerator... File ash/common/accelerators/accelerator_controller.cc (right): https://codereview.chromium.org/2586333003/diff/560001/ash/common/accelerator... ash/common/accelerators/accelerator_controller.cc:768: std::vector<ui::Accelerator> accelerators; On 2017/01/26 23:13:57, mfomitchev wrote: > nit: ui_accelerators for consistency Done. https://codereview.chromium.org/2586333003/diff/560001/ash/mus/accelerators/a... File ash/mus/accelerators/accelerator_controller_registrar.cc (right): https://codereview.chromium.org/2586333003/diff/560001/ash/mus/accelerators/a... ash/mus/accelerators/accelerator_controller_registrar.cc:23: void OnAcceleratorVectorAdded(const std::vector<ui::Accelerator>& accelerators, On 2017/01/26 23:13:57, mfomitchev wrote: > Nit: rename OnAcceleratorsAdded for consistency. Done. https://codereview.chromium.org/2586333003/diff/560001/ui/base/accelerators/a... File ui/base/accelerators/accelerator_manager_unittest.cc (right): https://codereview.chromium.org/2586333003/diff/560001/ui/base/accelerators/a... ui/base/accelerators/accelerator_manager_unittest.cc:74: // . OnAcceleratorsRegistered() -> A list of 'Register ' + id On 2017/01/26 23:13:57, mfomitchev wrote: > Nit: A list of "'Register ' + <id>" separated by whitespaces. Done. https://codereview.chromium.org/2586333003/diff/560001/ui/base/accelerators/a... ui/base/accelerators/accelerator_manager_unittest.cc:129: TestTarget target; On 2017/01/26 23:13:57, mfomitchev wrote: > Would be good to similarly update the Register test in > accelerator_controller_unittest.cc (both mus and non-mus versions). Done.
https://codereview.chromium.org/2586333003/diff/580001/ui/base/accelerators/a... File ui/base/accelerators/accelerator_manager.h (right): https://codereview.chromium.org/2586333003/diff/580001/ui/base/accelerators/a... ui/base/accelerators/accelerator_manager.h:52: HandlerPriority priority, Each accelerator may want a different priority, so that Register should take a vector of accelerator and priority pairs.
https://codereview.chromium.org/2586333003/diff/580001/ui/base/accelerators/a... File ui/base/accelerators/accelerator_manager.h (right): https://codereview.chromium.org/2586333003/diff/580001/ui/base/accelerators/a... ui/base/accelerators/accelerator_manager.h:52: HandlerPriority priority, On 2017/01/27 19:11:14, sky wrote: > Each accelerator may want a different priority, so that Register should take a > vector of accelerator and priority pairs. We don't currently have a use case for registering a bunch of accelerators at once with different priorities. Do you think we need to design for this now?
https://codereview.chromium.org/2586333003/diff/580001/ash/mus/accelerators/a... File ash/mus/accelerators/accelerator_controller_registrar.cc (right): https://codereview.chromium.org/2586333003/diff/580001/ash/mus/accelerators/a... ash/mus/accelerators/accelerator_controller_registrar.cc:104: for (auto iter = accelerators.begin(); iter != accelerators.end(); ++iter) for (const ui::Accelerator& accelerator : accelerators) https://codereview.chromium.org/2586333003/diff/580001/services/ui/ws/event_d... File services/ui/ws/event_dispatcher.cc (right): https://codereview.chromium.org/2586333003/diff/580001/services/ui/ws/event_d... services/ui/ws/event_dispatcher.cc:257: bool is_duplicate = false; I think this would be easier to follow if you remove the locals and have separate logs in each branch. https://codereview.chromium.org/2586333003/diff/580001/services/ui/ws/event_d... services/ui/ws/event_dispatcher.cc:268: LOG(ERROR) << error_message << " accelerator id=" << accelerator->id() As this isn't fatal, use DVLOG(1) (which is what mus does for similar errors, see WindowTree). https://codereview.chromium.org/2586333003/diff/580001/ui/base/accelerators/a... File ui/base/accelerators/accelerator_manager.cc (right): https://codereview.chromium.org/2586333003/diff/580001/ui/base/accelerators/a... ui/base/accelerators/accelerator_manager.cc:28: for (auto iter = accelerators.begin(); iter != accelerators.end(); ++iter) { for (const ui::Accelerator& accelerator : accelerators) is easier to read and shorter. https://codereview.chromium.org/2586333003/diff/580001/ui/base/accelerators/a... File ui/base/accelerators/accelerator_manager.h (right): https://codereview.chromium.org/2586333003/diff/580001/ui/base/accelerators/a... ui/base/accelerators/accelerator_manager.h:52: HandlerPriority priority, On 2017/01/27 23:12:01, mfomitchev wrote: > On 2017/01/27 19:11:14, sky wrote: > > Each accelerator may want a different priority, so that Register should take a > > vector of accelerator and priority pairs. > > We don't currently have a use case for registering a bunch of accelerators at > once with different priorities. Do you think we need to design for this now? Given we don't have a need for it now, I'm ok with that is here. One suggestion though. Given most sites only care about registering one accelerator at a time, how about providing a function with the same name that is inlined and calls the vector version? https://codereview.chromium.org/2586333003/diff/580001/ui/base/accelerators/a... File ui/base/accelerators/accelerator_manager_delegate.h (right): https://codereview.chromium.org/2586333003/diff/580001/ui/base/accelerators/a... ui/base/accelerators/accelerator_manager_delegate.h:16: // Called when a number of accelerators are registered at once. An accelerator How about: Called when new accelerators are registered. This is only called with newly registered accelerators. For example, if Register() is called with A and B, then OnAcceleratorsRegistered() is called with A and B. If Register() is subsequently called with A and C, then OnAcceleratorsRegistered() is only called with C, as A was already registered. https://codereview.chromium.org/2586333003/diff/580001/ui/base/accelerators/a... File ui/base/accelerators/accelerator_manager_unittest.cc (right): https://codereview.chromium.org/2586333003/diff/580001/ui/base/accelerators/a... ui/base/accelerators/accelerator_manager_unittest.cc:97: for (auto iter = accelerators.begin(); iter != accelerators.end(); ++iter) { Similar comment about for (const Accelerator& accelerator : accelerators And note you don't need ui:: in this file.
Also, generally chromium-reviews is automatically added to the cc list for all reviews. If you didn't explicitly remove it maybe there is a configuration issue?
The CQ bit was checked by thanhph@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks a lot Scott. I think I removed the chromium-reviews for the first few cls and forgot to put it back. I'll just leave it there next time for all reviews. Please review my new cl. Thanks, Thanh. https://codereview.chromium.org/2586333003/diff/580001/ash/mus/accelerators/a... File ash/mus/accelerators/accelerator_controller_registrar.cc (right): https://codereview.chromium.org/2586333003/diff/580001/ash/mus/accelerators/a... ash/mus/accelerators/accelerator_controller_registrar.cc:104: for (auto iter = accelerators.begin(); iter != accelerators.end(); ++iter) On 2017/01/28 00:09:15, sky wrote: > for (const ui::Accelerator& accelerator : accelerators) Great, thanks! https://codereview.chromium.org/2586333003/diff/580001/services/ui/ws/event_d... File services/ui/ws/event_dispatcher.cc (right): https://codereview.chromium.org/2586333003/diff/580001/services/ui/ws/event_d... services/ui/ws/event_dispatcher.cc:257: bool is_duplicate = false; On 2017/01/28 00:09:15, sky wrote: > I think this would be easier to follow if you remove the locals and have > separate logs in each branch. Done. https://codereview.chromium.org/2586333003/diff/580001/services/ui/ws/event_d... services/ui/ws/event_dispatcher.cc:268: LOG(ERROR) << error_message << " accelerator id=" << accelerator->id() On 2017/01/28 00:09:15, sky wrote: > As this isn't fatal, use DVLOG(1) (which is what mus does for similar errors, > see WindowTree). Done. https://codereview.chromium.org/2586333003/diff/580001/ui/base/accelerators/a... File ui/base/accelerators/accelerator_manager.cc (right): https://codereview.chromium.org/2586333003/diff/580001/ui/base/accelerators/a... ui/base/accelerators/accelerator_manager.cc:28: for (auto iter = accelerators.begin(); iter != accelerators.end(); ++iter) { On 2017/01/28 00:09:15, sky wrote: > for (const ui::Accelerator& accelerator : accelerators) > is easier to read and shorter. Done, thanks! https://codereview.chromium.org/2586333003/diff/580001/ui/base/accelerators/a... File ui/base/accelerators/accelerator_manager.h (right): https://codereview.chromium.org/2586333003/diff/580001/ui/base/accelerators/a... ui/base/accelerators/accelerator_manager.h:52: HandlerPriority priority, On 2017/01/28 00:09:15, sky wrote: > On 2017/01/27 23:12:01, mfomitchev wrote: > > On 2017/01/27 19:11:14, sky wrote: > > > Each accelerator may want a different priority, so that Register should take > a > > > vector of accelerator and priority pairs. > > > > We don't currently have a use case for registering a bunch of accelerators at > > once with different priorities. Do you think we need to design for this now? > > Given we don't have a need for it now, I'm ok with that is here. > > One suggestion though. Given most sites only care about registering one > accelerator at a time, how about providing a function with the same name that is > inlined and calls the vector version? I can't use the same name Register for inline function without extra factorization because the call to Register({accelerator}, priority, target) (also in unit tests) points the inline function instead of expected vector version and this results to infinitive recursion calls. I use the name RegisterAccelerator instead. If we keep the inline Register name, I need to change from {accelerator} to std::vector<ui::Accelerator>(accelerator), which is quite wordy. https://codereview.chromium.org/2586333003/diff/580001/ui/base/accelerators/a... File ui/base/accelerators/accelerator_manager_delegate.h (right): https://codereview.chromium.org/2586333003/diff/580001/ui/base/accelerators/a... ui/base/accelerators/accelerator_manager_delegate.h:16: // Called when a number of accelerators are registered at once. An accelerator On 2017/01/28 00:09:15, sky wrote: > How about: > Called when new accelerators are registered. This is only called with > newly registered accelerators. For example, if Register() is > called with A and B, then OnAcceleratorsRegistered() is called with A and B. If > Register() is subsequently called with A and C, then OnAcceleratorsRegistered() > is only called with C, as A was already registered. Done, thanks! This is clear. https://codereview.chromium.org/2586333003/diff/580001/ui/base/accelerators/a... File ui/base/accelerators/accelerator_manager_unittest.cc (right): https://codereview.chromium.org/2586333003/diff/580001/ui/base/accelerators/a... ui/base/accelerators/accelerator_manager_unittest.cc:97: for (auto iter = accelerators.begin(); iter != accelerators.end(); ++iter) { On 2017/01/28 00:09:15, sky wrote: > Similar comment about for (const Accelerator& accelerator : accelerators > > And note you don't need ui:: in this file. Done, thanks!
LGTM https://codereview.chromium.org/2586333003/diff/600001/ui/base/accelerators/a... File ui/base/accelerators/accelerator_manager.h (right): https://codereview.chromium.org/2586333003/diff/600001/ui/base/accelerators/a... ui/base/accelerators/accelerator_manager.h:55: // Register a keyboard accelerator for the specified target. This function Register->Registers https://codereview.chromium.org/2586333003/diff/600001/ui/base/accelerators/a... ui/base/accelerators/accelerator_manager.h:56: // calls the function Register with vector argument above. It's fairly common when referring to functions in comments to use (), e.g. Register(). https://codereview.chromium.org/2586333003/diff/600001/ui/base/accelerators/a... ui/base/accelerators/accelerator_manager.h:57: inline void RegisterAccelerator(const Accelerator& accelerator, Put the implementation here too. https://codereview.chromium.org/2586333003/diff/600001/ui/base/accelerators/a... File ui/base/accelerators/accelerator_manager_delegate.h (right): https://codereview.chromium.org/2586333003/diff/600001/ui/base/accelerators/a... ui/base/accelerators/accelerator_manager_delegate.h:19: // B. If Join this line and the next? https://codereview.chromium.org/2586333003/diff/600001/ui/base/accelerators/a... ui/base/accelerators/accelerator_manager_delegate.h:21: // OnAcceleratorsRegistered() Join this line and the next?
The CQ bit was unchecked by thanhph@google.com
Thanks Scott. CQ job is restarted. Thanh. https://codereview.chromium.org/2586333003/diff/600001/ui/base/accelerators/a... File ui/base/accelerators/accelerator_manager.h (right): https://codereview.chromium.org/2586333003/diff/600001/ui/base/accelerators/a... ui/base/accelerators/accelerator_manager.h:55: // Register a keyboard accelerator for the specified target. This function On 2017/01/30 17:13:31, sky wrote: > Register->Registers Done. https://codereview.chromium.org/2586333003/diff/600001/ui/base/accelerators/a... ui/base/accelerators/accelerator_manager.h:56: // calls the function Register with vector argument above. On 2017/01/30 17:13:31, sky wrote: > It's fairly common when referring to functions in comments to use (), e.g. > Register(). Done, thanks! https://codereview.chromium.org/2586333003/diff/600001/ui/base/accelerators/a... ui/base/accelerators/accelerator_manager.h:57: inline void RegisterAccelerator(const Accelerator& accelerator, On 2017/01/30 17:13:31, sky wrote: > Put the implementation here too. Done. https://codereview.chromium.org/2586333003/diff/600001/ui/base/accelerators/a... File ui/base/accelerators/accelerator_manager_delegate.h (right): https://codereview.chromium.org/2586333003/diff/600001/ui/base/accelerators/a... ui/base/accelerators/accelerator_manager_delegate.h:19: // B. If On 2017/01/30 17:13:31, sky wrote: > Join this line and the next? Done. https://codereview.chromium.org/2586333003/diff/600001/ui/base/accelerators/a... ui/base/accelerators/accelerator_manager_delegate.h:21: // OnAcceleratorsRegistered() On 2017/01/30 17:13:31, sky wrote: > Join this line and the next? Done, thanks!
The CQ bit was checked by thanhph@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by thanhph@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from mfomitchev@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/2586333003/#ps620001 (title: "Fix comments. Move inline function to .h")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 620001, "attempt_start_ts": 1485803399013330,
"parent_rev": "6886fb428da9df7e982ba3d7255a238b01290870", "commit_rev":
"edbf3f7ee62a25e213036c80b3c28ff6311a0dab"}
Message was sent while issue was closed.
Description was changed from ========== Make mash register initial batch of accelerators in single shot. Add OnAcceleratorsRegistered to register all accelerators during startup with single IPC. BUG=632050 ========== to ========== 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} Committed: https://chromium.googlesource.com/chromium/src/+/edbf3f7ee62a25e213036c80b3c2... ==========
Message was sent while issue was closed.
Committed patchset #25 (id:620001) as https://chromium.googlesource.com/chromium/src/+/edbf3f7ee62a25e213036c80b3c2... |
