Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2074643003:
MacViews: Views accelerators table should match the Cocoa one. (Closed)
|
Created:
4 years, 6 months ago by themblsha Modified:
4 years, 1 month ago CC:
chromium-reviews, tfarina, yusukes+watch_chromium.org, derat+watch_chromium.org Base URL:
ssh://bitbucket.browser.yandex-team.ru/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMacViews: Views accelerators table should match the Cocoa one.
Updated views/accelerator_table.cc to match accelerators_cocoa.mm. Added
eligible shortcuts from global_keyboard_shortcuts_mac.mm as well.
Also handle the case when the user configured cmd-left to mean "previous tab",
as it should take precedence over the built-in binding: we're consulting the
[NSApp mainMenu] using CommandForKeyEvent().
Lastly, the Cmd+{/} shortcuts are special as they would be impossible to
perform on German and French keyboard layouts if we would simply check for
keycodes as we do in accelerator_table.cc (crbug.com/25946).
In order to tackle last two problems I've added PreHandleKeyboardEvent and
HandleKeyboardEvent to the NativeBrowserFrame interface.
BUG=620315
Committed: https://crrev.com/cb9c6b9d9a764e89cb0703e48ddcfea3129e2ad3
Cr-Commit-Position: refs/heads/master@{#433574}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Fix review issues. #
Total comments: 31
Patch Set 3 : Add tapted's implementation + debugging. #
Total comments: 1
Patch Set 4 : Updated accelerator_table.cc, removed non-Cocoa shortcuts, added global_keyboard_shortcuts_mac.mm h… #
Total comments: 24
Patch Set 5 : Fix review issues. #
Total comments: 20
Patch Set 6 : Split global_keyboard_shortcuts_mac.mm into two platform-specific ones, simplified code. #Patch Set 7 : Split global_keyboard_shortcuts_mac.mm into two platform-specific ones, simplified code. #
Total comments: 29
Patch Set 8 : Fix review issues. #Patch Set 9 : Fix spelling in a comment. #
Total comments: 53
Patch Set 10 : Fixed a ton of review issues. #
Total comments: 4
Patch Set 11 : Restore kAcceleratorMap. #Patch Set 12 : e --> shortcut #
Total comments: 2
Patch Set 13 : Make (Pre)HandleKeyboardEvent pure virtual. #
Total comments: 18
Patch Set 14 : Fix sky's remarks. #Patch Set 15 : #ifdef out Ctrl shortcuts on Mac. #
Total comments: 13
Patch Set 16 : #ifdef out the Alt accelerators, sort the #if !Mac accelerators, fix includes. #
Total comments: 18
Patch Set 17 : Fix tapted's remarks. #Patch Set 18 : GetWindowKeyboardShortcutTable is now shared between Cocoa and MacViews. #
Total comments: 2
Patch Set 19 : Revert to Patch Set 17, rebase on top of master. #Patch Set 20 : Fix compilation. #Messages
Total messages: 88 (20 generated)
Description was changed from ========== MacViews: Views accelerators table should match the Cocoa one. Updated views/accelerator_table.cc to match accelerators_cocoa.mm. BUG=620315 ========== to ========== MacViews: Views accelerators table should match the Cocoa one. Updated views/accelerator_table.cc to match accelerators_cocoa.mm. BUG=620315 ==========
mblsha@yandex-team.ru changed reviewers: + tapted@chromium.org
What do you think?
https://codereview.chromium.org/2074643003/diff/1/chrome/browser/ui/views/acc... File chrome/browser/ui/views/accelerator_table.cc (right): https://codereview.chromium.org/2074643003/diff/1/chrome/browser/ui/views/acc... chrome/browser/ui/views/accelerator_table.cc:25: // TODO(jackhou): If-def out the accelerators that should not be on Mac. If things are complete after this, this TODO can probably be removed. https://codereview.chromium.org/2074643003/diff/1/chrome/browser/ui/views/acc... chrome/browser/ui/views/accelerator_table.cc:32: // NOTE: Keep this list in the same (mostly-alphabetical) order as Did you see this note? platform specific stuff should be down below under // Platform-specific key maps. The table becomes very hard to read with all these #ifdefs. Here's the approach I suggest: In GetAcceleratorList() or BrowserView::LoadAccelerators(), filter out any command using just `Alt` on Mac - there should be none of those. Try to only add stuff in the new #ifdef block you're adding under // Platform-specific key maps. If stuff still remains, just #ifdef it out, but have the `#else` part in the single #if (mac) block. https://codereview.chromium.org/2074643003/diff/1/chrome/browser/ui/views/acc... chrome/browser/ui/views/accelerator_table.cc:38: { ui::VKEY_OEM_4, kPlatformModifier, IDC_BACK }, Stuff like OEM_4 needs a comment
https://codereview.chromium.org/2074643003/diff/1/chrome/browser/ui/views/acc... File chrome/browser/ui/views/accelerator_table.cc (right): https://codereview.chromium.org/2074643003/diff/1/chrome/browser/ui/views/acc... chrome/browser/ui/views/accelerator_table.cc:25: // TODO(jackhou): If-def out the accelerators that should not be on Mac. On 2016/06/17 01:05:01, tapted wrote: > If things are complete after this, this TODO can probably be removed. Done. https://codereview.chromium.org/2074643003/diff/1/chrome/browser/ui/views/acc... chrome/browser/ui/views/accelerator_table.cc:32: // NOTE: Keep this list in the same (mostly-alphabetical) order as On 2016/06/17 01:05:01, tapted wrote: > Did you see this note? platform specific stuff should be down below under // > Platform-specific key maps. I saw "#if defined(OS_LINUX) && !defined(OS_CHROMEOS)" and thought that this sort of grouping by the command id would be better. > The table becomes very hard to read with all these #ifdefs. Here's the approach > I suggest: > > In GetAcceleratorList() or BrowserView::LoadAccelerators(), filter out any > command using just `Alt` on Mac - there should be none of those. Try to only add > stuff in the new #ifdef block you're adding under // Platform-specific key maps. > > If stuff still remains, just #ifdef it out, but have the `#else` part in the > single #if (mac) block. I tried to minimize the non-platform Mac ifdefs, there are only two left. Current CL also adds a bunch of accelerators that weren't present in cocoa: IDC_CLOSE_TAB and IDC_SELECT_TAB_0 through IDC_SELECT_TAB_7. https://codereview.chromium.org/2074643003/diff/1/chrome/browser/ui/views/acc... chrome/browser/ui/views/accelerator_table.cc:38: { ui::VKEY_OEM_4, kPlatformModifier, IDC_BACK }, On 2016/06/17 01:05:01, tapted wrote: > Stuff like OEM_4 needs a comment Done.
https://codereview.chromium.org/2074643003/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/accelerator_table.cc (left): https://codereview.chromium.org/2074643003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/accelerator_table.cc:38: { ui::VKEY_BACK, ui::EF_NONE, IDC_BACKSPACE_BACK }, why this? It looks like you're removing backspace to navigate backwards (which.. is happening, but it should be orthogonal to this) https://codereview.chromium.org/2074643003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/accelerator_table.cc:55: { ui::VKEY_BACK, ui::EF_SHIFT_DOWN, IDC_BACKSPACE_FORWARD }, this too - I don't think this should be removed https://codereview.chromium.org/2074643003/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/accelerator_table.cc (right): https://codereview.chromium.org/2074643003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/accelerator_table.cc:69: { ui::VKEY_9, ui::EF_CONTROL_DOWN, IDC_SELECT_LAST_TAB }, why this change? Cmd+9 to select the last tab works on a Cocoa browser. Ctrl+9 isn't anything. https://codereview.chromium.org/2074643003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/accelerator_table.cc:127: { ui::VKEY_U, kPlatformModifier | ui::EF_ALT_DOWN, IDC_VIEW_SOURCE }, you can't just add alt here - it will change other platforms. This probably needs to be #ifdef'd out on Mac so we can add a version with alt in the mac section https://codereview.chromium.org/2074643003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/accelerator_table.cc:134: #if !defined(OS_MACOSX) nit: blank line above https://codereview.chromium.org/2074643003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/accelerator_table.cc:135: { ui::VKEY_F1, ui::EF_NONE, IDC_HELP_PAGE_VIA_KEYBOARD }, Comment here like // Function keys aren't mapped on Mac. https://codereview.chromium.org/2074643003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/accelerator_table.cc:171: // Mac-specific shortcuts are defined in a Mac section below. I don't think this is right - we should keep the #else and filter out things below that we can't have on Mac. i.e. the "else" here is ~"all platforms should have these, but ChromeOS doesn't because it's special" https://codereview.chromium.org/2074643003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/accelerator_table.cc:195: { ui::VKEY_T, ui::EF_CONTROL_DOWN, IDC_NEW_TAB }, e.g. this should just be changed like ui::EF_CONTROL_DOWN -> kPlatformModifier so we avoid duplication https://codereview.chromium.org/2074643003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/accelerator_table.cc:203: #if defined(ENABLE_BASIC_PRINTING) this is always true on Mac, so we don't need to check again https://codereview.chromium.org/2074643003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/accelerator_table.cc:208: { ui::VKEY_C, ui::EF_COMMAND_DOWN, IDC_CONTENT_CONTEXT_COPY }, this isn't mapped for other platforms - why does mac need it? (same for some others) https://codereview.chromium.org/2074643003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/accelerator_table.cc:279: static std::vector<AcceleratorMapping> accelerators; Chome doesn't allow statics of non-primitive type since it places a destructor call in the process shutdown path. You can use CR_DEFINE_STATIC_LOCAL instead https://codereview.chromium.org/2074643003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/accelerator_table.cc:281: accelerators = std::vector<AcceleratorMapping>( The idiomatic way to do this is probably accelerators.insert(accelerators.begin(), std::begin(kAcceleratorMap), std::end(kAcceleratorMap)); although I guess assigning an rvalue will just call vector::swap too, but if you prefer that, it should still be constructed using std::end (and kAcceleratorMapLength isn't used for anything else so can be deleted) https://codereview.chromium.org/2074643003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/accelerator_table.cc:284: // Alt, Control and accelerators without modifiers are not very typical for I think we need a "harder" rule here for Mac -- we can't just say it's not typical since it starts to rely on coincidence with the current set of accelerators. E.g. we can say "Alt by itself (or with just shift) is never used on Mac since it's used to generate non-ASCII characters. Such commands are given Mac-specific bindings as well, so remove the mappings with Alt, but not those with Command or Control. then const kMask = ui::EF_COMMAND_DOWN | ui::EF_CONTROL_DOWN | ui::EF_ALT_DOWN; auto remove_start = remove_if(...[](..) { return (m.modifiers & kMask) == ui::EF_ALT_DOWN; } then we should have something like #if DCHECK_IS_ON() // For everything being removed, ensure there's another mapping. for (auto it = remove_start; it != accelerators.end(); ++it) { auto found = std::find_if(accelerators.start(), remove_start, [](const AcceleratorMapping& m) { return m.command_id == it->command_id; }); DCHECK(found != accelerators.end()); } #endif accelerators.erase(..) I'm undecided about the DCHECK_IS_ON bit - owners might not like it. A test might be a nicer place for it, but that requires exposing some extra stuff. Also, there might be some annoying cases that fail, but I think it's worth trying. https://codereview.chromium.org/2074643003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/accelerator_table.cc:293: return m.modifiers == ui::EF_NONE || why ui::EF_NONE ? Seems we might want those - most of those we don't are CrOS-specific and already filtered out. https://codereview.chromium.org/2074643003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/accelerator_table.cc:295: m.modifiers & ui::EF_CONTROL_DOWN; Ctrl + PageUp switches tabs on Mac, so this doesn't look right.
Here's a sorted command output from both |accelerators| and |accelerators_mblsha| so you could diff it locally: http://pastie.org/private/o74xdcawspggmigvniz1oq I'd say this could be solved by more #ifdefs, but it would be better than creating a new map altogether. https://codereview.chromium.org/2074643003/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/accelerator_table.cc (left): https://codereview.chromium.org/2074643003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/accelerator_table.cc:38: { ui::VKEY_BACK, ui::EF_NONE, IDC_BACKSPACE_BACK }, On 2016/06/22 03:06:09, tapted wrote: > why this? It looks like you're removing backspace to navigate backwards > (which.. is happening, but it should be orthogonal to this) No good reason, probably clumsiness on my part: this was done in the original patchset. https://codereview.chromium.org/2074643003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/accelerator_table.cc:55: { ui::VKEY_BACK, ui::EF_SHIFT_DOWN, IDC_BACKSPACE_FORWARD }, On 2016/06/22 03:06:09, tapted wrote: > this too - I don't think this should be removed Thanks! https://codereview.chromium.org/2074643003/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/accelerator_table.cc (right): https://codereview.chromium.org/2074643003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/accelerator_table.cc:69: { ui::VKEY_9, ui::EF_CONTROL_DOWN, IDC_SELECT_LAST_TAB }, On 2016/06/22 03:06:10, tapted wrote: > why this change? Cmd+9 to select the last tab works on a Cocoa browser. Ctrl+9 > isn't anything. Because it isn't present in the accelerators_cocoa.mm. Updated to use kPlatformModifier as well. https://codereview.chromium.org/2074643003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/accelerator_table.cc:127: { ui::VKEY_U, kPlatformModifier | ui::EF_ALT_DOWN, IDC_VIEW_SOURCE }, On 2016/06/22 03:06:09, tapted wrote: > you can't just add alt here - it will change other platforms. This probably > needs to be #ifdef'd out on Mac so we can add a version with alt in the mac > section It was that same original commit at fault again. Should've pay it more attention :-/ https://codereview.chromium.org/2074643003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/accelerator_table.cc:134: #if !defined(OS_MACOSX) On 2016/06/22 03:06:09, tapted wrote: > nit: blank line above Done. https://codereview.chromium.org/2074643003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/accelerator_table.cc:135: { ui::VKEY_F1, ui::EF_NONE, IDC_HELP_PAGE_VIA_KEYBOARD }, On 2016/06/22 03:06:10, tapted wrote: > Comment here like > // Function keys aren't mapped on Mac. Done. https://codereview.chromium.org/2074643003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/accelerator_table.cc:171: // Mac-specific shortcuts are defined in a Mac section below. On 2016/06/22 03:06:09, tapted wrote: > I don't think this is right - we should keep the #else and filter out things > below that we can't have on Mac. i.e. the "else" here is ~"all platforms should > have these, but ChromeOS doesn't because it's special" Ok, added a few more rules to the GetAcceleratorList() to ignore the non-Mac ones and removed this #elif. https://codereview.chromium.org/2074643003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/accelerator_table.cc:195: { ui::VKEY_T, ui::EF_CONTROL_DOWN, IDC_NEW_TAB }, On 2016/06/22 03:06:10, tapted wrote: > e.g. this should just be changed like ui::EF_CONTROL_DOWN -> kPlatformModifier > so we avoid duplication Good idea, squashed a couple more of duplicate items. https://codereview.chromium.org/2074643003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/accelerator_table.cc:203: #if defined(ENABLE_BASIC_PRINTING) On 2016/06/22 03:06:09, tapted wrote: > this is always true on Mac, so we don't need to check again Done. https://codereview.chromium.org/2074643003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/accelerator_table.cc:208: { ui::VKEY_C, ui::EF_COMMAND_DOWN, IDC_CONTENT_CONTEXT_COPY }, On 2016/06/22 03:06:09, tapted wrote: > this isn't mapped for other platforms - why does mac need it? (same for some > others) This was implemented in https://codereview.chromium.org/152643007 for https://codereview.chromium.org/161293002 So this is a static mapping of MainMenu.xib commands. https://codereview.chromium.org/2074643003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/accelerator_table.cc:279: static std::vector<AcceleratorMapping> accelerators; On 2016/06/22 03:06:09, tapted wrote: > Chome doesn't allow statics of non-primitive type since it places a destructor > call in the process shutdown path. You can use CR_DEFINE_STATIC_LOCAL instead I thought there was a compile-time warning for this in the Chromium-Style plugin? Done. https://codereview.chromium.org/2074643003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/accelerator_table.cc:281: accelerators = std::vector<AcceleratorMapping>( On 2016/06/22 03:06:09, tapted wrote: > The idiomatic way to do this is probably > > accelerators.insert(accelerators.begin(), std::begin(kAcceleratorMap), > std::end(kAcceleratorMap)); > > although I guess assigning an rvalue will just call vector::swap too, but if you > prefer that, it should still be constructed using std::end (and > kAcceleratorMapLength isn't used for anything else so can be deleted) No real preference there, thanks for the suggestion. https://codereview.chromium.org/2074643003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/accelerator_table.cc:293: return m.modifiers == ui::EF_NONE || On 2016/06/22 03:06:09, tapted wrote: > why ui::EF_NONE ? Seems we might want those - most of those we don't are > CrOS-specific and already filtered out. Good ones seem to be: IDC_STOP 33006 modifiers:; keycode:27 IDC_BACKSPACE_BACK 33010 modifiers:; keycode:8 But there are a bunch of unwanted ones: IDC_FOCUS_SEARCH 39002 modifiers:; keycode:170 IDC_FOCUS_MENU_BAR 39003 modifiers:; keycode:164 IDC_FOCUS_MENU_BAR 39003 modifiers:; keycode:165 IDC_FOCUS_MENU_BAR 39003 modifiers:; keycode:18 IDC_DEV_TOOLS_TOGGLE 40237 modifiers:; keycode:123 Is #if !defined(OS_MACOSX) advisable in that case? I tried to minimize these by using a custom find_if matcher in my code. https://codereview.chromium.org/2074643003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/accelerator_table.cc:295: m.modifiers & ui::EF_CONTROL_DOWN; On 2016/06/22 03:06:10, tapted wrote: > Ctrl + PageUp switches tabs on Mac, so this doesn't look right. Good ones seem to be: IDC_SELECT_NEXT_TAB 34016 modifiers:(Ctrl); keycode:34 IDC_SELECT_NEXT_TAB 34016 modifiers:(Ctrl); keycode:9 IDC_SELECT_PREVIOUS_TAB 34017 modifiers:(Ctrl)(Shift); keycode:9 IDC_SELECT_PREVIOUS_TAB 34017 modifiers:(Ctrl); keycode:33 But there seems to be a lot of unwanted ones: IDC_EXIT 34031 modifiers:(Ctrl)(Shift); keycode:81 IDC_VIEW_SOURCE 35002 modifiers:(Ctrl); keycode:85 IDC_BASIC_PRINT 35007 modifiers:(Ctrl)(Shift); keycode:80 IDC_ZOOM_PLUS 38001 modifiers:(Ctrl)(Shift); keycode:187 IDC_ZOOM_MINUS 38003 modifiers:(Ctrl)(Shift); keycode:189 IDC_FOCUS_SEARCH 39002 modifiers:(Ctrl); keycode:69 IDC_FOCUS_SEARCH 39002 modifiers:(Ctrl); keycode:75 IDC_DEV_TOOLS 40004 modifiers:(Ctrl)(Shift); keycode:73 IDC_DEV_TOOLS_CONSOLE 40005 modifiers:(Ctrl)(Shift); keycode:74 IDC_SHOW_HISTORY 40010 modifiers:(Ctrl); keycode:72 IDC_SHOW_BOOKMARK_MANAGER 40011 modifiers:(Ctrl)(Shift); keycode:79 IDC_SHOW_DOWNLOADS 40012 modifiers:(Ctrl); keycode:74 IDC_CLEAR_BROWSING_DATA 40013 modifiers:(Ctrl)(Shift); keycode:46 IDC_DEV_TOOLS_INSPECT 40023 modifiers:(Ctrl)(Shift); keycode:67
https://codereview.chromium.org/2074643003/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/accelerator_table.cc (right): https://codereview.chromium.org/2074643003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/accelerator_table.cc:69: { ui::VKEY_9, ui::EF_CONTROL_DOWN, IDC_SELECT_LAST_TAB }, On 2016/06/24 18:22:15, themblsha wrote: > On 2016/06/22 03:06:10, tapted wrote: > > why this change? Cmd+9 to select the last tab works on a Cocoa browser. Ctrl+9 > > isn't anything. > > Because it isn't present in the accelerators_cocoa.mm. Updated to use > kPlatformModifier as well. This isn't a good rationale. How does Cmd+9 work on Cocoa without the mapping? (is it mapped somewhere else?). Is Cmd+9 actually broken on mac_views_browser? Does adding this fix it? I guess this rationale needs to apply to the CL in general, and the bug you've filed. It's not enough to say "this doesn't match accelerators_cocoa.mm". We also need to identify that having them not match is causing a problem, and that mapping them all fixes all the problems. You need to test all the accelerators you're adding; confirm that they currently don't work, and that they function correctly after mapping them. It's possible there's other hidden stuff. Like "this key is intercepted elsewhere somehow so mapping here doesn't work". We need to verify that the changes being added are functional and useful, and not just say "make it look the same as accelerators_cocoa.mm" (i.e. we don't know whether mimicing accelerators_cocoa.mm in this way is the correct way to fix the underlying issues, because we haven't even properly identified what those underlying issues are). https://codereview.chromium.org/2074643003/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/accelerator_table.cc:279: static std::vector<AcceleratorMapping> accelerators; On 2016/06/24 18:22:15, themblsha wrote: > On 2016/06/22 03:06:09, tapted wrote: > > Chome doesn't allow statics of non-primitive type since it places a destructor > > call in the process shutdown path. You can use CR_DEFINE_STATIC_LOCAL instead > > I thought there was a compile-time warning for this in the Chromium-Style > plugin? Done. There probably should be :). We detect static initializers on globals with a script running in the waterfall, which picks them up, but very late. However, AFAIK, there's nothing for statics local to a function. https://codereview.chromium.org/2074643003/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/accelerator_table.cc (right): https://codereview.chromium.org/2074643003/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/accelerator_table.cc:205: #if defined(OS_MACOSX) Something weird has happened with the diff - I think it's been based on a local branch now
coincidentally, I was looking at some code involved with this today for http://crbug.com/19792 and it made me think of a few things - Ctrl+PgUp is probably handled by a mapping in global_keyboard_shortcuts_mac.mm and chrome_command_dispatcher_delegate.mm . mac_views_browser's browser window already sets up the plumbing for this in BrowserFrameMac::CreateNSWindow(..), so expect things in global_keyboard_shortcuts_mac.mm are already working fine for mac_views_browser - Activating other things in the mainMenu need to "flash" the top-level menu that they're in when the accelerator is pressed -- adding a mapping in the views accelerator table as well might not do this properly - things appearing in the mainMenu might not need anything in the accelerator table at all, but the command dispatching for those might have some holes for a mac_views_browser BrowserFrame. This might mean some commands we are converting to use Ctrl should instead just be removed. - maybe we can just keep accelerators_cocoa as is, and use it on mac_views_browser for the info it provides, but not actually use it to process commands -- ui/base/cocoa/command_dispatcher, chrome_command_dispatcher_delegate.mm and BrowserWindowCommandHandler exist for this So, e.g., maybe a good first step should be just to remove things from accelerator_table.cc that are "wrong" on Mac (e.g. things with Alt) AND that have a corresponding item in the mainMenu that is appropriately handled by BrowserWindowCommandHandler already, and that work in mac_views_browser already (e.g. flashing the mainMenu). Then we can follow-up with adding mappings for commands that are missing a handler for some reason. But let's start with a more focussed problem. E.g. the CL description should be something like "Ctrl+t shouldn't open a tab on mac_views_browser - only Cmd+t should - and that's already handled by <thing>. Allowing <thing> to handle it ensures the mainMenu on Mac flashes appropriately to give the user feedback. Because of this accelerator_table.cc doesn't need the `open new tab` mapping on Mac and there's no downside to removing it [CHECK!]. Also do this for commands foo, bar, baz, etc." Hope that helps! And sorry for not picking up on this sooner. I'd hoped making the CL easier to follow would make it clearer that the changes was needed, but I don't think that's the case.
On 2016/06/28 05:19:55, tapted wrote: > > Because it isn't present in the accelerators_cocoa.mm. Updated to use > > kPlatformModifier as well. > > This isn't a good rationale. How does Cmd+9 work on Cocoa without the mapping? > (is it mapped somewhere else?). Is Cmd+9 actually broken on mac_views_browser? > Does adding this fix it? CommandForWindowKeyboardShortcut at chrome/browser/global_keyboard_shortcuts_mac.mm is being used in both Cocoa. It lists some shortcuts that should work then tab contents is focused, and some when it's not. Same table is also used in MacViews, but only when the tab contents don't have focus, otherwise a different code path is used altogether. I tried commenting out the GetWindowKeyboardShortcutTable table, and it results in weird problems when trying to invoke shortcuts with activated omnibox. Will try again tomorrow to get a better understanding how this whole thing works.
Finally had the chance to check how the global_keyboard_shortcuts_mac.mm tables work (https://codereview.chromium.org/1255783002). Turns out they're still called in MacViews, but only when handling NativeWidgetMacNSWindow's performKeyEquivalent:. Here's how the tables are called in practice (inverted callstack-style): CommandForBrowserKeyboardShortcut: * HandleExtraBrowserKeyboardShortcut * -[ChromeCommandDispatcherDelegate handleExtraKeyboardShortcut:] * -[BrowserWindowUtils handleKeyboardEvent:inWindow:] * BrowserWindowCocoa::PreHandleKeyboardEvent * BrowserWindowCocoa::HandleKeyboardEvent * -[PermissionBubbleWindow performKeyEquivalent:] CommandForWindowKeyboardShortcut: * HandleExtraWindowKeyboardShortcut * -[ChromeCommandDispatcherDelegate prePerformKeyEquivalent:] * -[NativeWidgetMacNSWindow performKeyEquivalent:] * -[ChromeEventProcessingWindow performKeyEquivalent:] * -[ChromeCommandDispatcherDelegate handleExtraKeyboardShortcut:] * same as with CommandForBrowserKeyboardShortcut CommandForDelayedWindowKeyboardShortcut: * HandleDelayedWindowKeyboardShortcut * -[ChromeCommandDispatcherDelegate prePerformKeyEquivalent:] * -[NativeWidgetMacNSWindow performKeyEquivalent:] * -[ChromeEventProcessingWindow performKeyEquivalent:] * -[ChromeCommandDispatcherDelegate handleExtraKeyboardShortcut:] * same as with CommandForBrowserKeyboardShortcut They are not called when the shortcut is pressed while the WebView is in focus, as such events are handled in BrowserView::PreHandleKeyboardEvent and BrowserView::HandleKeyboardEvent, and they know nothing of global_keyboard_shortcuts_mac.mm. So I've implemented BrowserViewPlatform helper class to help route the events there. But it seems that the only unique remaining raison d'etre of global_keyboard_shortcuts_mac.mm seems to be the handling of ⌘{ / ⌘} commands, as they could be pressed as ⌘⌥8 / ⌘⌥9 on German keyboard layout (and French is even weirder). accelerator_table.cc seems to handle all other shortcuts just fine. The remaining thing that still puzzles me is CommandForKeyEvent: https://cs.chromium.org/search/?q=CommandForKeyEvent&sq=package:chromium&type=cs, it's used for chrome::IsChromeAccelerator on MacViews (and I've used it in BrowserViewPlatform). To just handle the ⌘{ / ⌘} shortcuts the logic in browser_view_platform_mac.mm's HandleExtraKeyboardShortcut is fully sufficient.
nice - this is looking really neat. Hopefully we can make better use of NativeBrowserFrame - see comments. Also we should start moving some of the key points into the CL description before sending to owners -- e.g. the Use Case for user-customization of the shortcuts via System Preferences is an interesting one. https://codereview.chromium.org/2074643003/diff/60001/chrome/browser/global_k... File chrome/browser/global_keyboard_shortcuts_mac.mm (right): https://codereview.chromium.org/2074643003/diff/60001/chrome/browser/global_k... chrome/browser/global_keyboard_shortcuts_mac.mm:55: // {false, false, true, false, kVK_PageDown, 0, IDC_SELECT_NEXT_TAB}, If removing these makes sense for the regular Chrome Cocoa browser, then we need a separate CL for that. https://codereview.chromium.org/2074643003/diff/60001/chrome/browser/global_k... chrome/browser/global_keyboard_shortcuts_mac.mm:80: // FIXME(mblsha): Hmm, I can't trigger downloads in Cocoa version on do you mean it doesn't work in the Chrome Canary? It works for me - Cmd+Alt+l opens a chrome://downloads tab. But it's possible Chrome does the wrong thing for non-US keyboard layouts. https://codereview.chromium.org/2074643003/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/accelerator_table.cc (right): https://codereview.chromium.org/2074643003/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/accelerator_table.cc:297: CR_DEFINE_STATIC_LOCAL(std::vector<AcceleratorMapping>, accelerators, ()); Since this is for testing, there's no need to reserve space in the data segment for this. Can this just do return std:vector<..>(std::begin(kAcceleratorMap), std::end(kAcceleratorMap)) ? https://codereview.chromium.org/2074643003/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/accelerator_table.cc:307: if (accelerators.empty()) { nit: early exit instead if (!accelerators.empty()) return accelerators; https://codereview.chromium.org/2074643003/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/accelerator_table.cc:335: return control_whitelist.find(m.command_id) == nit: return control_whitelist.count(m.command_id) == 0; (and optionally combine the two, like return (m.modifiers & ui::EF_CONTROL_DOWN) != 0 && control_whitelist.count(m.command_id) == 0; https://codereview.chromium.org/2074643003/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/accelerator_table.cc:366: // anywhere else. update comment? (i.e. Cmd+X is defined above for Mac - perhaps comment there as well, like "Even though Ctrl+X is not mapped on other platforms, it is mapped on Mac because/to <reason>" https://codereview.chromium.org/2074643003/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/browser_view.cc (right): https://codereview.chromium.org/2074643003/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_view.cc:1421: if (BrowserViewPlatform::PreHandleKeyboardEvent(event, browser())) I'm not an owner here, but based on the surrounding code, it might be OK to put this in #ifdef. That would allow an in-place comment like #if defined(OS_MACOSX) // On Mac, users can configure their own shortcuts for commands in the // mainMenu via System Preferences (even while Chrome is running). So // first scan through system menus and possibly handle the event natively // so the menu bar flashes. ... #endif But! There's actually already a mechanism for adding platform-specific codepaths using the |native_browser_frame_| member. So I think PreHandleKeyboardEvent probably belongs in native_browser_frame.h (and probably get a native_browser_frame.cc so that we can return false everywhere but mac). But we'd need to defer to whatever an OWNER wants -- pkasting or sky edit: based on later code - maybe NativeBrowserFrame would allow a lot more code reuse here https://codereview.chromium.org/2074643003/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_view.cc:1464: unhandled_keyboard_event_handler_.HandleKeyboardEvent(event, There is already a mac-specific UnhandledKeyboardEventHandler -- can we reuse that here? Or comment why we need the separate HandleKeyboardEvent for Mac https://codereview.chromium.org/2074643003/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/browser_view_platform.h (right): https://codereview.chromium.org/2074643003/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_view_platform.h:23: // Returns true if the |event| was handled by the platform implementation. nit: blank line before https://codereview.chromium.org/2074643003/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/browser_view_platform_mac.mm (right): https://codereview.chromium.org/2074643003/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_view_platform_mac.mm:23: bool HandleExtraKeyboardShortcut( We shouldn't copy all this stuff. Is there a way to move/re-use the stuff already in chrome_command_dispatcher_delegate.mm ? browser_frame_mac.mm is already installing a ChromeCommandDispatcherDelegate -- can we access it via |BrowserFrame::native_browser_frame_|? https://codereview.chromium.org/2074643003/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_view_platform_mac.mm:96: // static. Copied from BrowserWindowCocoa::PreHandleKeyboardEvent nit: Move Copied from.. into the function body. https://codereview.chromium.org/2074643003/diff/60001/chrome/test/BUILD.gn File chrome/test/BUILD.gn (right): https://codereview.chromium.org/2074643003/diff/60001/chrome/test/BUILD.gn#ne... chrome/test/BUILD.gn:4738: if (!is_mac) { This should be in a separate CL that cites a new bug. (There is http://crbug.com/412234 but that's to get toolkit-views unit_tests passing when mac_views_browser=false [in fact some of those in the block above need to be moved out of the !is_mac condition altogether for http://crbug.com/412234] But we still need a new bug to get tests up with mac_views_browser=true so we can cite it here to explain why these tests are excluded even when mac_views_browser=true)
Description was changed from ========== MacViews: Views accelerators table should match the Cocoa one. Updated views/accelerator_table.cc to match accelerators_cocoa.mm. BUG=620315 ========== to ========== MacViews: Views accelerators table should match the Cocoa one. Updated views/accelerator_table.cc to match accelerators_cocoa.mm. Also handle the case when the user configured cmd-left to mean "previous tab", as it should take precedence over the built-in binding. BUG=620315 ==========
https://codereview.chromium.org/2074643003/diff/60001/chrome/browser/global_k... File chrome/browser/global_keyboard_shortcuts_mac.mm (right): https://codereview.chromium.org/2074643003/diff/60001/chrome/browser/global_k... chrome/browser/global_keyboard_shortcuts_mac.mm:55: // {false, false, true, false, kVK_PageDown, 0, IDC_SELECT_NEXT_TAB}, On 2016/10/17 07:02:47, tapted wrote: > If removing these makes sense for the regular Chrome Cocoa browser, then we need > a separate CL for that. These tables are still required for Cocoa browser. Created a separate one to handle Cmd+Shift+CurlyBraces. I want to make the standard tables unusable on non-MacViews builds. What's the best way to ensure that? https://codereview.chromium.org/2074643003/diff/60001/chrome/browser/global_k... chrome/browser/global_keyboard_shortcuts_mac.mm:80: // FIXME(mblsha): Hmm, I can't trigger downloads in Cocoa version on On 2016/10/17 07:02:47, tapted wrote: > do you mean it doesn't work in the Chrome Canary? It works for me - Cmd+Alt+l > opens a chrome://downloads tab. But it's possible Chrome does the wrong thing > for non-US keyboard layouts. Whoops, weren't testing on the Canary. Added it to accelerators_table.cc. Thanks! https://codereview.chromium.org/2074643003/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/accelerator_table.cc (right): https://codereview.chromium.org/2074643003/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/accelerator_table.cc:297: CR_DEFINE_STATIC_LOCAL(std::vector<AcceleratorMapping>, accelerators, ()); On 2016/10/17 07:02:47, tapted wrote: > Since this is for testing, there's no need to reserve space in the data segment > for this. Can this just do return std:vector<..>(std::begin(kAcceleratorMap), > std::end(kAcceleratorMap)) ? Good idea! Fixed. https://codereview.chromium.org/2074643003/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/accelerator_table.cc:307: if (accelerators.empty()) { On 2016/10/17 07:02:47, tapted wrote: > nit: early exit instead > > if (!accelerators.empty()) > return accelerators; Done. https://codereview.chromium.org/2074643003/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/accelerator_table.cc:335: return control_whitelist.find(m.command_id) == On 2016/10/17 07:02:47, tapted wrote: > nit: return control_whitelist.count(m.command_id) == 0; > > (and optionally combine the two, like > > return (m.modifiers & ui::EF_CONTROL_DOWN) != 0 && > control_whitelist.count(m.command_id) == 0; Done. https://codereview.chromium.org/2074643003/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/accelerator_table.cc:366: // anywhere else. On 2016/10/17 07:02:47, tapted wrote: > update comment? (i.e. Cmd+X is defined above for Mac - perhaps comment there as > well, like "Even though Ctrl+X is not mapped on other platforms, it is mapped on > Mac because/to <reason>" Probably the IDC_CONTENT_CONTEXT_CUT / COPY / PASTE bindings are not needed in the MacViews after all. They were added to the accelerators_cocoa.mm to match the bindings in MainMenu.xib (https://codereview.chromium.org/152643007), and the relevant MainMenu.xib change is (https://chromiumcodereview.appspot.com/23005021). I removed them, and Cut / Copy / Paste still works fine within the web pages. https://codereview.chromium.org/2074643003/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/browser_view.cc (right): https://codereview.chromium.org/2074643003/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_view.cc:1421: if (BrowserViewPlatform::PreHandleKeyboardEvent(event, browser())) On 2016/10/17 07:02:47, tapted wrote: > I'm not an owner here, but based on the surrounding code, it might be OK to put > this in #ifdef. That would allow an in-place comment like > > #if defined(OS_MACOSX) > // On Mac, users can configure their own shortcuts for commands in the > // mainMenu via System Preferences (even while Chrome is running). So > // first scan through system menus and possibly handle the event natively > // so the menu bar flashes. > ... > #endif > > But! There's actually already a mechanism for adding platform-specific codepaths > using the |native_browser_frame_| member. > > So I think PreHandleKeyboardEvent probably belongs in native_browser_frame.h > (and probably get a native_browser_frame.cc so that we can return false > everywhere but mac). But we'd need to defer to whatever an OWNER wants -- > pkasting or sky > > edit: based on later code - maybe NativeBrowserFrame would allow a lot more code > reuse here Thanks! Moved the code there. https://codereview.chromium.org/2074643003/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_view.cc:1464: unhandled_keyboard_event_handler_.HandleKeyboardEvent(event, On 2016/10/17 07:02:47, tapted wrote: > There is already a mac-specific UnhandledKeyboardEventHandler -- can we reuse > that here? Or comment why we need the separate HandleKeyboardEvent for Mac Tried to do that: https://gist.github.com/mblsha/d3ddab85dfff543efe8d4b808ae203aa But got unresolved external linker error due to the use of BrowserView, and decided to leave this as is. https://codereview.chromium.org/2074643003/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/browser_view_platform.h (right): https://codereview.chromium.org/2074643003/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_view_platform.h:23: // Returns true if the |event| was handled by the platform implementation. On 2016/10/17 07:02:47, tapted wrote: > nit: blank line before Done. https://codereview.chromium.org/2074643003/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/browser_view_platform_mac.mm (right): https://codereview.chromium.org/2074643003/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_view_platform_mac.mm:23: bool HandleExtraKeyboardShortcut( On 2016/10/17 07:02:47, tapted wrote: > We shouldn't copy all this stuff. Is there a way to move/re-use the stuff > already in chrome_command_dispatcher_delegate.mm ? > > browser_frame_mac.mm is already installing a ChromeCommandDispatcherDelegate -- > can we access it via |BrowserFrame::native_browser_frame_|? Made this function public in chrome_command_dispatcher_delegate.mm. I've uncommented the shortcuts in global_keyboard_shortcuts_mac.mm, and they're mostly duplicating the ones in accelerator_table.cc. I think this duplication is bad, and ChromeCommandDispatcherDelegate on MacViews should behave differently, without consulting those tables (and consult the single MacViews-specific one). https://codereview.chromium.org/2074643003/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_view_platform_mac.mm:96: // static. Copied from BrowserWindowCocoa::PreHandleKeyboardEvent On 2016/10/17 07:02:47, tapted wrote: > nit: Move Copied from.. into the function body. Done. https://codereview.chromium.org/2074643003/diff/60001/chrome/test/BUILD.gn File chrome/test/BUILD.gn (right): https://codereview.chromium.org/2074643003/diff/60001/chrome/test/BUILD.gn#ne... chrome/test/BUILD.gn:4738: if (!is_mac) { On 2016/10/17 07:02:48, tapted wrote: > This should be in a separate CL that cites a new bug. (There is > http://crbug.com/412234 but that's to get toolkit-views unit_tests passing when > mac_views_browser=false [in fact some of those in the block above need to be > moved out of the !is_mac condition altogether for http://crbug.com/412234] But > we still need a new bug to get tests up with mac_views_browser=true so we can > cite it here to explain why these tests are excluded even when > mac_views_browser=true) Ok, will do this after updating the current CL :-)
https://codereview.chromium.org/2074643003/diff/80001/chrome/test/BUILD.gn File chrome/test/BUILD.gn (right): https://codereview.chromium.org/2074643003/diff/80001/chrome/test/BUILD.gn#ne... chrome/test/BUILD.gn:4701: if (!is_mac || mac_views_browser) { Created https://codereview.chromium.org/2440743002/
Ping?
Sorry for the delay. I wanted to patch this in and play around (some of my suggestions might not work), but I've been swamped. But I think it's looking pretty neat. OWNERS may have different opinions about the stuff here, so we need to make sure there are appropriate comments that justify why we're adding complexity to BrowserView/BrowserFrame etc. For example, it might help to add comments to the interfaces for PreHandleKeyboardEvent that mention custom keybindings in the native menubar. I haven't looked closely at the tests yet. I'll try to get to this tomorrow. Thanks! https://codereview.chromium.org/2074643003/diff/80001/chrome/browser/global_k... File chrome/browser/global_keyboard_shortcuts_mac.mm (right): https://codereview.chromium.org/2074643003/diff/80001/chrome/browser/global_k... chrome/browser/global_keyboard_shortcuts_mac.mm:40: // FIXME(mblsha): What's the best way to check for that, that won't trigger on We should just move the tables into separate files. E.g. chrome/browser/accelerator_table_cocoa_mac.mm/h chrome/browser/accelerator_table_views_mac.mm/h and then filter them out in the .gn files https://codereview.chromium.org/2074643003/diff/80001/chrome/browser/global_k... chrome/browser/global_keyboard_shortcuts_mac.mm:279: event, ^int(bool cmdKey, bool shiftKey, bool cntrlKey, bool optKey, I don't think this needs to be a block -- there's nothing captured. You could declare a helper method in an anonymous namespace and just pass a function pointer. I think even neater would probably just be something like a `bool check_windows_table`. Then move: CommandForBrowserKeyboardShortcut to accelerator_table_cocoa_mac.mm CommandForMacViewsKeyboardShortcut to accelerator_table_cocoa_mac.mm (but call it CommandForBrowserKeyboardShortcut) But also this might not work - it's very complex, and we need to simplify and add comments to make it less complex. (and ObjC blocks add particular complexity that only a few developers understand well, so are best avoided). https://codereview.chromium.org/2074643003/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/accelerator_table_unittest_mac.mm (right): https://codereview.chromium.org/2074643003/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/accelerator_table_unittest_mac.mm:176: // VerifyTable(GetWindowKeyboardShortcutTable, "WindowKeyboardShortcutTable"); We don't commit commented-out code. I'm guessing there's a story here - maybe a TODO "GetWindowKeyboardShortcutTable, GetDelayedWindowKeyboardShortcutTable, and GetBrowserKeyboardShortcutTable should also be verified, but they aren't because <reason> https://codereview.chromium.org/2074643003/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/browser_frame_mac.mm (right): https://codereview.chromium.org/2074643003/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_frame_mac.mm:22: bool ShouldHandleKeyboardEvent(const content::NativeWebKeyboardEvent& event) { comment? https://codereview.chromium.org/2074643003/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_frame_mac.mm:31: DCHECK(event.os_event != NULL); DCHECK(event.os_event) - this should probably be at the stat of the method https://codereview.chromium.org/2074643003/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_frame_mac.mm:143: // Copied from BrowserWindowCocoa::PreHandleKeyboardEvent(). BrowserWindowCocoa may not be with us forever, so this comment needs to be standalone. We need to capture "why Mac is special" here. i.e. why does BrowserFrameMac need special handling for weirdness that the Mac platform throws at us and other platforms don't.
https://codereview.chromium.org/2074643003/diff/80001/chrome/browser/global_k... File chrome/browser/global_keyboard_shortcuts_mac.mm (right): https://codereview.chromium.org/2074643003/diff/80001/chrome/browser/global_k... chrome/browser/global_keyboard_shortcuts_mac.mm:40: // FIXME(mblsha): What's the best way to check for that, that won't trigger on On 2016/10/26 08:40:47, tapted wrote: > We should just move the tables into separate files. E.g. > > chrome/browser/accelerator_table_cocoa_mac.mm/h > chrome/browser/accelerator_table_views_mac.mm/h > > and then filter them out in the .gn files The normal tables are also used by the -[ChromeCommandDispatcherDelegate handleExtraKeyboardShortcut:] * -[CommandDispatcher performKeyEquivalent:] * -[NativeWidgetMacNSWindow performKeyEquivalent:] So we probably need to create a Views-specific ChromeCommandDispatcherDelegate? https://codereview.chromium.org/2074643003/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/accelerator_table_unittest_mac.mm (right): https://codereview.chromium.org/2074643003/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/accelerator_table_unittest_mac.mm:176: // VerifyTable(GetWindowKeyboardShortcutTable, "WindowKeyboardShortcutTable"); On 2016/10/26 08:40:47, tapted wrote: > We don't commit commented-out code. I'm guessing there's a story here - maybe a > TODO "GetWindowKeyboardShortcutTable, GetDelayedWindowKeyboardShortcutTable, and > GetBrowserKeyboardShortcutTable should also be verified, but they aren't because > <reason> Yep, I should remove it in the final version. Ideally there would be no need for such tests if those tables are left out of the compilation. https://codereview.chromium.org/2074643003/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/browser_frame_mac.mm (right): https://codereview.chromium.org/2074643003/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_frame_mac.mm:22: bool ShouldHandleKeyboardEvent(const content::NativeWebKeyboardEvent& event) { On 2016/10/26 08:40:47, tapted wrote: > comment? It's adapted from +[BrowserWindowUtils shouldHandleKeyboardEvent:] plus one check from +handleKeyboardEvent:inWindow:, as I was trying to preserve the original Cocoa behaviour. Guess I need to research it more.
https://codereview.chromium.org/2074643003/diff/80001/chrome/browser/global_k... File chrome/browser/global_keyboard_shortcuts_mac.mm (right): https://codereview.chromium.org/2074643003/diff/80001/chrome/browser/global_k... chrome/browser/global_keyboard_shortcuts_mac.mm:40: // FIXME(mblsha): What's the best way to check for that, that won't trigger on On 2016/10/26 17:10:03, themblsha wrote: > On 2016/10/26 08:40:47, tapted wrote: > > We should just move the tables into separate files. E.g. > > > > chrome/browser/accelerator_table_cocoa_mac.mm/h > > chrome/browser/accelerator_table_views_mac.mm/h > > > > and then filter them out in the .gn files > > The normal tables are also used by the > > -[ChromeCommandDispatcherDelegate handleExtraKeyboardShortcut:] > * -[CommandDispatcher performKeyEquivalent:] > * -[NativeWidgetMacNSWindow performKeyEquivalent:] > > So we probably need to create a Views-specific ChromeCommandDispatcherDelegate? I think we can still use ChromeCommandDispatcherDelegate as-is. The trick is to link in the appropriate set of CommandFor{[Delayed]Window,Browser}KeyboardShortcut methods that make sense for mac_views_browser. So your current global_keyboard_shortcuts_mac.mm needs to split into 3 files global_keyboard_shortcuts_cocoa_mac.mm global_keyboard_shortcuts_views_mac.mm global_keyboard_shortcuts_mac.mm The sum of global_keyboard_shortcuts_cocoa_mac.mm and global_keyboard_shortcuts_mac.mm should be identical to what Chrome currently has. global_keyboard_shortcuts_views_mac.mm defines the versions of CommandFor{[Delayed]Window,Browser}KeyboardShortcut that mac_views_browser_wants global_keyboard_shortcuts_mac.mm holds the shared bits. https://codereview.chromium.org/2074643003/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/browser_frame_mac.mm (right): https://codereview.chromium.org/2074643003/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_frame_mac.mm:22: bool ShouldHandleKeyboardEvent(const content::NativeWebKeyboardEvent& event) { On 2016/10/26 17:10:03, themblsha wrote: > On 2016/10/26 08:40:47, tapted wrote: > > comment? > > It's adapted from +[BrowserWindowUtils shouldHandleKeyboardEvent:] plus one > check from +handleKeyboardEvent:inWindow:, as I was trying to preserve the > original Cocoa behaviour. Guess I need to research it more. Hm - it's possible that this and HandleExtraKeyboardShortcut should be absorbed into ChromeCommandDispatcherDelegate.. But yeah - we still need to nail down the role that ChromeCommandDispatcherDelegate is playing in the event sequence for a BrowserFrameMac. Perhaps my earlier suggestion will clear things up a bit. But currently it looks like `HandleExtraKeyboardShortcut` is being invoked one time too many for BrowserFrameMac
https://codereview.chromium.org/2074643003/diff/80001/chrome/browser/global_k... File chrome/browser/global_keyboard_shortcuts_mac.mm (right): https://codereview.chromium.org/2074643003/diff/80001/chrome/browser/global_k... chrome/browser/global_keyboard_shortcuts_mac.mm:40: // FIXME(mblsha): What's the best way to check for that, that won't trigger on On 2016/10/28 04:59:48, tapted wrote: > On 2016/10/26 17:10:03, themblsha wrote: > > On 2016/10/26 08:40:47, tapted wrote: > > > We should just move the tables into separate files. E.g. > > > > > > chrome/browser/accelerator_table_cocoa_mac.mm/h > > > chrome/browser/accelerator_table_views_mac.mm/h > > > > > > and then filter them out in the .gn files > > > > The normal tables are also used by the > > > > -[ChromeCommandDispatcherDelegate handleExtraKeyboardShortcut:] > > * -[CommandDispatcher performKeyEquivalent:] > > * -[NativeWidgetMacNSWindow performKeyEquivalent:] > > > > So we probably need to create a Views-specific > ChromeCommandDispatcherDelegate? > > I think we can still use ChromeCommandDispatcherDelegate as-is. The trick is to > link in the appropriate set of > CommandFor{[Delayed]Window,Browser}KeyboardShortcut methods that make sense for > mac_views_browser. > > So your current global_keyboard_shortcuts_mac.mm needs to split into 3 files > > global_keyboard_shortcuts_cocoa_mac.mm > global_keyboard_shortcuts_views_mac.mm > global_keyboard_shortcuts_mac.mm > > > The sum of global_keyboard_shortcuts_cocoa_mac.mm and > global_keyboard_shortcuts_mac.mm should be identical to what Chrome currently > has. > > global_keyboard_shortcuts_views_mac.mm defines the versions of > CommandFor{[Delayed]Window,Browser}KeyboardShortcut that mac_views_browser_wants > > global_keyboard_shortcuts_mac.mm holds the shared bits. That's a great idea and is much simpler than what I tried to do privately before your comment. Thanks! https://codereview.chromium.org/2074643003/diff/80001/chrome/browser/global_k... chrome/browser/global_keyboard_shortcuts_mac.mm:279: event, ^int(bool cmdKey, bool shiftKey, bool cntrlKey, bool optKey, On 2016/10/26 08:40:47, tapted wrote: > I don't think this needs to be a block -- there's nothing captured. You could > declare a helper method in an anonymous namespace and just pass a function > pointer. I think even neater would probably just be something like a `bool > check_windows_table`. Then move: > > CommandForBrowserKeyboardShortcut to accelerator_table_cocoa_mac.mm > CommandForMacViewsKeyboardShortcut to accelerator_table_cocoa_mac.mm (but call > it CommandForBrowserKeyboardShortcut) > > But also this might not work - it's very complex, and we need to simplify and > add comments to make it less complex. (and ObjC blocks add particular complexity > that only a few developers understand well, so are best avoided). Ah, yes, the implicit capturing of blocks. Probably should've just used a func pointer and a C++ lambda. Anyway, I simplified the code and it should be clearer now. https://codereview.chromium.org/2074643003/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/accelerator_table_unittest_mac.mm (right): https://codereview.chromium.org/2074643003/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/accelerator_table_unittest_mac.mm:176: // VerifyTable(GetWindowKeyboardShortcutTable, "WindowKeyboardShortcutTable"); On 2016/10/26 17:10:03, themblsha wrote: > On 2016/10/26 08:40:47, tapted wrote: > > We don't commit commented-out code. I'm guessing there's a story here - maybe > a > > TODO "GetWindowKeyboardShortcutTable, GetDelayedWindowKeyboardShortcutTable, > and > > GetBrowserKeyboardShortcutTable should also be verified, but they aren't > because > > <reason> > > Yep, I should remove it in the final version. Ideally there would be no need for > such tests if those tables are left out of the compilation. With the split now all the tables are always used (but they're almost empty on MacViews), so I've uncommented the checks. https://codereview.chromium.org/2074643003/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/browser_frame_mac.mm (right): https://codereview.chromium.org/2074643003/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_frame_mac.mm:22: bool ShouldHandleKeyboardEvent(const content::NativeWebKeyboardEvent& event) { On 2016/10/28 04:59:48, tapted wrote: > On 2016/10/26 17:10:03, themblsha wrote: > > On 2016/10/26 08:40:47, tapted wrote: > > > comment? > > > > It's adapted from +[BrowserWindowUtils shouldHandleKeyboardEvent:] plus one > > check from +handleKeyboardEvent:inWindow:, as I was trying to preserve the > > original Cocoa behaviour. Guess I need to research it more. > > Hm - it's possible that this and HandleExtraKeyboardShortcut should be absorbed > into ChromeCommandDispatcherDelegate.. > > But yeah - we still need to nail down the role that > ChromeCommandDispatcherDelegate is playing in the event sequence for a > BrowserFrameMac. Perhaps my earlier suggestion will clear things up a bit. But > currently it looks like `HandleExtraKeyboardShortcut` is being invoked one time > too many for BrowserFrameMac Without my changes the only code path that should call ChromeCommandDispatcherDelegate seems to be this one: -[ChromeCommandDispatcherDelegate prePerformKeyEquivalent:] * -[CommandDispatcher performKeyEquivalent:] * -[NativeWidgetMacNSWindow performKeyEquivalent:] The key equivalent system will automatically consult the Menu in case the window won't eat the event. https://codereview.chromium.org/2074643003/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_frame_mac.mm:31: DCHECK(event.os_event != NULL); On 2016/10/26 08:40:47, tapted wrote: > DCHECK(event.os_event) - this should probably be at the stat of the method I guess it could fail in case the event was simulated (using DevTools or some other means), so I'd prefer to leave it in its current position. https://codereview.chromium.org/2074643003/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_frame_mac.mm:143: // Copied from BrowserWindowCocoa::PreHandleKeyboardEvent(). On 2016/10/26 08:40:47, tapted wrote: > BrowserWindowCocoa may not be with us forever, so this comment needs to be > standalone. We need to capture "why Mac is special" here. i.e. why does > BrowserFrameMac need special handling for weirdness that the Mac platform throws > at us and other platforms don't. Tried my best to fill the code with sensible comments :-) https://codereview.chromium.org/2074643003/diff/110001/chrome/browser/global_... File chrome/browser/global_keyboard_shortcuts_cocoa_mac.mm (right): https://codereview.chromium.org/2074643003/diff/110001/chrome/browser/global_... chrome/browser/global_keyboard_shortcuts_cocoa_mac.mm:7: #include "base/macros.h" I get a strange presubmit warning here, even though the code is formatted similar to global_keyboard_shortcuts_mac.mm. Is it just using the wrong collation or I'm missing something stupid? Your #include order seems to be broken. Remember to use the right collation (LC_COLLATE=C) and check https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes chrome/browser/global_keyboard_shortcuts_cocoa_mac.mm:7: - line belongs before previous line \ https://codereview.chromium.org/2074643003/diff/110001/chrome/browser/ui/view... File chrome/browser/ui/views/frame/browser_frame_mac.mm (right): https://codereview.chromium.org/2074643003/diff/110001/chrome/browser/ui/view... chrome/browser/ui/views/frame/browser_frame_mac.mm:30: // be outdated. Added comment about the outdated comment. Will remove it in next update. Should I file a bug about this?
Description was changed from
==========
MacViews: Views accelerators table should match the Cocoa one.
Updated views/accelerator_table.cc to match accelerators_cocoa.mm.
Also handle the case when the user configured cmd-left to mean "previous tab",
as it should take precedence over the built-in binding.
BUG=620315
==========
to
==========
MacViews: Views accelerators table should match the Cocoa one.
Updated views/accelerator_table.cc to match accelerators_cocoa.mm.
Also handle the case when the user configured cmd-left to mean "previous tab",
as it should take precedence over the built-in binding: we're consulting the
[NSApp mainMenu] using CommandForKeyEvent().
Lastly, the Cmd+{/} shortcuts are special as they would be impossible to perform
on German and French keyboard layouts (crbug.com/25946).
In order to tackle last two problems I've added PreHandleKeyboardEvent and
HandleKeyboardEvent to the NativeBrowserFrame interface.
BUG=620315
==========
https://codereview.chromium.org/2074643003/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/browser_frame_mac.mm (right): https://codereview.chromium.org/2074643003/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_frame_mac.mm:31: DCHECK(event.os_event != NULL); On 2016/10/28 17:32:22, themblsha wrote: > On 2016/10/26 08:40:47, tapted wrote: > > DCHECK(event.os_event) - this should probably be at the stat of the method > > I guess it could fail in case the event was simulated (using DevTools or some > other means), so I'd prefer to leave it in its current position. Ah, good point - I think I've encountered strangeness about this too, in bug triage: https://bugs.chromium.org/p/chromium/issues/detail?id=627221#c2 However, the code is still weird. If event.os_event is null then the code on line 24 will behave unexpectedly `[null type] != NSKeyDown` turns into `0 != NSKeyDown` which is always true. So it's impossible for this DCHECK to fail, even if event.os_event is null. So we should have the skip_in_browser check first (let's check ::Char here too, since the "Ignore synthesized events" comment applies to both), then the DCHECK, then the NSKeyDown check. https://codereview.chromium.org/2074643003/diff/110001/chrome/browser/BUILD.gn File chrome/browser/BUILD.gn (right): https://codereview.chromium.org/2074643003/diff/110001/chrome/browser/BUILD.g... chrome/browser/BUILD.gn:3771: if (is_mac) { this should be merged with the if (is_mac) on line 3394 https://codereview.chromium.org/2074643003/diff/110001/chrome/browser/global_... File chrome/browser/global_keyboard_shortcuts_cocoa_mac.mm (right): https://codereview.chromium.org/2074643003/diff/110001/chrome/browser/global_... chrome/browser/global_keyboard_shortcuts_cocoa_mac.mm:7: #include "base/macros.h" On 2016/10/28 17:32:22, themblsha wrote: > I get a strange presubmit warning here, even though the code is formatted > similar to global_keyboard_shortcuts_mac.mm. Is it just using the wrong > collation or I'm missing something stupid? Yeah - this one is a confusing message. The complaint is that the header on line 5 doesn't match this file any more (i.e. only "global_keyboard_shortcuts_cocoa_mac.h" is allowed there - not "global_keyboard_shortcuts_mac.h"). You just need to merge the two blocks and resort. https://codereview.chromium.org/2074643003/diff/110001/chrome/browser/ui/view... File chrome/browser/ui/views/accelerator_table_unittest_mac.mm (right): https://codereview.chromium.org/2074643003/diff/110001/chrome/browser/ui/view... chrome/browser/ui/views/accelerator_table_unittest_mac.mm:11: #include "chrome/browser/global_keyboard_shortcuts_mac.h" nit: import https://codereview.chromium.org/2074643003/diff/110001/chrome/browser/ui/view... chrome/browser/ui/views/accelerator_table_unittest_mac.mm:15: #include "ui/events/keycodes/keyboard_code_conversion_mac.h" nit: import https://codereview.chromium.org/2074643003/diff/110001/chrome/browser/ui/view... chrome/browser/ui/views/accelerator_table_unittest_mac.mm:48: a.modifiers & ui::EF_CONTROL_DOWN; nit: (a.modifiers & ui::EF_CONTROL_DOWN) != 0 https://codereview.chromium.org/2074643003/diff/110001/chrome/browser/ui/view... chrome/browser/ui/views/accelerator_table_unittest_mac.mm:91: return std::find_if(accelerator_list.begin(), accelerator_list.end(), This is a bit mind-blowing. I'd suggest declaring an operator< in the test file bool operator<(const AcceleratorMapping& lhs, const AcceleratorMapping& rhs) { return std::tie(lhs.command_id, lhs.keycode, lhs.modifiers) < std::tie(rhs.command_id, ..etc) } Then declare accelerator_list as a std::set and use `return accelerator_list.count(m);` here. https://codereview.chromium.org/2074643003/diff/110001/chrome/browser/ui/view... chrome/browser/ui/views/accelerator_table_unittest_mac.mm:129: static void VerifyTable( nit: no `static` https://codereview.chromium.org/2074643003/diff/110001/chrome/browser/ui/view... File chrome/browser/ui/views/frame/browser_frame.h (right): https://codereview.chromium.org/2074643003/diff/110001/chrome/browser/ui/view... chrome/browser/ui/views/frame/browser_frame.h:99: // Returns true if the |event| was handled by the platform implementation. I think this needs to borrow more words from browser_window.h. E.g. // Returns true if the |event| was handled by the platform implementation // before sending it to the renderer. E.g., it may be swallowed by a native // menu bar. https://codereview.chromium.org/2074643003/diff/110001/chrome/browser/ui/view... chrome/browser/ui/views/frame/browser_frame.h:102: // Returns true if the |event| was handled by the platform implementation. // Returns true if the |event| was handled by the platform implementation, // if the renderer did not process it. https://codereview.chromium.org/2074643003/diff/110001/chrome/browser/ui/view... File chrome/browser/ui/views/frame/browser_frame_mac.mm (right): https://codereview.chromium.org/2074643003/diff/110001/chrome/browser/ui/view... chrome/browser/ui/views/frame/browser_frame_mac.mm:7: #include "chrome/browser/global_keyboard_shortcuts_mac.h" nit: import https://codereview.chromium.org/2074643003/diff/110001/chrome/browser/ui/view... chrome/browser/ui/views/frame/browser_frame_mac.mm:12: #import "chrome/browser/ui/cocoa/chrome_command_dispatcher_delegate.h" remove dupe https://codereview.chromium.org/2074643003/diff/110001/chrome/browser/ui/view... chrome/browser/ui/views/frame/browser_frame_mac.mm:30: // be outdated. On 2016/10/28 17:32:23, themblsha wrote: > Added comment about the outdated comment. Will remove it in next update. > > Should I file a bug about this? I think it's still mostly correct -- backspace may still trigger a notice. You're right that skip_in_browser is now used for other things, but I think it's OK. https://codereview.chromium.org/2074643003/diff/110001/chrome/browser/ui/view... chrome/browser/ui/views/frame/browser_frame_mac.mm:34: // Ignore syntesized keyboard events. http://codereview.chromium.org/460105 syntesized -> synthesized https://codereview.chromium.org/2074643003/diff/110001/chrome/browser/ui/view... chrome/browser/ui/views/frame/browser_frame_mac.mm:159: // CommandForKeyEventOnMacViews consults the [NSApp mainMenu] and Mac-specific update comment (CommandForKeyEvent)
mblsha@yandex-team.ru changed reviewers: + pkasting@chromium.org
Description was changed from
==========
MacViews: Views accelerators table should match the Cocoa one.
Updated views/accelerator_table.cc to match accelerators_cocoa.mm.
Also handle the case when the user configured cmd-left to mean "previous tab",
as it should take precedence over the built-in binding: we're consulting the
[NSApp mainMenu] using CommandForKeyEvent().
Lastly, the Cmd+{/} shortcuts are special as they would be impossible to perform
on German and French keyboard layouts (crbug.com/25946).
In order to tackle last two problems I've added PreHandleKeyboardEvent and
HandleKeyboardEvent to the NativeBrowserFrame interface.
BUG=620315
==========
to
==========
MacViews: Views accelerators table should match the Cocoa one.
Updated views/accelerator_table.cc to match accelerators_cocoa.mm. Added
eligible shortcuts from global_keyboard_shortcuts_mac.mm as well.
Also handle the case when the user configured cmd-left to mean "previous tab",
as it should take precedence over the built-in binding: we're consulting the
[NSApp mainMenu] using CommandForKeyEvent().
Lastly, the Cmd+{/} shortcuts are special as they would be impossible to perform
on German and French keyboard layouts (crbug.com/25946).
In order to tackle last two problems I've added PreHandleKeyboardEvent and
HandleKeyboardEvent to the NativeBrowserFrame interface.
BUG=620315
==========
+pkasting. Peter, could you please take a look as well? https://codereview.chromium.org/2074643003/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/frame/browser_frame_mac.mm (right): https://codereview.chromium.org/2074643003/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/frame/browser_frame_mac.mm:31: DCHECK(event.os_event != NULL); On 2016/10/31 10:16:57, tapted wrote: > On 2016/10/28 17:32:22, themblsha wrote: > > On 2016/10/26 08:40:47, tapted wrote: > > > DCHECK(event.os_event) - this should probably be at the stat of the method > > > > I guess it could fail in case the event was simulated (using DevTools or some > > other means), so I'd prefer to leave it in its current position. > > Ah, good point - I think I've encountered strangeness about this too, in bug > triage: https://bugs.chromium.org/p/chromium/issues/detail?id=627221#c2 > > However, the code is still weird. If event.os_event is null then the code on > line 24 will behave unexpectedly > > `[null type] != NSKeyDown` turns into `0 != NSKeyDown` which is always true. So > it's impossible for this DCHECK to fail, even if event.os_event is null. > > So we should have the skip_in_browser check first (let's check ::Char here too, > since the "Ignore synthesized events" comment applies to both), then the DCHECK, > then the NSKeyDown check. Makes sense, done. https://codereview.chromium.org/2074643003/diff/110001/chrome/browser/BUILD.gn File chrome/browser/BUILD.gn (right): https://codereview.chromium.org/2074643003/diff/110001/chrome/browser/BUILD.g... chrome/browser/BUILD.gn:3771: if (is_mac) { On 2016/10/31 10:16:57, tapted wrote: > this should be merged with the if (is_mac) on line 3394 Thanks. I'm getting lost in these huge GN files. https://codereview.chromium.org/2074643003/diff/110001/chrome/browser/global_... File chrome/browser/global_keyboard_shortcuts_cocoa_mac.mm (right): https://codereview.chromium.org/2074643003/diff/110001/chrome/browser/global_... chrome/browser/global_keyboard_shortcuts_cocoa_mac.mm:7: #include "base/macros.h" On 2016/10/31 10:16:57, tapted wrote: > On 2016/10/28 17:32:22, themblsha wrote: > > I get a strange presubmit warning here, even though the code is formatted > > similar to global_keyboard_shortcuts_mac.mm. Is it just using the wrong > > collation or I'm missing something stupid? > > Yeah - this one is a confusing message. The complaint is that the header on line > 5 doesn't match this file any more (i.e. only > "global_keyboard_shortcuts_cocoa_mac.h" is allowed there - not > "global_keyboard_shortcuts_mac.h"). You just need to merge the two blocks and > resort. Thanks :-) https://codereview.chromium.org/2074643003/diff/110001/chrome/browser/ui/view... File chrome/browser/ui/views/accelerator_table_unittest_mac.mm (right): https://codereview.chromium.org/2074643003/diff/110001/chrome/browser/ui/view... chrome/browser/ui/views/accelerator_table_unittest_mac.mm:11: #include "chrome/browser/global_keyboard_shortcuts_mac.h" On 2016/10/31 10:16:57, tapted wrote: > nit: import Done. https://codereview.chromium.org/2074643003/diff/110001/chrome/browser/ui/view... chrome/browser/ui/views/accelerator_table_unittest_mac.mm:15: #include "ui/events/keycodes/keyboard_code_conversion_mac.h" On 2016/10/31 10:16:57, tapted wrote: > nit: import Done. https://codereview.chromium.org/2074643003/diff/110001/chrome/browser/ui/view... chrome/browser/ui/views/accelerator_table_unittest_mac.mm:48: a.modifiers & ui::EF_CONTROL_DOWN; On 2016/10/31 10:16:57, tapted wrote: > nit: (a.modifiers & ui::EF_CONTROL_DOWN) != 0 Done. https://codereview.chromium.org/2074643003/diff/110001/chrome/browser/ui/view... chrome/browser/ui/views/accelerator_table_unittest_mac.mm:91: return std::find_if(accelerator_list.begin(), accelerator_list.end(), On 2016/10/31 10:16:57, tapted wrote: > This is a bit mind-blowing. I'd suggest declaring an operator< in the test file > > bool operator<(const AcceleratorMapping& lhs, const AcceleratorMapping& rhs) { > return std::tie(lhs.command_id, lhs.keycode, lhs.modifiers) < > std::tie(rhs.command_id, ..etc) > } > > Then declare accelerator_list as a std::set and use `return > accelerator_list.count(m);` here. Whoa, std::tie makes comparators sooo easy. Replaced the vectors with sets so the resulting operation became very easy. https://codereview.chromium.org/2074643003/diff/110001/chrome/browser/ui/view... chrome/browser/ui/views/accelerator_table_unittest_mac.mm:129: static void VerifyTable( On 2016/10/31 10:16:57, tapted wrote: > nit: no `static` Done. https://codereview.chromium.org/2074643003/diff/110001/chrome/browser/ui/view... File chrome/browser/ui/views/frame/browser_frame.h (right): https://codereview.chromium.org/2074643003/diff/110001/chrome/browser/ui/view... chrome/browser/ui/views/frame/browser_frame.h:99: // Returns true if the |event| was handled by the platform implementation. On 2016/10/31 10:16:57, tapted wrote: > I think this needs to borrow more words from browser_window.h. E.g. > > // Returns true if the |event| was handled by the platform implementation > // before sending it to the renderer. E.g., it may be swallowed by a native > // menu bar. Done. https://codereview.chromium.org/2074643003/diff/110001/chrome/browser/ui/view... chrome/browser/ui/views/frame/browser_frame.h:102: // Returns true if the |event| was handled by the platform implementation. On 2016/10/31 10:16:57, tapted wrote: > // Returns true if the |event| was handled by the platform implementation, > // if the renderer did not process it. Done. https://codereview.chromium.org/2074643003/diff/110001/chrome/browser/ui/view... File chrome/browser/ui/views/frame/browser_frame_mac.mm (right): https://codereview.chromium.org/2074643003/diff/110001/chrome/browser/ui/view... chrome/browser/ui/views/frame/browser_frame_mac.mm:7: #include "chrome/browser/global_keyboard_shortcuts_mac.h" On 2016/10/31 10:16:57, tapted wrote: > nit: import Done. https://codereview.chromium.org/2074643003/diff/110001/chrome/browser/ui/view... chrome/browser/ui/views/frame/browser_frame_mac.mm:12: #import "chrome/browser/ui/cocoa/chrome_command_dispatcher_delegate.h" On 2016/10/31 10:16:57, tapted wrote: > remove dupe Done. https://codereview.chromium.org/2074643003/diff/110001/chrome/browser/ui/view... chrome/browser/ui/views/frame/browser_frame_mac.mm:34: // Ignore syntesized keyboard events. http://codereview.chromium.org/460105 On 2016/10/31 10:16:57, tapted wrote: > syntesized -> synthesized Done. https://codereview.chromium.org/2074643003/diff/110001/chrome/browser/ui/view... chrome/browser/ui/views/frame/browser_frame_mac.mm:159: // CommandForKeyEventOnMacViews consults the [NSApp mainMenu] and Mac-specific On 2016/10/31 10:16:57, tapted wrote: > update comment (CommandForKeyEvent) Done.
Whoops, forgot to fix the spelling inside one of the comments.
On 2016/10/31 16:56:30, themblsha wrote: > +pkasting. > > Peter, could you please take a look as well? What specifically did you want me to look at? I don't speak Mac/ObjC, so I wouldn't be a good reviewer of the whole CL here, but I could look at parts of it?
On 2016/10/31 17:19:15, Peter Kasting wrote:
> On 2016/10/31 16:56:30, themblsha wrote:
> > +pkasting.
> >
> > Peter, could you please take a look as well?
>
> What specifically did you want me to look at? I don't speak Mac/ObjC, so I
> wouldn't be a good reviewer of the whole CL here, but I could look at parts of
> it?
native_browser_frame* / browser_frame*: I've added platform-specific
PreHandleKeyboardEvent / HandleKeyboardEvent.
It's made this way for two reasons:
1. Handle the case when the user configured cmd-left to mean "previous tab", as
it should take precedence over the built-in binding: we're consulting the [NSApp
mainMenu] using CommandForKeyEvent().
2. The Cmd+{/} shortcuts are special as they would be impossible to perform on
German and French keyboard layouts if we would check for keycodes, so this
leaves out the accelerator_table.cc option (crbug.com/25946).
Description was changed from
==========
MacViews: Views accelerators table should match the Cocoa one.
Updated views/accelerator_table.cc to match accelerators_cocoa.mm. Added
eligible shortcuts from global_keyboard_shortcuts_mac.mm as well.
Also handle the case when the user configured cmd-left to mean "previous tab",
as it should take precedence over the built-in binding: we're consulting the
[NSApp mainMenu] using CommandForKeyEvent().
Lastly, the Cmd+{/} shortcuts are special as they would be impossible to perform
on German and French keyboard layouts (crbug.com/25946).
In order to tackle last two problems I've added PreHandleKeyboardEvent and
HandleKeyboardEvent to the NativeBrowserFrame interface.
BUG=620315
==========
to
==========
MacViews: Views accelerators table should match the Cocoa one.
Updated views/accelerator_table.cc to match accelerators_cocoa.mm. Added
eligible shortcuts from global_keyboard_shortcuts_mac.mm as well.
Also handle the case when the user configured cmd-left to mean "previous tab",
as it should take precedence over the built-in binding: we're consulting the
[NSApp mainMenu] using CommandForKeyEvent().
Lastly, the Cmd+{/} shortcuts are special as they would be impossible to
perform on German and French keyboard layouts if we would check for keycodes
(crbug.com/25946).
In order to tackle last two problems I've added PreHandleKeyboardEvent and
HandleKeyboardEvent to the NativeBrowserFrame interface.
BUG=620315
==========
Description was changed from
==========
MacViews: Views accelerators table should match the Cocoa one.
Updated views/accelerator_table.cc to match accelerators_cocoa.mm. Added
eligible shortcuts from global_keyboard_shortcuts_mac.mm as well.
Also handle the case when the user configured cmd-left to mean "previous tab",
as it should take precedence over the built-in binding: we're consulting the
[NSApp mainMenu] using CommandForKeyEvent().
Lastly, the Cmd+{/} shortcuts are special as they would be impossible to
perform on German and French keyboard layouts if we would check for keycodes
(crbug.com/25946).
In order to tackle last two problems I've added PreHandleKeyboardEvent and
HandleKeyboardEvent to the NativeBrowserFrame interface.
BUG=620315
==========
to
==========
MacViews: Views accelerators table should match the Cocoa one.
Updated views/accelerator_table.cc to match accelerators_cocoa.mm. Added
eligible shortcuts from global_keyboard_shortcuts_mac.mm as well.
Also handle the case when the user configured cmd-left to mean "previous tab",
as it should take precedence over the built-in binding: we're consulting the
[NSApp mainMenu] using CommandForKeyEvent().
Lastly, the Cmd+{/} shortcuts are special as they would be impossible to
perform on German and French keyboard layouts if we would simply check for
keycodes as we do in accelerator_table.cc (crbug.com/25946).
In order to tackle last two problems I've added PreHandleKeyboardEvent and
HandleKeyboardEvent to the NativeBrowserFrame interface.
BUG=620315
==========
The code in the cross-platform files is fine. I don't have a good sense for whether this is the right way to plumb this particular check, though. I would ask ben or sky about whether using virtual methods on the browser frame to insert these particular checks is the correct plumbing. LGTM otherwise. https://codereview.chromium.org/2074643003/diff/150001/chrome/browser/global_... File chrome/browser/global_keyboard_shortcuts_views_mac.mm (right): https://codereview.chromium.org/2074643003/diff/150001/chrome/browser/global_... chrome/browser/global_keyboard_shortcuts_views_mac.mm:12: const KeyboardShortcutData* GetWindowKeyboardShortcutTable( Nit: Out of scope for this CL, but consider modifying the Mac code that calls these sorts of functions to instead have them directly return std::vector<KeyboardShortcutData>. These can be constructed fairly simply (see my accelerator table comments) and passed around efficiently in C++11, and then iterated over at the other end using range-based for. https://codereview.chromium.org/2074643003/diff/150001/chrome/browser/ui/view... File chrome/browser/ui/views/accelerator_table.cc (right): https://codereview.chromium.org/2074643003/diff/150001/chrome/browser/ui/view... chrome/browser/ui/views/accelerator_table.cc:295: std::end(kAcceleratorMap)); Nit: Or just inline kAcceleratorMap here: return { { ui::VKEY_LEFT, ui::EF_ALT_DOWN, IDC_BACK }, { ui::VKEY_BACK, ui::EF_NONE, IDC_BACKSPACE_BACK }, .... }; Then use this function when building |accelerators| in GetAcceleratorList() below. https://codereview.chromium.org/2074643003/diff/150001/chrome/browser/ui/view... chrome/browser/ui/views/accelerator_table.cc:309: // Command or Control. Nit: I suggest combining these two remove calls: auto RemoveAccelerators = [](const AcceleratorMapping& m) { // Alt by itself (or with just shift) is never used on Mac since it's used // ... ... // Control modifier is rarely used on Mac, so we allow it only in several // ... ... }; accelerators.erase(std::remove_if(accelerators.begin(), accelerators.end(), RemoveAccelerators), accelerators.end()); This is not only more efficient, it reduces the amount of boilerplate (yay readability) and makes it simpler to safely add more such tweaks in the future. https://codereview.chromium.org/2074643003/diff/150001/chrome/browser/ui/view... chrome/browser/ui/views/accelerator_table.cc:310: const int kNonShiftMask = Nit: constexpr (2 places) https://codereview.chromium.org/2074643003/diff/150001/chrome/browser/ui/view... chrome/browser/ui/views/accelerator_table.cc:319: // whiltelisted cases. Nit: whitelisted https://codereview.chromium.org/2074643003/diff/150001/chrome/browser/ui/view... chrome/browser/ui/views/accelerator_table.cc:320: const std::set<int> control_whitelist = { Nit: kControlWhitelist
lgtm % nits Thanks! https://codereview.chromium.org/2074643003/diff/150001/chrome/browser/ui/view... File chrome/browser/ui/views/accelerator_table.cc (right): https://codereview.chromium.org/2074643003/diff/150001/chrome/browser/ui/view... chrome/browser/ui/views/accelerator_table.cc:233: // VKEY_OEM_2 is Slash '/?' key nit: fullstop at end. https://codereview.chromium.org/2074643003/diff/150001/chrome/browser/ui/view... File chrome/browser/ui/views/accelerator_table_unittest_mac.mm (right): https://codereview.chromium.org/2074643003/diff/150001/chrome/browser/ui/view... chrome/browser/ui/views/accelerator_table_unittest_mac.mm:17: namespace { nit: nest this inside `namespace chrome {` and remove the extra chrome:: prefixes https://codereview.chromium.org/2074643003/diff/150001/chrome/browser/ui/view... chrome/browser/ui/views/accelerator_table_unittest_mac.mm:19: // operator< inside anonymous namespace will cause compile-time errors. Yeah - it needs to satisfy the requirements of Koenig Lookup, but I actually like this better since it means we won't get conflicts if someone later decides a public comparator might be useful. We can probably omit this comment. https://codereview.chromium.org/2074643003/diff/150001/chrome/browser/ui/view... chrome/browser/ui/views/accelerator_table_unittest_mac.mm:112: // Remove all accelerators that are present in |accelerator_list| nit: fullstop at end https://codereview.chromium.org/2074643003/diff/150001/chrome/browser/ui/view... chrome/browser/ui/views/accelerator_table_unittest_mac.mm:114: filtered_list.erase(m); optional: there's an STL algorithm that will do this in linear time rather than n*log(n), but it's a real mouthful. std::set_difference( full_accelerator_list.begin(), full_accelerator_list.end(), accelerator_list.begin(), accelerator_list.end(), std::inserter(filtered_list, filtered_list.end())); https://codereview.chromium.org/2074643003/diff/150001/chrome/browser/ui/view... File chrome/browser/ui/views/frame/browser_frame_mac.mm (right): https://codereview.chromium.org/2074643003/diff/150001/chrome/browser/ui/view... chrome/browser/ui/views/frame/browser_frame_mac.mm:23: // if it was ignored by the renderer. http://codereview.chromium.org/295003 nit: http://codereview.chromium.org/295003 -> See http://crbug.com/25000. (we don't usually cite codereview urls) https://codereview.chromium.org/2074643003/diff/150001/chrome/browser/ui/view... chrome/browser/ui/views/frame/browser_frame_mac.mm:27: // Ignore synthesized keyboard events. http://codereview.chromium.org/460105 See http://crbug.com/23221.
I came to write the STLSetDifference() reply, and saw some other possibilities for improvement too :) https://codereview.chromium.org/2074643003/diff/150001/chrome/browser/ui/view... File chrome/browser/ui/views/accelerator_table_unittest_mac.mm (right): https://codereview.chromium.org/2074643003/diff/150001/chrome/browser/ui/view... chrome/browser/ui/views/accelerator_table_unittest_mac.mm:17: namespace { On 2016/11/01 00:22:50, tapted wrote: > nit: nest this inside `namespace chrome {` and remove the extra chrome:: > prefixes Note that we're trying to kill the chrome namespace entirely. If it's easy, feel free to remove the namespace scoping from AcceleratorMapping. If not, then either namespace everything in this file or qualify everything. https://codereview.chromium.org/2074643003/diff/150001/chrome/browser/ui/view... chrome/browser/ui/views/accelerator_table_unittest_mac.mm:19: // operator< inside anonymous namespace will cause compile-time errors. On 2016/11/01 00:22:50, tapted wrote: > Yeah - it needs to satisfy the requirements of Koenig Lookup, but I actually > like this better since it means we won't get conflicts if someone later decides > a public comparator might be useful. We can probably omit this comment. Can we supply a lambda to the std::set template? That would allow writing this in a more condensed way. https://codereview.chromium.org/2074643003/diff/150001/chrome/browser/ui/view... chrome/browser/ui/views/accelerator_table_unittest_mac.mm:33: // the same vector data. Nit: I think you could omit this comment (and the type alias) by just making this a lambda in the relevant function below. Then it seems more clear what it's doing: // Convert the accelerator lists to sets for easy searching/filtering. auto ToSet = [](const std::vector<chrome::AcceleratorMapping>& v) { return std::set<chrome::AcceleratorMapping, AcceleratorMappingComparator>( v.begin(), v.end()); } auto accelerators = ToSet(GetAcceleratorList()); auto unfiltered_accelerators = ToSet(GetUnfilteredAcceleratorListForTesting()); https://codereview.chromium.org/2074643003/diff/150001/chrome/browser/ui/view... chrome/browser/ui/views/accelerator_table_unittest_mac.mm:74: EXPECT_TRUE(entry != accelerator_list.end()) Nit: Can this use EXPECT_NE? If not, I suggest using EXPECT_FALSE(entry == filtered_list.end()) so that failure isn't mentally a double-negative like "if it's _not_ true that the entry isn't the end, ..." (3 places) https://codereview.chromium.org/2074643003/diff/150001/chrome/browser/ui/view... chrome/browser/ui/views/accelerator_table_unittest_mac.mm:104: const AcceleratorMappingSet accelerator_list( Nit: It seems kinda misleading to use "list" in the name of a variable that's a set. I suggest just "accelerators". (2 places) https://codereview.chromium.org/2074643003/diff/150001/chrome/browser/ui/view... chrome/browser/ui/views/accelerator_table_unittest_mac.mm:114: filtered_list.erase(m); On 2016/11/01 00:22:50, tapted wrote: > optional: there's an STL algorithm that will do this in linear time rather than > n*log(n), but it's a real mouthful. > > std::set_difference( > full_accelerator_list.begin(), full_accelerator_list.end(), > accelerator_list.begin(), accelerator_list.end(), > std::inserter(filtered_list, filtered_list.end())); See base::STLSetDifference(), which wraps this in some syntactic sugar to give you: auto filtered_list = base::STLSetDifference(full_accelerator_list, accelerator_list); https://codereview.chromium.org/2074643003/diff/150001/chrome/browser/ui/view... chrome/browser/ui/views/accelerator_table_unittest_mac.mm:144: namespace { Nit: I don't think there's any style rule on this, but it seems more common to combine all anonymous namespace functions into one group atop the file rather than a separate namespace above each applicable test. https://codereview.chromium.org/2074643003/diff/150001/chrome/browser/ui/view... chrome/browser/ui/views/accelerator_table_unittest_mac.mm:183: } Nit: Not exactly simple, but simpler than this conditional: EXPECT_FALSE(modifiers == entry.modifiers && it->chrome_command == entry.command_id && (it->vkey_code ? (it->vkey_code == vkey_code) : (it->key_char == character || it->key_char == shifted_character))) << "Duplicate command: " << entry.command_id << " in table " << table_name;
Also I need to add some more reviewers for the global_keyboard_shortcuts*. Owners list avi, mark, rsesek and thakis. Whom should I add? https://codereview.chromium.org/2074643003/diff/150001/chrome/browser/global_... File chrome/browser/global_keyboard_shortcuts_views_mac.mm (right): https://codereview.chromium.org/2074643003/diff/150001/chrome/browser/global_... chrome/browser/global_keyboard_shortcuts_views_mac.mm:12: const KeyboardShortcutData* GetWindowKeyboardShortcutTable( On 2016/10/31 18:07:32, Peter Kasting wrote: > Nit: Out of scope for this CL, but consider modifying the Mac code that calls > these sorts of functions to instead have them directly return > std::vector<KeyboardShortcutData>. These can be constructed fairly simply (see > my accelerator table comments) and passed around efficiently in C++11, and then > iterated over at the other end using range-based for. Not sure what you mean by C++11: move semantics? But it would imply that the table would need to be efficiently created from something to be moved to somewhere else? Instead I created static vectors using CR_DEFINE_STATIC_LOCAL and pass them around using const-references. Did I miss something? https://codereview.chromium.org/2074643003/diff/150001/chrome/browser/ui/view... File chrome/browser/ui/views/accelerator_table.cc (right): https://codereview.chromium.org/2074643003/diff/150001/chrome/browser/ui/view... chrome/browser/ui/views/accelerator_table.cc:233: // VKEY_OEM_2 is Slash '/?' key On 2016/11/01 00:22:50, tapted wrote: > nit: fullstop at end. Done. https://codereview.chromium.org/2074643003/diff/150001/chrome/browser/ui/view... chrome/browser/ui/views/accelerator_table.cc:295: std::end(kAcceleratorMap)); On 2016/10/31 18:07:32, Peter Kasting wrote: > Nit: Or just inline kAcceleratorMap here: > > return { > { ui::VKEY_LEFT, ui::EF_ALT_DOWN, IDC_BACK }, > { ui::VKEY_BACK, ui::EF_NONE, IDC_BACKSPACE_BACK }, > .... > }; > > Then use this function when building |accelerators| in GetAcceleratorList() > below. I tried to preserve the original formatting of the kAcceleratorMap table in order not to break the git blame. Should I reformat the table as well? https://codereview.chromium.org/2074643003/diff/150001/chrome/browser/ui/view... chrome/browser/ui/views/accelerator_table.cc:309: // Command or Control. On 2016/10/31 18:07:32, Peter Kasting wrote: > Nit: I suggest combining these two remove calls: > > auto RemoveAccelerators = [](const AcceleratorMapping& m) { > // Alt by itself (or with just shift) is never used on Mac since it's used > // ... > ... > > // Control modifier is rarely used on Mac, so we allow it only in several > // ... > ... > }; > accelerators.erase(std::remove_if(accelerators.begin(), accelerators.end(), > RemoveAccelerators), > accelerators.end()); > > This is not only more efficient, it reduces the amount of boilerplate (yay > readability) and makes it simpler to safely add more such tweaks in the future. Yep, looks much better now, thanks! https://codereview.chromium.org/2074643003/diff/150001/chrome/browser/ui/view... chrome/browser/ui/views/accelerator_table.cc:310: const int kNonShiftMask = On 2016/10/31 18:07:32, Peter Kasting wrote: > Nit: constexpr (2 places) Tried to apply it to the std::set as well, but got this error: ../../chrome/browser/ui/views/accelerator_table.cc:318:29: error: constexpr variable cannot have non-literal type 'const std::set<int>' constexpr std::set<int> control_whitelist = { Did you mean some other const expression? https://codereview.chromium.org/2074643003/diff/150001/chrome/browser/ui/view... chrome/browser/ui/views/accelerator_table.cc:319: // whiltelisted cases. On 2016/10/31 18:07:32, Peter Kasting wrote: > Nit: whitelisted Done. https://codereview.chromium.org/2074643003/diff/150001/chrome/browser/ui/view... chrome/browser/ui/views/accelerator_table.cc:320: const std::set<int> control_whitelist = { On 2016/10/31 18:07:32, Peter Kasting wrote: > Nit: kControlWhitelist Done. https://codereview.chromium.org/2074643003/diff/150001/chrome/browser/ui/view... File chrome/browser/ui/views/accelerator_table_unittest_mac.mm (right): https://codereview.chromium.org/2074643003/diff/150001/chrome/browser/ui/view... chrome/browser/ui/views/accelerator_table_unittest_mac.mm:17: namespace { On 2016/11/01 00:46:21, Peter Kasting wrote: > Note that we're trying to kill the chrome namespace entirely. If it's easy, > feel free to remove the namespace scoping from AcceleratorMapping. If not, then > either namespace everything in this file or qualify everything. Is there a crbug for that? I don't feel that I completely understand what that implies: removing the `namespace chrome` in src/chrome/browser/ui/views/accelerator_table.h, and fixing all the resulting compilation errors on all platforms? This type is referenced only in seven files, so it shouldn't be too hard. Added `using namespace chrome;` here and removed the namespace from the test. https://codereview.chromium.org/2074643003/diff/150001/chrome/browser/ui/view... chrome/browser/ui/views/accelerator_table_unittest_mac.mm:33: // the same vector data. On 2016/11/01 00:46:22, Peter Kasting wrote: > Nit: I think you could omit this comment (and the type alias) by just making > this a lambda in the relevant function below. Then it seems more clear what > it's doing: > > // Convert the accelerator lists to sets for easy searching/filtering. > auto ToSet = [](const std::vector<chrome::AcceleratorMapping>& v) { > return std::set<chrome::AcceleratorMapping, AcceleratorMappingComparator>( > v.begin(), v.end()); > } > auto accelerators = ToSet(GetAcceleratorList()); > auto unfiltered_accelerators = > ToSet(GetUnfilteredAcceleratorListForTesting()); > Did it using a lambda. But not sure what's the best to forward-declare the comparator lambda type, using a function pointer for now, as the alternative that used decltype(lambda) was a little longer. https://codereview.chromium.org/2074643003/diff/150001/chrome/browser/ui/view... chrome/browser/ui/views/accelerator_table_unittest_mac.mm:74: EXPECT_TRUE(entry != accelerator_list.end()) On 2016/11/01 00:46:21, Peter Kasting wrote: > Nit: Can this use EXPECT_NE? If not, I suggest using EXPECT_FALSE(entry == > filtered_list.end()) so that failure isn't mentally a double-negative like "if > it's _not_ true that the entry isn't the end, ..." (3 places) Looks much better, thanks! https://codereview.chromium.org/2074643003/diff/150001/chrome/browser/ui/view... chrome/browser/ui/views/accelerator_table_unittest_mac.mm:104: const AcceleratorMappingSet accelerator_list( On 2016/11/01 00:46:22, Peter Kasting wrote: > Nit: It seems kinda misleading to use "list" in the name of a variable that's a > set. I suggest just "accelerators". (2 places) Done. I thought that the _list suffix would be fine as it's a copy from GetAcceleratorList() but it's good to have a second opinion :-) https://codereview.chromium.org/2074643003/diff/150001/chrome/browser/ui/view... chrome/browser/ui/views/accelerator_table_unittest_mac.mm:112: // Remove all accelerators that are present in |accelerator_list| On 2016/11/01 00:22:50, tapted wrote: > nit: fullstop at end Done. https://codereview.chromium.org/2074643003/diff/150001/chrome/browser/ui/view... chrome/browser/ui/views/accelerator_table_unittest_mac.mm:114: filtered_list.erase(m); On 2016/11/01 00:46:22, Peter Kasting wrote: > See base::STLSetDifference(), which wraps this in some syntactic sugar to give > you: > > auto filtered_list = > base::STLSetDifference(full_accelerator_list, accelerator_list); That's great! There's one big gotcha though: std::set_difference needs to know about the comparator, otherwise it won't compile. The easiest way to make it work would be to add a global operator< for AcceleratorMapping, but I guess that's not considered kosher. Found base::ContainsKey() to be useful though. https://codereview.chromium.org/2074643003/diff/150001/chrome/browser/ui/view... chrome/browser/ui/views/accelerator_table_unittest_mac.mm:114: filtered_list.erase(m); On 2016/11/01 00:22:50, tapted wrote: > optional: there's an STL algorithm that will do this in linear time rather than > n*log(n), but it's a real mouthful. Yeah, I found it in the STL reference and thought that it was yucky and verbose, and we'd need to pass the comparator as well. https://codereview.chromium.org/2074643003/diff/150001/chrome/browser/ui/view... chrome/browser/ui/views/accelerator_table_unittest_mac.mm:144: namespace { On 2016/11/01 00:46:22, Peter Kasting wrote: > Nit: I don't think there's any style rule on this, but it seems more common to > combine all anonymous namespace functions into one group atop the file rather > than a separate namespace above each applicable test. Done. https://codereview.chromium.org/2074643003/diff/150001/chrome/browser/ui/view... chrome/browser/ui/views/accelerator_table_unittest_mac.mm:183: } On 2016/11/01 00:46:22, Peter Kasting wrote: > Nit: Not exactly simple, but simpler than this conditional: > > EXPECT_FALSE(modifiers == entry.modifiers && > it->chrome_command == entry.command_id && > (it->vkey_code ? (it->vkey_code == vkey_code) > : (it->key_char == character || > it->key_char == shifted_character))) > << "Duplicate command: " << entry.command_id > << " in table " << table_name; Thanks! https://codereview.chromium.org/2074643003/diff/150001/chrome/browser/ui/view... File chrome/browser/ui/views/frame/browser_frame_mac.mm (right): https://codereview.chromium.org/2074643003/diff/150001/chrome/browser/ui/view... chrome/browser/ui/views/frame/browser_frame_mac.mm:23: // if it was ignored by the renderer. http://codereview.chromium.org/295003 On 2016/11/01 00:22:50, tapted wrote: > nit: http://codereview.chromium.org/295003 -> See http://crbug.com/25000. > > (we don't usually cite codereview urls) Ah, noted. Thanks! https://codereview.chromium.org/2074643003/diff/150001/chrome/browser/ui/view... chrome/browser/ui/views/frame/browser_frame_mac.mm:27: // Ignore synthesized keyboard events. http://codereview.chromium.org/460105 On 2016/11/01 00:22:50, tapted wrote: > See http://crbug.com/23221. Done.
https://codereview.chromium.org/2074643003/diff/150001/chrome/browser/global_... File chrome/browser/global_keyboard_shortcuts_views_mac.mm (right): https://codereview.chromium.org/2074643003/diff/150001/chrome/browser/global_... chrome/browser/global_keyboard_shortcuts_views_mac.mm:12: const KeyboardShortcutData* GetWindowKeyboardShortcutTable( On 2016/11/01 14:36:24, themblsha wrote: > On 2016/10/31 18:07:32, Peter Kasting wrote: > > Nit: Out of scope for this CL, but consider modifying the Mac code that calls > > these sorts of functions to instead have them directly return > > std::vector<KeyboardShortcutData>. These can be constructed fairly simply > (see > > my accelerator table comments) and passed around efficiently in C++11, and > then > > iterated over at the other end using range-based for. > > Not sure what you mean by C++11: move semantics? But it would imply that the > table would need to be efficiently created from something to be moved to > somewhere else? I simply mean that the combination of things like move-constructors, initializer lists, and the RVO lets you do something like this without paying much efficiency cost: std::vector<T> ConstructT() { return {{1, 2}, {3, 4}, ...}; } std::vector<T> my_t = ConstructT(); ...so I would ultimately move toward syntax like that for these functions instead of passing around pointers and sizes. CR_DEFINE_STATIC_LOCAL is possible too, it's just trickier to read and results in these objects sticking around in memory. I tend to try to avoid it if I can, though that's based on more gut reaction than hard logic. https://codereview.chromium.org/2074643003/diff/150001/chrome/browser/ui/view... File chrome/browser/ui/views/accelerator_table.cc (right): https://codereview.chromium.org/2074643003/diff/150001/chrome/browser/ui/view... chrome/browser/ui/views/accelerator_table.cc:295: std::end(kAcceleratorMap)); On 2016/11/01 14:36:24, themblsha wrote: > On 2016/10/31 18:07:32, Peter Kasting wrote: > > Nit: Or just inline kAcceleratorMap here: > > > > return { > > { ui::VKEY_LEFT, ui::EF_ALT_DOWN, IDC_BACK }, > > { ui::VKEY_BACK, ui::EF_NONE, IDC_BACKSPACE_BACK }, > > .... > > }; > > > > Then use this function when building |accelerators| in GetAcceleratorList() > > below. > > I tried to preserve the original formatting of the kAcceleratorMap table in > order not to break the git blame. Should I reformat the table as well? Yes, you should reformat/move it (don't insert your global accessor in the middle of the anonymous namespace). Don't worry about git blame. If you want to make sure someone can really understand what's going on, do the change to inline this as its own CL, with all the functional changes from this current CL separate. https://codereview.chromium.org/2074643003/diff/150001/chrome/browser/ui/view... chrome/browser/ui/views/accelerator_table.cc:310: const int kNonShiftMask = On 2016/11/01 14:36:24, themblsha wrote: > On 2016/10/31 18:07:32, Peter Kasting wrote: > > Nit: constexpr (2 places) > > Tried to apply it to the std::set as well, but got this error: > ../../chrome/browser/ui/views/accelerator_table.cc:318:29: error: constexpr > variable cannot have non-literal type 'const std::set<int>' > constexpr std::set<int> control_whitelist = { > > Did you mean some other const expression? That means those sets can't be constexpr because their constructors are not declared constexpr. Don't worry about it, they can just be const. https://codereview.chromium.org/2074643003/diff/150001/chrome/browser/ui/view... File chrome/browser/ui/views/accelerator_table_unittest_mac.mm (right): https://codereview.chromium.org/2074643003/diff/150001/chrome/browser/ui/view... chrome/browser/ui/views/accelerator_table_unittest_mac.mm:17: namespace { On 2016/11/01 14:36:25, themblsha wrote: > On 2016/11/01 00:46:21, Peter Kasting wrote: > > Note that we're trying to kill the chrome namespace entirely. If it's easy, > > feel free to remove the namespace scoping from AcceleratorMapping. If not, > then > > either namespace everything in this file or qualify everything. > > Is there a crbug for that? I don't feel that I completely understand what that > implies: removing the `namespace chrome` in > src/chrome/browser/ui/views/accelerator_table.h, and fixing all the resulting > compilation errors on all platforms? This type is referenced only in seven > files, so it shouldn't be too hard. Right, you got it. Here's the bug: https://bugs.chromium.org/p/chromium/issues/detail?id=289619 Don't feel the need to do things like that in this CL unless they're easy and you want to (and you're touching a lot of the files already). Separate changes like that can easily go in separate CLs too. https://codereview.chromium.org/2074643003/diff/150001/chrome/browser/ui/view... chrome/browser/ui/views/accelerator_table_unittest_mac.mm:33: // the same vector data. On 2016/11/01 14:36:24, themblsha wrote: > On 2016/11/01 00:46:22, Peter Kasting wrote: > > Nit: I think you could omit this comment (and the type alias) by just making > > this a lambda in the relevant function below. Then it seems more clear what > > it's doing: > > > > // Convert the accelerator lists to sets for easy searching/filtering. > > auto ToSet = [](const std::vector<chrome::AcceleratorMapping>& v) { > > return std::set<chrome::AcceleratorMapping, AcceleratorMappingComparator>( > > v.begin(), v.end()); > > } > > auto accelerators = ToSet(GetAcceleratorList()); > > auto unfiltered_accelerators = > > ToSet(GetUnfilteredAcceleratorListForTesting()); > > > > Did it using a lambda. But not sure what's the best to forward-declare the > comparator lambda type, using a function pointer for now, as the alternative > that used decltype(lambda) was a little longer. You mean, storing the comparator lambda in an auto locally, then after that using decltype of that auto to define the set template? I don't know the syntax for that precisely offhand, so I'd have to see the actual code diff to compare the readability. The decltype route sounds nice in principle because it lets you just change the lambda to whatever you want without worrying about also updating a parallel comparator type, but the difference seems small. https://codereview.chromium.org/2074643003/diff/150001/chrome/browser/ui/view... chrome/browser/ui/views/accelerator_table_unittest_mac.mm:114: filtered_list.erase(m); On 2016/11/01 14:36:25, themblsha wrote: > On 2016/11/01 00:46:22, Peter Kasting wrote: > > See base::STLSetDifference(), which wraps this in some syntactic sugar to give > > you: > > > > auto filtered_list = > > base::STLSetDifference(full_accelerator_list, accelerator_list); > > That's great! There's one big gotcha though: std::set_difference needs to know > about the comparator, otherwise it won't compile. The easiest way to make it > work would be to add a global operator< for AcceleratorMapping, but I guess > that's not considered kosher. It isn't? Looks allowed to me by the Google style guide, with the requirement that the behavior must be what you'd expect, and if you overload one comparison operator you should overload them all. On one hand that seems like overkill just to allow this function, but OTOH it would eliminate the need for your lambda above, which would be nice. https://codereview.chromium.org/2074643003/diff/170001/chrome/browser/ui/view... File chrome/browser/ui/views/accelerator_table_unittest_mac.mm (right): https://codereview.chromium.org/2074643003/diff/170001/chrome/browser/ui/view... chrome/browser/ui/views/accelerator_table_unittest_mac.mm:138: for (const auto& m : accelerators) { Nit: {} not necessary
still lgtm with a couple of nits. We do also need to re-indent (as pkasting says - don't worry about git blame - the `-w` git blame arg helps - hopefully rietveld still presents the diff in a meaningful way for the other OWNERS you need. If not, a separate CL for the "return std::vector" stuff is best) On 2016/11/01 14:36:25, themblsha wrote: > Also I need to add some more reviewers for the global_keyboard_shortcuts*. > > Owners list avi, mark, rsesek and thakis. Whom should I add? weirdly only thakis@ will officially tick the box for global_keyboard_shortcuts_mac.h But you also need sky@ for: On 2016/10/31 18:07:32, Peter Kasting wrote: > I would > ask ben or sky about whether using virtual methods on the browser frame to > insert these particular checks is the correct plumbing. https://codereview.chromium.org/2074643003/diff/170001/chrome/browser/global_... File chrome/browser/global_keyboard_shortcuts_mac.mm (right): https://codereview.chromium.org/2074643003/diff/170001/chrome/browser/global_... chrome/browser/global_keyboard_shortcuts_mac.mm:84: for (const auto& e : table) { nit: e -> shortcut (or maybe entry, but "e" isn't a common abbreviation - see https://google.github.io/styleguide/cppguide.html#General_Naming_Rules) https://codereview.chromium.org/2074643003/diff/170001/chrome/browser/global_... File chrome/browser/global_keyboard_shortcuts_mac_unittest.mm (right): https://codereview.chromium.org/2074643003/diff/170001/chrome/browser/global_... chrome/browser/global_keyboard_shortcuts_mac_unittest.mm:21: for (const auto& e : GetWindowKeyboardShortcutTable()) { e -> shortcut, more below
https://codereview.chromium.org/2074643003/diff/150001/chrome/browser/global_... File chrome/browser/global_keyboard_shortcuts_views_mac.mm (right): https://codereview.chromium.org/2074643003/diff/150001/chrome/browser/global_... chrome/browser/global_keyboard_shortcuts_views_mac.mm:12: const KeyboardShortcutData* GetWindowKeyboardShortcutTable( On 2016/11/01 18:20:58, Peter Kasting wrote: > On 2016/11/01 14:36:24, themblsha wrote: > > On 2016/10/31 18:07:32, Peter Kasting wrote: > > > Nit: Out of scope for this CL, but consider modifying the Mac code that > calls > > > these sorts of functions to instead have them directly return > > > std::vector<KeyboardShortcutData>. These can be constructed fairly simply > > (see > > > my accelerator table comments) and passed around efficiently in C++11, and > > then > > > iterated over at the other end using range-based for. > > > > Not sure what you mean by C++11: move semantics? But it would imply that the > > table would need to be efficiently created from something to be moved to > > somewhere else? > > I simply mean that the combination of things like move-constructors, initializer > lists, and the RVO lets you do something like this without paying much > efficiency cost: > > std::vector<T> ConstructT() { return {{1, 2}, {3, 4}, ...}; } > std::vector<T> my_t = ConstructT(); > > ...so I would ultimately move toward syntax like that for these functions > instead of passing around pointers and sizes. > > CR_DEFINE_STATIC_LOCAL is possible too, it's just trickier to read and results > in these objects sticking around in memory. I tend to try to avoid it if I can, > though that's based on more gut reaction than hard logic. Will the std::vector just wrap the initializer list without costly copying of data during each initialization? I tried to test it using a small example, compiled it to LLVM IR and the construction function does quite a lot of stuff before the first branch instruction. These table functions are called at least once per each accelerator processing pass. Could be twice: PreHandle + Handle. https://codereview.chromium.org/2074643003/diff/150001/chrome/browser/ui/view... File chrome/browser/ui/views/accelerator_table.cc (right): https://codereview.chromium.org/2074643003/diff/150001/chrome/browser/ui/view... chrome/browser/ui/views/accelerator_table.cc:295: std::end(kAcceleratorMap)); On 2016/11/01 18:20:58, Peter Kasting wrote: > On 2016/11/01 14:36:24, themblsha wrote: > > On 2016/10/31 18:07:32, Peter Kasting wrote: > > > Nit: Or just inline kAcceleratorMap here: > > > > > > return { > > > { ui::VKEY_LEFT, ui::EF_ALT_DOWN, IDC_BACK }, > > > { ui::VKEY_BACK, ui::EF_NONE, IDC_BACKSPACE_BACK }, > > > .... > > > }; > > > > > > Then use this function when building |accelerators| in GetAcceleratorList() > > > below. > > > > I tried to preserve the original formatting of the kAcceleratorMap table in > > order not to break the git blame. Should I reformat the table as well? > > Yes, you should reformat/move it (don't insert your global accessor in the > middle of the anonymous namespace). Don't worry about git blame. > > If you want to make sure someone can really understand what's going on, do the > change to inline this as its own CL, with all the functional changes from this > current CL separate. Ok, I'd better do it separately alongside the de-chrome-ing of AcceleratorMapping. I should create a crbug for that or use the one I'm using for this CL? Or use https://bugs.chromium.org/p/chromium/issues/detail?id=289619 ? https://codereview.chromium.org/2074643003/diff/150001/chrome/browser/ui/view... File chrome/browser/ui/views/accelerator_table_unittest_mac.mm (right): https://codereview.chromium.org/2074643003/diff/150001/chrome/browser/ui/view... chrome/browser/ui/views/accelerator_table_unittest_mac.mm:33: // the same vector data. On 2016/11/01 18:20:59, Peter Kasting wrote: > On 2016/11/01 14:36:24, themblsha wrote: > > On 2016/11/01 00:46:22, Peter Kasting wrote: > > > Nit: I think you could omit this comment (and the type alias) by just making > > > this a lambda in the relevant function below. Then it seems more clear what > > > it's doing: > > > > > > // Convert the accelerator lists to sets for easy searching/filtering. > > > auto ToSet = [](const std::vector<chrome::AcceleratorMapping>& v) { > > > return std::set<chrome::AcceleratorMapping, > AcceleratorMappingComparator>( > > > v.begin(), v.end()); > > > } > > > auto accelerators = ToSet(GetAcceleratorList()); > > > auto unfiltered_accelerators = > > > ToSet(GetUnfilteredAcceleratorListForTesting()); > > > > > > > Did it using a lambda. But not sure what's the best to forward-declare the > > comparator lambda type, using a function pointer for now, as the alternative > > that used decltype(lambda) was a little longer. > > You mean, storing the comparator lambda in an auto locally, then after that > using decltype of that auto to define the set template? I don't know the syntax > for that precisely offhand, so I'd have to see the actual code diff to compare > the readability. The decltype route sounds nice in principle because it lets > you just change the lambda to whatever you want without worrying about also > updating a parallel comparator type, but the difference seems small. auto AcceleratorMappingComparator = [](const AcceleratorMapping& lhs, const AcceleratorMapping& rhs) { return std::tie(lhs.command_id, lhs.keycode, lhs.modifiers) < std::tie(rhs.command_id, rhs.keycode, rhs.modifiers); }; auto ToSet = [&AcceleratorMappingComparator]( const std::vector<AcceleratorMapping>& v) { return std::set<AcceleratorMapping, decltype(AcceleratorMappingComparator)>( v.begin(), v.end(), AcceleratorMappingComparator); }; I like the version I submitted better :-) https://codereview.chromium.org/2074643003/diff/150001/chrome/browser/ui/view... chrome/browser/ui/views/accelerator_table_unittest_mac.mm:114: filtered_list.erase(m); On 2016/11/01 18:20:59, Peter Kasting wrote: > On 2016/11/01 14:36:25, themblsha wrote: > > On 2016/11/01 00:46:22, Peter Kasting wrote: > > > See base::STLSetDifference(), which wraps this in some syntactic sugar to > give > > > you: > > > > > > auto filtered_list = > > > base::STLSetDifference(full_accelerator_list, accelerator_list); > > > > That's great! There's one big gotcha though: std::set_difference needs to know > > about the comparator, otherwise it won't compile. The easiest way to make it > > work would be to add a global operator< for AcceleratorMapping, but I guess > > that's not considered kosher. > > It isn't? Looks allowed to me by the Google style guide, with the requirement > that the behavior must be what you'd expect, and if you overload one comparison > operator you should overload them all. > > On one hand that seems like overkill just to allow this function, but OTOH it > would eliminate the need for your lambda above, which would be nice. It says "if your type doesn't have a natural ordering, but you want to store it in a std::set, use a custom comparator rather than overloading <." So I guess the lambda comparator lives on. https://codereview.chromium.org/2074643003/diff/170001/chrome/browser/ui/view... File chrome/browser/ui/views/accelerator_table_unittest_mac.mm (right): https://codereview.chromium.org/2074643003/diff/170001/chrome/browser/ui/view... chrome/browser/ui/views/accelerator_table_unittest_mac.mm:138: for (const auto& m : accelerators) { On 2016/11/01 18:20:59, Peter Kasting wrote: > Nit: {} not necessary Done.
mblsha@yandex-team.ru changed reviewers: + sky@chromium.org, thakis@chromium.org
+thakis +sky On 2016/10/31 18:07:32, Peter Kasting wrote: > I would ask ben or sky about whether using virtual methods on the browser frame to > insert these particular checks is the correct plumbing. Also please take a look at global_keyboard_shortcuts*, as I need more approvals for them :-)
https://codereview.chromium.org/2074643003/diff/150001/chrome/browser/global_... File chrome/browser/global_keyboard_shortcuts_views_mac.mm (right): https://codereview.chromium.org/2074643003/diff/150001/chrome/browser/global_... chrome/browser/global_keyboard_shortcuts_views_mac.mm:12: const KeyboardShortcutData* GetWindowKeyboardShortcutTable( On 2016/11/02 17:31:37, themblsha wrote: > On 2016/11/01 18:20:58, Peter Kasting wrote: > > On 2016/11/01 14:36:24, themblsha wrote: > > > On 2016/10/31 18:07:32, Peter Kasting wrote: > > > > Nit: Out of scope for this CL, but consider modifying the Mac code that > > calls > > > > these sorts of functions to instead have them directly return > > > > std::vector<KeyboardShortcutData>. These can be constructed fairly simply > > > (see > > > > my accelerator table comments) and passed around efficiently in C++11, and > > > then > > > > iterated over at the other end using range-based for. > > > > > > Not sure what you mean by C++11: move semantics? But it would imply that the > > > table would need to be efficiently created from something to be moved to > > > somewhere else? > > > > I simply mean that the combination of things like move-constructors, > initializer > > lists, and the RVO lets you do something like this without paying much > > efficiency cost: > > > > std::vector<T> ConstructT() { return {{1, 2}, {3, 4}, ...}; } > > std::vector<T> my_t = ConstructT(); > > > > ...so I would ultimately move toward syntax like that for these functions > > instead of passing around pointers and sizes. > > > > CR_DEFINE_STATIC_LOCAL is possible too, it's just trickier to read and results > > in these objects sticking around in memory. I tend to try to avoid it if I > can, > > though that's based on more gut reaction than hard logic. > > Will the std::vector just wrap the initializer list without costly copying of > data during each initialization? > > I tried to test it using a small example, compiled it to LLVM IR and the > construction function does quite a lot of stuff before the first branch > instruction. > > These table functions are called at least once per each accelerator processing > pass. Could be twice: PreHandle + Handle. I see. In that case you might indeed want to avoid directly returning a vector, since I think we'll copy the data once during each init. The CR_DEFINE_STATIC_LOCAL route would be one way to achieve this. https://codereview.chromium.org/2074643003/diff/150001/chrome/browser/ui/view... File chrome/browser/ui/views/accelerator_table.cc (right): https://codereview.chromium.org/2074643003/diff/150001/chrome/browser/ui/view... chrome/browser/ui/views/accelerator_table.cc:295: std::end(kAcceleratorMap)); On 2016/11/02 17:31:37, themblsha wrote: > On 2016/11/01 18:20:58, Peter Kasting wrote: > > On 2016/11/01 14:36:24, themblsha wrote: > > > On 2016/10/31 18:07:32, Peter Kasting wrote: > > > > Nit: Or just inline kAcceleratorMap here: > > > > > > > > return { > > > > { ui::VKEY_LEFT, ui::EF_ALT_DOWN, IDC_BACK }, > > > > { ui::VKEY_BACK, ui::EF_NONE, IDC_BACKSPACE_BACK }, > > > > .... > > > > }; > > > > > > > > Then use this function when building |accelerators| in > GetAcceleratorList() > > > > below. > > > > > > I tried to preserve the original formatting of the kAcceleratorMap table in > > > order not to break the git blame. Should I reformat the table as well? > > > > Yes, you should reformat/move it (don't insert your global accessor in the > > middle of the anonymous namespace). Don't worry about git blame. > > > > If you want to make sure someone can really understand what's going on, do the > > change to inline this as its own CL, with all the functional changes from this > > current CL separate. > > Ok, I'd better do it separately alongside the de-chrome-ing of > AcceleratorMapping. I should create a crbug for that or use the one I'm using > for this CL? Or use https://bugs.chromium.org/p/chromium/issues/detail?id=289619 > ? For de-chrome-ing, use bug 289619. For inlining kAcceleratorMap, I'd probably do it with no bug at all, but you're welcome to do whatever you want there. https://codereview.chromium.org/2074643003/diff/150001/chrome/browser/ui/view... File chrome/browser/ui/views/accelerator_table_unittest_mac.mm (right): https://codereview.chromium.org/2074643003/diff/150001/chrome/browser/ui/view... chrome/browser/ui/views/accelerator_table_unittest_mac.mm:33: // the same vector data. On 2016/11/02 17:31:37, themblsha wrote: > On 2016/11/01 18:20:59, Peter Kasting wrote: > > On 2016/11/01 14:36:24, themblsha wrote: > > > On 2016/11/01 00:46:22, Peter Kasting wrote: > > > > Nit: I think you could omit this comment (and the type alias) by just > making > > > > this a lambda in the relevant function below. Then it seems more clear > what > > > > it's doing: > > > > > > > > // Convert the accelerator lists to sets for easy searching/filtering. > > > > auto ToSet = [](const std::vector<chrome::AcceleratorMapping>& v) { > > > > return std::set<chrome::AcceleratorMapping, > > AcceleratorMappingComparator>( > > > > v.begin(), v.end()); > > > > } > > > > auto accelerators = ToSet(GetAcceleratorList()); > > > > auto unfiltered_accelerators = > > > > ToSet(GetUnfilteredAcceleratorListForTesting()); > > > > > > > > > > Did it using a lambda. But not sure what's the best to forward-declare the > > > comparator lambda type, using a function pointer for now, as the alternative > > > that used decltype(lambda) was a little longer. > > > > You mean, storing the comparator lambda in an auto locally, then after that > > using decltype of that auto to define the set template? I don't know the > syntax > > for that precisely offhand, so I'd have to see the actual code diff to compare > > the readability. The decltype route sounds nice in principle because it lets > > you just change the lambda to whatever you want without worrying about also > > updating a parallel comparator type, but the difference seems small. > > auto AcceleratorMappingComparator = > [](const AcceleratorMapping& lhs, const AcceleratorMapping& rhs) { > return std::tie(lhs.command_id, lhs.keycode, lhs.modifiers) < > std::tie(rhs.command_id, rhs.keycode, rhs.modifiers); > }; > auto ToSet = [&AcceleratorMappingComparator]( > const std::vector<AcceleratorMapping>& v) { > return std::set<AcceleratorMapping, decltype(AcceleratorMappingComparator)>( > v.begin(), v.end(), AcceleratorMappingComparator); > }; > > I like the version I submitted better :-) It's pretty much a tossup for me. I might tweak a bit for brevity: auto Comparator = [](const AcceleratorMapping& lhs, const AcceleratorMapping& rhs) { return std::tie(lhs.command_id, lhs.keycode, lhs.modifiers) < std::tie(rhs.command_id, rhs.keycode, rhs.modifiers); }; auto ToSet = [&](const std::vector<AcceleratorMapping>& v) { return std::set<AcceleratorMapping, decltype(Comparator)>( v.begin(), v.end(), Comparator); }; ...which to me makes it a slight win, but not a big deal either way. https://codereview.chromium.org/2074643003/diff/150001/chrome/browser/ui/view... chrome/browser/ui/views/accelerator_table_unittest_mac.mm:114: filtered_list.erase(m); On 2016/11/02 17:31:37, themblsha wrote: > On 2016/11/01 18:20:59, Peter Kasting wrote: > > On 2016/11/01 14:36:25, themblsha wrote: > > > On 2016/11/01 00:46:22, Peter Kasting wrote: > > > > See base::STLSetDifference(), which wraps this in some syntactic sugar to > > give > > > > you: > > > > > > > > auto filtered_list = > > > > base::STLSetDifference(full_accelerator_list, accelerator_list); > > > > > > That's great! There's one big gotcha though: std::set_difference needs to > know > > > about the comparator, otherwise it won't compile. The easiest way to make it > > > work would be to add a global operator< for AcceleratorMapping, but I guess > > > that's not considered kosher. > > > > It isn't? Looks allowed to me by the Google style guide, with the requirement > > that the behavior must be what you'd expect, and if you overload one > comparison > > operator you should overload them all. > > > > On one hand that seems like overkill just to allow this function, but OTOH it > > would eliminate the need for your lambda above, which would be nice. > > It says "if your type doesn't have a natural ordering, but you want to store it > in a std::set, use a custom comparator rather than overloading <." Ah, I missed that! Thanks.
https://codereview.chromium.org/2074643003/diff/210001/chrome/browser/ui/view... File chrome/browser/ui/views/frame/native_browser_frame.h (right): https://codereview.chromium.org/2074643003/diff/210001/chrome/browser/ui/view... chrome/browser/ui/views/frame/native_browser_frame.h:45: virtual bool PreHandleKeyboardEvent( Make this pure virtual to match the rest of the functions in this class.
Can I add someone instead of thakis? Or should I try to ping him more? https://codereview.chromium.org/2074643003/diff/210001/chrome/browser/ui/view... File chrome/browser/ui/views/frame/native_browser_frame.h (right): https://codereview.chromium.org/2074643003/diff/210001/chrome/browser/ui/view... chrome/browser/ui/views/frame/native_browser_frame.h:45: virtual bool PreHandleKeyboardEvent( On 2016/11/07 16:53:34, sky wrote: > Make this pure virtual to match the rest of the functions in this class. Done.
https://codereview.chromium.org/2074643003/diff/230001/chrome/browser/global_... File chrome/browser/global_keyboard_shortcuts_views_mac.mm (right): https://codereview.chromium.org/2074643003/diff/230001/chrome/browser/global_... chrome/browser/global_keyboard_shortcuts_views_mac.mm:7: #include "chrome/browser/global_keyboard_shortcuts_mac.h" this should be the first include. https://codereview.chromium.org/2074643003/diff/230001/chrome/browser/ui/view... File chrome/browser/ui/views/accelerator_table.cc (right): https://codereview.chromium.org/2074643003/diff/230001/chrome/browser/ui/view... chrome/browser/ui/views/accelerator_table.cc:300: (GetUnfilteredAcceleratorListForTesting())); Having a non-testing function call a for-testing function is confusing. How about create a GetAllAcceleratorMappings in the anonymous namespace that both call? https://codereview.chromium.org/2074643003/diff/230001/chrome/browser/ui/view... chrome/browser/ui/views/accelerator_table.cc:308: const std::set<int> kControlWhitelist = { optional: the style guide says you can optionally name non-POD types with kFoo style, but generally we don't do this. Please use control_whitelist. https://codereview.chromium.org/2074643003/diff/230001/chrome/browser/ui/view... chrome/browser/ui/views/accelerator_table.cc:312: auto remove_accelerators = [&kControlWhitelist](const AcceleratorMapping& m) { It seems like you're trying to allow for accelerators to be defined that don't work on the mac, but pruned here. Is that right? If so, I think all this should be a DCHECK and the table should be fixed for mac. To do otherwise may result in odd behavior when accelerators are removed that one wouldn't expect to be removed. https://codereview.chromium.org/2074643003/diff/230001/chrome/browser/ui/view... File chrome/browser/ui/views/accelerator_table_unittest_mac.mm (right): https://codereview.chromium.org/2074643003/diff/230001/chrome/browser/ui/view... chrome/browser/ui/views/accelerator_table_unittest_mac.mm:13: #include "chrome/browser/ui/views/accelerator_table.h" This should be the first include. https://codereview.chromium.org/2074643003/diff/230001/chrome/browser/ui/view... File chrome/browser/ui/views/frame/browser_frame.h (right): https://codereview.chromium.org/2074643003/diff/230001/chrome/browser/ui/view... chrome/browser/ui/views/frame/browser_frame.h:102: bool PreHandleKeyboardEvent(const content::NativeWebKeyboardEvent& event); Please use an enum for the return type to make the return value clear.
tapted: what do you think about the sky's proposal and my resulting change I've uploaded as a gist? https://codereview.chromium.org/2074643003/diff/230001/chrome/browser/global_... File chrome/browser/global_keyboard_shortcuts_views_mac.mm (right): https://codereview.chromium.org/2074643003/diff/230001/chrome/browser/global_... chrome/browser/global_keyboard_shortcuts_views_mac.mm:7: #include "chrome/browser/global_keyboard_shortcuts_mac.h" On 2016/11/08 18:34:09, sky wrote: > this should be the first include. This results in a warning, because the include file name doesn't match the .mm: Your #include order seems to be broken. Remember to use the right collation (LC_COLLATE=C) and check https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes chrome/browser/global_keyboard_shortcuts_cocoa_mac.mm:7: - line belongs before previous line \ OK, I ignored the warning. https://codereview.chromium.org/2074643003/diff/230001/chrome/browser/ui/view... File chrome/browser/ui/views/accelerator_table.cc (right): https://codereview.chromium.org/2074643003/diff/230001/chrome/browser/ui/view... chrome/browser/ui/views/accelerator_table.cc:300: (GetUnfilteredAcceleratorListForTesting())); On 2016/11/08 18:34:10, sky wrote: > Having a non-testing function call a for-testing function is confusing. How > about create a GetAllAcceleratorMappings in the anonymous namespace that both > call? Good idea, done. https://codereview.chromium.org/2074643003/diff/230001/chrome/browser/ui/view... chrome/browser/ui/views/accelerator_table.cc:308: const std::set<int> kControlWhitelist = { On 2016/11/08 18:34:09, sky wrote: > optional: the style guide says you can optionally name non-POD types with kFoo > style, but generally we don't do this. Please use control_whitelist. pkasting originally asked me to use the kCamelCase name. Reverted. https://codereview.chromium.org/2074643003/diff/230001/chrome/browser/ui/view... chrome/browser/ui/views/accelerator_table.cc:312: auto remove_accelerators = [&kControlWhitelist](const AcceleratorMapping& m) { On 2016/11/08 18:34:10, sky wrote: > It seems like you're trying to allow for accelerators to be defined that don't > work on the mac, but pruned here. Is that right? If so, I think all this should > be a DCHECK and the table should be fixed for mac. To do otherwise may result in > odd behavior when accelerators are removed that one wouldn't expect to be > removed. Originally I was doing this, but it resulted in a lot of #ifdefs, and tapted wanted a cleaner approach. Here's how it would look with additional #ifdefs (I've included a diff as well as complete table): https://gist.github.com/mblsha/0cf9de8da549e23a3ebd01cf56dbebe5 Do you think that the readability tradeoff is worth it for maintainability?
On 2016/11/02 17:42:03, themblsha wrote: > +thakis which file do you want me to look at? (sorry for delay, was traveling for a bit) > +sky > > On 2016/10/31 18:07:32, Peter Kasting wrote: > > I would ask ben or sky about whether using virtual methods on the browser > frame to > > insert these particular checks is the correct plumbing. > > Also please take a look at global_keyboard_shortcuts*, as I need more approvals > for them :-)
Yay! thakis: Please look at the chrome/browser/global_keyboard_shortcuts* :-)
On 2016/11/09 14:54:02, themblsha wrote: > tapted: what do you think about the sky's proposal and my resulting change I've > uploaded as a gist? > > https://codereview.chromium.org/2074643003/diff/230001/chrome/browser/global_... > File chrome/browser/global_keyboard_shortcuts_views_mac.mm (right): > > https://codereview.chromium.org/2074643003/diff/230001/chrome/browser/global_... > chrome/browser/global_keyboard_shortcuts_views_mac.mm:7: #include > "chrome/browser/global_keyboard_shortcuts_mac.h" > On 2016/11/08 18:34:09, sky wrote: > > this should be the first include. > > This results in a warning, because the include file name doesn't match the .mm: > > Your #include order seems to be broken. Remember to use the right collation > (LC_COLLATE=C) and check > https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes > chrome/browser/global_keyboard_shortcuts_cocoa_mac.mm:7: - line belongs > before previous line \ > > OK, I ignored the warning. > > https://codereview.chromium.org/2074643003/diff/230001/chrome/browser/ui/view... > File chrome/browser/ui/views/accelerator_table.cc (right): > > https://codereview.chromium.org/2074643003/diff/230001/chrome/browser/ui/view... > chrome/browser/ui/views/accelerator_table.cc:300: > (GetUnfilteredAcceleratorListForTesting())); > On 2016/11/08 18:34:10, sky wrote: > > Having a non-testing function call a for-testing function is confusing. How > > about create a GetAllAcceleratorMappings in the anonymous namespace that both > > call? > > Good idea, done. > > https://codereview.chromium.org/2074643003/diff/230001/chrome/browser/ui/view... > chrome/browser/ui/views/accelerator_table.cc:308: const std::set<int> > kControlWhitelist = { > On 2016/11/08 18:34:09, sky wrote: > > optional: the style guide says you can optionally name non-POD types with kFoo > > style, but generally we don't do this. Please use control_whitelist. > > pkasting originally asked me to use the kCamelCase name. Reverted. > > https://codereview.chromium.org/2074643003/diff/230001/chrome/browser/ui/view... > chrome/browser/ui/views/accelerator_table.cc:312: auto remove_accelerators = > [&kControlWhitelist](const AcceleratorMapping& m) { > On 2016/11/08 18:34:10, sky wrote: > > It seems like you're trying to allow for accelerators to be defined that don't > > work on the mac, but pruned here. Is that right? If so, I think all this > should > > be a DCHECK and the table should be fixed for mac. To do otherwise may result > in > > odd behavior when accelerators are removed that one wouldn't expect to be > > removed. > > Originally I was doing this, but it resulted in a lot of #ifdefs, and tapted > wanted a cleaner approach. > > Here's how it would look with additional #ifdefs (I've included a diff as well > as complete table): > https://gist.github.com/mblsha/0cf9de8da549e23a3ebd01cf56dbebe5 That looks ugly because you didn't group them. If instead you group all the !mac accelerators won't it look at lot more readable? > > Do you think that the readability tradeoff is worth it for maintainability? I think grouping them isn't that bad, and as I mentioned it's less mysterious.
lgtm
Ok, grouped them at the bottom. I left IDC_EXIT in place, because it's in the area of effect of the comment that's just before it.
https://codereview.chromium.org/2074643003/diff/270001/chrome/browser/ui/view... File chrome/browser/ui/views/accelerator_table.cc (right): https://codereview.chromium.org/2074643003/diff/270001/chrome/browser/ui/view... chrome/browser/ui/views/accelerator_table.cc:315: auto remove_accelerators = [](const AcceleratorMapping& m) { Isn't this unnecessary now? A NDEBUG block if you want to verify that is fine.
https://codereview.chromium.org/2074643003/diff/230001/chrome/browser/global_... File chrome/browser/global_keyboard_shortcuts_views_mac.mm (right): https://codereview.chromium.org/2074643003/diff/230001/chrome/browser/global_... chrome/browser/global_keyboard_shortcuts_views_mac.mm:7: #include "chrome/browser/global_keyboard_shortcuts_mac.h" On 2016/11/09 14:54:02, themblsha wrote: > On 2016/11/08 18:34:09, sky wrote: > > this should be the first include. > > This results in a warning, because the include file name doesn't match the .mm: > > Your #include order seems to be broken. Remember to use the right collation > (LC_COLLATE=C) and check > https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes > chrome/browser/global_keyboard_shortcuts_cocoa_mac.mm:7: - line belongs > before previous line \ > > OK, I ignored the warning. In this case, I think your previous ordering was right, since there is no _views_mac.h. (Or in other words, I think the warning is correct.) https://codereview.chromium.org/2074643003/diff/230001/chrome/browser/ui/view... File chrome/browser/ui/views/accelerator_table.cc (right): https://codereview.chromium.org/2074643003/diff/230001/chrome/browser/ui/view... chrome/browser/ui/views/accelerator_table.cc:308: const std::set<int> kControlWhitelist = { On 2016/11/09 14:54:02, themblsha wrote: > On 2016/11/08 18:34:09, sky wrote: > > optional: the style guide says you can optionally name non-POD types with kFoo > > style, but generally we don't do this. Please use control_whitelist. > > pkasting originally asked me to use the kCamelCase name. Reverted. Scott, can you point to the relevant style guide text on this? I found http://google.github.io/styleguide/cppguide.html#Constant_Names , but that doesn't seem to distinguish between POD and non-POD types. Nor does https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... . I'm not aware that we normally make any distinction. https://codereview.chromium.org/2074643003/diff/230001/chrome/browser/ui/view... chrome/browser/ui/views/accelerator_table.cc:312: auto remove_accelerators = [&kControlWhitelist](const AcceleratorMapping& m) { On 2016/11/09 14:54:02, themblsha wrote: > On 2016/11/08 18:34:10, sky wrote: > > It seems like you're trying to allow for accelerators to be defined that don't > > work on the mac, but pruned here. Is that right? If so, I think all this > should > > be a DCHECK and the table should be fixed for mac. To do otherwise may result > in > > odd behavior when accelerators are removed that one wouldn't expect to be > > removed. > > Originally I was doing this, but it resulted in a lot of #ifdefs, and tapted > wanted a cleaner approach. > > Here's how it would look with additional #ifdefs (I've included a diff as well > as complete table): > https://gist.github.com/mblsha/0cf9de8da549e23a3ebd01cf56dbebe5 > > Do you think that the readability tradeoff is worth it for maintainability? Personally, I much prefer the current solution if the alternative doesn't actually git rid of this lambda but merely simplifies it. I think Scott was hoping to kill this lambda and erase(remove_if()) call entirely; is that possible?
https://codereview.chromium.org/2074643003/diff/230001/chrome/browser/ui/view... File chrome/browser/ui/views/accelerator_table.cc (right): https://codereview.chromium.org/2074643003/diff/230001/chrome/browser/ui/view... chrome/browser/ui/views/accelerator_table.cc:308: const std::set<int> kControlWhitelist = { On 2016/11/09 18:39:12, Peter Kasting wrote: > On 2016/11/09 14:54:02, themblsha wrote: > > On 2016/11/08 18:34:09, sky wrote: > > > optional: the style guide says you can optionally name non-POD types with > kFoo > > > style, but generally we don't do this. Please use control_whitelist. > > > > pkasting originally asked me to use the kCamelCase name. Reverted. > > Scott, can you point to the relevant style guide text on this? I found > http://google.github.io/styleguide/cppguide.html#Constant_Names , but that > doesn't seem to distinguish between POD and non-POD types. Nor does > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... > . > > I'm not aware that we normally make any distinction. I said the style guide allows it. My comment was more that generally I haven't seem the kFoo style for statics like this. That's why I prefixed the comment with 'optional'. https://codereview.chromium.org/2074643003/diff/230001/chrome/browser/ui/view... chrome/browser/ui/views/accelerator_table.cc:312: auto remove_accelerators = [&kControlWhitelist](const AcceleratorMapping& m) { On 2016/11/09 18:39:12, Peter Kasting wrote: > On 2016/11/09 14:54:02, themblsha wrote: > > On 2016/11/08 18:34:10, sky wrote: > > > It seems like you're trying to allow for accelerators to be defined that > don't > > > work on the mac, but pruned here. Is that right? If so, I think all this > > should > > > be a DCHECK and the table should be fixed for mac. To do otherwise may > result > > in > > > odd behavior when accelerators are removed that one wouldn't expect to be > > > removed. > > > > Originally I was doing this, but it resulted in a lot of #ifdefs, and tapted > > wanted a cleaner approach. > > > > Here's how it would look with additional #ifdefs (I've included a diff as well > > as complete table): > > https://gist.github.com/mblsha/0cf9de8da549e23a3ebd01cf56dbebe5 > > > > Do you think that the readability tradeoff is worth it for maintainability? > > Personally, I much prefer the current solution if the alternative doesn't > actually git rid of this lambda but merely simplifies it. I think Scott was > hoping to kill this lambda and erase(remove_if()) call entirely; is that > possible? That's right. I'm hoping removing of accelerators can go away completely. I'm ok with a NDEBUG block that loops over ensuring a bad accelerator doesn't exist.
https://codereview.chromium.org/2074643003/diff/230001/chrome/browser/ui/view... File chrome/browser/ui/views/accelerator_table.cc (right): https://codereview.chromium.org/2074643003/diff/230001/chrome/browser/ui/view... chrome/browser/ui/views/accelerator_table.cc:308: const std::set<int> kControlWhitelist = { On 2016/11/09 18:42:43, sky wrote: > On 2016/11/09 18:39:12, Peter Kasting wrote: > > On 2016/11/09 14:54:02, themblsha wrote: > > > On 2016/11/08 18:34:09, sky wrote: > > > > optional: the style guide says you can optionally name non-POD types with > > kFoo > > > > style, but generally we don't do this. Please use control_whitelist. > > > > > > pkasting originally asked me to use the kCamelCase name. Reverted. > > > > Scott, can you point to the relevant style guide text on this? I found > > http://google.github.io/styleguide/cppguide.html#Constant_Names , but that > > doesn't seem to distinguish between POD and non-POD types. Nor does > > > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... > > . > > > > I'm not aware that we normally make any distinction. > > I said the style guide allows it. My comment was more that generally I haven't > seem the kFoo style for statics like this. That's why I prefixed the comment > with 'optional'. OK. I don't know that we commonly have const non-PODs, so I'm not sure whether there is any consistent pattern. I thought the kFoo name made it clearer that this is a compile-time constant, since that's how we do it for other types, but I don't feel strongly.
The CQ bit was checked by tapted@chromium.org
lgtm On 2016/11/09 14:54:02, themblsha wrote: > tapted: what do you think about the sky's proposal and my resulting change I've uploaded as a gist? https://codereview.chromium.org/2074643003/diff/270001/chrome/browser/ui/view... File chrome/browser/ui/views/accelerator_table.cc (right): https://codereview.chromium.org/2074643003/diff/270001/chrome/browser/ui/view... chrome/browser/ui/views/accelerator_table.cc:11: #include <set> (remember to update the #includes) https://codereview.chromium.org/2074643003/diff/270001/chrome/browser/ui/view... chrome/browser/ui/views/accelerator_table.cc:37: // the Windows accelerators in ../../app/chrome_dll.rc. This comment was one of the motivations for the filtering approach. I don't there's a neat way to maintain this requirement without filtering. A better alternative might be a completely separate kAcceleratorMap for Mac? Or we could drop the other {alphabetical, chrome_dll.rc} ordering requirements and just cluster things by platform. https://codereview.chromium.org/2074643003/diff/270001/chrome/browser/ui/view... chrome/browser/ui/views/accelerator_table.cc:231: // Control modifier is rarely used on Mac, so we allow it only in several This is now in !Mac, so the comment doesn't look right here. https://codereview.chromium.org/2074643003/diff/270001/chrome/browser/ui/view... chrome/browser/ui/views/accelerator_table.cc:235: { ui::VKEY_I, ui::EF_SHIFT_DOWN | ui::EF_CONTROL_DOWN, IDC_DEV_TOOLS }, (E.g. Moving this down violates the desire to keep this in the same order as chrome_dll.rc)
The patchset sent to the CQ was uploaded after l-g-t-m from pkasting@chromium.org, thakis@chromium.org Link to the patchset: https://codereview.chromium.org/2074643003/#ps270001 (title: "#ifdef out Ctrl shortcuts on Mac.")
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 tapted@chromium.org
ugh not lgtm yet. .. Oh the dangers of different platforms deciding to flip which side the default button should go on..
On 2016/11/09 14:54:02, themblsha wrote: > tapted: what do you think about the sky's proposal and my resulting change I've uploaded as a gist? See the comments I've added. Basically I defer to sky on this, but there are some other downsides I've highlighted. I probably also have a stronger aversion to #ifdefs than most. Perhaps we need to see how it looks with Alt removed as well. Patchset 15 is currently dropping the ordering requirement, so if we head in this direction the comment about chrome_dll.rcshould be updated.
I think Scott's desire to avoid filtering, when we can easily do this by simply making the list look like what we want, is reasonable. I also think it's reasonable to keep the filtering method, if the alternative involves either significant duplication of accelerators, contortions with lots of different ifdefs, or other methods that would cause meaningful readability/maintenance risks. In the end, you should make the call based on what will be easiest for a future reader to understand and maintain, and hardest to introduce bugs and inconsistencies into.
https://codereview.chromium.org/2074643003/diff/230001/chrome/browser/global_... File chrome/browser/global_keyboard_shortcuts_views_mac.mm (right): https://codereview.chromium.org/2074643003/diff/230001/chrome/browser/global_... chrome/browser/global_keyboard_shortcuts_views_mac.mm:7: #include "chrome/browser/global_keyboard_shortcuts_mac.h" On 2016/11/09 18:39:12, Peter Kasting wrote: > In this case, I think your previous ordering was right, since there is no > _views_mac.h. (Or in other words, I think the warning is correct.) Ok, restored the includes :-) https://codereview.chromium.org/2074643003/diff/230001/chrome/browser/ui/view... File chrome/browser/ui/views/accelerator_table.cc (right): https://codereview.chromium.org/2074643003/diff/230001/chrome/browser/ui/view... chrome/browser/ui/views/accelerator_table.cc:312: auto remove_accelerators = [&kControlWhitelist](const AcceleratorMapping& m) { On 2016/11/09 18:42:43, sky wrote: > On 2016/11/09 18:39:12, Peter Kasting wrote: > > Personally, I much prefer the current solution if the alternative doesn't > > actually git rid of this lambda but merely simplifies it. I think Scott was > > hoping to kill this lambda and erase(remove_if()) call entirely; is that > > possible? > > That's right. I'm hoping removing of accelerators can go away completely. I'm ok > with a NDEBUG block that loops over ensuring a bad accelerator doesn't exist. Ah! Okay, I pruned the accelerator list some more, and removed the checks. They're still present in the unit_test. Now it's probably better, but it's a little bit easier to get lost in the #ifdefs. https://codereview.chromium.org/2074643003/diff/270001/chrome/browser/ui/view... File chrome/browser/ui/views/accelerator_table.cc (right): https://codereview.chromium.org/2074643003/diff/270001/chrome/browser/ui/view... chrome/browser/ui/views/accelerator_table.cc:37: // the Windows accelerators in ../../app/chrome_dll.rc. On 2016/11/09 22:29:52, tapted wrote: > Or we could drop the other {alphabetical, chrome_dll.rc} ordering requirements > and just cluster things by platform. BTW is there any reason that chrome_dll.rc is not strictly alphabetical? I've tried re-sorting it and it results in this diff: https://gist.github.com/mblsha/514db7237dbbedc20d2ae9f21f6eee81 https://codereview.chromium.org/2074643003/diff/270001/chrome/browser/ui/view... chrome/browser/ui/views/accelerator_table.cc:231: // Control modifier is rarely used on Mac, so we allow it only in several On 2016/11/09 22:29:51, tapted wrote: > This is now in !Mac, so the comment doesn't look right here. I've now also added a bit about Alt-shortcuts. My intention was to describe why all these shortcuts are bunched up in !Mac section so it would be easier to find a correct place for new accelerators in the future. I've tried re-wording it, but it becomes less concise and less clear of the intention. https://codereview.chromium.org/2074643003/diff/270001/chrome/browser/ui/view... chrome/browser/ui/views/accelerator_table.cc:235: { ui::VKEY_I, ui::EF_SHIFT_DOWN | ui::EF_CONTROL_DOWN, IDC_DEV_TOOLS }, On 2016/11/09 22:29:52, tapted wrote: > (E.g. Moving this down violates the desire to keep this in the same order as > chrome_dll.rc) Ohcrap. I've re-sorted the accelerators in this section now, but I don't know how to satisfy the same ordering as with chrome_dll.rc :/ https://codereview.chromium.org/2074643003/diff/270001/chrome/browser/ui/view... chrome/browser/ui/views/accelerator_table.cc:315: auto remove_accelerators = [](const AcceleratorMapping& m) { On 2016/11/09 18:30:32, sky wrote: > Isn't this unnecessary now? A NDEBUG block if you want to verify that is fine. We still need to remove the Alt+VKEY and Alt+Shift+VKEY combinations. There's no whitelist so they're completely pruned.
https://codereview.chromium.org/2074643003/diff/270001/chrome/browser/ui/view... File chrome/browser/ui/views/accelerator_table.cc (right): https://codereview.chromium.org/2074643003/diff/270001/chrome/browser/ui/view... chrome/browser/ui/views/accelerator_table.cc:37: // the Windows accelerators in ../../app/chrome_dll.rc. On 2016/11/10 14:18:05, themblsha wrote: > On 2016/11/09 22:29:52, tapted wrote: > > Or we could drop the other {alphabetical, chrome_dll.rc} ordering requirements > > and just cluster things by platform. > > BTW is there any reason that chrome_dll.rc is not strictly alphabetical? I've > tried re-sorting it and it results in this diff: > https://gist.github.com/mblsha/514db7237dbbedc20d2ae9f21f6eee81 Not that I know of. I vote for "sort that list", and that takes care of some of the issues with "try to keep this list in that list's order". https://codereview.chromium.org/2074643003/diff/270001/chrome/browser/ui/view... chrome/browser/ui/views/accelerator_table.cc:315: auto remove_accelerators = [](const AcceleratorMapping& m) { On 2016/11/10 14:18:05, themblsha wrote: > On 2016/11/09 18:30:32, sky wrote: > > Isn't this unnecessary now? A NDEBUG block if you want to verify that is fine. > > We still need to remove the Alt+VKEY and Alt+Shift+VKEY combinations. There's no > whitelist so they're completely pruned. If we still need this lambda regardless, then I'm skeptical of the tradeoff that simplifies this lambda but adds more ifdefs above. Is there an updated version of the diff just between those two approaches?
lgtm with the following, if sky is happy :) https://codereview.chromium.org/2074643003/diff/290001/chrome/browser/ui/view... File chrome/browser/ui/views/accelerator_table.cc (right): https://codereview.chromium.org/2074643003/diff/290001/chrome/browser/ui/view... chrome/browser/ui/views/accelerator_table.cc:12: #include <tuple> I think you can remove all of these now https://codereview.chromium.org/2074643003/diff/290001/chrome/browser/ui/view... chrome/browser/ui/views/accelerator_table.cc:36: // NOTE: Keep this list in the same (mostly-alphabetical) order as Perhaps update this comment. Something like, "Between each ifdef block, keep the list in the same (mostly-alphabetical) order as the Windows accelerators in ../../app/chrome_dll.rc." https://codereview.chromium.org/2074643003/diff/290001/chrome/browser/ui/view... chrome/browser/ui/views/accelerator_table.cc:68: { ui::VKEY_TAB, ui::EF_CONTROL_DOWN, IDC_SELECT_NEXT_TAB }, The "Control modifier is rarely used on Mac, so we allow it only in several specific cases here." comment belongs up here https://codereview.chromium.org/2074643003/diff/290001/chrome/browser/ui/view... chrome/browser/ui/views/accelerator_table.cc:107: #endif // OS_LINIUX && !OS_CHROMEOS https://codereview.chromium.org/2074643003/diff/290001/chrome/browser/ui/view... chrome/browser/ui/views/accelerator_table.cc:154: #else // !OS_CHROMEOS here? https://codereview.chromium.org/2074643003/diff/290001/chrome/browser/ui/view... chrome/browser/ui/views/accelerator_table.cc:163: // For each entry here add an entry into kChromeCmdId2AshActionId below It's unclear where this comment applies. Perhaps "For each entry until the end of the !OS_CHROMEOS block, and an entry.." https://codereview.chromium.org/2074643003/diff/290001/chrome/browser/ui/view... chrome/browser/ui/views/accelerator_table.cc:221: #else // !OS_MACOSX https://codereview.chromium.org/2074643003/diff/290001/chrome/browser/ui/view... chrome/browser/ui/views/accelerator_table.cc:222: // Control modifier is rarely used on Mac, so we allow it only in several (move this - see previous comment) https://codereview.chromium.org/2074643003/diff/290001/chrome/browser/ui/view... chrome/browser/ui/views/accelerator_table.cc:227: // bindings as well, so remove the mappings with Alt, but not those with -> "bindings as well. Mapping with just Alt appear here, and should have an alternative mapping in the block above." (it would be nice to check this in a unit test too, but of course we can't without exposing a full list of commands somehow :/)
LGTM - thanks
Turns out I need to fix at least two tests that relied on old global_keyboard_shortcuts_mac tables, so I'll do that and then try to commit. https://codereview.chromium.org/2074643003/diff/270001/chrome/browser/ui/view... File chrome/browser/ui/views/accelerator_table.cc (right): https://codereview.chromium.org/2074643003/diff/270001/chrome/browser/ui/view... chrome/browser/ui/views/accelerator_table.cc:37: // the Windows accelerators in ../../app/chrome_dll.rc. On 2016/11/10 17:56:38, Peter Kasting wrote: > On 2016/11/10 14:18:05, themblsha wrote: > > On 2016/11/09 22:29:52, tapted wrote: > > > Or we could drop the other {alphabetical, chrome_dll.rc} ordering > requirements > > > and just cluster things by platform. > > > > BTW is there any reason that chrome_dll.rc is not strictly alphabetical? I've > > tried re-sorting it and it results in this diff: > > https://gist.github.com/mblsha/514db7237dbbedc20d2ae9f21f6eee81 > > Not that I know of. > > I vote for "sort that list", and that takes care of some of the issues with "try > to keep this list in that list's order". Ok, I'll create a separate CL for that. https://codereview.chromium.org/2074643003/diff/270001/chrome/browser/ui/view... chrome/browser/ui/views/accelerator_table.cc:315: auto remove_accelerators = [](const AcceleratorMapping& m) { On 2016/11/10 17:56:38, Peter Kasting wrote: > On 2016/11/10 14:18:05, themblsha wrote: > > On 2016/11/09 18:30:32, sky wrote: > > > Isn't this unnecessary now? A NDEBUG block if you want to verify that is > fine. > > > > We still need to remove the Alt+VKEY and Alt+Shift+VKEY combinations. There's > no > > whitelist so they're completely pruned. > > If we still need this lambda regardless, then I'm skeptical of the tradeoff that > simplifies this lambda but adds more ifdefs above. Is there an updated version > of the diff just between those two approaches? No, lambda is not required anymore. Here's a complete diff: https://gist.github.com/mblsha/945b8039d0b2e9c60e9f41aa18c19945 https://codereview.chromium.org/2074643003/diff/290001/chrome/browser/ui/view... File chrome/browser/ui/views/accelerator_table.cc (right): https://codereview.chromium.org/2074643003/diff/290001/chrome/browser/ui/view... chrome/browser/ui/views/accelerator_table.cc:12: #include <tuple> On 2016/11/11 00:02:19, tapted wrote: > I think you can remove all of these now Done. https://codereview.chromium.org/2074643003/diff/290001/chrome/browser/ui/view... chrome/browser/ui/views/accelerator_table.cc:36: // NOTE: Keep this list in the same (mostly-alphabetical) order as On 2016/11/11 00:02:18, tapted wrote: > Perhaps update this comment. Something like, "Between each ifdef block, keep the > list in the same (mostly-alphabetical) order as the Windows accelerators in > ../../app/chrome_dll.rc." Thanks! https://codereview.chromium.org/2074643003/diff/290001/chrome/browser/ui/view... chrome/browser/ui/views/accelerator_table.cc:68: { ui::VKEY_TAB, ui::EF_CONTROL_DOWN, IDC_SELECT_NEXT_TAB }, On 2016/11/11 00:02:19, tapted wrote: > The "Control modifier is rarely used on Mac, so we allow it only in several > specific cases here." comment belongs up here Done. https://codereview.chromium.org/2074643003/diff/290001/chrome/browser/ui/view... chrome/browser/ui/views/accelerator_table.cc:107: #endif On 2016/11/11 00:02:18, tapted wrote: > // OS_LINIUX && !OS_CHROMEOS Done. https://codereview.chromium.org/2074643003/diff/290001/chrome/browser/ui/view... chrome/browser/ui/views/accelerator_table.cc:154: #else On 2016/11/11 00:02:18, tapted wrote: > // !OS_CHROMEOS here? Done. https://codereview.chromium.org/2074643003/diff/290001/chrome/browser/ui/view... chrome/browser/ui/views/accelerator_table.cc:163: // For each entry here add an entry into kChromeCmdId2AshActionId below On 2016/11/11 00:02:19, tapted wrote: > It's unclear where this comment applies. Perhaps > > "For each entry until the end of the !OS_CHROMEOS block, and an entry.." Done. https://codereview.chromium.org/2074643003/diff/290001/chrome/browser/ui/view... chrome/browser/ui/views/accelerator_table.cc:221: #else On 2016/11/11 00:02:18, tapted wrote: > // !OS_MACOSX Done. https://codereview.chromium.org/2074643003/diff/290001/chrome/browser/ui/view... chrome/browser/ui/views/accelerator_table.cc:222: // Control modifier is rarely used on Mac, so we allow it only in several On 2016/11/11 00:02:19, tapted wrote: > (move this - see previous comment) Done. https://codereview.chromium.org/2074643003/diff/290001/chrome/browser/ui/view... chrome/browser/ui/views/accelerator_table.cc:227: // bindings as well, so remove the mappings with Alt, but not those with On 2016/11/11 00:02:18, tapted wrote: > -> "bindings as well. Mapping with just Alt appear here, and should have an > alternative mapping in the block above." > > (it would be nice to check this in a unit test too, but of course we can't > without exposing a full list of commands somehow :/) Yeah. Another option would be to parse this file with a different set of defines, but this would be pretty hacky and there's no infrastructure for that.
On 2016/11/11 12:20:33, themblsha wrote:
> Turns out I need to fix at least two tests that relied on old
> global_keyboard_shortcuts_mac tables, so I'll do that and then try to commit.
Turns out this CL in its current form breaks activating shortcuts while Omnibox
has focus and the shortcut is not listed in the MainMenu.xib. Previously they
were handled using the GetWindowKeyboardShortcutTable /
GetDelayedWindowKeyboardShortcutTable tables, but now after pruning the old
shortcuts don't work anymore.
Tab contents get the keypress like this:
-[NSApplication(NSEvent) sendEvent:]
* -[NativeWidgetMacNSWindow sendEvent:]
* -[BaseView keyDown:]
* -[RenderWidgetHostViewCocoa keyEvent:]
When Omnibox is in focus the only relevant calltree is this one:
-[NativeWidgetMacNSWindow performKeyEquivalent:]
* -[CommandDispatcher performKeyEquivalent:]
* -[ChromeCommandDispatcherDelegate prePerformKeyEquivalent:]
CommandDispatcher is used by both Cocoa and MacViews in performKeyEquivalent:,
PreHandleKeyboardEvent and HandleKeyboardEvent.
I've tried hacking together this fix:
https://gist.github.com/mblsha/44bd5f0609c2f0b6c73077ccf310befc, but it breaks
at least Cmd+Left / Cmd+Right. It's used both to move the cursor to the start /
end of the line, and to move Back / Forward when navigating the webpages. With
this fix both happen at the same time.
Another approach would be to share GetWindowKeyboardShortcutTable() between Mac
and MacViews, it would work reliably at the cost of duplicate command processing
while the tab contents have focus.
Would the latter approach be acceptable or I should investigate other options?
I moved GetWindowKeyboardShortcutTable to a common place, updated a couple of comments and added a new test to verify that all* GetWindowKeyboardShortcutTable's accelerators are present in accelerator_table.cc. * -- with exception of a very small whitelist. If there won't be any objections for another 20 hours or so I'll try committing it like this.
https://codereview.chromium.org/2074643003/diff/330001/chrome/browser/global_... File chrome/browser/global_keyboard_shortcuts_mac.mm (right): https://codereview.chromium.org/2074643003/diff/330001/chrome/browser/global_... chrome/browser/global_keyboard_shortcuts_mac.mm:98: // Omnibox is in focus. This doesn't sound right. BrowserView::LoadAccelerators() should add everything from accelerator_table.cc into the views::FocusManager, which is associated with the entire Widget. So those accelerators should be consulted when the omnibox has focus. We shouldn't need to duplicate the mapping that hooks into the native NSEvent event dispatch. Accelerators are processed in Widget::OnKeyEvent, but only if the event wasn't marked as processed. Is the problem that Textfield::OnKeyPressed(..) is saying the event has been handled, when it should not?
https://codereview.chromium.org/2074643003/diff/330001/chrome/browser/global_... File chrome/browser/global_keyboard_shortcuts_mac.mm (right): https://codereview.chromium.org/2074643003/diff/330001/chrome/browser/global_... chrome/browser/global_keyboard_shortcuts_mac.mm:98: // Omnibox is in focus. On 2016/11/14 22:44:49, tapted wrote: > This doesn't sound right. > > Is the problem that Textfield::OnKeyPressed(..) is saying the event has been > handled, when it should not? No. The Views code doesn't see the event at all. This is what happens when you press Cmd+1 while Omnibox is in focus: 1. -[NativeWidgetMacNSWindow performKeyEquivalent:] This is where the Cocoa and current MacViews implementation handle commands using global_keyboard_shortcuts_mac.mm. 2. -[BridgedContentView keyDown:] If the -performKeyEquivalent: returned NO the -keyDown: is called and it attempts to translate the key event to the Cocoa Text System command using -interpretKeyEvents:. * -[NSView interpretKeyEvents:] * -[NSTextInputContext handleEvent:] * -[NSTextInputContext _handleEvent:options:allowingSyntheticEvent:completionHandler:] Internally consults NSKeyBindingManager and does some TSM stuff. Quite a lot of code there. -[BridgedContentView keyUp:] is not called in this case. You could test by yourself: make sure the GetWindowKeyboardShortcutTable, GetDelayedWindowKeyboardShortcutTable and GetBrowserKeyboardShortcutTable tables are all empty, focus the Omnibox and try pressing Cmd+1/2/3 and see that nothing happens. How about trying to commit this CL as is and try to improve accelerator handling separately?
On 2016/11/15 11:43:33, themblsha wrote: > https://codereview.chromium.org/2074643003/diff/330001/chrome/browser/global_... > File chrome/browser/global_keyboard_shortcuts_mac.mm (right): > > https://codereview.chromium.org/2074643003/diff/330001/chrome/browser/global_... > chrome/browser/global_keyboard_shortcuts_mac.mm:98: // Omnibox is in focus. > On 2016/11/14 22:44:49, tapted wrote: > > This doesn't sound right. > > > > Is the problem that Textfield::OnKeyPressed(..) is saying the event has been > > handled, when it should not? > > No. The Views code doesn't see the event at all. > > This is what happens when you press Cmd+1 while Omnibox is in focus: > > 1. -[NativeWidgetMacNSWindow performKeyEquivalent:] > This is where the Cocoa and current MacViews implementation handle commands > using global_keyboard_shortcuts_mac.mm. > > 2. -[BridgedContentView keyDown:] > If the -performKeyEquivalent: returned NO the -keyDown: is called and it > attempts to translate the key event to the Cocoa Text System command using > -interpretKeyEvents:. > > * -[NSView interpretKeyEvents:] > * -[NSTextInputContext handleEvent:] > * -[NSTextInputContext > _handleEvent:options:allowingSyntheticEvent:completionHandler:] > Internally consults NSKeyBindingManager and does some TSM stuff. Quite a > lot of code there. > > -[BridgedContentView keyUp:] is not called in this case. > > You could test by yourself: make sure the GetWindowKeyboardShortcutTable, > GetDelayedWindowKeyboardShortcutTable and GetBrowserKeyboardShortcutTable tables > are all empty, focus the Omnibox and try pressing Cmd+1/2/3 and see that nothing > happens. > > How about trying to commit this CL as is and try to improve accelerator handling > separately? Let's keep digging. We don't normally land code we think isn't right - it has a habit of staying around. If there's a bug in the accelerator handling, you're right it would be nice to fix it in a separate CL, but it should land before this, not after. The > * -[NSView interpretKeyEvents:] stuff is necessary because a user could remap Cmd+1 in ~/Library/KeyBindings/DefaultKeyBinding.dict to an editing command if they wanted to. But if it's not mapped to any selector it will end up at -[BridgedContentView doCommandBySelector:], and if ([self respondsToSelector:selector]) [self performSelector:selector withObject:nil]; else [[self nextResponder] doCommandBySelector:selector]; This fragment maybe needs to look something like if ([self respondsToSelector:selector]) { [self performSelector:selector withObject:nil]; return; } if (keyDownEvent_) { ui::KeyEvent event(keyDownEvent_); [self handleKeyEvent:&event]; if (event->handled) return; } [[self nextResponder] doCommandBySelector:selector]; If that's the bug, we should land a separate fix first and ensure there's proper test coverage.
On 2016/11/15 12:09:59, tapted wrote: > On 2016/11/15 11:43:33, themblsha wrote: > > > https://codereview.chromium.org/2074643003/diff/330001/chrome/browser/global_... > > File chrome/browser/global_keyboard_shortcuts_mac.mm (right): > > > > > https://codereview.chromium.org/2074643003/diff/330001/chrome/browser/global_... > > chrome/browser/global_keyboard_shortcuts_mac.mm:98: // Omnibox is in focus. > > On 2016/11/14 22:44:49, tapted wrote: > > > This doesn't sound right. > > > > > > Is the problem that Textfield::OnKeyPressed(..) is saying the event has been > > > handled, when it should not? > > > > No. The Views code doesn't see the event at all. > > > > This is what happens when you press Cmd+1 while Omnibox is in focus: > > > > 1. -[NativeWidgetMacNSWindow performKeyEquivalent:] > > This is where the Cocoa and current MacViews implementation handle commands > > using global_keyboard_shortcuts_mac.mm. > > > > 2. -[BridgedContentView keyDown:] > > If the -performKeyEquivalent: returned NO the -keyDown: is called and it > > attempts to translate the key event to the Cocoa Text System command using > > -interpretKeyEvents:. > > > > * -[NSView interpretKeyEvents:] > > * -[NSTextInputContext handleEvent:] > > * -[NSTextInputContext > > _handleEvent:options:allowingSyntheticEvent:completionHandler:] > > Internally consults NSKeyBindingManager and does some TSM stuff. Quite > a > > lot of code there. > > > > -[BridgedContentView keyUp:] is not called in this case. > > > > You could test by yourself: make sure the GetWindowKeyboardShortcutTable, > > GetDelayedWindowKeyboardShortcutTable and GetBrowserKeyboardShortcutTable > tables > > are all empty, focus the Omnibox and try pressing Cmd+1/2/3 and see that > nothing > > happens. > > > > How about trying to commit this CL as is and try to improve accelerator > handling > > separately? > > Let's keep digging. We don't normally land code we think isn't right - it has a > habit of staying around. If there's a bug in the accelerator handling, you're > right it would be nice to fix it in a separate CL, but it should land before > this, not after. > > The > > * -[NSView interpretKeyEvents:] > stuff is necessary because a user could remap Cmd+1 in > ~/Library/KeyBindings/DefaultKeyBinding.dict to an editing command if they > wanted to. > > But if it's not mapped to any selector it will end up at -[BridgedContentView > doCommandBySelector:], and Ah, actually it probably won't end up at doCommandBySelector: for this particular key combo :/. But that's one of the paths we need to consider. So, another option might be to set `keyDownEvent_ = nil` in doCommandBySelector:. Then just put the if (keyDownEvent_) { ui::KeyEvent event(keyDownEvent_); [self handleKeyEvent:&event]; keyDownEvent = nil; if (event->handled) return; } in -[BridgedContentView keyDown:], after the interpretKeyEvents call.
Turns out there's different behaviour for different accelerators:
1. keyDown: with an accelerator that could result in a command (noop: and
insertBacktab: with my bindings);
2. keyDown: that doesn't map to a command;
3. keyUp:. The accelerator processing doesn't work in this case I guess because
it only wants to deal with KeyDown events.
Here's the full list of all accelerators I've tested:
{Cmd+}, IDC_SELECT_NEXT_TAB}
{Cmd+{, IDC_SELECT_PREVIOUS_TAB}
performKeyEquivalent: (handled there by ChromeCommandDispatcherDelegate,
so no further processing)
{Ctrl+PageDown, IDC_SELECT_NEXT_TAB}
performKeyEquivalent:
keyUp:
{Ctrl+Tab, IDC_SELECT_NEXT_TAB}
performKeyEquivalent:
keyUp:
{Ctrl+PageUp, IDC_SELECT_PREVIOUS_TAB}
performKeyEquivalent:
doCommandBySelector: (insertBacktab:)
{Shift+Ctrl+Tab,IDC_SELECT_PREVIOUS_TAB}
performKeyEquivalent:
keyUp:
{Cmd+1, IDC_SELECT_TAB_0}
{Cmd+2, IDC_SELECT_TAB_1}
{Cmd+3, IDC_SELECT_TAB_2}
{Cmd+4, IDC_SELECT_TAB_3}
{Cmd+5, IDC_SELECT_TAB_4}
{Cmd+6, IDC_SELECT_TAB_5}
{Cmd+7, IDC_SELECT_TAB_6}
{Cmd+8, IDC_SELECT_TAB_7}
{Cmd+9, IDC_SELECT_LAST_TAB}
{Cmd+Shift+M, IDC_SHOW_AVATAR_MENU}
performKeyEquivalent:
keyDown:
doCommandBySelector: (noop:)
{Cmd+Opt+L, IDC_SHOW_DOWNLOADS}
performKeyEquivalent:
keyDown:
On 2016/11/15 15:03:39, themblsha wrote:
> Turns out there's different behaviour for different accelerators:
> 1. keyDown: with an accelerator that could result in a command (noop: and
> insertBacktab: with my bindings);
> 2. keyDown: that doesn't map to a command;
> 3. keyUp:. The accelerator processing doesn't work in this case I guess
because
> it only wants to deal with KeyDown events.
>
> Here's the full list of all accelerators I've tested:
>
> {Cmd+}, IDC_SELECT_NEXT_TAB}
> {Cmd+{, IDC_SELECT_PREVIOUS_TAB}
> performKeyEquivalent: (handled there by ChromeCommandDispatcherDelegate,
> so no further processing)
>
> {Ctrl+PageDown, IDC_SELECT_NEXT_TAB}
> performKeyEquivalent:
> keyUp:
>
> {Ctrl+Tab, IDC_SELECT_NEXT_TAB}
> performKeyEquivalent:
> keyUp:
>
> {Ctrl+PageUp, IDC_SELECT_PREVIOUS_TAB}
> performKeyEquivalent:
> doCommandBySelector: (insertBacktab:)
>
> {Shift+Ctrl+Tab,IDC_SELECT_PREVIOUS_TAB}
> performKeyEquivalent:
> keyUp:
>
> {Cmd+1, IDC_SELECT_TAB_0}
> {Cmd+2, IDC_SELECT_TAB_1}
> {Cmd+3, IDC_SELECT_TAB_2}
> {Cmd+4, IDC_SELECT_TAB_3}
> {Cmd+5, IDC_SELECT_TAB_4}
> {Cmd+6, IDC_SELECT_TAB_5}
> {Cmd+7, IDC_SELECT_TAB_6}
> {Cmd+8, IDC_SELECT_TAB_7}
> {Cmd+9, IDC_SELECT_LAST_TAB}
> {Cmd+Shift+M, IDC_SHOW_AVATAR_MENU}
> performKeyEquivalent:
> keyDown:
> doCommandBySelector: (noop:)
>
> {Cmd+Opt+L, IDC_SHOW_DOWNLOADS}
> performKeyEquivalent:
> keyDown:
It looks like we just need to add an override -[BridgedContentView
performKeyEquivalent:] then. One that simply returns `NO` might funnel these
events to keyDown instead. Otherwise, we could call a common method from both
keyDown and performKeyEquivalent. Combined with my earlier suggestion this
should be enough. Sorry if that's not clear - I probably won't have time to
start exploring this myself for another 24 hours. So if you get stuck, I might
be able to put something together to help then.
But let's continue the discussion on a new CL / bug. Patchset 17 looks good here
- I think we can land a fix first to make that work. Patchset 18 does not look
good.
Created a CL for proposed fix: https://codereview.chromium.org/2505943002, thanks for your suggestion!
The CQ bit was checked by mblsha@yandex-team.ru
The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org, pkasting@chromium.org, tapted@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/2074643003/#ps350001 (title: "Revert to Patch Set 17, rebase on top of master.")
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
Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by mblsha@yandex-team.ru
The patchset sent to the CQ was uploaded after l-g-t-m from tapted@chromium.org, thakis@chromium.org, pkasting@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/2074643003/#ps370001 (title: "Fix compilation.")
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": 370001, "attempt_start_ts": 1479745302908960,
"parent_rev": "1beb487eba67e68930472e57255f2017e2f35a28", "commit_rev":
"323555a8fd4af9edb7e706087f7499095fe2a65f"}
Message was sent while issue was closed.
Description was changed from
==========
MacViews: Views accelerators table should match the Cocoa one.
Updated views/accelerator_table.cc to match accelerators_cocoa.mm. Added
eligible shortcuts from global_keyboard_shortcuts_mac.mm as well.
Also handle the case when the user configured cmd-left to mean "previous tab",
as it should take precedence over the built-in binding: we're consulting the
[NSApp mainMenu] using CommandForKeyEvent().
Lastly, the Cmd+{/} shortcuts are special as they would be impossible to
perform on German and French keyboard layouts if we would simply check for
keycodes as we do in accelerator_table.cc (crbug.com/25946).
In order to tackle last two problems I've added PreHandleKeyboardEvent and
HandleKeyboardEvent to the NativeBrowserFrame interface.
BUG=620315
==========
to
==========
MacViews: Views accelerators table should match the Cocoa one.
Updated views/accelerator_table.cc to match accelerators_cocoa.mm. Added
eligible shortcuts from global_keyboard_shortcuts_mac.mm as well.
Also handle the case when the user configured cmd-left to mean "previous tab",
as it should take precedence over the built-in binding: we're consulting the
[NSApp mainMenu] using CommandForKeyEvent().
Lastly, the Cmd+{/} shortcuts are special as they would be impossible to
perform on German and French keyboard layouts if we would simply check for
keycodes as we do in accelerator_table.cc (crbug.com/25946).
In order to tackle last two problems I've added PreHandleKeyboardEvent and
HandleKeyboardEvent to the NativeBrowserFrame interface.
BUG=620315
==========
Message was sent while issue was closed.
Committed patchset #20 (id:370001)
Message was sent while issue was closed.
Description was changed from
==========
MacViews: Views accelerators table should match the Cocoa one.
Updated views/accelerator_table.cc to match accelerators_cocoa.mm. Added
eligible shortcuts from global_keyboard_shortcuts_mac.mm as well.
Also handle the case when the user configured cmd-left to mean "previous tab",
as it should take precedence over the built-in binding: we're consulting the
[NSApp mainMenu] using CommandForKeyEvent().
Lastly, the Cmd+{/} shortcuts are special as they would be impossible to
perform on German and French keyboard layouts if we would simply check for
keycodes as we do in accelerator_table.cc (crbug.com/25946).
In order to tackle last two problems I've added PreHandleKeyboardEvent and
HandleKeyboardEvent to the NativeBrowserFrame interface.
BUG=620315
==========
to
==========
MacViews: Views accelerators table should match the Cocoa one.
Updated views/accelerator_table.cc to match accelerators_cocoa.mm. Added
eligible shortcuts from global_keyboard_shortcuts_mac.mm as well.
Also handle the case when the user configured cmd-left to mean "previous tab",
as it should take precedence over the built-in binding: we're consulting the
[NSApp mainMenu] using CommandForKeyEvent().
Lastly, the Cmd+{/} shortcuts are special as they would be impossible to
perform on German and French keyboard layouts if we would simply check for
keycodes as we do in accelerator_table.cc (crbug.com/25946).
In order to tackle last two problems I've added PreHandleKeyboardEvent and
HandleKeyboardEvent to the NativeBrowserFrame interface.
BUG=620315
Committed: https://crrev.com/cb9c6b9d9a764e89cb0703e48ddcfea3129e2ad3
Cr-Commit-Position: refs/heads/master@{#433574}
==========
Message was sent while issue was closed.
Patchset 20 (id:??) landed as https://crrev.com/cb9c6b9d9a764e89cb0703e48ddcfea3129e2ad3 Cr-Commit-Position: refs/heads/master@{#433574} |
