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

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 13044014: Make sure manifest specified shortcut for Extension Command can not override the built-in shortcuts. (Closed)

Created:
7 years, 9 months ago by Finnur
Modified:
7 years, 8 months ago
CC:
chromium-reviews, Aaron Boodman, tfarina, chromium-apps-reviews_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Visibility:
Public.

Description

Make sure manifest specified shortcut for Extension Command can not override the built-in shortcuts. User is still free to override manually. BUG=226994 TEST=Automated test included. Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=194594

Patch Set 1 #

Patch Set 2 : GTK and a test #

Patch Set 3 : . #

Patch Set 4 : Ready for try server #

Patch Set 5 : Adding missing Cocoa file #

Patch Set 6 : Disabling test for Mac (not implemented yet) #

Total comments: 2

Patch Set 7 : Adding Mac support #

Patch Set 8 : Add one missing Mac file change #

Patch Set 9 : Missing includes #

Patch Set 10 : One more missing include #

Total comments: 2

Patch Set 11 : Ash/Aura/Views split #

Patch Set 12 : Cocoa CL #

Total comments: 1

Patch Set 13 : Views and Aura #

Patch Set 14 : Adding includes and avoiding name conflicts #

Patch Set 15 : Fixing Mac compile error #

Patch Set 16 : Adding traces for Chromeos #

Patch Set 17 : Fix for ChromeOS (ash) #

Total comments: 4

Patch Set 18 : Mac's secondary shortcuts can be overwritten #

Unified diffs Side-by-side diffs Delta from patch set Stats (+277 lines, -18 lines) Patch
M chrome/browser/extensions/api/commands/command_service.cc View 1 2 3 4 5 6 7 8 9 10 11 12 2 chunks +29 lines, -16 lines 0 comments Download
M chrome/browser/extensions/extension_keybinding_apitest.cc View 1 2 3 4 5 6 1 chunk +40 lines, -0 lines 0 comments Download
A chrome/browser/ui/accelerator_utils.h View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +23 lines, -0 lines 0 comments Download
A chrome/browser/ui/cocoa/accelerator_utils_cocoa.mm View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 1 chunk +53 lines, -0 lines 0 comments Download
M chrome/browser/ui/cocoa/accelerators_cocoa.h View 1 2 3 4 5 6 7 1 chunk +4 lines, -0 lines 0 comments Download
A chrome/browser/ui/gtk/accelerator_utils_gtk.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +23 lines, -0 lines 0 comments Download
A chrome/browser/ui/views/accelerator_utils_aura.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 1 chunk +38 lines, -0 lines 0 comments Download
A chrome/browser/ui/views/accelerator_utils_views.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 16 1 chunk +23 lines, -0 lines 0 comments Download
M chrome/browser/ui/views/frame/browser_view.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 1 chunk +3 lines, -0 lines 0 comments Download
M chrome/browser/ui/views/frame/browser_view.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 1 chunk +4 lines, -0 lines 0 comments Download
M chrome/chrome_browser_ui.gypi View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 5 chunks +7 lines, -2 lines 0 comments Download
A chrome/test/data/extensions/api_test/keybinding/dont_overwrite_system/background.js View 1 2 1 chunk +11 lines, -0 lines 0 comments Download
A chrome/test/data/extensions/api_test/keybinding/dont_overwrite_system/manifest.json View 1 2 3 4 5 6 1 chunk +19 lines, -0 lines 0 comments Download

Messages

Total messages: 16 (0 generated)
Finnur
7 years, 8 months ago (2013-04-05 14:43:56 UTC) #1
Yoyo Zhou
LGTM https://codereview.chromium.org/13044014/diff/18001/chrome/browser/ui/cocoa/extensions/extension_keybinding_registry_cocoa.mm File chrome/browser/ui/cocoa/extensions/extension_keybinding_registry_cocoa.mm (right): https://codereview.chromium.org/13044014/diff/18001/chrome/browser/ui/cocoa/extensions/extension_keybinding_registry_cocoa.mm#newcode27 chrome/browser/ui/cocoa/extensions/extension_keybinding_registry_cocoa.mm:27: // TODO(finnur): Implement this. Is the issue that ...
7 years, 8 months ago (2013-04-05 18:09:50 UTC) #2
Finnur
https://codereview.chromium.org/13044014/diff/18001/chrome/browser/ui/cocoa/extensions/extension_keybinding_registry_cocoa.mm File chrome/browser/ui/cocoa/extensions/extension_keybinding_registry_cocoa.mm (right): https://codereview.chromium.org/13044014/diff/18001/chrome/browser/ui/cocoa/extensions/extension_keybinding_registry_cocoa.mm#newcode27 chrome/browser/ui/cocoa/extensions/extension_keybinding_registry_cocoa.mm:27: // TODO(finnur): Implement this. The issue is that the ...
7 years, 8 months ago (2013-04-05 19:50:03 UTC) #3
Finnur
Updated to add Mac. Scott, Elliot and Nico, can you do an OWNERS check on: ...
7 years, 8 months ago (2013-04-09 16:00:24 UTC) #4
Nico
https://codereview.chromium.org/13044014/diff/27002/chrome/browser/ui/cocoa/extensions/extension_keybinding_registry_cocoa.mm File chrome/browser/ui/cocoa/extensions/extension_keybinding_registry_cocoa.mm (right): https://codereview.chromium.org/13044014/diff/27002/chrome/browser/ui/cocoa/extensions/extension_keybinding_registry_cocoa.mm#newcode28 chrome/browser/ui/cocoa/extensions/extension_keybinding_registry_cocoa.mm:28: bool extensions::ExtensionKeybindingRegistry::IsChromeAccelerator( Is this supposed to return true if ...
7 years, 8 months ago (2013-04-09 16:10:18 UTC) #5
sky
https://codereview.chromium.org/13044014/diff/27002/chrome/browser/ui/views/extensions/extension_keybinding_registry_views.cc File chrome/browser/ui/views/extensions/extension_keybinding_registry_views.cc (right): https://codereview.chromium.org/13044014/diff/27002/chrome/browser/ui/views/extensions/extension_keybinding_registry_views.cc#newcode25 chrome/browser/ui/views/extensions/extension_keybinding_registry_views.cc:25: chrome::GetAcceleratorList(); This is only applicable to aura and I ...
7 years, 8 months ago (2013-04-09 16:38:01 UTC) #6
Elliot Glaysher
gtk lgtm
7 years, 8 months ago (2013-04-09 17:01:48 UTC) #7
Finnur
Nico & Scott: What do you think of the latest changes?
7 years, 8 months ago (2013-04-10 14:57:56 UTC) #8
sky
I like this better, but I suggest promoting the function IsAcceleratedRegistered to the chrome namespace ...
7 years, 8 months ago (2013-04-10 15:34:31 UTC) #9
Finnur
Nico, Scott: Please take another look. Scott: I started off with a separate file for ...
7 years, 8 months ago (2013-04-15 12:48:57 UTC) #10
sky
LGTM
7 years, 8 months ago (2013-04-15 14:21:31 UTC) #11
Finnur
Forgot to add that I had already removed a bit of cruft locally, but did ...
7 years, 8 months ago (2013-04-15 14:30:31 UTC) #12
Finnur
Nico, can you take a look at accelerator_utils_cocoa.mm again? Thanks!
7 years, 8 months ago (2013-04-16 10:47:08 UTC) #13
Nico
lgtm, looks great. https://codereview.chromium.org/13044014/diff/77001/chrome/browser/ui/cocoa/accelerator_utils_cocoa.mm File chrome/browser/ui/cocoa/accelerator_utils_cocoa.mm (right): https://codereview.chromium.org/13044014/diff/77001/chrome/browser/ui/cocoa/accelerator_utils_cocoa.mm#newcode55 chrome/browser/ui/cocoa/accelerator_utils_cocoa.mm:55: iter != accelerators->end(); ++iter) { I ...
7 years, 8 months ago (2013-04-16 21:44:54 UTC) #14
Finnur
https://codereview.chromium.org/13044014/diff/77001/chrome/browser/ui/cocoa/accelerator_utils_cocoa.mm File chrome/browser/ui/cocoa/accelerator_utils_cocoa.mm (right): https://codereview.chromium.org/13044014/diff/77001/chrome/browser/ui/cocoa/accelerator_utils_cocoa.mm#newcode55 chrome/browser/ui/cocoa/accelerator_utils_cocoa.mm:55: iter != accelerators->end(); ++iter) { Good point. We should ...
7 years, 8 months ago (2013-04-17 14:28:06 UTC) #15
Finnur
7 years, 8 months ago (2013-04-17 15:07:51 UTC) #16
Message was sent while issue was closed. 
Committed patchset #18 manually as r194594 (presubmit successful).

Powered by Google App Engine
This is Rietveld 408576698