Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2520093003:
WindowManagerClient::AddAccelerator() should take an array (Closed)
DescriptionWindowManagerClient::AddAccelerator() should take an array.
To reduce IPC calls between window_tree_client.cc and window_tree.cc.
BUG=632049
Committed: https://crrev.com/62cbfb83c3ad3a8244d063d297a81fe6d9a32866
Cr-Commit-Position: refs/heads/master@{#436115}
Patch Set 1 #Patch Set 2 : Combine two arguments of Accelerator in a struct. #Patch Set 3 : Fix unittests to be compatible with the struct declaration. #Patch Set 4 : Use array for AddAccelerators. #Patch Set 5 : run cl format #Patch Set 6 : Fix unittest to use array of accelerators. #Patch Set 7 : Forget to add these files to previous patch. #
Total comments: 18
Patch Set 8 : Cleanup/Update format. #Patch Set 9 : mojo::Array has changed to std::vector due to this yesterday https://codereview.chromium.org/251188… #Patch Set 10 : Change ui/aura/mus/window_tree_client.cc for the same above reason. #Patch Set 11 : Change mojo::Array to std::vector. #Patch Set 12 : Edit/format function/names. Add all accelerators in the array instead of first sucessfully added no… #Patch Set 13 : Create anonymous namespace helper for AcceleratorTransport mojom struct. Cleanup/format codes. #
Total comments: 14
Patch Set 14 : Modify helper to return vector and leverage helper in other files.Rename/format code. #
Total comments: 20
Patch Set 15 : Rename/refactor code. Add unit test for multiple accelerators. #
Total comments: 17
Patch Set 16 : Refactor code, reduce code size, change namespace. #
Total comments: 6
Patch Set 17 : Add CreateAcceleratorTransport and rename helper to CreateAcceleratorVector. Add one more test case… #
Total comments: 18
Patch Set 18 : Add/Update comments. Improve duplication accelerator test. #
Total comments: 2
Patch Set 19 : Rename AcceleratorTransport to Accelerator, CreateAcceleratorTransport to CreateAccelerator. #Patch Set 20 : Search/Replace to transport in accelerator_transport. #
Total comments: 2
Patch Set 21 : Modify test to cover more cases. Edit comments. #Patch Set 22 : Adding 1 test that has two unique and one duped accelerators. #
Total comments: 27
Patch Set 23 : Refactor codes, edit comments. #Patch Set 24 : Re-upload due to build.gn conflict. #
Total comments: 4
Patch Set 25 : Improve comments. #
Total comments: 10
Patch Set 26 : Modify comments, use MakeUnique. #Patch Set 27 : Use AddAccelerator() in AddAccelerators(). #
Total comments: 2
Patch Set 28 : Add comment. #
Total comments: 2
Patch Set 29 : Leave unit test EventDispatcherTest.AddMultipleAccelerators out for next bug 632050. Remove functio… #
Total comments: 2
Patch Set 30 : Use std::move instead of Clone() #Messages
Total messages: 154 (93 generated)
Description was changed from ========== Try to put 2 arguments in a struct for AddAccelerator. BUG=632049 ========== to ========== To reduce IPC calls between window_tree_client.cc and window_tree.cc BUG=632049 ==========
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: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) 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: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) 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: 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...
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 commit-bot@chromium.org
Dry run: Try jobs failed on following builders: blimp_linux_dbg on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/blimp_linux_dbg...)
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...
thanhph@chromium.org changed reviewers: + mfomitchev@google.com
Hi Mikhail, I need to change several files (existing unit tests and other related depending files to make WindowManagerClient::AddAccelerator() use an array as inputs. All commit bots were happy at some point. Let me know your input. I'm still working on cleaning this up. Thanks, Thanh.
thanhph@chromium.org changed reviewers: + mfomitchev@chromium.org - mfomitchev@google.com
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_...)
https://codereview.chromium.org/2520093003/diff/120001/ash/mus/accelerators/a... File ash/mus/accelerators/accelerator_controller_registrar.cc (right): https://codereview.chromium.org/2520093003/diff/120001/ash/mus/accelerators/a... ash/mus/accelerators/accelerator_controller_registrar.cc:128: ui::mojom::AcceleratorEventPtr accelerator_pre_event( This doesn't make much sense. You are just creating an array containing a single accelerator and still doing one mojo IPC per accelerator, just wrapping it in an array. https://codereview.chromium.org/2520093003/diff/120001/services/ui/public/cpp... File services/ui/public/cpp/window_manager_delegate.h (right): https://codereview.chromium.org/2520093003/diff/120001/services/ui/public/cpp... services/ui/public/cpp/window_manager_delegate.h:51: mojo::Array<mojom::AcceleratorEventPtr> accelerator_event, nit: Use plural form to make it a bit more clear what the var stands for: AddAccelerators, accelerator_events, etc. https://codereview.chromium.org/2520093003/diff/120001/services/ui/public/cpp... File services/ui/public/cpp/window_tree_client.cc (right): https://codereview.chromium.org/2520093003/diff/120001/services/ui/public/cpp... services/ui/public/cpp/window_tree_client.cc:1439: void WindowTreeClient::AddAccelerator( nit: AddAccelerators https://codereview.chromium.org/2520093003/diff/120001/services/ui/public/cpp... services/ui/public/cpp/window_tree_client.cc:1440: mojo::Array<mojom::AcceleratorEventPtr> multi_accelerators, Try to be consistent in naming: you are using accelerator_events elsewhere, so use the same name here. https://codereview.chromium.org/2520093003/diff/120001/services/ui/public/int... File services/ui/public/interfaces/window_manager.mojom (right): https://codereview.chromium.org/2520093003/diff/120001/services/ui/public/int... services/ui/public/interfaces/window_manager.mojom:141: struct AcceleratorEvent{ "AcceleratorEvent" implies to me "event that triggered accelerator". This looks like just an Accelerator, so call it that. Or AcceleratorTransport if you want to have a different name than Accelerator in ui::ws https://codereview.chromium.org/2520093003/diff/120001/services/ui/ws/event_d... File services/ui/ws/event_dispatcher.cc (left): https://codereview.chromium.org/2520093003/diff/120001/services/ui/ws/event_d... services/ui/ws/event_dispatcher.cc:250: // If an accelerator with the same id or matcher already exists, then abort. Update the comment, but don't remove it please https://codereview.chromium.org/2520093003/diff/120001/services/ui/ws/event_d... File services/ui/ws/event_dispatcher.cc (right): https://codereview.chromium.org/2520093003/diff/120001/services/ui/ws/event_d... services/ui/ws/event_dispatcher.cc:254: bool does_accelerator_exist = false; accelerator_exists https://codereview.chromium.org/2520093003/diff/120001/services/ui/ws/event_d... services/ui/ws/event_dispatcher.cc:255: for (const auto& pair : accelerators_) You need braces for multi-line body https://codereview.chromium.org/2520093003/diff/120001/services/ui/ws/event_d... services/ui/ws/event_dispatcher.cc:264: return true; This is a bit too early to return - you've just inserted one accelerator, you need to iterate through all of them.
https://codereview.chromium.org/2520093003/diff/120001/ash/mus/accelerators/a... File ash/mus/accelerators/accelerator_controller_registrar.cc (right): https://codereview.chromium.org/2520093003/diff/120001/ash/mus/accelerators/a... ash/mus/accelerators/accelerator_controller_registrar.cc:128: ui::mojom::AcceleratorEventPtr accelerator_pre_event( On 2016/11/22 22:44:16, mfomitchev wrote: > This doesn't make much sense. You are just creating an array containing a single > accelerator and still doing one mojo IPC per accelerator, just wrapping it in an > array. Nvm, I guess you have to do this b/c you removed the mojom method for registering a single accelerator. I think it's reasonable. Just create a helper method in the anonymous namespace (in this cc file) which creates a mojo::Array<ui::mojom::AcceleratorEventPtr> out of id and event matcher, so that you don't duplicate the code. And please use consistent names.
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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
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_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 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 commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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 commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Thanks Mikhail. Commit bots were mostly happy. Please review my new cl. While adding multiple accelerators, only 1 Boolean success is allowed to return which indicates true if all accelerators were not in the pool and false if any of the accelerators is already in the pool. https://codereview.chromium.org/2520093003/diff/120001/ash/mus/accelerators/a... File ash/mus/accelerators/accelerator_controller_registrar.cc (right): https://codereview.chromium.org/2520093003/diff/120001/ash/mus/accelerators/a... ash/mus/accelerators/accelerator_controller_registrar.cc:128: ui::mojom::AcceleratorEventPtr accelerator_pre_event( On 2016/11/22 23:07:13, mfomitchev wrote: > On 2016/11/22 22:44:16, mfomitchev wrote: > > This doesn't make much sense. You are just creating an array containing a > single > > accelerator and still doing one mojo IPC per accelerator, just wrapping it in > an > > array. > > Nvm, I guess you have to do this b/c you removed the mojom method for > registering a single accelerator. I think it's reasonable. Just create a helper > method in the anonymous namespace (in this cc file) which creates a > mojo::Array<ui::mojom::AcceleratorEventPtr> out of id and event matcher, so that > you don't duplicate the code. And please use consistent names. Thanks! I created a wrapper around single pair of (id, event_matcher). Let me know how it goes. Should I copy this helper to event_dispatcher_unittest.cc also? https://codereview.chromium.org/2520093003/diff/120001/services/ui/public/cpp... File services/ui/public/cpp/window_manager_delegate.h (right): https://codereview.chromium.org/2520093003/diff/120001/services/ui/public/cpp... services/ui/public/cpp/window_manager_delegate.h:51: mojo::Array<mojom::AcceleratorEventPtr> accelerator_event, On 2016/11/22 22:44:16, mfomitchev wrote: > nit: Use plural form to make it a bit more clear what the var stands for: > AddAccelerators, accelerator_events, etc. Done. I forgot to rename this to multi_accelerators. https://codereview.chromium.org/2520093003/diff/120001/services/ui/public/cpp... File services/ui/public/cpp/window_tree_client.cc (right): https://codereview.chromium.org/2520093003/diff/120001/services/ui/public/cpp... services/ui/public/cpp/window_tree_client.cc:1439: void WindowTreeClient::AddAccelerator( On 2016/11/22 22:44:16, mfomitchev wrote: > nit: AddAccelerators Done. https://codereview.chromium.org/2520093003/diff/120001/services/ui/public/cpp... services/ui/public/cpp/window_tree_client.cc:1440: mojo::Array<mojom::AcceleratorEventPtr> multi_accelerators, On 2016/11/22 22:44:16, mfomitchev wrote: > Try to be consistent in naming: you are using accelerator_events elsewhere, so > use the same name here. Done, I use multi_accelerators as the main name. https://codereview.chromium.org/2520093003/diff/120001/services/ui/public/int... File services/ui/public/interfaces/window_manager.mojom (right): https://codereview.chromium.org/2520093003/diff/120001/services/ui/public/int... services/ui/public/interfaces/window_manager.mojom:141: struct AcceleratorEvent{ On 2016/11/22 22:44:16, mfomitchev wrote: > "AcceleratorEvent" implies to me "event that triggered accelerator". This looks > like just an Accelerator, so call it that. Or AcceleratorTransport if you want > to have a different name than Accelerator in ui::ws Done, I use AcceleratorTransport to avoid name conflict/confusion during mass renaming. https://codereview.chromium.org/2520093003/diff/120001/services/ui/ws/event_d... File services/ui/ws/event_dispatcher.cc (right): https://codereview.chromium.org/2520093003/diff/120001/services/ui/ws/event_d... services/ui/ws/event_dispatcher.cc:254: bool does_accelerator_exist = false; On 2016/11/22 22:44:16, mfomitchev wrote: > accelerator_exists Done. https://codereview.chromium.org/2520093003/diff/120001/services/ui/ws/event_d... services/ui/ws/event_dispatcher.cc:255: for (const auto& pair : accelerators_) On 2016/11/22 22:44:16, mfomitchev wrote: > You need braces for multi-line body Done, thanks! https://codereview.chromium.org/2520093003/diff/120001/services/ui/ws/event_d... services/ui/ws/event_dispatcher.cc:264: return true; On 2016/11/22 22:44:16, mfomitchev wrote: > This is a bit too early to return - you've just inserted one accelerator, you > need to iterate through all of them. Done, thanks!
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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
On 2016/11/24 16:10:13, thanhph wrote: > Thanks Mikhail. Commit bots were mostly happy. Please review my new cl. While > adding multiple accelerators, only 1 Boolean success is allowed to return which > indicates true if all accelerators were not in the pool and false if any of the > accelerators is already in the pool. > > https://codereview.chromium.org/2520093003/diff/120001/ash/mus/accelerators/a... > File ash/mus/accelerators/accelerator_controller_registrar.cc (right): > > https://codereview.chromium.org/2520093003/diff/120001/ash/mus/accelerators/a... > ash/mus/accelerators/accelerator_controller_registrar.cc:128: > ui::mojom::AcceleratorEventPtr accelerator_pre_event( > On 2016/11/22 23:07:13, mfomitchev wrote: > > On 2016/11/22 22:44:16, mfomitchev wrote: > > > This doesn't make much sense. You are just creating an array containing a > > single > > > accelerator and still doing one mojo IPC per accelerator, just wrapping it > in > > an > > > array. > > > > Nvm, I guess you have to do this b/c you removed the mojom method for > > registering a single accelerator. I think it's reasonable. Just create a > helper > > method in the anonymous namespace (in this cc file) which creates a > > mojo::Array<ui::mojom::AcceleratorEventPtr> out of id and event matcher, so > that > > you don't duplicate the code. And please use consistent names. > > Thanks! I created a wrapper around single pair of (id, event_matcher). Let me > know how it goes. Should I copy this helper to event_dispatcher_unittest.cc > also? > > https://codereview.chromium.org/2520093003/diff/120001/services/ui/public/cpp... > File services/ui/public/cpp/window_manager_delegate.h (right): > > https://codereview.chromium.org/2520093003/diff/120001/services/ui/public/cpp... > services/ui/public/cpp/window_manager_delegate.h:51: > mojo::Array<mojom::AcceleratorEventPtr> accelerator_event, > On 2016/11/22 22:44:16, mfomitchev wrote: > > nit: Use plural form to make it a bit more clear what the var stands for: > > AddAccelerators, accelerator_events, etc. > > Done. I forgot to rename this to multi_accelerators. > > https://codereview.chromium.org/2520093003/diff/120001/services/ui/public/cpp... > File services/ui/public/cpp/window_tree_client.cc (right): > > https://codereview.chromium.org/2520093003/diff/120001/services/ui/public/cpp... > services/ui/public/cpp/window_tree_client.cc:1439: void > WindowTreeClient::AddAccelerator( > On 2016/11/22 22:44:16, mfomitchev wrote: > > nit: AddAccelerators > > Done. > > https://codereview.chromium.org/2520093003/diff/120001/services/ui/public/cpp... > services/ui/public/cpp/window_tree_client.cc:1440: > mojo::Array<mojom::AcceleratorEventPtr> multi_accelerators, > On 2016/11/22 22:44:16, mfomitchev wrote: > > Try to be consistent in naming: you are using accelerator_events elsewhere, so > > use the same name here. > > Done, I use multi_accelerators as the main name. > > https://codereview.chromium.org/2520093003/diff/120001/services/ui/public/int... > File services/ui/public/interfaces/window_manager.mojom (right): > > https://codereview.chromium.org/2520093003/diff/120001/services/ui/public/int... > services/ui/public/interfaces/window_manager.mojom:141: struct AcceleratorEvent{ > On 2016/11/22 22:44:16, mfomitchev wrote: > > "AcceleratorEvent" implies to me "event that triggered accelerator". This > looks > > like just an Accelerator, so call it that. Or AcceleratorTransport if you want > > to have a different name than Accelerator in ui::ws > > Done, I use AcceleratorTransport to avoid name conflict/confusion during mass > renaming. > > https://codereview.chromium.org/2520093003/diff/120001/services/ui/ws/event_d... > File services/ui/ws/event_dispatcher.cc (right): > > https://codereview.chromium.org/2520093003/diff/120001/services/ui/ws/event_d... > services/ui/ws/event_dispatcher.cc:254: bool does_accelerator_exist = false; > On 2016/11/22 22:44:16, mfomitchev wrote: > > accelerator_exists > > Done. > > https://codereview.chromium.org/2520093003/diff/120001/services/ui/ws/event_d... > services/ui/ws/event_dispatcher.cc:255: for (const auto& pair : accelerators_) > On 2016/11/22 22:44:16, mfomitchev wrote: > > You need braces for multi-line body > > Done, thanks! > > https://codereview.chromium.org/2520093003/diff/120001/services/ui/ws/event_d... > services/ui/ws/event_dispatcher.cc:264: return true; > On 2016/11/22 22:44:16, mfomitchev wrote: > > This is a bit too early to return - you've just inserted one accelerator, you > > need to iterate through all of them. > > Done, thanks! Actually the helper class can be used in other files. Is there a file that I can put this helper in, then include this file where I want to use the helper?
https://codereview.chromium.org/2520093003/diff/230001/ash/mus/accelerators/a... File ash/mus/accelerators/accelerator_controller_registrar.cc (right): https://codereview.chromium.org/2520093003/diff/230001/ash/mus/accelerators/a... ash/mus/accelerators/accelerator_controller_registrar.cc:21: namespace { Just use the existing anonymous namespace in ash::mus below. https://codereview.chromium.org/2520093003/diff/230001/ash/mus/accelerators/a... ash/mus/accelerators/accelerator_controller_registrar.cc:142: AcceleratorTransportWrapper pre_accelerator_transport_wrapper( I don't think this struct helps us much. Why not just create a method which would return std::vector<ui::mojom::AcceleratorTransportPtr> (i.e. whatever you need to pass to AddAccelerators())? https://codereview.chromium.org/2520093003/diff/230001/services/ui/public/cpp... File services/ui/public/cpp/window_manager_delegate.h (right): https://codereview.chromium.org/2520093003/diff/230001/services/ui/public/cpp... services/ui/public/cpp/window_manager_delegate.h:51: std::vector<mojom::AcceleratorTransportPtr> multi_accelerators, I don't think "multi_" is providing value. The plural form already tells us that there are many accelerators. Is there any reason we shouldn't rename this to "accelerators"? Same goes to every other place "multi_" is used in this CL. https://codereview.chromium.org/2520093003/diff/230001/services/ui/ws/event_d... File services/ui/ws/event_dispatcher.cc (right): https://codereview.chromium.org/2520093003/diff/230001/services/ui/ws/event_d... services/ui/ws/event_dispatcher.cc:255: // If an accelerator with the same id or matcher is already in the pool, If an accelerator with the same id or matcher already exists, skip it. https://codereview.chromium.org/2520093003/diff/230001/services/ui/ws/event_d... services/ui/ws/event_dispatcher.cc:265: if (!accelerator_exists) Swap if/else: start with a true condition, so that that there's no double negatives. https://codereview.chromium.org/2520093003/diff/230001/services/ui/ws/event_d... File services/ui/ws/event_dispatcher.h (right): https://codereview.chromium.org/2520093003/diff/230001/services/ui/ws/event_d... services/ui/ws/event_dispatcher.h:126: // Adds an accelerator with the given id and event-matcher. If an accelerator Please update the comment
https://codereview.chromium.org/2520093003/diff/230001/services/ui/ws/event_d... File services/ui/ws/event_dispatcher_unittest.cc (right): https://codereview.chromium.org/2520093003/diff/230001/services/ui/ws/event_d... services/ui/ws/event_dispatcher_unittest.cc:389: ui::mojom::AcceleratorTransportPtr accelerator_transport( Try to get rid of the duplicated code here, e.g. by putting it into a method. This code block appears over and over. In general, whenever you find yourself writing the same code more than once - this is a signal that you should consider putting it into a function or try to find some other way to remove duplication.
https://codereview.chromium.org/2520093003/diff/230001/ash/mus/accelerators/a... File ash/mus/accelerators/accelerator_controller_registrar.cc (right): https://codereview.chromium.org/2520093003/diff/230001/ash/mus/accelerators/a... ash/mus/accelerators/accelerator_controller_registrar.cc:21: namespace { On 2016/11/24 19:56:13, mfomitchev wrote: > Just use the existing anonymous namespace in ash::mus below. I put this helper in different file accelerator_transport_util.h under namespace ash::mus since it's used in unit test as well. https://codereview.chromium.org/2520093003/diff/230001/ash/mus/accelerators/a... ash/mus/accelerators/accelerator_controller_registrar.cc:142: AcceleratorTransportWrapper pre_accelerator_transport_wrapper( On 2016/11/24 19:56:13, mfomitchev wrote: > I don't think this struct helps us much. Why not just create a method which > would return std::vector<ui::mojom::AcceleratorTransportPtr> (i.e. whatever you > need to pass to AddAccelerators())? Done. Please review the accelerator_transport_util.cc. https://codereview.chromium.org/2520093003/diff/230001/services/ui/public/cpp... File services/ui/public/cpp/window_manager_delegate.h (right): https://codereview.chromium.org/2520093003/diff/230001/services/ui/public/cpp... services/ui/public/cpp/window_manager_delegate.h:51: std::vector<mojom::AcceleratorTransportPtr> multi_accelerators, On 2016/11/24 19:56:13, mfomitchev wrote: > I don't think "multi_" is providing value. The plural form already tells us that > there are many accelerators. Is there any reason we shouldn't rename this to > "accelerators"? Same goes to every other place "multi_" is used in this CL. Done. https://codereview.chromium.org/2520093003/diff/230001/services/ui/ws/event_d... File services/ui/ws/event_dispatcher.cc (right): https://codereview.chromium.org/2520093003/diff/230001/services/ui/ws/event_d... services/ui/ws/event_dispatcher.cc:255: // If an accelerator with the same id or matcher is already in the pool, On 2016/11/24 19:56:13, mfomitchev wrote: > If an accelerator with the same id or matcher already exists, skip it. Done. https://codereview.chromium.org/2520093003/diff/230001/services/ui/ws/event_d... services/ui/ws/event_dispatcher.cc:265: if (!accelerator_exists) On 2016/11/24 19:56:13, mfomitchev wrote: > Swap if/else: start with a true condition, so that that there's no double > negatives. Done. https://codereview.chromium.org/2520093003/diff/230001/services/ui/ws/event_d... File services/ui/ws/event_dispatcher.h (right): https://codereview.chromium.org/2520093003/diff/230001/services/ui/ws/event_d... services/ui/ws/event_dispatcher.h:126: // Adds an accelerator with the given id and event-matcher. If an accelerator On 2016/11/24 19:56:13, mfomitchev wrote: > Please update the comment Done. https://codereview.chromium.org/2520093003/diff/230001/services/ui/ws/event_d... File services/ui/ws/event_dispatcher_unittest.cc (right): https://codereview.chromium.org/2520093003/diff/230001/services/ui/ws/event_d... services/ui/ws/event_dispatcher_unittest.cc:389: ui::mojom::AcceleratorTransportPtr accelerator_transport( On 2016/11/24 21:21:04, mfomitchev wrote: > Try to get rid of the duplicated code here, e.g. by putting it into a method. > This code block appears over and over. In general, whenever you find yourself > writing the same code more than once - this is a signal that you should consider > putting it into a function or try to find some other way to remove duplication. Done, thanks! I use the helper defined in accelerator_transport_util.h.
https://codereview.chromium.org/2520093003/diff/250001/ash/mus/accelerators/a... File ash/mus/accelerators/accelerator_controller_registrar.cc (right): https://codereview.chromium.org/2520093003/diff/250001/ash/mus/accelerators/a... ash/mus/accelerators/accelerator_controller_registrar.cc:132: AddAcceleratorHelper(std::move(pre_accelerators), You are never passing a non-empty vector to AddAcceleratorHelper. So perhaps just make it take id and matcher and return an std::vector<ui::mojom::AcceleratorTransportPtr>? If do need to add accelerators to non-empty vector: Make AddAcceleratorHelper take a pointer to std::vector<ui::mojom::AcceleratorTransportPtr> and return nothing and make it modify the provided std::vector<ui::mojom::AcceleratorTransportPtr>. This way we don't have to do std::move twice. I find doing std::move a little confusing it terms of semantics, since we are not actually transferring the ownership. https://codereview.chromium.org/2520093003/diff/250001/services/ui/common/acc... File services/ui/common/accelerator_transport_util.cc (right): https://codereview.chromium.org/2520093003/diff/250001/services/ui/common/acc... services/ui/common/accelerator_transport_util.cc:17: accelerator_transport_ptr->event_matcher = event_matcher.Clone(); use std::move instead of Clone() https://codereview.chromium.org/2520093003/diff/250001/services/ui/common/acc... File services/ui/common/accelerator_transport_util.h (right): https://codereview.chromium.org/2520093003/diff/250001/services/ui/common/acc... services/ui/common/accelerator_transport_util.h:5: #ifndef SERVICES_UI_COMMON_ACCELERATOR_TRANSPORT_UTIL_H_ Rather than creating a new file, perhaps rename services/ui/common/event_matcher_util.* to services/ui/common/accelerator_util.*, and put AddAcceleratorHelper() in there https://codereview.chromium.org/2520093003/diff/250001/services/ui/common/acc... services/ui/common/accelerator_transport_util.h:14: // Adding an accelerator to a vector of accelerators. A little more detail would be nice, since there's a lot of args, e.g: Construct accelerator from the provided |id| and |event_matcher| and add it to the provided |accelerators| vector. https://codereview.chromium.org/2520093003/diff/250001/services/ui/public/cpp... File services/ui/public/cpp/window_manager_delegate.h (right): https://codereview.chromium.org/2520093003/diff/250001/services/ui/public/cpp... services/ui/public/cpp/window_manager_delegate.h:17: #include "services/ui/public/interfaces/event_matcher.mojom.h" We don't need this anymore https://codereview.chromium.org/2520093003/diff/250001/services/ui/public/int... File services/ui/public/interfaces/window_manager.mojom (right): https://codereview.chromium.org/2520093003/diff/250001/services/ui/public/int... services/ui/public/interfaces/window_manager.mojom:172: AddAccelerators(array<AcceleratorTransport> accelerators) => (bool success); Document the semantics of the return value, since it's no longer trivial. https://codereview.chromium.org/2520093003/diff/250001/services/ui/ws/event_d... File services/ui/ws/event_dispatcher.h (right): https://codereview.chromium.org/2520093003/diff/250001/services/ui/ws/event_d... services/ui/ws/event_dispatcher.h:17: #include "services/ui/public/interfaces/event_matcher.mojom.h" remove https://codereview.chromium.org/2520093003/diff/250001/services/ui/ws/event_d... services/ui/ws/event_dispatcher.h:126: // iterating through all accelerators, if an accelerator with the same id Adds the supplied accelerators. If an accelerator with the same id or matcher already exists - skip it. Returns true if all accelerators were added successfully. https://codereview.chromium.org/2520093003/diff/250001/services/ui/ws/event_d... File services/ui/ws/event_dispatcher_unittest.cc (right): https://codereview.chromium.org/2520093003/diff/250001/services/ui/ws/event_d... services/ui/ws/event_dispatcher_unittest.cc:381: TEST_F(EventDispatcherTest, AcceleratorBasic) { We need some coverage for adding more than one accelerator at a time. We could test this in AcceleratorBasic, or create a separate test case if AcceleratorBasic gets too big. https://codereview.chromium.org/2520093003/diff/250001/ui/aura/mus/window_tre... File ui/aura/mus/window_tree_client.h (right): https://codereview.chromium.org/2520093003/diff/250001/ui/aura/mus/window_tre... ui/aura/mus/window_tree_client.h:23: #include "services/ui/common/accelerator_transport_util.h" This isn't used in .h
Description was changed from ========== To reduce IPC calls between window_tree_client.cc and window_tree.cc BUG=632049 ========== to ========== WindowManagerClient::AddAccelerator() should take an array. To reduce IPC calls between window_tree_client.cc and window_tree.cc BUG=632049 ==========
Description was changed from ========== WindowManagerClient::AddAccelerator() should take an array. To reduce IPC calls between window_tree_client.cc and window_tree.cc BUG=632049 ========== to ========== WindowManagerClient::AddAccelerator() should take an array. To reduce IPC calls between window_tree_client.cc and window_tree.cc. BUG=632049 ==========
https://codereview.chromium.org/2520093003/diff/250001/ash/mus/accelerators/a... File ash/mus/accelerators/accelerator_controller_registrar.cc (right): https://codereview.chromium.org/2520093003/diff/250001/ash/mus/accelerators/a... ash/mus/accelerators/accelerator_controller_registrar.cc:132: AddAcceleratorHelper(std::move(pre_accelerators), On 2016/11/29 01:55:45, mfomitchev wrote: > You are never passing a non-empty vector to AddAcceleratorHelper. So perhaps > just make it take id and matcher and return an > std::vector<ui::mojom::AcceleratorTransportPtr>? > > If do need to add accelerators to non-empty vector: > Make AddAcceleratorHelper take a pointer to > std::vector<ui::mojom::AcceleratorTransportPtr> and return nothing and make it > modify the provided std::vector<ui::mojom::AcceleratorTransportPtr>. This way we > don't have to do std::move twice. I find doing std::move a little confusing it > terms of semantics, since we are not actually transferring the ownership. Done, thanks! I use the first approach since the later didn't work. When I use std::move to pass std::vector argument to the void AddAcceleratorHelper(std::vector<ui::mojom::AcceleratorTransportPtr>, uint32_t id, ui::mojom::EventMatcherPtr event_matcher), the pointer to std::vector is gone thus I can't reuse the std::vector on the return function. https://codereview.chromium.org/2520093003/diff/250001/services/ui/common/acc... File services/ui/common/accelerator_transport_util.cc (right): https://codereview.chromium.org/2520093003/diff/250001/services/ui/common/acc... services/ui/common/accelerator_transport_util.cc:17: accelerator_transport_ptr->event_matcher = event_matcher.Clone(); On 2016/11/29 01:55:45, mfomitchev wrote: > use std::move instead of Clone() Done. https://codereview.chromium.org/2520093003/diff/250001/services/ui/common/acc... File services/ui/common/accelerator_transport_util.h (right): https://codereview.chromium.org/2520093003/diff/250001/services/ui/common/acc... services/ui/common/accelerator_transport_util.h:5: #ifndef SERVICES_UI_COMMON_ACCELERATOR_TRANSPORT_UTIL_H_ On 2016/11/29 01:55:46, mfomitchev wrote: > Rather than creating a new file, perhaps rename > services/ui/common/event_matcher_util.* to > services/ui/common/accelerator_util.*, and put AddAcceleratorHelper() in there Done, thanks! https://codereview.chromium.org/2520093003/diff/250001/services/ui/common/acc... services/ui/common/accelerator_transport_util.h:14: // Adding an accelerator to a vector of accelerators. On 2016/11/29 01:55:45, mfomitchev wrote: > A little more detail would be nice, since there's a lot of args, e.g: > Construct accelerator from the provided |id| and |event_matcher| and add it to > the provided |accelerators| vector. Done, thanks! https://codereview.chromium.org/2520093003/diff/250001/services/ui/public/cpp... File services/ui/public/cpp/window_manager_delegate.h (right): https://codereview.chromium.org/2520093003/diff/250001/services/ui/public/cpp... services/ui/public/cpp/window_manager_delegate.h:17: #include "services/ui/public/interfaces/event_matcher.mojom.h" On 2016/11/29 01:55:46, mfomitchev wrote: > We don't need this anymore Done, thanks! https://codereview.chromium.org/2520093003/diff/250001/services/ui/public/int... File services/ui/public/interfaces/window_manager.mojom (right): https://codereview.chromium.org/2520093003/diff/250001/services/ui/public/int... services/ui/public/interfaces/window_manager.mojom:172: AddAccelerators(array<AcceleratorTransport> accelerators) => (bool success); On 2016/11/29 01:55:46, mfomitchev wrote: > Document the semantics of the return value, since it's no longer trivial. Done. https://codereview.chromium.org/2520093003/diff/250001/services/ui/ws/event_d... File services/ui/ws/event_dispatcher.h (right): https://codereview.chromium.org/2520093003/diff/250001/services/ui/ws/event_d... services/ui/ws/event_dispatcher.h:17: #include "services/ui/public/interfaces/event_matcher.mojom.h" On 2016/11/29 01:55:46, mfomitchev wrote: > remove Done. https://codereview.chromium.org/2520093003/diff/250001/services/ui/ws/event_d... services/ui/ws/event_dispatcher.h:126: // iterating through all accelerators, if an accelerator with the same id On 2016/11/29 01:55:46, mfomitchev wrote: > Adds the supplied accelerators. If an accelerator with the same id or matcher > already exists - skip it. Returns true if all accelerators were added > successfully. Done. https://codereview.chromium.org/2520093003/diff/250001/services/ui/ws/event_d... File services/ui/ws/event_dispatcher_unittest.cc (right): https://codereview.chromium.org/2520093003/diff/250001/services/ui/ws/event_d... services/ui/ws/event_dispatcher_unittest.cc:381: TEST_F(EventDispatcherTest, AcceleratorBasic) { On 2016/11/29 01:55:46, mfomitchev wrote: > We need some coverage for adding more than one accelerator at a time. We could > test this in AcceleratorBasic, or create a separate test case if > AcceleratorBasic gets too big. Done, I created a new test. Thanks! https://codereview.chromium.org/2520093003/diff/250001/ui/aura/mus/window_tre... File ui/aura/mus/window_tree_client.h (right): https://codereview.chromium.org/2520093003/diff/250001/ui/aura/mus/window_tre... ui/aura/mus/window_tree_client.h:23: #include "services/ui/common/accelerator_transport_util.h" On 2016/11/29 01:55:46, mfomitchev wrote: > This isn't used in .h Done.
https://codereview.chromium.org/2520093003/diff/270001/ash/mus/accelerators/a... File ash/mus/accelerators/accelerator_controller_registrar.cc (right): https://codereview.chromium.org/2520093003/diff/270001/ash/mus/accelerators/a... ash/mus/accelerators/accelerator_controller_registrar.cc:129: std::vector<ui::mojom::AcceleratorTransportPtr> pre_accelerators; Don't declare the variable on a separate line and then assign a value to it. Do it all in one line. https://codereview.chromium.org/2520093003/diff/270001/services/ui/common/acc... File services/ui/common/accelerator_util.h (right): https://codereview.chromium.org/2520093003/diff/270001/services/ui/common/acc... services/ui/common/accelerator_util.h:13: namespace ash { This file is in ui/common, so this shouldn't be in ash::mus namespace, just put it into ui namespace like CreateKeyMatcher. They are very close semantically - that's why we are putting them into the same file. https://codereview.chromium.org/2520093003/diff/270001/services/ui/ws/event_d... File services/ui/ws/event_dispatcher_unittest.cc (right): https://codereview.chromium.org/2520093003/diff/270001/services/ui/ws/event_d... services/ui/ws/event_dispatcher_unittest.cc:389: std::vector<ui::mojom::AcceleratorTransportPtr> accelerators_1; These three lines could be combined into one. This applies to all other uses of AddAcceleratorHelper below.
https://codereview.chromium.org/2520093003/diff/270001/services/ui/common/acc... File services/ui/common/accelerator_util.h (right): https://codereview.chromium.org/2520093003/diff/270001/services/ui/common/acc... services/ui/common/accelerator_util.h:34: #endif // SERVICES_UI_COMMON_ACCELERATOR_TRANSPORT_UTIL_H_ update comment https://codereview.chromium.org/2520093003/diff/270001/services/ui/public/int... File services/ui/public/interfaces/window_manager.mojom (right): https://codereview.chromium.org/2520093003/diff/270001/services/ui/public/int... services/ui/public/interfaces/window_manager.mojom:168: // id or matcher already exists, skip it.Returns true if all accelerators were Nit: separate the comments that are specific to AddAccelerators from the comments that apply to both Add and Remove and put them right before AddAcceleraotrs. https://codereview.chromium.org/2520093003/diff/270001/services/ui/ws/event_d... File services/ui/ws/event_dispatcher.h (right): https://codereview.chromium.org/2520093003/diff/270001/services/ui/ws/event_d... services/ui/ws/event_dispatcher.h:129: std::vector<ui::mojom::AcceleratorTransportPtr> accelerators); no need for ui:: prefix - we are in ui namespace already. https://codereview.chromium.org/2520093003/diff/270001/services/ui/ws/event_d... File services/ui/ws/event_dispatcher_unittest.cc (right): https://codereview.chromium.org/2520093003/diff/270001/services/ui/ws/event_d... services/ui/ws/event_dispatcher_unittest.cc:449: std::vector<ui::mojom::AcceleratorTransportPtr> accelerators_1; We shouldn't create a vector just to take it's last element. Just add the Accelerators one by one to the vector. Feel free to create a helper method to avoid code duplication. https://codereview.chromium.org/2520093003/diff/270001/services/ui/ws/event_d... services/ui/ws/event_dispatcher_unittest.cc:495: EXPECT_FALSE(dispatcher.AddAccelerators(std::move(accelerators))); We should also test the case where some ids/matchers are duped and others aren't and make sure FALSE is returned and also non-duped accelerators are added.
https://codereview.chromium.org/2520093003/diff/270001/ash/mus/accelerators/a... File ash/mus/accelerators/accelerator_controller_registrar.cc (right): https://codereview.chromium.org/2520093003/diff/270001/ash/mus/accelerators/a... ash/mus/accelerators/accelerator_controller_registrar.cc:129: std::vector<ui::mojom::AcceleratorTransportPtr> pre_accelerators; On 2016/11/29 17:37:26, mfomitchev wrote: > Don't declare the variable on a separate line and then assign a value to it. Do > it all in one line. Done, thanks! https://codereview.chromium.org/2520093003/diff/270001/services/ui/common/acc... File services/ui/common/accelerator_util.h (right): https://codereview.chromium.org/2520093003/diff/270001/services/ui/common/acc... services/ui/common/accelerator_util.h:13: namespace ash { On 2016/11/29 17:37:26, mfomitchev wrote: > This file is in ui/common, so this shouldn't be in ash::mus namespace, just put > it into ui namespace like CreateKeyMatcher. They are very close semantically - > that's why we are putting them into the same file. Done, thanks! https://codereview.chromium.org/2520093003/diff/270001/services/ui/ws/event_d... File services/ui/ws/event_dispatcher_unittest.cc (right): https://codereview.chromium.org/2520093003/diff/270001/services/ui/ws/event_d... services/ui/ws/event_dispatcher_unittest.cc:389: std::vector<ui::mojom::AcceleratorTransportPtr> accelerators_1; On 2016/11/29 17:37:27, mfomitchev wrote: > These three lines could be combined into one. This applies to all other uses of > AddAcceleratorHelper below. Done, really nice. I changed other files as well. https://codereview.chromium.org/2520093003/diff/270001/services/ui/ws/event_d... services/ui/ws/event_dispatcher_unittest.cc:449: std::vector<ui::mojom::AcceleratorTransportPtr> accelerators_1; On 2016/11/29 18:23:15, mfomitchev wrote: > We shouldn't create a vector just to take it's last element. Just add the > Accelerators one by one to the vector. Feel free to create a helper method to > avoid code duplication. I use one-liner to add the back of the newly created vector to vector accelerators as below. Let me know if it's ok. I could also add the helper for constructing single ui::mojom::AcceleratorTransportPtr as I did in previous cl. accelerators.push_back(std::move(ui::AddAcceleratorHelper(accelerator_1, std::move(matcher)).back())); https://codereview.chromium.org/2520093003/diff/270001/services/ui/ws/event_d... services/ui/ws/event_dispatcher_unittest.cc:495: EXPECT_FALSE(dispatcher.AddAccelerators(std::move(accelerators))); On 2016/11/29 18:23:15, mfomitchev wrote: > We should also test the case where some ids/matchers are duped and others aren't > and make sure FALSE is returned and also non-duped accelerators are added. accelerators_1 and accelerators_3 already have their ids duplicated here (whose ids equal accelerator_1).
https://codereview.chromium.org/2520093003/diff/270001/services/ui/ws/event_d... File services/ui/ws/event_dispatcher_unittest.cc (right): https://codereview.chromium.org/2520093003/diff/270001/services/ui/ws/event_d... services/ui/ws/event_dispatcher_unittest.cc:449: std::vector<ui::mojom::AcceleratorTransportPtr> accelerators_1; On 2016/11/29 19:06:41, thanhph wrote: > On 2016/11/29 18:23:15, mfomitchev wrote: > > We shouldn't create a vector just to take it's last element. Just add the > > Accelerators one by one to the vector. Feel free to create a helper method to > > avoid code duplication. > > I use one-liner to add the back of the newly created vector to vector > accelerators as below. Let me know if it's ok. I could also add the helper for > constructing single ui::mojom::AcceleratorTransportPtr as I did in previous cl. > > accelerators.push_back(std::move(ui::AddAcceleratorHelper(accelerator_1, > std::move(matcher)).back())); I think adding a helper for constructing a single ui::mojom::AcceleratorTransportPtr would be better. https://codereview.chromium.org/2520093003/diff/270001/services/ui/ws/event_d... services/ui/ws/event_dispatcher_unittest.cc:495: EXPECT_FALSE(dispatcher.AddAccelerators(std::move(accelerators))); All three ids are dups here, right? We need a test where some are dups, and other aren't, and the test should also confirm that non-dups are successfully added. https://codereview.chromium.org/2520093003/diff/290001/services/ui/common/acc... File services/ui/common/accelerator_util.h (right): https://codereview.chromium.org/2520093003/diff/290001/services/ui/common/acc... services/ui/common/accelerator_util.h:22: std::vector<ui::mojom::AcceleratorTransportPtr> AddAcceleratorHelper( Call this CreateAcceleratorVector() or something like that to convey the semantics better. https://codereview.chromium.org/2520093003/diff/290001/services/ui/ws/event_d... File services/ui/ws/event_dispatcher_unittest.cc (right): https://codereview.chromium.org/2520093003/diff/290001/services/ui/ws/event_d... services/ui/ws/event_dispatcher_unittest.cc:389: ui::AddAcceleratorHelper(accelerator_1, std::move(matcher)))); I don't think you need ui:: anywhere here? https://codereview.chromium.org/2520093003/diff/290001/services/ui/ws/event_d... services/ui/ws/event_dispatcher_unittest.cc:448: // Adding all accelerators with the different ids should pass. Adding unique accelerators should pass.
https://codereview.chromium.org/2520093003/diff/270001/services/ui/ws/event_d... File services/ui/ws/event_dispatcher_unittest.cc (right): https://codereview.chromium.org/2520093003/diff/270001/services/ui/ws/event_d... services/ui/ws/event_dispatcher_unittest.cc:449: std::vector<ui::mojom::AcceleratorTransportPtr> accelerators_1; On 2016/11/29 19:21:19, mfomitchev wrote: > On 2016/11/29 19:06:41, thanhph wrote: > > On 2016/11/29 18:23:15, mfomitchev wrote: > > > We shouldn't create a vector just to take it's last element. Just add the > > > Accelerators one by one to the vector. Feel free to create a helper method > to > > > avoid code duplication. > > > > I use one-liner to add the back of the newly created vector to vector > > accelerators as below. Let me know if it's ok. I could also add the helper for > > constructing single ui::mojom::AcceleratorTransportPtr as I did in previous > cl. > > > > accelerators.push_back(std::move(ui::AddAcceleratorHelper(accelerator_1, > > std::move(matcher)).back())); > > I think adding a helper for constructing a single > ui::mojom::AcceleratorTransportPtr would be better. Done, this is cleaner. Thanks! https://codereview.chromium.org/2520093003/diff/270001/services/ui/ws/event_d... services/ui/ws/event_dispatcher_unittest.cc:495: EXPECT_FALSE(dispatcher.AddAccelerators(std::move(accelerators))); On 2016/11/29 19:21:19, mfomitchev wrote: > All three ids are dups here, right? > We need a test where some are dups, and other aren't, and the test should also > confirm that non-dups are successfully added. Done, thanks! https://codereview.chromium.org/2520093003/diff/290001/services/ui/ws/event_d... File services/ui/ws/event_dispatcher_unittest.cc (right): https://codereview.chromium.org/2520093003/diff/290001/services/ui/ws/event_d... services/ui/ws/event_dispatcher_unittest.cc:389: ui::AddAcceleratorHelper(accelerator_1, std::move(matcher)))); On 2016/11/29 19:21:19, mfomitchev wrote: > I don't think you need ui:: anywhere here? Done.
https://codereview.chromium.org/2520093003/diff/290001/services/ui/common/acc... File services/ui/common/accelerator_util.h (right): https://codereview.chromium.org/2520093003/diff/290001/services/ui/common/acc... services/ui/common/accelerator_util.h:22: std::vector<ui::mojom::AcceleratorTransportPtr> AddAcceleratorHelper( On 2016/11/29 19:21:19, mfomitchev wrote: > Call this CreateAcceleratorVector() or something like that to convey the > semantics better. Done. https://codereview.chromium.org/2520093003/diff/290001/services/ui/ws/event_d... File services/ui/ws/event_dispatcher_unittest.cc (right): https://codereview.chromium.org/2520093003/diff/290001/services/ui/ws/event_d... services/ui/ws/event_dispatcher_unittest.cc:448: // Adding all accelerators with the different ids should pass. On 2016/11/29 19:21:19, mfomitchev wrote: > Adding unique accelerators should pass. Done.
https://codereview.chromium.org/2520093003/diff/310001/services/ui/common/acc... File services/ui/common/accelerator_util.h (right): https://codereview.chromium.org/2520093003/diff/310001/services/ui/common/acc... services/ui/common/accelerator_util.h:21: // to an empty |accelerators| vector. comment needs updating - there's no |accelerators| anymore. https://codereview.chromium.org/2520093003/diff/310001/services/ui/common/acc... services/ui/common/accelerator_util.h:26: ui::mojom::AcceleratorTransportPtr CreateAcceleratorTransport( document https://codereview.chromium.org/2520093003/diff/310001/services/ui/public/int... File services/ui/public/interfaces/window_manager.mojom (right): https://codereview.chromium.org/2520093003/diff/310001/services/ui/public/int... services/ui/public/interfaces/window_manager.mojom:141: struct AcceleratorTransport{ Are we appending "Transport" to the struct name in any other mojo structs? If not, can we just call this Accelerator? E.g. we have gxf::Point and gfx::mojom::Point. We don't call the point mojo struct "gfx::mojom::PointTransport". If we do this, we should probably also rename CreateAcceleratorTransport to CreateAccelerator. https://codereview.chromium.org/2520093003/diff/310001/services/ui/ws/event_d... File services/ui/ws/event_dispatcher_unittest.cc (right): https://codereview.chromium.org/2520093003/diff/310001/services/ui/ws/event_d... services/ui/ws/event_dispatcher_unittest.cc:455: matcher = ui::CreateKeyMatcher(ui::mojom::KeyboardCode::W, These are all dupes - they have the same matcher as accelerator_1 https://codereview.chromium.org/2520093003/diff/310001/services/ui/ws/event_d... services/ui/ws/event_dispatcher_unittest.cc:482: // Check if accelerator_4 is already added to the dispatcher. This isn't really a conclusive way to test that the accelerator was added successfully. Instead use an approach similar to that in EventMatching with triggering the accelerator and calling GetAndClearLastAccelerator() on the test delegate for example.
https://codereview.chromium.org/2520093003/diff/310001/services/ui/common/acc... File services/ui/common/accelerator_util.h (right): https://codereview.chromium.org/2520093003/diff/310001/services/ui/common/acc... services/ui/common/accelerator_util.h:21: // to an empty |accelerators| vector. On 2016/11/29 23:27:41, mfomitchev wrote: > comment needs updating - there's no |accelerators| anymore. Done. https://codereview.chromium.org/2520093003/diff/310001/services/ui/common/acc... services/ui/common/accelerator_util.h:26: ui::mojom::AcceleratorTransportPtr CreateAcceleratorTransport( On 2016/11/29 23:27:41, mfomitchev wrote: > document Done. https://codereview.chromium.org/2520093003/diff/310001/services/ui/public/int... File services/ui/public/interfaces/window_manager.mojom (right): https://codereview.chromium.org/2520093003/diff/310001/services/ui/public/int... services/ui/public/interfaces/window_manager.mojom:141: struct AcceleratorTransport{ On 2016/11/29 23:27:41, mfomitchev wrote: > Are we appending "Transport" to the struct name in any other mojo structs? If > not, can we just call this Accelerator? E.g. we have gxf::Point and > gfx::mojom::Point. We don't call the point mojo struct > "gfx::mojom::PointTransport". > If we do this, we should probably also rename CreateAcceleratorTransport to > CreateAccelerator. The name Accelerator is already used in class ui::Accelerator https://cs.chromium.org/chromium/src/ui/base/accelerators/accelerator.h?q=Acc... I rather call it AcceleratorTransport, AcceleratorStruct or some other names to avoid confusion. https://codereview.chromium.org/2520093003/diff/310001/services/ui/ws/event_d... File services/ui/ws/event_dispatcher_unittest.cc (right): https://codereview.chromium.org/2520093003/diff/310001/services/ui/ws/event_d... services/ui/ws/event_dispatcher_unittest.cc:455: matcher = ui::CreateKeyMatcher(ui::mojom::KeyboardCode::W, On 2016/11/29 23:27:41, mfomitchev wrote: > These are all dupes - they have the same matcher as accelerator_1 Nice! The AcceleratorBasic test belongs to other owner so I leave it as it's for now. https://codereview.chromium.org/2520093003/diff/310001/services/ui/ws/event_d... services/ui/ws/event_dispatcher_unittest.cc:482: // Check if accelerator_4 is already added to the dispatcher. On 2016/11/29 23:27:41, mfomitchev wrote: > This isn't really a conclusive way to test that the accelerator was added > successfully. Instead use an approach similar to that in EventMatching with > triggering the accelerator and calling GetAndClearLastAccelerator() on the test > delegate for example. Done, thanks!!
https://codereview.chromium.org/2520093003/diff/310001/services/ui/public/int... File services/ui/public/interfaces/window_manager.mojom (right): https://codereview.chromium.org/2520093003/diff/310001/services/ui/public/int... services/ui/public/interfaces/window_manager.mojom:141: struct AcceleratorTransport{ On 2016/11/30 01:31:20, thanhph wrote: > On 2016/11/29 23:27:41, mfomitchev wrote: > > Are we appending "Transport" to the struct name in any other mojo structs? If > > not, can we just call this Accelerator? E.g. we have gxf::Point and > > gfx::mojom::Point. We don't call the point mojo struct > > "gfx::mojom::PointTransport". > > If we do this, we should probably also rename CreateAcceleratorTransport to > > CreateAccelerator. > > The name Accelerator is already used in class ui::Accelerator > https://cs.chromium.org/chromium/src/ui/base/accelerators/accelerator.h?q=Acc... > > I rather call it AcceleratorTransport, AcceleratorStruct or some other names to > avoid confusion. Yes, just like gfx::mojom::Point and gfx::Point. There's a ton mojo structs like this who have counterparts with the same name in different namespaces, so this shouldn't be confusing. At least as long as they represent the same thing, just in different contexts (which they do). As a side node, there's also an Accelerator in ui::ws: https://cs.chromium.org/chromium/src/services/ui/ws/accelerator.h. https://codereview.chromium.org/2520093003/diff/310001/services/ui/ws/event_d... File services/ui/ws/event_dispatcher_unittest.cc (right): https://codereview.chromium.org/2520093003/diff/310001/services/ui/ws/event_d... services/ui/ws/event_dispatcher_unittest.cc:455: matcher = ui::CreateKeyMatcher(ui::mojom::KeyboardCode::W, On 2016/11/30 01:31:20, thanhph wrote: > On 2016/11/29 23:27:41, mfomitchev wrote: > > These are all dupes - they have the same matcher as accelerator_1 > > Nice! The AcceleratorBasic test belongs to other owner so I leave it as it's for > now. I am not sure what you mean? This is AddAccelerators test case which you have added. https://codereview.chromium.org/2520093003/diff/330001/services/ui/ws/event_d... File services/ui/ws/event_dispatcher_unittest.cc (right): https://codereview.chromium.org/2520093003/diff/330001/services/ui/ws/event_d... services/ui/ws/event_dispatcher_unittest.cc:492: dispatcher.RemoveAccelerator(accelerator_1); I don't think we need to test Remove here - it's tested elsewhere.
https://codereview.chromium.org/2520093003/diff/310001/services/ui/public/int... File services/ui/public/interfaces/window_manager.mojom (right): https://codereview.chromium.org/2520093003/diff/310001/services/ui/public/int... services/ui/public/interfaces/window_manager.mojom:141: struct AcceleratorTransport{ On 2016/11/30 02:33:04, mfomitchev wrote: > On 2016/11/30 01:31:20, thanhph wrote: > > On 2016/11/29 23:27:41, mfomitchev wrote: > > > Are we appending "Transport" to the struct name in any other mojo structs? > If > > > not, can we just call this Accelerator? E.g. we have gxf::Point and > > > gfx::mojom::Point. We don't call the point mojo struct > > > "gfx::mojom::PointTransport". > > > If we do this, we should probably also rename CreateAcceleratorTransport to > > > CreateAccelerator. > > > > The name Accelerator is already used in class ui::Accelerator > > > https://cs.chromium.org/chromium/src/ui/base/accelerators/accelerator.h?q=Acc... > > > > I rather call it AcceleratorTransport, AcceleratorStruct or some other names > to > > avoid confusion. > > Yes, just like gfx::mojom::Point and gfx::Point. There's a ton mojo structs like > this who have counterparts with the same name in different namespaces, so this > shouldn't be confusing. At least as long as they represent the same thing, just > in different contexts (which they do). As a side node, there's also an > Accelerator in ui::ws: > https://cs.chromium.org/chromium/src/services/ui/ws/accelerator.h. Done, thanks! https://codereview.chromium.org/2520093003/diff/310001/services/ui/ws/event_d... File services/ui/ws/event_dispatcher_unittest.cc (right): https://codereview.chromium.org/2520093003/diff/310001/services/ui/ws/event_d... services/ui/ws/event_dispatcher_unittest.cc:455: matcher = ui::CreateKeyMatcher(ui::mojom::KeyboardCode::W, On 2016/11/30 02:33:04, mfomitchev wrote: > On 2016/11/30 01:31:20, thanhph wrote: > > On 2016/11/29 23:27:41, mfomitchev wrote: > > > These are all dupes - they have the same matcher as accelerator_1 > > > > Nice! The AcceleratorBasic test belongs to other owner so I leave it as it's > for > > now. > > I am not sure what you mean? This is AddAccelerators test case which you have > added. Yes I added the test case AddAccelerators. I meant the test case above AcceleratorBasic that can be refactored like you suggest here but I'll leave it as it's since it belongs to other owner. https://codereview.chromium.org/2520093003/diff/330001/services/ui/ws/event_d... File services/ui/ws/event_dispatcher_unittest.cc (right): https://codereview.chromium.org/2520093003/diff/330001/services/ui/ws/event_d... services/ui/ws/event_dispatcher_unittest.cc:492: dispatcher.RemoveAccelerator(accelerator_1); On 2016/11/30 02:33:04, mfomitchev wrote: > I don't think we need to test Remove here - it's tested elsewhere. Done.
https://codereview.chromium.org/2520093003/diff/310001/services/ui/ws/event_d... File services/ui/ws/event_dispatcher_unittest.cc (right): https://codereview.chromium.org/2520093003/diff/310001/services/ui/ws/event_d... services/ui/ws/event_dispatcher_unittest.cc:455: matcher = ui::CreateKeyMatcher(ui::mojom::KeyboardCode::W, I am not suggesting a refactor. I am saying that a. accelerators 4, 5, and 6 are all duplicates of 1, 2, and 3, because their matchers are the same. Your comment below says "Adding accelerator_6 with the same id accelerator_5 should fail.", but in fact all of the additions should fail. b. We still need a test that attempts to add a few unique and one duplicate accelerator. The test should confirm that: - false is returned - the unique accelerators were added - (if the duped accelerator had a different id but the same matcher) the accelerator id associated with the matcher didn't change. https://codereview.chromium.org/2520093003/diff/360001/services/ui/ws/event_d... File services/ui/ws/event_dispatcher_unittest.cc (right): https://codereview.chromium.org/2520093003/diff/360001/services/ui/ws/event_d... services/ui/ws/event_dispatcher_unittest.cc:475: // Check if accelerator_4 is already added to the dispatcher. It's not added, it's a dupe of accelerator_1 because it has the same matcher. Your EXPECT_EQ below confirms that.
https://codereview.chromium.org/2520093003/diff/310001/services/ui/ws/event_d... File services/ui/ws/event_dispatcher_unittest.cc (right): https://codereview.chromium.org/2520093003/diff/310001/services/ui/ws/event_d... services/ui/ws/event_dispatcher_unittest.cc:455: matcher = ui::CreateKeyMatcher(ui::mojom::KeyboardCode::W, On 2016/11/30 17:58:40, mfomitchev wrote: > I am not suggesting a refactor. I am saying that > a. accelerators 4, 5, and 6 are all duplicates of 1, 2, and 3, because their > matchers are the same. Your comment below says "Adding accelerator_6 with the > same id accelerator_5 should fail.", but in fact all of the additions should > fail. > > b. We still need a test that attempts to add a few unique and one duplicate > accelerator. The test should confirm that: > - false is returned > - the unique accelerators were added > - (if the duped accelerator had a different id but the same matcher) the > accelerator id associated with the matcher didn't change. Done, thanks! I added the tests as you mentioned. https://codereview.chromium.org/2520093003/diff/360001/services/ui/ws/event_d... File services/ui/ws/event_dispatcher_unittest.cc (right): https://codereview.chromium.org/2520093003/diff/360001/services/ui/ws/event_d... services/ui/ws/event_dispatcher_unittest.cc:475: // Check if accelerator_4 is already added to the dispatcher. On 2016/11/30 17:58:40, mfomitchev wrote: > It's not added, it's a dupe of accelerator_1 because it has the same matcher. > Your EXPECT_EQ below confirms that. 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
https://codereview.chromium.org/2520093003/diff/310001/services/ui/ws/event_d... File services/ui/ws/event_dispatcher_unittest.cc (right): https://codereview.chromium.org/2520093003/diff/310001/services/ui/ws/event_d... services/ui/ws/event_dispatcher_unittest.cc:455: matcher = ui::CreateKeyMatcher(ui::mojom::KeyboardCode::W, On 2016/11/30 23:35:13, thanhph wrote: > On 2016/11/30 17:58:40, mfomitchev wrote: > > I am not suggesting a refactor. I am saying that > > a. accelerators 4, 5, and 6 are all duplicates of 1, 2, and 3, because their > > matchers are the same. Your comment below says "Adding accelerator_6 with the > > same id accelerator_5 should fail.", but in fact all of the additions should > > fail. > > > > b. We still need a test that attempts to add a few unique and one duplicate > > accelerator. The test should confirm that: > > - false is returned > > - the unique accelerators were added > > - (if the duped accelerator had a different id but the same matcher) the > > accelerator id associated with the matcher didn't change. > > Done, thanks! I added the tests as you mentioned. I only see a test that adds a bunch of unique accelerators successfuly, and then tests that add a single (duped) accelerator unsuccessfully. We need a test that attempts to add a few accelerators at once, and some are added successfully, while others aren't. E.g. two unique and one duped.
https://codereview.chromium.org/2520093003/diff/310001/services/ui/ws/event_d... File services/ui/ws/event_dispatcher_unittest.cc (right): https://codereview.chromium.org/2520093003/diff/310001/services/ui/ws/event_d... services/ui/ws/event_dispatcher_unittest.cc:455: matcher = ui::CreateKeyMatcher(ui::mojom::KeyboardCode::W, On 2016/12/01 03:58:07, mfomitchev wrote: > On 2016/11/30 23:35:13, thanhph wrote: > > On 2016/11/30 17:58:40, mfomitchev wrote: > > > I am not suggesting a refactor. I am saying that > > > a. accelerators 4, 5, and 6 are all duplicates of 1, 2, and 3, because their > > > matchers are the same. Your comment below says "Adding accelerator_6 with > the > > > same id accelerator_5 should fail.", but in fact all of the additions should > > > fail. > > > > > > b. We still need a test that attempts to add a few unique and one duplicate > > > accelerator. The test should confirm that: > > > - false is returned > > > - the unique accelerators were added > > > - (if the duped accelerator had a different id but the same matcher) the > > > accelerator id associated with the matcher didn't change. > > > > Done, thanks! I added the tests as you mentioned. > > I only see a test that adds a bunch of unique accelerators successfuly, and then > tests that add a single (duped) accelerator unsuccessfully. We need a test that > attempts to add a few accelerators at once, and some are added successfully, > while others aren't. E.g. two unique and one duped. I just added this case in the new cl. Thanks!
Nice! Almost there, just a bunch of nits. https://codereview.chromium.org/2520093003/diff/400001/services/ui/common/acc... File services/ui/common/accelerator_util.cc (right): https://codereview.chromium.org/2520093003/diff/400001/services/ui/common/acc... services/ui/common/accelerator_util.cc:1: // Copyright 2015 The Chromium Authors. All rights reserved. nit: 2016 https://codereview.chromium.org/2520093003/diff/400001/services/ui/common/acc... File services/ui/common/accelerator_util.h (right): https://codereview.chromium.org/2520093003/diff/400001/services/ui/common/acc... services/ui/common/accelerator_util.h:1: // Copyright 2015 The Chromium Authors. All rights reserved. nit: 2016 https://codereview.chromium.org/2520093003/diff/400001/services/ui/public/int... File services/ui/public/interfaces/window_manager.mojom (right): https://codereview.chromium.org/2520093003/diff/400001/services/ui/public/int... services/ui/public/interfaces/window_manager.mojom:141: struct Accelerator{ nit: space after Accelerator https://codereview.chromium.org/2520093003/diff/400001/services/ui/public/int... services/ui/public/interfaces/window_manager.mojom:168: // id or matcher already exists, skip it.Returns true if all accelerators were Nit: There's some stuff in this comment specific to Add, while most of it applies to both Add and Remove, which makes the comment a little confusionl. Can you please clarify which part applies to what. E.g. you can separate the comments that are specific to AddAccelerators from the comments that apply to both Add and Remove and put them right before AddAccelerators method below. https://codereview.chromium.org/2520093003/diff/400001/services/ui/ws/event_d... File services/ui/ws/event_dispatcher_unittest.cc (right): https://codereview.chromium.org/2520093003/diff/400001/services/ui/ws/event_d... services/ui/ws/event_dispatcher_unittest.cc:424: TEST_F(EventDispatcherTest, AddAccelerators) { Can we call this test AddMultipleAccelerators or something similar to highlight the fact we are testing adding a bunch at once? https://codereview.chromium.org/2520093003/diff/400001/services/ui/ws/event_d... services/ui/ws/event_dispatcher_unittest.cc:424: TEST_F(EventDispatcherTest, AddAccelerators) { nit: consider putting this after EventMatching, since you are relying on the functionality tested in that test to work here. https://codereview.chromium.org/2520093003/diff/400001/services/ui/ws/event_d... services/ui/ws/event_dispatcher_unittest.cc:447: EXPECT_TRUE(dispatcher.AddAccelerators(std::move(accelerators))); Add a test for accelerator_3 below confirming that it works. https://codereview.chromium.org/2520093003/diff/400001/services/ui/ws/event_d... services/ui/ws/event_dispatcher_unittest.cc:470: ui::KeyEvent key_1(ui::ET_KEY_PRESSED, ui::VKEY_R, ui::EF_CONTROL_DOWN); nit: Just call this key, otherwise it makes one think there's some connection with accelerator_1, which is not the case. https://codereview.chromium.org/2520093003/diff/400001/services/ui/ws/event_d... services/ui/ws/event_dispatcher_unittest.cc:473: // Check if the accelerator_7 was still added regardless of failling nit: move this comment about key_1 declaration, since these three lines are all testing what's described in this comment. https://codereview.chromium.org/2520093003/diff/400001/services/ui/ws/event_d... services/ui/ws/event_dispatcher_unittest.cc:478: ui::KeyEvent key_2(ui::ET_KEY_PRESSED, ui::VKEY_W, ui::EF_CONTROL_DOWN); ditto: key https://codereview.chromium.org/2520093003/diff/400001/services/ui/ws/event_d... services/ui/ws/event_dispatcher_unittest.cc:481: // Check if the accelerator_1 id hasn't changed because of accelerator_4 ditto: move https://codereview.chromium.org/2520093003/diff/400001/services/ui/ws/event_d... services/ui/ws/event_dispatcher_unittest.cc:486: accelerators.clear(); I don't think we need the test below - you've already tested this just above with accelerator_4. https://codereview.chromium.org/2520093003/diff/400001/services/ui/ws/window_... File services/ui/ws/window_manager_state_unittest.cc (right): https://codereview.chromium.org/2520093003/diff/400001/services/ui/ws/window_... services/ui/ws/window_manager_state_unittest.cc:235: std::vector<ui::mojom::AcceleratorPtr> accelerators = nit: no need to create a local var https://codereview.chromium.org/2520093003/diff/400001/ui/aura/mus/window_tre... File ui/aura/mus/window_tree_client.cc (right): https://codereview.chromium.org/2520093003/diff/400001/ui/aura/mus/window_tre... ui/aura/mus/window_tree_client.cc:1435: std::vector<ui::mojom::AcceleratorPtr> accelerators = nit: no need to create a local var
https://codereview.chromium.org/2520093003/diff/400001/services/ui/common/acc... File services/ui/common/accelerator_util.cc (right): https://codereview.chromium.org/2520093003/diff/400001/services/ui/common/acc... services/ui/common/accelerator_util.cc:1: // Copyright 2015 The Chromium Authors. All rights reserved. On 2016/12/01 15:32:36, mfomitchev wrote: > nit: 2016 Done. https://codereview.chromium.org/2520093003/diff/400001/services/ui/common/acc... File services/ui/common/accelerator_util.h (right): https://codereview.chromium.org/2520093003/diff/400001/services/ui/common/acc... services/ui/common/accelerator_util.h:1: // Copyright 2015 The Chromium Authors. All rights reserved. On 2016/12/01 15:32:36, mfomitchev wrote: > nit: 2016 Done. https://codereview.chromium.org/2520093003/diff/400001/services/ui/public/int... File services/ui/public/interfaces/window_manager.mojom (right): https://codereview.chromium.org/2520093003/diff/400001/services/ui/public/int... services/ui/public/interfaces/window_manager.mojom:141: struct Accelerator{ On 2016/12/01 15:32:36, mfomitchev wrote: > nit: space after Accelerator Done. https://codereview.chromium.org/2520093003/diff/400001/services/ui/public/int... services/ui/public/interfaces/window_manager.mojom:168: // id or matcher already exists, skip it.Returns true if all accelerators were On 2016/12/01 15:32:36, mfomitchev wrote: > Nit: There's some stuff in this comment specific to Add, while most of it > applies to both Add and Remove, which makes the comment a little confusionl. Can > you please clarify which part applies to what. E.g. you can separate the > comments that are specific to AddAccelerators from the > comments that apply to both Add and Remove and put them right before > AddAccelerators method below. Done. https://codereview.chromium.org/2520093003/diff/400001/services/ui/ws/event_d... File services/ui/ws/event_dispatcher_unittest.cc (right): https://codereview.chromium.org/2520093003/diff/400001/services/ui/ws/event_d... services/ui/ws/event_dispatcher_unittest.cc:424: TEST_F(EventDispatcherTest, AddAccelerators) { On 2016/12/01 15:32:36, mfomitchev wrote: > nit: consider putting this after EventMatching, since you are relying on the > functionality tested in that test to work here. Done, thanks! https://codereview.chromium.org/2520093003/diff/400001/services/ui/ws/event_d... services/ui/ws/event_dispatcher_unittest.cc:447: EXPECT_TRUE(dispatcher.AddAccelerators(std::move(accelerators))); On 2016/12/01 15:32:36, mfomitchev wrote: > Add a test for accelerator_3 below confirming that it works. Done. https://codereview.chromium.org/2520093003/diff/400001/services/ui/ws/event_d... services/ui/ws/event_dispatcher_unittest.cc:470: ui::KeyEvent key_1(ui::ET_KEY_PRESSED, ui::VKEY_R, ui::EF_CONTROL_DOWN); On 2016/12/01 15:32:37, mfomitchev wrote: > nit: Just call this key, otherwise it makes one think there's some connection > with accelerator_1, which is not the case. Done. https://codereview.chromium.org/2520093003/diff/400001/services/ui/ws/event_d... services/ui/ws/event_dispatcher_unittest.cc:473: // Check if the accelerator_7 was still added regardless of failling On 2016/12/01 15:32:37, mfomitchev wrote: > nit: move this comment about key_1 declaration, since these three lines are all > testing what's described in this comment. Done. https://codereview.chromium.org/2520093003/diff/400001/services/ui/ws/event_d... services/ui/ws/event_dispatcher_unittest.cc:478: ui::KeyEvent key_2(ui::ET_KEY_PRESSED, ui::VKEY_W, ui::EF_CONTROL_DOWN); On 2016/12/01 15:32:36, mfomitchev wrote: > ditto: key Done. https://codereview.chromium.org/2520093003/diff/400001/services/ui/ws/event_d... services/ui/ws/event_dispatcher_unittest.cc:481: // Check if the accelerator_1 id hasn't changed because of accelerator_4 On 2016/12/01 15:32:37, mfomitchev wrote: > ditto: move Done. https://codereview.chromium.org/2520093003/diff/400001/services/ui/ws/window_... File services/ui/ws/window_manager_state_unittest.cc (right): https://codereview.chromium.org/2520093003/diff/400001/services/ui/ws/window_... services/ui/ws/window_manager_state_unittest.cc:235: std::vector<ui::mojom::AcceleratorPtr> accelerators = On 2016/12/01 15:32:37, mfomitchev wrote: > nit: no need to create a local var Done, thanks! https://codereview.chromium.org/2520093003/diff/400001/ui/aura/mus/window_tre... File ui/aura/mus/window_tree_client.cc (right): https://codereview.chromium.org/2520093003/diff/400001/ui/aura/mus/window_tre... ui/aura/mus/window_tree_client.cc:1435: std::vector<ui::mojom::AcceleratorPtr> accelerators = On 2016/12/01 15:32:37, mfomitchev wrote: > nit: no need to create a local var 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...
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...) chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
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/2520093003/diff/400001/services/ui/public/int... File services/ui/public/interfaces/window_manager.mojom (right): https://codereview.chromium.org/2520093003/diff/400001/services/ui/public/int... services/ui/public/interfaces/window_manager.mojom:141: struct Accelerator{ On 2016/12/01 19:03:19, thanhph wrote: > On 2016/12/01 15:32:36, mfomitchev wrote: > > nit: space after Accelerator > > Done. No, not a blank line. Space. I.e. "Accelerator {" https://codereview.chromium.org/2520093003/diff/440001/services/ui/public/int... File services/ui/public/interfaces/window_manager.mojom (right): https://codereview.chromium.org/2520093003/diff/440001/services/ui/public/int... services/ui/public/interfaces/window_manager.mojom:172: // Accelerator ids 1 << 31 and above are reserved for internal use. This comment applies to both Add and Remove, but now it looks like it's specific to Remove. I'd suggest a different way of organizing: top comment should apply to both add and remove. You can leave it largerly the same as it was before your change. Then another comment before AddAccelerators describing its behavior (if an accelerator with the same id or matcher already exists...). https://codereview.chromium.org/2520093003/diff/440001/services/ui/ws/event_d... File services/ui/ws/event_dispatcher_unittest.cc (right): https://codereview.chromium.org/2520093003/diff/440001/services/ui/ws/event_d... services/ui/ws/event_dispatcher_unittest.cc:493: // check if accelerator_3 is added Move this to before the line with key declaration. Capitalize first letter. Period at the end.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Commit bots are happy! https://codereview.chromium.org/2520093003/diff/440001/services/ui/public/int... File services/ui/public/interfaces/window_manager.mojom (right): https://codereview.chromium.org/2520093003/diff/440001/services/ui/public/int... services/ui/public/interfaces/window_manager.mojom:172: // Accelerator ids 1 << 31 and above are reserved for internal use. On 2016/12/01 20:51:51, mfomitchev wrote: > This comment applies to both Add and Remove, but now it looks like it's specific > to Remove. I'd suggest a different way of organizing: top comment should apply > to both add and remove. You can leave it largerly the same as it was before your > change. Then another comment before AddAccelerators describing its behavior (if > an accelerator with the same id or matcher already exists...). Done, thanks! https://codereview.chromium.org/2520093003/diff/440001/services/ui/ws/event_d... File services/ui/ws/event_dispatcher_unittest.cc (right): https://codereview.chromium.org/2520093003/diff/440001/services/ui/ws/event_d... services/ui/ws/event_dispatcher_unittest.cc:493: // check if accelerator_3 is added On 2016/12/01 20:51:51, mfomitchev wrote: > Move this to before the line with key declaration. Capitalize first letter. > Period at the end. Done.
thanhph@chromium.org changed reviewers: + sky@chromium.org
sky@chromium.org: Please review changes in
thanhph@chromium.org changed reviewers: + rockot@chromium.org
rockot@chromium.org: Please review changes in services/ui/public/interfaces/window_manager.mojom.
https://codereview.chromium.org/2520093003/diff/460001/ash/mus/accelerators/a... File ash/mus/accelerators/accelerator_controller_registrar.cc (right): https://codereview.chromium.org/2520093003/diff/460001/ash/mus/accelerators/a... ash/mus/accelerators/accelerator_controller_registrar.cc:129: window_manager_->window_manager_client()->AddAccelerators( Is this only the first change you're intending to do? As the way you have it now you only call add with a single vector, so that in effect this change makes the call have more overhead. https://codereview.chromium.org/2520093003/diff/460001/services/ui/public/int... File services/ui/public/interfaces/window_manager.mojom (right): https://codereview.chromium.org/2520093003/diff/460001/services/ui/public/int... services/ui/public/interfaces/window_manager.mojom:171: // If an accelerator with the same id or the same matcher already exists, skip How about: 'This ignores any accelerators already defined with the same id or matcher.' https://codereview.chromium.org/2520093003/diff/460001/services/ui/ws/event_d... File services/ui/ws/event_dispatcher.cc (right): https://codereview.chromium.org/2520093003/diff/460001/services/ui/ws/event_d... services/ui/ws/event_dispatcher.cc:252: new Accelerator(iter->get()->id, *(iter->get()->event_matcher))); Use MakeUnique (see threads on chomium-dev). https://codereview.chromium.org/2520093003/diff/460001/services/ui/ws/event_d... File services/ui/ws/event_dispatcher.h (right): https://codereview.chromium.org/2520093003/diff/460001/services/ui/ws/event_d... services/ui/ws/event_dispatcher.h:128: bool AddAccelerators(std::vector<ui::mojom::AcceleratorPtr> accelerators); I think this code would be easier if you lave this function as it was and have the caller be responsible for calling multiple times. That way the caller can decide how to handle failures.
LGTM
LGTM
https://codereview.chromium.org/2520093003/diff/460001/ash/mus/accelerators/a... File ash/mus/accelerators/accelerator_controller_registrar.cc (right): https://codereview.chromium.org/2520093003/diff/460001/ash/mus/accelerators/a... ash/mus/accelerators/accelerator_controller_registrar.cc:129: window_manager_->window_manager_client()->AddAccelerators( On 2016/12/01 22:17:04, sky wrote: > Is this only the first change you're intending to do? As the way you have it now > you only call add with a single vector, so that in effect this change makes the > call have more overhead. To change this I need to change the callback function OnAcceleratorAdded to handle multiple accelerators. I can create another bug which is to reduce overhead of registering accelerators. This seems relate to this one I'm working on next https://bugs.chromium.org/p/chromium/issues/detail?id=632050. https://codereview.chromium.org/2520093003/diff/460001/services/ui/public/int... File services/ui/public/interfaces/window_manager.mojom (right): https://codereview.chromium.org/2520093003/diff/460001/services/ui/public/int... services/ui/public/interfaces/window_manager.mojom:171: // If an accelerator with the same id or the same matcher already exists, skip On 2016/12/01 22:17:04, sky wrote: > How about: 'This ignores any accelerators already defined with the same id or > matcher.' Done, thanks! https://codereview.chromium.org/2520093003/diff/460001/services/ui/ws/event_d... File services/ui/ws/event_dispatcher.cc (right): https://codereview.chromium.org/2520093003/diff/460001/services/ui/ws/event_d... services/ui/ws/event_dispatcher.cc:252: new Accelerator(iter->get()->id, *(iter->get()->event_matcher))); On 2016/12/01 22:17:04, sky wrote: > Use MakeUnique (see threads on chomium-dev). Done, thanks! https://codereview.chromium.org/2520093003/diff/460001/services/ui/ws/event_d... File services/ui/ws/event_dispatcher.h (right): https://codereview.chromium.org/2520093003/diff/460001/services/ui/ws/event_d... services/ui/ws/event_dispatcher.h:128: bool AddAccelerators(std::vector<ui::mojom::AcceleratorPtr> accelerators); On 2016/12/01 22:17:04, sky wrote: > I think this code would be easier if you lave this function as it was and have > the caller be responsible for calling multiple times. That way the caller can > decide how to handle failures. Nice, I put this AddAccelerator back. Thanks!
https://codereview.chromium.org/2520093003/diff/460001/ash/mus/accelerators/a... File ash/mus/accelerators/accelerator_controller_registrar.cc (right): https://codereview.chromium.org/2520093003/diff/460001/ash/mus/accelerators/a... ash/mus/accelerators/accelerator_controller_registrar.cc:129: window_manager_->window_manager_client()->AddAccelerators( On 2016/12/01 23:32:41, thanhph wrote: > On 2016/12/01 22:17:04, sky wrote: > > Is this only the first change you're intending to do? As the way you have it > now > > you only call add with a single vector, so that in effect this change makes > the > > call have more overhead. > > To change this I need to change the callback function OnAcceleratorAdded to > handle multiple accelerators. I can create another bug which is to reduce > overhead of registering accelerators. This seems relate to this one I'm working > on next https://bugs.chromium.org/p/chromium/issues/detail?id=632050. I don't think that would help as registration adds accelerators one at a time. Instead I think you need a function to be called on AcceleratorControllerRegistrar after AcceleratorController::Init has been called. Something like OnAcceleratorsRegistered(). This function would then iterate over the internal structure and make the request to register the accelerators as a single array. You should do this separately. I just wanted to make sure you were going to make use of the function that takes an array. https://codereview.chromium.org/2520093003/diff/500001/services/ui/ws/event_d... File services/ui/ws/event_dispatcher.h (right): https://codereview.chromium.org/2520093003/diff/500001/services/ui/ws/event_d... services/ui/ws/event_dispatcher.h:133: bool AddAccelerators(std::vector<ui::mojom::AcceleratorPtr> accelerators); This should only be used from WindowTreeClient. Inline it there.
https://codereview.chromium.org/2520093003/diff/460001/ash/mus/accelerators/a... File ash/mus/accelerators/accelerator_controller_registrar.cc (right): https://codereview.chromium.org/2520093003/diff/460001/ash/mus/accelerators/a... ash/mus/accelerators/accelerator_controller_registrar.cc:129: window_manager_->window_manager_client()->AddAccelerators( On 2016/12/02 00:11:16, sky wrote: > On 2016/12/01 23:32:41, thanhph wrote: > > On 2016/12/01 22:17:04, sky wrote: > > > Is this only the first change you're intending to do? As the way you have it > > now > > > you only call add with a single vector, so that in effect this change makes > > the > > > call have more overhead. > > > > To change this I need to change the callback function OnAcceleratorAdded to > > handle multiple accelerators. I can create another bug which is to reduce > > overhead of registering accelerators. This seems relate to this one I'm > working > > on next https://bugs.chromium.org/p/chromium/issues/detail?id=632050. > > I don't think that would help as registration adds accelerators one at a time. > Instead I think you need a function to be called on > AcceleratorControllerRegistrar after AcceleratorController::Init has been > called. Something like OnAcceleratorsRegistered(). This function would then > iterate over the internal structure and make the request to register the > accelerators as a single array. > > You should do this separately. I just wanted to make sure you were going to make > use of the function that takes an array. Thanks Scott, I'll keep this in mind. Should I put this comment thread also in this bug https://bugs.chromium.org/p/chromium/issues/detail?id=632050 ? For what I understand, bug 63250 is to resolve what you mention here. Alternatively, we can merge these 2 bugs into bug 63249 and I can proceed with the refactor. Thanks! https://codereview.chromium.org/2520093003/diff/500001/services/ui/ws/event_d... File services/ui/ws/event_dispatcher.h (right): https://codereview.chromium.org/2520093003/diff/500001/services/ui/ws/event_d... services/ui/ws/event_dispatcher.h:133: bool AddAccelerators(std::vector<ui::mojom::AcceleratorPtr> accelerators); On 2016/12/02 00:11:16, sky wrote: > This should only be used from WindowTreeClient. Inline it there. Done. I added the comment right before this function.
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.
On 2016/12/02 15:11:19, thanhph wrote: > https://codereview.chromium.org/2520093003/diff/460001/ash/mus/accelerators/a... > File ash/mus/accelerators/accelerator_controller_registrar.cc (right): > > https://codereview.chromium.org/2520093003/diff/460001/ash/mus/accelerators/a... > ash/mus/accelerators/accelerator_controller_registrar.cc:129: > window_manager_->window_manager_client()->AddAccelerators( > On 2016/12/02 00:11:16, sky wrote: > > On 2016/12/01 23:32:41, thanhph wrote: > > > On 2016/12/01 22:17:04, sky wrote: > > > > Is this only the first change you're intending to do? As the way you have > it > > > now > > > > you only call add with a single vector, so that in effect this change > makes > > > the > > > > call have more overhead. > > > > > > To change this I need to change the callback function OnAcceleratorAdded to > > > handle multiple accelerators. I can create another bug which is to reduce > > > overhead of registering accelerators. This seems relate to this one I'm > > working > > > on next https://bugs.chromium.org/p/chromium/issues/detail?id=632050. > > > > I don't think that would help as registration adds accelerators one at a time. > > Instead I think you need a function to be called on > > AcceleratorControllerRegistrar after AcceleratorController::Init has been > > called. Something like OnAcceleratorsRegistered(). This function would then > > iterate over the internal structure and make the request to register the > > accelerators as a single array. > > > > You should do this separately. I just wanted to make sure you were going to > make > > use of the function that takes an array. > > Thanks Scott, I'll keep this in mind. Should I put this comment thread also in > this bug https://bugs.chromium.org/p/chromium/issues/detail?id=632050 ? Sure. If you add 632050 to the BUG= of this cl when this is landed both bugs are updated. > For what I understand, bug 63250 is to resolve what you mention here. > Alternatively, we can merge these 2 bugs into bug 63249 and I can proceed with > the refactor. Thanks! I'm fine either way. > > https://codereview.chromium.org/2520093003/diff/500001/services/ui/ws/event_d... > File services/ui/ws/event_dispatcher.h (right): > > https://codereview.chromium.org/2520093003/diff/500001/services/ui/ws/event_d... > services/ui/ws/event_dispatcher.h:133: bool > AddAccelerators(std::vector<ui::mojom::AcceleratorPtr> accelerators); > On 2016/12/02 00:11:16, sky wrote: > > This should only be used from WindowTreeClient. Inline it there. > > Done. I added the comment right before this function.
https://codereview.chromium.org/2520093003/diff/520001/services/ui/ws/event_d... File services/ui/ws/event_dispatcher.h (right): https://codereview.chromium.org/2520093003/diff/520001/services/ui/ws/event_d... services/ui/ws/event_dispatcher.h:134: bool AddAccelerators(std::vector<ui::mojom::AcceleratorPtr> accelerators); Sorry if I wasn't clear. I'm suggesting you do *not* add this function. Instead WindowTreeClient iterates over the array calling AddAccelerator.
sky@chromium.org changed reviewers: + tsepez@chromium.org - rockot@chromium.org
On 2016/12/01 21:35:55, thanhph wrote: > mailto:rockot@chromium.org: Please review changes in > services/ui/public/interfaces/window_manager.mojom. You need a security reviewer for that. I'm going to remove Ken and add tsepez@. Also, you don't have chromium-reviews cc'd. Generally chromium-reviews is cc'd on all patchs. I'm adding that too.
mojom LGTM
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...
On 2016/12/02 17:25:31, sky wrote: > On 2016/12/02 15:11:19, thanhph wrote: > > > https://codereview.chromium.org/2520093003/diff/460001/ash/mus/accelerators/a... > > File ash/mus/accelerators/accelerator_controller_registrar.cc (right): > > > > > https://codereview.chromium.org/2520093003/diff/460001/ash/mus/accelerators/a... > > ash/mus/accelerators/accelerator_controller_registrar.cc:129: > > window_manager_->window_manager_client()->AddAccelerators( > > On 2016/12/02 00:11:16, sky wrote: > > > On 2016/12/01 23:32:41, thanhph wrote: > > > > On 2016/12/01 22:17:04, sky wrote: > > > > > Is this only the first change you're intending to do? As the way you > have > > it > > > > now > > > > > you only call add with a single vector, so that in effect this change > > makes > > > > the > > > > > call have more overhead. > > > > > > > > To change this I need to change the callback function OnAcceleratorAdded > to > > > > handle multiple accelerators. I can create another bug which is to reduce > > > > overhead of registering accelerators. This seems relate to this one I'm > > > working > > > > on next https://bugs.chromium.org/p/chromium/issues/detail?id=632050. > > > > > > I don't think that would help as registration adds accelerators one at a > time. > > > Instead I think you need a function to be called on > > > AcceleratorControllerRegistrar after AcceleratorController::Init has been > > > called. Something like OnAcceleratorsRegistered(). This function would then > > > iterate over the internal structure and make the request to register the > > > accelerators as a single array. > > > > > > You should do this separately. I just wanted to make sure you were going to > > make > > > use of the function that takes an array. > > > > Thanks Scott, I'll keep this in mind. Should I put this comment thread also in > > this bug https://bugs.chromium.org/p/chromium/issues/detail?id=632050 ? > > Sure. If you add 632050 to the BUG= of this cl when this is landed both bugs are > updated. > > > For what I understand, bug 63250 is to resolve what you mention here. > > Alternatively, we can merge these 2 bugs into bug 63249 and I can proceed with > > the refactor. Thanks! > > I'm fine either way. Great, I saved this thread to comment section in bug https://bugs.chromium.org/p/chromium/issues/detail?id=632050. I'll resolve this one next.
On 2016/12/02 17:38:42, Tom Sepez wrote: > mojom LGTM Thanks for reviewing this cl!
On 2016/12/02 17:27:50, sky wrote: > On 2016/12/01 21:35:55, thanhph wrote: > > mailto:rockot@chromium.org: Please review changes in > > services/ui/public/interfaces/window_manager.mojom. > > You need a security reviewer for that. I'm going to remove Ken and add tsepez@. > Also, you don't have chromium-reviews cc'd. Generally chromium-reviews is cc'd > on all patchs. I'm adding that too. Thanks Scott, I'll add chromium-reviews next time also.
thanhph@chromium.org changed reviewers: - tsepez@chromium.org
https://codereview.chromium.org/2520093003/diff/520001/services/ui/ws/event_d... File services/ui/ws/event_dispatcher.h (right): https://codereview.chromium.org/2520093003/diff/520001/services/ui/ws/event_d... services/ui/ws/event_dispatcher.h:134: bool AddAccelerators(std::vector<ui::mojom::AcceleratorPtr> accelerators); On 2016/12/02 17:25:42, sky wrote: > Sorry if I wasn't clear. I'm suggesting you do *not* add this function. Instead > WindowTreeClient iterates over the array calling AddAccelerator. Thanks for the explantion Scott. I removed function AddAccelerators out of event_dispatcher.(cc|h) in the new cl. window_tree.cc still has WindowTree::AddAccelerators function while window_tree_client.cc has WindowTreeClient::AddAccelerators. I also leave out the unittest EventDispatcherTest.AddMultipleAccelerators for now since the function EventDispatcher::AddAccelerators no long exists, maybe it can be moved to the next bug 632050. Let me know if this change works. Thanks!
LGTM - you could write the test using WindowTree. There are examples of that, but I'm fine with pushing that to a future cl.
https://codereview.chromium.org/2520093003/diff/540001/services/ui/ws/window_... File services/ui/ws/window_tree.cc (right): https://codereview.chromium.org/2520093003/diff/540001/services/ui/ws/window_... services/ui/ws/window_tree.cc:1716: iter->get()->id, iter->get()->event_matcher.Clone())) use std::move instead of Clone()
On 2016/12/02 22:09:17, sky wrote: > LGTM - you could write the test using WindowTree. There are examples of that, > but I'm fine with pushing that to a future cl. Great, thanks Scott.
thanhph@chromium.org changed reviewers: + tsepez@chromium.org
tsepez@ : Can you take owner/stamp the file services/ui/public/interfaces/window_manager.mojom? Thanks! Thanh. https://codereview.chromium.org/2520093003/diff/540001/services/ui/ws/window_... File services/ui/ws/window_tree.cc (right): https://codereview.chromium.org/2520093003/diff/540001/services/ui/ws/window_... services/ui/ws/window_tree.cc:1716: iter->get()->id, iter->get()->event_matcher.Clone())) On 2016/12/02 22:19:02, mfomitchev wrote: > use std::move instead of Clone() Done, thanks!
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...
On 2016/12/02 22:26:21, thanhph wrote: > tsepez@ : Can you take owner/stamp the file > services/ui/public/interfaces/window_manager.mojom? > > Thanks! > Thanh. You already have and LGTM from tsepez@. Chromite Butler simply doesn't show the state for SECURITY_OWNERS-owned files, but CQ knows that you have an LGTM, so just click Commit.
On 2016/12/02 22:33:51, mfomitchev wrote: > On 2016/12/02 22:26:21, thanhph wrote: > > tsepez@ : Can you take owner/stamp the file > > services/ui/public/interfaces/window_manager.mojom? > > > > Thanks! > > Thanh. > > You already have and LGTM from tsepez@. Chromite Butler simply doesn't show the > state for SECURITY_OWNERS-owned files, but CQ knows that you have an LGTM, so > just click Commit. Thanks, good to know Mikhail. Commit bots will finish the rest then.
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
The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/2520093003/#ps560001 (title: "Use std::move instead of Clone()")
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": 560001, "attempt_start_ts": 1480726850004540,
"parent_rev": "a04b60a52308c527ac83abf7488b37b3daa0ab94", "commit_rev":
"960842408a8b942890a3b81f30ecfe36cabd975b"}
CQ is committing da patch.
Bot data: {"patchset_id": 560001, "attempt_start_ts": 1480726850004540,
"parent_rev": "29c562e9fd812d8819f16d08323192637fca23df", "commit_rev":
"c30f7dd3a18180652fb08ba40703bd7048875e3d"}
Message was sent while issue was closed.
Description was changed from ========== WindowManagerClient::AddAccelerator() should take an array. To reduce IPC calls between window_tree_client.cc and window_tree.cc. BUG=632049 ========== to ========== WindowManagerClient::AddAccelerator() should take an array. To reduce IPC calls between window_tree_client.cc and window_tree.cc. BUG=632049 ==========
Message was sent while issue was closed.
Committed patchset #30 (id:560001)
Message was sent while issue was closed.
Description was changed from ========== WindowManagerClient::AddAccelerator() should take an array. To reduce IPC calls between window_tree_client.cc and window_tree.cc. BUG=632049 ========== to ========== WindowManagerClient::AddAccelerator() should take an array. To reduce IPC calls between window_tree_client.cc and window_tree.cc. BUG=632049 Committed: https://crrev.com/62cbfb83c3ad3a8244d063d297a81fe6d9a32866 Cr-Commit-Position: refs/heads/master@{#436115} ==========
Message was sent while issue was closed.
Patchset 30 (id:??) landed as https://crrev.com/62cbfb83c3ad3a8244d063d297a81fe6d9a32866 Cr-Commit-Position: refs/heads/master@{#436115}
Message was sent while issue was closed.
One thing you missed is updating ui/aura/mus/window_manager_delegate. If you could do that in a follow on patch that would be great. Hopefully we can get rid of most of services/ui/public/cpp soon so there is just one client.
Message was sent while issue was closed.
On 2016/12/03 17:11:15, sky wrote: > One thing you missed is updating ui/aura/mus/window_manager_delegate. If you > could do that in a follow on patch that would be great. Hopefully we can get rid > of most of services/ui/public/cpp soon so there is just one client. Good call Scott.I'm working on this now and will cl upload the new patch here later. Thanks!
Message was sent while issue was closed.
Description was changed from ========== WindowManagerClient::AddAccelerator() should take an array. To reduce IPC calls between window_tree_client.cc and window_tree.cc. BUG=632049 Committed: https://crrev.com/62cbfb83c3ad3a8244d063d297a81fe6d9a32866 Cr-Commit-Position: refs/heads/master@{#436115} ========== to ========== WindowManagerClient::AddAccelerator() should take an array. To reduce IPC calls between window_tree_client.cc and window_tree.cc. BUG=632049 Committed: https://crrev.com/62cbfb83c3ad3a8244d063d297a81fe6d9a32866 Cr-Commit-Position: refs/heads/master@{#436115} ==========
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...
On 2016/12/05 15:54:48, thanhph wrote: > On 2016/12/03 17:11:15, sky wrote: > > One thing you missed is updating ui/aura/mus/window_manager_delegate. If you > > could do that in a follow on patch that would be great. Hopefully we can get > rid > > of most of services/ui/public/cpp soon so there is just one client. > > Good call Scott.I'm working on this now and will cl upload the new patch here > later. Thanks! Scott: I have just updated the following files in the new cl. Let me know if this update is sufficient. Thanks! ui/aura/mus/window_manager_delegate.h ui/aura/mus/window_tree_client.cc ui/aura/mus/window_tree_client.h
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_...)
On 2016/12/05 16:20:36, thanhph wrote: > On 2016/12/05 15:54:48, thanhph wrote: > > On 2016/12/03 17:11:15, sky wrote: > > > One thing you missed is updating ui/aura/mus/window_manager_delegate. If you > > > could do that in a follow on patch that would be great. Hopefully we can get > > rid > > > of most of services/ui/public/cpp soon so there is just one client. > > > > Good call Scott.I'm working on this now and will cl upload the new patch here > > later. Thanks! > > Scott: I have just updated the following files in the new cl. Let me know if > this update is sufficient. Thanks! > > ui/aura/mus/window_manager_delegate.h > ui/aura/mus/window_tree_client.cc > ui/aura/mus/window_tree_client.h Somehow there are some conflicts when I apply this patchset 31 from master branch. I'll re-close this issue and create another follow-on issue to add this patch set in.
Message was sent while issue was closed.
Patchset #31 (id:580001) has been deleted
Message was sent while issue was closed.
Yeah, you can't use the same CL once it was committed. You have to create a new CL (not just a new patch within the same CL). On Mon, Dec 5, 2016 at 11:35 AM <thanhph@chromium.org> wrote: > On 2016/12/05 16:20:36, thanhph wrote: > > On 2016/12/05 15:54:48, thanhph wrote: > > > On 2016/12/03 17:11:15, sky wrote: > > > > One thing you missed is updating > ui/aura/mus/window_manager_delegate. If > you > > > > could do that in a follow on patch that would be great. Hopefully we > can > get > > > rid > > > > of most of services/ui/public/cpp soon so there is just one client. > > > > > > Good call Scott.I'm working on this now and will cl upload the new > patch > here > > > later. Thanks! > > > > Scott: I have just updated the following files in the new cl. Let me > know if > > this update is sufficient. Thanks! > > > > ui/aura/mus/window_manager_delegate.h > > ui/aura/mus/window_tree_client.cc > > ui/aura/mus/window_tree_client.h > > Somehow there are some conflicts when I apply this patchset 31 from master > branch. I'll re-close this issue and create another follow-on issue to add > this > patch set in. > > > https://codereview.chromium.org/2520093003/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
You actually can reopen a patch once it's landed, but you should reserve that for when a patch is reverted after it lands. In this case a new patch is the way to go. -Scott On Mon, Dec 5, 2016 at 9:41 AM, Mikhail Fomitchev <mfomitchev@google.com> wrote: > Yeah, you can't use the same CL once it was committed. You have to create a > new CL (not just a new patch within the same CL). > > > On Mon, Dec 5, 2016 at 11:35 AM <thanhph@chromium.org> wrote: >> >> On 2016/12/05 16:20:36, thanhph wrote: >> > On 2016/12/05 15:54:48, thanhph wrote: >> > > On 2016/12/03 17:11:15, sky wrote: >> > > > One thing you missed is updating >> > > > ui/aura/mus/window_manager_delegate. If >> you >> > > > could do that in a follow on patch that would be great. Hopefully we >> > > > can >> get >> > > rid >> > > > of most of services/ui/public/cpp soon so there is just one client. >> > > >> > > Good call Scott.I'm working on this now and will cl upload the new >> > > patch >> here >> > > later. Thanks! >> > >> > Scott: I have just updated the following files in the new cl. Let me >> > know if >> > this update is sufficient. Thanks! >> > >> > ui/aura/mus/window_manager_delegate.h >> > ui/aura/mus/window_tree_client.cc >> > ui/aura/mus/window_tree_client.h >> >> Somehow there are some conflicts when I apply this patchset 31 from master >> branch. I'll re-close this issue and create another follow-on issue to add >> this >> patch set in. >> >> >> https://codereview.chromium.org/2520093003/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
On 2016/12/05 18:13:19, sky wrote: > You actually can reopen a patch once it's landed, but you should > reserve that for when a patch is reverted after it lands. In this case > a new patch is the way to go. > > -Scott > > On Mon, Dec 5, 2016 at 9:41 AM, Mikhail Fomitchev <mailto:mfomitchev@google.com> > wrote: > > Yeah, you can't use the same CL once it was committed. You have to create a > > new CL (not just a new patch within the same CL). > > > > > > On Mon, Dec 5, 2016 at 11:35 AM <mailto:thanhph@chromium.org> wrote: > >> > >> On 2016/12/05 16:20:36, thanhph wrote: > >> > On 2016/12/05 15:54:48, thanhph wrote: > >> > > On 2016/12/03 17:11:15, sky wrote: > >> > > > One thing you missed is updating > >> > > > ui/aura/mus/window_manager_delegate. If > >> you > >> > > > could do that in a follow on patch that would be great. Hopefully we > >> > > > can > >> get > >> > > rid > >> > > > of most of services/ui/public/cpp soon so there is just one client. > >> > > > >> > > Good call Scott.I'm working on this now and will cl upload the new > >> > > patch > >> here > >> > > later. Thanks! > >> > > >> > Scott: I have just updated the following files in the new cl. Let me > >> > know if > >> > this update is sufficient. Thanks! > >> > > >> > ui/aura/mus/window_manager_delegate.h > >> > ui/aura/mus/window_tree_client.cc > >> > ui/aura/mus/window_tree_client.h > >> > >> Somehow there are some conflicts when I apply this patchset 31 from master > >> branch. I'll re-close this issue and create another follow-on issue to add > >> this > >> patch set in. > >> > >> > >> https://codereview.chromium.org/2520093003/ > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. > Gotcha! Thanks for the explanation. Scott: You could review my follow-up patch here: https://codereview.chromium.org/2552783002/. Thanks! |
