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

Issue 149325: Add facitility for Global Keyboard shortcuts. (Closed)

Created:
11 years, 5 months ago by jeremy
Modified:
9 years, 6 months ago
CC:
chromium-reviews_googlegroups.com, John Grabowski
Visibility:
Public.

Description

Add facitility for Global Keyboard shortcuts. Also, do some housekeeping for our current shortcuts. * Command-Option-l - Downloads [same shortcut as Safari]. * Command-Shift-[/] - Back/forward tab. * cntrl-pageup/pagedn - Back/forward tab. Global Keyboard events are intercepted by a new BrowserWindow custom class. BUG=12537, 15486 TEST=Check that above keyboard shortcuts work as advertised.

Patch Set 1 #

Total comments: 7
Unified diffs Side-by-side diffs Delta from patch set Stats (+226 lines, -12 lines) Patch
M chrome/app/nibs/en.lproj/BrowserWindow.xib View 3 chunks +10 lines, -2 lines 0 comments Download
M chrome/app/nibs/en.lproj/MainMenu.xib View 13 chunks +54 lines, -10 lines 0 comments Download
M chrome/browser/cocoa/browser_window_controller.h View 1 chunk +5 lines, -0 lines 0 comments Download
M chrome/browser/cocoa/browser_window_controller.mm View 1 chunk +5 lines, -0 lines 1 comment Download
A chrome/browser/cocoa/browser_window_impl.h View 1 chunk +17 lines, -0 lines 2 comments Download
A chrome/browser/cocoa/browser_window_impl.mm View 1 chunk +38 lines, -0 lines 4 comments Download
A chrome/browser/global_keyboard_shortcuts_mac.h View 1 chunk +27 lines, -0 lines 0 comments Download
A chrome/browser/global_keyboard_shortcuts_mac.mm View 1 chunk +43 lines, -0 lines 0 comments Download
A chrome/browser/global_keyboard_shortcuts_mac_unittest.cc View 1 chunk +22 lines, -0 lines 0 comments Download
M chrome/chrome.gyp View 3 chunks +5 lines, -0 lines 0 comments Download

Messages

Total messages: 2 (0 generated)
jeremy
11 years, 5 months ago (2009-07-08 13:32:51 UTC) #1
pink (ping after 24hrs)
11 years, 5 months ago (2009-07-08 13:48:17 UTC) #2
LGTM with these fixed.

http://codereview.chromium.org/149325/diff/1/5
File chrome/browser/cocoa/browser_window_controller.mm (right):

http://codereview.chromium.org/149325/diff/1/5#newcode674
Line 674: - (void)executeCommand:(int)command {
do you want to re-tool commandDispatch: to go through this method? It bugs me a
little that we now have two methods that basically do the same thing, though I
understand why. At least maybe put them next to each other in the code?

http://codereview.chromium.org/149325/diff/1/6
File chrome/browser/cocoa/browser_window_impl.h (right):

http://codereview.chromium.org/149325/diff/1/6#newcode10
Line 10: // Cocoa class representing a Chrome browser window.
state why we are overriding NSWindow. For what purpose? Why does this class need
to exist?

http://codereview.chromium.org/149325/diff/1/6#newcode11
Line 11: @interface ChromeBrowserWindowImpl : NSWindow
isn't calling it an "impl" redundant? In fact, this is an interface, not an
implementation, but of course it has an implementation. You can't have one w/out
the other. I'd say drop the "impl" part.

http://codereview.chromium.org/149325/diff/1/7
File chrome/browser/cocoa/browser_window_impl.mm (right):

http://codereview.chromium.org/149325/diff/1/7#newcode12
Line 12: - (BOOL)performKeyEquivalent:(NSEvent *)event {
* goes with the type, no space.

http://codereview.chromium.org/149325/diff/1/7#newcode13
Line 13: 
omit blank line

http://codereview.chromium.org/149325/diff/1/7#newcode16
Line 16: bool cmd_key = modifers & NSCommandKeyMask;
use obj-c naming in obj-c code, eg |cmdKey|

http://codereview.chromium.org/149325/diff/1/7#newcode16
Line 16: bool cmd_key = modifers & NSCommandKeyMask;
make these const? *shrug* just a thought.

Powered by Google App Engine
This is Rietveld 408576698