Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(20)

Issue 503080: Make cmd-{/} shortcut keys work on any keyboard layouts on Mac (Closed)

Created:
11 years ago by kinuko
Modified:
9 years, 7 months ago
CC:
chromium-reviews_googlegroups.com
Visibility:
Public.

Description

Make cmd-{/} shortcut keys work on any keyboard layouts (including non-US ones) on Mac. BUG=25946 TEST=GlobalKeyboardShortcuts.ShortcutsToWindowCommand TEST=Go to a Web page, open several tabs and press Cmd-{/}. Make sure the browser tab switches to next or prev. This should work on any keyboards (including ones with non-US layout). Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=35525

Patch Set 1 #

Patch Set 2 : add Cmd-{ Cmd-} fixes #

Total comments: 16

Patch Set 3 : reflect comments #

Patch Set 4 : fix unittest and line-wrapping #

Total comments: 11

Patch Set 5 : reflect comments #

Total comments: 12

Patch Set 6 : add unittest and more fixes #

Total comments: 2

Patch Set 7 : rename fix #

Unified diffs Side-by-side diffs Delta from patch set Stats (+191 lines, -50 lines) Patch
M chrome/browser/cocoa/browser_window_cocoa.mm View 1 2 3 4 2 chunks +4 lines, -2 lines 0 comments Download
M chrome/browser/cocoa/chrome_event_processing_window.mm View 1 2 2 chunks +4 lines, -3 lines 0 comments Download
M chrome/browser/global_keyboard_shortcuts_mac.h View 1 2 3 4 2 chunks +20 lines, -4 lines 0 comments Download
M chrome/browser/global_keyboard_shortcuts_mac.mm View 1 2 3 4 5 6 chunks +91 lines, -33 lines 0 comments Download
M chrome/browser/global_keyboard_shortcuts_mac_unittest.mm View 3 4 5 6 6 chunks +72 lines, -8 lines 0 comments Download

Messages

Total messages: 12 (0 generated)
kinuko
Hi, this patch tries to add Cmd-+ shortcut key for zooming in and also to ...
11 years ago (2009-12-22 14:41:01 UTC) #1
jeremy
+avi Could you please check that this works properly with the Hebrew keyboard layout? http://codereview.chromium.org/503080/diff/2001/2002 ...
11 years ago (2009-12-22 15:34:04 UTC) #2
Avi (use Gerrit)
Looks reasonable; I agree with what Jeremy says. I wasn't the one who worked on ...
11 years ago (2009-12-22 16:00:26 UTC) #3
Nico
What Jeremy said (except for the "use if here" statement – I like the ternary ...
11 years ago (2009-12-22 17:56:04 UTC) #4
kinuko
Thanks for reviewing. I updated the code to reflect the comments. I've tested the new ...
10 years, 12 months ago (2009-12-24 14:59:14 UTC) #5
jeremy
http://codereview.chromium.org/503080/diff/9002/10009 File chrome/browser/cocoa/browser_window_cocoa.mm (right): http://codereview.chromium.org/503080/diff/9002/10009#newcode485 chrome/browser/cocoa/browser_window_cocoa.mm:485: // own customizations. Could you elaborate on the customizations ...
10 years, 12 months ago (2009-12-24 15:45:31 UTC) #6
Nico
The Right Fix for cmd-+ I believe is to change the shortcut of "zoom" from ...
10 years, 12 months ago (2009-12-27 21:24:02 UTC) #7
kinuko
Thanks for reviewing, I've updated the patch. Also I changed the subject and description of ...
10 years, 12 months ago (2009-12-28 12:38:07 UTC) #8
Nico
Nearly there…one test to add and a few more questions. http://codereview.chromium.org/503080/diff/7011/7015 File chrome/browser/global_keyboard_shortcuts_mac.mm (right): http://codereview.chromium.org/503080/diff/7011/7015#newcode78 ...
10 years, 11 months ago (2009-12-28 16:31:13 UTC) #9
kinuko
http://codereview.chromium.org/503080/diff/7011/7015 File chrome/browser/global_keyboard_shortcuts_mac.mm (right): http://codereview.chromium.org/503080/diff/7011/7015#newcode78 chrome/browser/global_keyboard_shortcuts_mac.mm:78: DCHECK((shortcut.key_char == 0) || (shortcut.vkey_code == 0)); On 2009/12/28 ...
10 years, 11 months ago (2009-12-30 07:20:56 UTC) #10
Nico
LGTM given you address the first (and only) comment below. Thanks for the patch! http://codereview.chromium.org/503080/diff/11009/11015 ...
10 years, 11 months ago (2009-12-30 21:04:32 UTC) #11
kinuko
10 years, 11 months ago (2010-01-05 09:40:15 UTC) #12
Addressed the rename issue (rebased with r35519).  I'm submitting this one -
please let me know if you see any problems.  Thanks!

On 2009/12/30 21:04:32, Nico wrote:
> LGTM given you address the first (and only) comment below.
> 
> Thanks for the patch!
> 
> http://codereview.chromium.org/503080/diff/11009/11015
> File chrome/browser/global_keyboard_shortcuts_mac_unittest.mm (right):
> 
> http://codereview.chromium.org/503080/diff/11009/11015#newcode1
> chrome/browser/global_keyboard_shortcuts_mac_unittest.mm:1: // Copyright (c)
> 2009 The Chromium Authors. All rights reserved.
> Somehow svn now missed that this is a rename and treats it as delete/add
> instead. Can you turn it back into a rename, so that history stays intact?
> 
> http://codereview.chromium.org/503080/diff/11009/11015#newcode125
> chrome/browser/global_keyboard_shortcuts_mac_unittest.mm:125: // cmd-'{' /
> cmd-shift-'[' on ansi
> Wow, it's terrible that cocoa sends '[' as transformed and '{' as
untransformed
> text here. that seems all backwards.
> 
> (just complaining about cocoa, your code is correct)

Powered by Google App Engine
This is Rietveld 408576698