Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
8689003:
Register global accelerators and add placeholders for handling them. (Closed)
|
Created:
9 years, 1 month ago by mazda Modified:
9 years ago CC:
chromium-reviews Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionRegister global accelerators and add placeholders for handling them.
I added these accelerators based on the following sources in the Chromium OS window_manager.git:
- layout2/layout_manager2.cc
- window_manager.cc
BUG=97255
TEST=Manual
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=112522
Patch Set 1 #
Total comments: 9
Patch Set 2 : Address comments #
Total comments: 3
Patch Set 3 : Add TakeScreenshot and issue comments #
Total comments: 5
Patch Set 4 : Fix the test case of ToggleDesktopFullScreen #Patch Set 5 : Fix typo #
Total comments: 4
Patch Set 6 : Rebase and address comments #
Total comments: 4
Patch Set 7 : Address comments #
Messages
Total messages: 16 (0 generated)
http://codereview.chromium.org/8689003/diff/1/ui/aura_shell/shell_accelerator... File ui/aura_shell/shell_accelerator_controller.cc (right): http://codereview.chromium.org/8689003/diff/1/ui/aura_shell/shell_accelerator... ui/aura_shell/shell_accelerator_controller.cc:30: kToggleMaximized, many of these are related to UI that i don't think is going to be implemented in aura. i think that it'd be better to hold off on adding them here until they're needed. i would say that we should stick with just kCycleBackward and kCycleForward (which is misspelled as "Fowrard" here) for now. http://codereview.chromium.org/8689003/diff/1/ui/aura_shell/shell_accelerator... ui/aura_shell/shell_accelerator_controller.cc:36: kToggleProfiler, kToggleClientWindowDebugging, kToggleDamageDebugging, and kToggleProfiler should be removed -- they don't make sense without the X window manager http://codereview.chromium.org/8689003/diff/1/ui/aura_shell/shell_accelerator... File ui/aura_shell/shell_accelerator_controller.h (right): http://codereview.chromium.org/8689003/diff/1/ui/aura_shell/shell_accelerator... ui/aura_shell/shell_accelerator_controller.h:61: std::map<ui::Accelerator, int> accelerators_; add a comment describing what the int value represents
OK... can you also add a stub of a unit test file with one example test so that we can add unit tests easily as these get implemented? http://codereview.chromium.org/8689003/diff/1/ui/aura_shell/shell_accelerator... File ui/aura_shell/shell_accelerator_controller.cc (right): http://codereview.chromium.org/8689003/diff/1/ui/aura_shell/shell_accelerator... ui/aura_shell/shell_accelerator_controller.cc:97: bool CycleFowrard() { Forward
Also what derat says is good. -Ben On Mon, Nov 28, 2011 at 8:40 AM, <ben@chromium.org> wrote: > OK... can you also add a stub of a unit test file with one example test so > that > we can add unit tests easily as these get implemented? > > > > http://codereview.chromium.**org/8689003/diff/1/ui/aura_** > shell/shell_accelerator_**controller.cc<http://codereview.chromium.org/8689003/diff/1/ui/aura_shell/shell_accelerator_controller.cc> > File ui/aura_shell/shell_**accelerator_controller.cc (right): > > http://codereview.chromium.**org/8689003/diff/1/ui/aura_** > shell/shell_accelerator_**controller.cc#newcode97<http://codereview.chromium.org/8689003/diff/1/ui/aura_shell/shell_accelerator_controller.cc#newcode97> > ui/aura_shell/shell_**accelerator_controller.cc:97: bool CycleFowrard() { > Forward > > http://codereview.chromium.**org/8689003/<http://codereview.chromium.org/8689... >
Thank you for the review. I deleted accelerators that might not be implemented in aura and added a unit test. I also changed the style of enum values to macro style following Chromium's convention. Please take another look. http://codereview.chromium.org/8689003/diff/1/ui/aura_shell/shell_accelerator... File ui/aura_shell/shell_accelerator_controller.cc (right): http://codereview.chromium.org/8689003/diff/1/ui/aura_shell/shell_accelerator... ui/aura_shell/shell_accelerator_controller.cc:30: kToggleMaximized, On 2011/11/28 16:39:54, Daniel Erat wrote: > many of these are related to UI that i don't think is going to be implemented in > aura. i think that it'd be better to hold off on adding them here until they're > needed. i would say that we should stick with just kCycleBackward and > kCycleForward (which is misspelled as "Fowrard" here) for now. OK. I hold off most of them for now. We also need TakeRootScreenshot? There is already an issue (http://crbug.com/105198). http://codereview.chromium.org/8689003/diff/1/ui/aura_shell/shell_accelerator... ui/aura_shell/shell_accelerator_controller.cc:36: kToggleProfiler, On 2011/11/28 16:39:54, Daniel Erat wrote: > kToggleClientWindowDebugging, kToggleDamageDebugging, and kToggleProfiler should > be removed -- they don't make sense without the X window manager Done. http://codereview.chromium.org/8689003/diff/1/ui/aura_shell/shell_accelerator... ui/aura_shell/shell_accelerator_controller.cc:97: bool CycleFowrard() { On 2011/11/28 16:40:15, Ben Goodger (Google) wrote: > Forward Done. http://codereview.chromium.org/8689003/diff/1/ui/aura_shell/shell_accelerator... File ui/aura_shell/shell_accelerator_controller.h (right): http://codereview.chromium.org/8689003/diff/1/ui/aura_shell/shell_accelerator... ui/aura_shell/shell_accelerator_controller.h:61: std::map<ui::Accelerator, int> accelerators_; On 2011/11/28 16:39:54, Daniel Erat wrote: > add a comment describing what the int value represents Done.
LGTM with a few comments http://codereview.chromium.org/8689003/diff/1/ui/aura_shell/shell_accelerator... File ui/aura_shell/shell_accelerator_controller.cc (right): http://codereview.chromium.org/8689003/diff/1/ui/aura_shell/shell_accelerator... ui/aura_shell/shell_accelerator_controller.cc:30: kToggleMaximized, On 2011/11/29 15:19:35, mazda wrote: > On 2011/11/28 16:39:54, Daniel Erat wrote: > > many of these are related to UI that i don't think is going to be implemented > in > > aura. i think that it'd be better to hold off on adding them here until > they're > > needed. i would say that we should stick with just kCycleBackward and > > kCycleForward (which is misspelled as "Fowrard" here) for now. > > OK. I hold off most of them for now. > We also need TakeRootScreenshot? There is already an issue > (http://crbug.com/105198). yeah, i'm okay with leaving screenshot-taking in since we'll need it. maybe just rename it to TAKE_SCREENSHOT for now? http://codereview.chromium.org/8689003/diff/8001/ui/aura_shell/shell_accelera... File ui/aura_shell/shell_accelerator_controller.cc (right): http://codereview.chromium.org/8689003/diff/8001/ui/aura_shell/shell_accelera... ui/aura_shell/shell_accelerator_controller.cc:24: TOGGLE_FULL_SCREEN, i don't understand why we need the fullscreen action -- it's already defined in chrome/browser/ui/views/accelerator_table.cc
> yeah, i'm okay with leaving screenshot-taking in since we'll need it. maybe > just rename it to TAKE_SCREENSHOT for now? Renamed it to TAKE_SCREENSHOT. I also added comments on issue numbers in the placeholder functions. Thanks, http://codereview.chromium.org/8689003/diff/8001/ui/aura_shell/shell_accelera... File ui/aura_shell/shell_accelerator_controller.cc (right): http://codereview.chromium.org/8689003/diff/8001/ui/aura_shell/shell_accelera... ui/aura_shell/shell_accelerator_controller.cc:24: TOGGLE_FULL_SCREEN, On 2011/11/29 16:46:00, Daniel Erat wrote: > i don't understand why we need the fullscreen action -- it's already defined in > chrome/browser/ui/views/accelerator_table.cc This action is a bit different from the fullscreen action defined in accelerator_table.cc. This action maximizes aura::Desktop while the fullscreen action defined in accelerator_table.cc maximizes a chrome tab. But I agree that they are confusing and they conflict on Windows, so I think it's better to change the name and key binding. What do you think of changing them as follows? enum name: TOGGLE_DESKTOP_FULL_SCREEN key binding: Ctrl F11 I changed them that way in the updated CL.
lgtm
http://codereview.chromium.org/8689003/diff/8001/ui/aura_shell/shell_accelera... File ui/aura_shell/shell_accelerator_controller.cc (right): http://codereview.chromium.org/8689003/diff/8001/ui/aura_shell/shell_accelera... ui/aura_shell/shell_accelerator_controller.cc:24: TOGGLE_FULL_SCREEN, On 2011/11/30 14:56:10, mazda wrote: > On 2011/11/29 16:46:00, Daniel Erat wrote: > > i don't understand why we need the fullscreen action -- it's already defined > in > > chrome/browser/ui/views/accelerator_table.cc > > This action is a bit different from the fullscreen action defined in > accelerator_table.cc. > This action maximizes aura::Desktop while the fullscreen action defined in > accelerator_table.cc maximizes a chrome tab. > But I agree that they are confusing and they conflict on Windows, so I think > it's better to change the name and key binding. > > What do you think of changing them as follows? > > enum name: TOGGLE_DESKTOP_FULL_SCREEN > key binding: Ctrl F11 > > I changed them that way in the updated CL. Thanks, makes more sense now. http://codereview.chromium.org/8689003/diff/13001/ui/aura_shell/shell_acceler... File ui/aura_shell/shell_accelerator_controller.cc (right): http://codereview.chromium.org/8689003/diff/13001/ui/aura_shell/shell_acceler... ui/aura_shell/shell_accelerator_controller.cc:21: CYCLE_FORWRARD, typo: FORWRARD -> FORWARD http://codereview.chromium.org/8689003/diff/13001/ui/aura_shell/shell_acceler... ui/aura_shell/shell_accelerator_controller.cc:47: bool CycleBackward() { do we really need return values for all of these, or are you just doing it so you can return directly from the case statements in AcceleratorPressed()? (presumably the return value controls whether we stop event propagation or not, but at the time when we're actually trying to perform these actions, i think that we always want to stop propagation, right?)
http://codereview.chromium.org/8689003/diff/13001/ui/aura_shell/shell_acceler... File ui/aura_shell/shell_accelerator_controller.cc (right): http://codereview.chromium.org/8689003/diff/13001/ui/aura_shell/shell_acceler... ui/aura_shell/shell_accelerator_controller.cc:21: CYCLE_FORWRARD, On 2011/11/30 16:25:31, Daniel Erat wrote: > typo: FORWRARD -> FORWARD Oh... Thanks again. http://codereview.chromium.org/8689003/diff/13001/ui/aura_shell/shell_acceler... ui/aura_shell/shell_accelerator_controller.cc:47: bool CycleBackward() { On 2011/11/30 16:25:31, Daniel Erat wrote: > do we really need return values for all of these, or are you just doing it so > you can return directly from the case statements in AcceleratorPressed()? > (presumably the return value controls whether we stop event propagation or not, > but at the time when we're actually trying to perform these actions, i think > that we always want to stop propagation, right?) Right. It's mostly for convenience of using them in the case statements as you mentioned, and the return value controls event propagation. I also thought it might be useful to enable each function to choose not handling the accelerator under certain condition. But I cannot think of a concrete case so far, so I'm not stick with the idea of returning bool value.
http://codereview.chromium.org/8689003/diff/13001/ui/aura_shell/shell_acceler... File ui/aura_shell/shell_accelerator_controller.cc (right): http://codereview.chromium.org/8689003/diff/13001/ui/aura_shell/shell_acceler... ui/aura_shell/shell_accelerator_controller.cc:47: bool CycleBackward() { On 2011/11/30 18:36:41, mazda wrote: > On 2011/11/30 16:25:31, Daniel Erat wrote: > > do we really need return values for all of these, or are you just doing it so > > you can return directly from the case statements in AcceleratorPressed()? > > (presumably the return value controls whether we stop event propagation or > not, > > but at the time when we're actually trying to perform these actions, i think > > that we always want to stop propagation, right?) > > Right. It's mostly for convenience of using them in the case statements as you > mentioned, and the return value controls event propagation. > I also thought it might be useful to enable each function to choose not handling > the accelerator under certain condition. But I cannot think of a concrete case > so far, so I'm not stick with the idea of returning bool value. I'd be more okay with the return values if these methods had names like HandleCycleBackward() and HandleTakeScreenshot(), I think. Without any comments saying otherwise, when a function has a bool return value, I assume that it's going to return true on success and false otherwise. If taking a screenshot fails for some reason, we'd still want propagation to stop, though. (Also, I'll point out that even though you're saving a break or return for each of the commands in AcceleratorPressed(), those lines are getting added back as a "return true" at the end of each of these methods.) :-P http://codereview.chromium.org/8689003/diff/16007/ui/aura_shell/shell_acceler... File ui/aura_shell/shell_accelerator_controller.cc (right): http://codereview.chromium.org/8689003/diff/16007/ui/aura_shell/shell_acceler... ui/aura_shell/shell_accelerator_controller.cc:53: bool CycleForwrard() { one more (extra 'r' after the 'w') :-) http://codereview.chromium.org/8689003/diff/16007/ui/aura_shell/shell_acceler... File ui/aura_shell/shell_accelerator_controller.h (right): http://codereview.chromium.org/8689003/diff/16007/ui/aura_shell/shell_acceler... ui/aura_shell/shell_accelerator_controller.h:62: // A map from accelerators to action IDs used in the implementation. instead of "action IDs", mind saying "AcceleratorAction values", just to make it really clear that these aren't IDC_ defines from chrome/app/chrome_command_ids.h?
> I'd be more okay with the return values if these methods had names like > HandleCycleBackward() and HandleTakeScreenshot(), I think. Without any comments > saying otherwise, when a function has a bool return value, I assume that it's > going to return true on success and false otherwise. If taking a screenshot > fails for some reason, we'd still want propagation to stop, though. Changed the names to Handle*. Thanks. http://codereview.chromium.org/8689003/diff/16007/ui/aura_shell/shell_acceler... File ui/aura_shell/shell_accelerator_controller.cc (right): http://codereview.chromium.org/8689003/diff/16007/ui/aura_shell/shell_acceler... ui/aura_shell/shell_accelerator_controller.cc:53: bool CycleForwrard() { On 2011/11/30 20:47:36, Daniel Erat wrote: > one more (extra 'r' after the 'w') :-) Done... http://codereview.chromium.org/8689003/diff/16007/ui/aura_shell/shell_acceler... File ui/aura_shell/shell_accelerator_controller.h (right): http://codereview.chromium.org/8689003/diff/16007/ui/aura_shell/shell_acceler... ui/aura_shell/shell_accelerator_controller.h:62: // A map from accelerators to action IDs used in the implementation. On 2011/11/30 20:47:36, Daniel Erat wrote: > instead of "action IDs", mind saying "AcceleratorAction values", just to make it > really clear that these aren't IDC_ defines from > chrome/app/chrome_command_ids.h? Done.
lgtm http://codereview.chromium.org/8689003/diff/21001/ui/aura_shell/shell_acceler... File ui/aura_shell/shell_accelerator_controller.cc (right): http://codereview.chromium.org/8689003/diff/21001/ui/aura_shell/shell_acceler... ui/aura_shell/shell_accelerator_controller.cc:156: std::map<ui::Accelerator, int>::const_iterator i = nit: i've seen 'it' used for iterators more frequently (with 'i' reserved for int indexes used in loops) http://codereview.chromium.org/8689003/diff/21001/ui/aura_shell/shell_acceler... ui/aura_shell/shell_accelerator_controller.cc:173: NOTREACHED(); nit: NOTREACHED() << "Unhandled action " << it->second;
Fixed nits. Thanks! http://codereview.chromium.org/8689003/diff/21001/ui/aura_shell/shell_acceler... File ui/aura_shell/shell_accelerator_controller.cc (right): http://codereview.chromium.org/8689003/diff/21001/ui/aura_shell/shell_acceler... ui/aura_shell/shell_accelerator_controller.cc:156: std::map<ui::Accelerator, int>::const_iterator i = On 2011/12/01 18:13:38, Daniel Erat wrote: > nit: i've seen 'it' used for iterators more frequently (with 'i' reserved for > int indexes used in loops) Done. http://codereview.chromium.org/8689003/diff/21001/ui/aura_shell/shell_acceler... ui/aura_shell/shell_accelerator_controller.cc:173: NOTREACHED(); On 2011/12/01 18:13:38, Daniel Erat wrote: > nit: NOTREACHED() << "Unhandled action " << it->second; Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mazda@chromium.org/8689003/23002
Change committed as 112522 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
