Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
23445013:
Parse media keys for named command in the manifest. (Closed)
|
Created:
7 years, 3 months ago by zhchbin Modified:
7 years, 3 months ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, jshin+watch_chromium.org, extensions-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionParse media keys for named command in the manifest.
BUG=131612
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=221143
Patch Set 1 #
Total comments: 17
Patch Set 2 : Remove useless codes and add a test case for commands like "Ctrl+MediaNextTrack." #
Total comments: 2
Patch Set 3 : Revise. #
Total comments: 3
Patch Set 4 : Doc. #
Total comments: 2
Patch Set 5 : revise doc. #
Total comments: 2
Patch Set 6 : nit fix. #
Total comments: 3
Messages
Total messages: 27 (0 generated)
@finnur, PTAL. This is the first patch to support global hotkey & media keys. I test it on Windows. Please help me test whether it works on Chrome OS & OSX. Note: on linux platform, it wouldn't work if gnome-settings-daemon has grabbed these keys. Because the chrome didn't receive the media keys pressed event even though it's focused.
> Note: on linux platform, it wouldn't work if gnome-settings-daemon has grabbed > these keys. Because the chrome didn't receive the media keys pressed event even > though it's focused. Just to be clear, I read this as: "If gnome-settings-daemon is set to listen for these keys, then Chrome will never receive those keystrokes". Correct? Now, I'm not familiar with the gnome-settings-daemon, so I've CC-ed Elliot to help answer this question: Wouldn't we be registering keyboard shortcuts with this daemon on Linux and therefore be guaranteed to get them? If not, this might just end up being a limitation we'll have to live with. > Please help me test whether it works on Chrome OS & OSX. I can do OSX but I don't have a dev Chrome OS machine handy. I'm not sure how you are testing this, but just to be sure -- a proper test is something along these lines: 1) Create an extension with a named command foo which binds itself to a MediaKey. 2) Create a .crx with this extension. 3) Drag and drop the .crx onto the chrome://extensions page 4) Note that the install bubble for the extension states that a Media key can be used. Hmm... or was that only for browser actions and page actions? I don't remember. 5) Open 'Keyboard Shortcuts' on chrome://extensions page and note that the Media key is listed. 6) Change the binding to Ctrl+Shift+A (using the Keyboard Shortcuts overly) 7) Try to use Ctrl+Media key and see it being rejected like any other invalid key. 8) Change it back to the Media Key. 9) Watch event foo get triggered when you press the Media Key. https://codereview.chromium.org/23445013/diff/1/chrome/common/extensions/comm... File chrome/common/extensions/command.cc (right): https://codereview.chromium.org/23445013/diff/1/chrome/common/extensions/comm... chrome/common/extensions/command.cc:63: return ui::Accelerator(ui::VKEY_MEDIA_STOP, ui::EF_NONE); This seems a bit ad-hoc. Also, I think we need to make sure modifiers don't work (such as Ctrl+Shift+MediaKeys). I would delete this code block and implement it below. https://codereview.chromium.org/23445013/diff/1/chrome/common/extensions/comm... chrome/common/extensions/command.cc:152: key = ui::VKEY_TAB; Add values::kKeyMediaNextTrack (and the other media keys) after the vKeyTab clause and assign ui::VKEY_MEDIA_NEXT_TRACK (etc) to |key|. Remember to include should_parse_media_keys in the if clause for *each* key. https://codereview.chromium.org/23445013/diff/1/chrome/common/extensions/comm... chrome/common/extensions/command.cc:191: } Add an if clause: if ((key == ui::VKEY_MEDIA_NEXT_TRACK || key == ui::VKEY_MEDIA_PREV_TRACK || key == ui::VKEY_MEDIA_PLAY_PAUSE || key == ui::VKEY_MEDIA_STOP) && (shift || ctrl || alt || command)) { // Set *error to a message saying Media Keys cannot have a modifier. return ui::Accelerator(); } https://codereview.chromium.org/23445013/diff/1/chrome/common/extensions/comm... File chrome/common/extensions/command_unittest.cc (right): https://codereview.chromium.org/23445013/diff/1/chrome/common/extensions/comm... chrome/common/extensions/command_unittest.cc:117: { false, none, "_execute_page_action", "MediaPrevTrack", ""}, Add a failure test for Ctrl+Shift+MediaKey (one permutation should be enough). https://codereview.chromium.org/23445013/diff/1/chrome/common/extensions/docs... File chrome/common/extensions/docs/examples/api/mediaKeysCommand/manifest.json (right): https://codereview.chromium.org/23445013/diff/1/chrome/common/extensions/docs... chrome/common/extensions/docs/examples/api/mediaKeysCommand/manifest.json:39: } I don't think this sample is needed, but if so, there are a few issues with it: 1) There's no mediaKeys API (as the path for this doc indicates). Media Key support will be tucked into the Commands API and we already have an example for the Commands API, so this doesn't add much. 2) There's a browser action in this sample that is not needed. However, I was expecting a corresponding change to the Commands API doc that explains that you can now listen for some of the Media keys. See: chrome\common\extensions\docs\templates\intros\commands.html ... unless we mean to not support Media keys with non-global shortcuts (then we keep the docs as-is). I don't know which makes sense, but I'll ask the PM.
OSX suffers from the same problem: The Media key (I tried NextTrack) is not seen by Chrome. But I guess that's expected.
https://chromiumcodereview.appspot.com/23445013/diff/1/chrome/browser/extensi... File chrome/browser/extensions/api/commands/command_service_new.cc (left): https://chromiumcodereview.appspot.com/23445013/diff/1/chrome/browser/extensi... chrome/browser/extensions/api/commands/command_service_new.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. This whole file should be deleted. I think it is dead code and it is not mentioned in the .gypi build files. It is a fork off of command_service.cc and has since become out of date. No idea why it was checked in -- I think it was by mistake.
On 2013/08/28 14:15:16, Finnur wrote: > Wouldn't we be registering keyboard shortcuts with this daemon on > Linux and therefore be guaranteed to get them? If not, this might > just end up being a limitation we'll have to live with. I'm not really sure how that would work. IIUC, in the X11 architecture, the session and window managers get first crack at the keyboard events; We're the last to receive an event if it was unhandled by either of them. Nor do I know how to register with GSD (in the specific case) or everything that could be in the slot that GSD takes up (for the general case). I think this is a limitation you'll have to live with.
Thanks very much for the review!! > Just to be clear, I read this as: "If gnome-settings-daemon is set to listen > for these keys, then Chrome will never receive those keystrokes". Correct? > Now, I'm not familiar with the gnome-settings-daemon, so I've CC-ed Elliot to > help answer this question: Wouldn't we be registering keyboard shortcuts with > this daemon on Linux and therefore be guaranteed to get them? If not, this > might just end up being a limitation we'll have to live with. Yes, that's exactly what I mean. GSD provide MediaPlayerKeyPressed signal (DBUS) to the application which want to know these event, via this way I think we can implement it to support global media keys. After that, it's easy to support non-global media keys. Thanks @Elliot. > 4) Note that the install bubble for the extension states that a Media key can > be used. Hmm... or was that only for browser actions and page actions? I don't > remember. I make sure that it was only for browser actions and page actions. > 5) Open 'Keyboard Shortcuts' on chrome://extensions page and note that the > Media key is listed. Tested. > 6) Change the binding to Ctrl+Shift+A (using the Keyboard Shortcuts overly) > 7) Try to use Ctrl+Media key and see it being rejected like any other invalid > key. > 8) Change it back to the Media Key. > 9) Watch event foo get triggered when you press the Media Key. I haven't add code to support specifid media keys via chrome://extensions. I know it should be supported and would like do it step by step. WDYT? https://codereview.chromium.org/23445013/diff/1/chrome/browser/extensions/api... File chrome/browser/extensions/api/commands/command_service_new.cc (left): https://codereview.chromium.org/23445013/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/commands/command_service_new.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2013/08/28 15:46:34, Finnur wrote: > This whole file should be deleted. I think it is dead code and it is not > mentioned in the .gypi build files. It is a fork off of command_service.cc and > has since become out of date. No idea why it was checked in -- I think it was by > mistake. Done. https://codereview.chromium.org/23445013/diff/1/chrome/common/extensions/comm... File chrome/common/extensions/command.cc (right): https://codereview.chromium.org/23445013/diff/1/chrome/common/extensions/comm... chrome/common/extensions/command.cc:68: if (tokens.size() < 2 || tokens.size() > 3) { On 2013/08/28 14:15:16, Finnur wrote: > This seems a bit ad-hoc. Also, I think we need to make sure modifiers don't work > (such as Ctrl+Shift+MediaKeys). I would delete this code block and implement it > below. Note: if the size of tokens is 1, it will fail and return. Our case: "MediaNextTrack", there is no '+' in the accelerator and the size of tokens is 1. https://codereview.chromium.org/23445013/diff/1/chrome/common/extensions/docs... File chrome/common/extensions/docs/examples/api/mediaKeysCommand/manifest.json (right): https://codereview.chromium.org/23445013/diff/1/chrome/common/extensions/docs... chrome/common/extensions/docs/examples/api/mediaKeysCommand/manifest.json:39: } On 2013/08/28 14:15:16, Finnur wrote: > I don't think this sample is needed, but if so, there are a few issues with it: > 1) There's no mediaKeys API (as the path for this doc indicates). Media Key > support will be tucked into the Commands API and we already have an example for > the Commands API, so this doesn't add much. > 2) There's a browser action in this sample that is not needed. > > However, I was expecting a corresponding change to the Commands API doc that > explains that you can now listen for some of the Media keys. > See: > chrome\common\extensions\docs\templates\intros\commands.html > > ... unless we mean to not support Media keys with non-global shortcuts (then we > keep the docs as-is). I don't know which makes sense, but I'll ask the PM. Removal of example: Done. Doc commands API doc: I would like to add it after this cl? Because currently I can only make sure it works fine on Windows platform.
https://codereview.chromium.org/23445013/diff/1/chrome/browser/extensions/api... File chrome/browser/extensions/api/commands/command_service_new.cc (right): https://codereview.chromium.org/23445013/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/commands/command_service_new.cc:365: } // namespace extensions Wait, does the file still exist but is now empty? I ask because I was expecting to see a D in front of the file in the file list for this CL, to signify that it has been deleted. Maybe I'm mis-remembering how file deletions are represented... https://codereview.chromium.org/23445013/diff/1/chrome/common/extensions/comm... File chrome/common/extensions/command.cc (right): https://codereview.chromium.org/23445013/diff/1/chrome/common/extensions/comm... chrome/common/extensions/command.cc:68: if (tokens.size() < 2 || tokens.size() > 3) { Yes, change this if statement to become: if (tokens.size() == 0 || (tokens.size() == 1 && DoesRequireModifier(accelerator)) || tokens.size() > 3) { ... do what it does now (show error) ... } Then implement DoesRequireModifier to return true for all accelerators but the Media Keys. https://codereview.chromium.org/23445013/diff/1/chrome/common/extensions/docs... File chrome/common/extensions/docs/examples/api/mediaKeysCommand/manifest.json (right): https://codereview.chromium.org/23445013/diff/1/chrome/common/extensions/docs... chrome/common/extensions/docs/examples/api/mediaKeysCommand/manifest.json:39: } Fine. https://codereview.chromium.org/23445013/diff/11001/chrome/common/extensions/... File chrome/common/extensions/command.cc (right): https://codereview.chromium.org/23445013/diff/11001/chrome/common/extensions/... chrome/common/extensions/command.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. > I haven't add code to support specifid media keys via chrome://extensions. I > know it should be supported and would like do it step by step. WDYT? Ideally the .js changes should be part of this changelist, but you can do that as your next followup CL if you want. You'll need to modify extension_command_list.js in chrome\browser\resources\extensions to add the new Media key codes and implement a similar function as the one I mentioned on the C++ side (RequiresModifiers), because the code in the .js file also expects (and requires) modifiers to be present.
https://codereview.chromium.org/23445013/diff/1/chrome/browser/extensions/api... File chrome/browser/extensions/api/commands/command_service_new.cc (right): https://codereview.chromium.org/23445013/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/commands/command_service_new.cc:365: } // namespace extensions On 2013/08/29 11:20:52, Finnur wrote: > Wait, does the file still exist but is now empty? I ask because I was expecting > to see a D in front of the file in the file list for this CL, to signify that it > has been deleted. Maybe I'm mis-remembering how file deletions are > represented... Not, I have deleted the file...Maybe just a wrong mark? https://codereview.chromium.org/23445013/diff/1/chrome/common/extensions/comm... File chrome/common/extensions/command.cc (right): https://codereview.chromium.org/23445013/diff/1/chrome/common/extensions/comm... chrome/common/extensions/command.cc:68: if (tokens.size() < 2 || tokens.size() > 3) { On 2013/08/29 11:20:52, Finnur wrote: > Yes, change this if statement to become: > > if (tokens.size() == 0 || > (tokens.size() == 1 && DoesRequireModifier(accelerator)) || > tokens.size() > 3) { > ... do what it does now (show error) ... > } > > Then implement DoesRequireModifier to return true for all accelerators but the > Media Keys. Done. https://codereview.chromium.org/23445013/diff/1/chrome/common/extensions/comm... chrome/common/extensions/command.cc:152: key = ui::VKEY_TAB; On 2013/08/28 14:15:16, Finnur wrote: > Add values::kKeyMediaNextTrack (and the other media keys) after the vKeyTab > clause and assign ui::VKEY_MEDIA_NEXT_TRACK (etc) to |key|. Remember to include > should_parse_media_keys in the if clause for *each* key. Done. https://codereview.chromium.org/23445013/diff/1/chrome/common/extensions/comm... chrome/common/extensions/command.cc:191: } On 2013/08/28 14:15:16, Finnur wrote: > Add an if clause: > > if ((key == ui::VKEY_MEDIA_NEXT_TRACK || > key == ui::VKEY_MEDIA_PREV_TRACK || > key == ui::VKEY_MEDIA_PLAY_PAUSE || > key == ui::VKEY_MEDIA_STOP) && > (shift || ctrl || alt || command)) { > // Set *error to a message saying Media Keys cannot have a modifier. > return ui::Accelerator(); > } Done. https://codereview.chromium.org/23445013/diff/1/chrome/common/extensions/comm... File chrome/common/extensions/command_unittest.cc (right): https://codereview.chromium.org/23445013/diff/1/chrome/common/extensions/comm... chrome/common/extensions/command_unittest.cc:117: { false, none, "_execute_page_action", "MediaPrevTrack", ""}, On 2013/08/28 14:15:16, Finnur wrote: > Add a failure test for Ctrl+Shift+MediaKey (one permutation should be enough). Done. https://codereview.chromium.org/23445013/diff/11001/chrome/common/extensions/... File chrome/common/extensions/command.cc (right): https://codereview.chromium.org/23445013/diff/11001/chrome/common/extensions/... chrome/common/extensions/command.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2013/08/29 11:20:52, Finnur wrote: > > I haven't add code to support specifid media keys via chrome://extensions. I > > know it should be supported and would like do it step by step. WDYT? > > Ideally the .js changes should be part of this changelist, but you can do that > as your next followup CL if you want. You'll need to modify > extension_command_list.js in chrome\browser\resources\extensions to add the new > Media key codes and implement a similar function as the one I mentioned on the > C++ side (RequiresModifiers), because the code in the .js file also expects (and > requires) modifiers to be present. Thanks for your instruction. I'm getting more and more familiar with these code now. I prefer to adding it in my next followup CL, because I want to focus on some specified codes. Thanks for your understanding. https://codereview.chromium.org/23445013/diff/18001/chrome/common/extensions/... File chrome/common/extensions/command.cc (right): https://codereview.chromium.org/23445013/diff/18001/chrome/common/extensions/... chrome/common/extensions/command.cc:484: if (error->empty()) { Note: Check whether the error message is empty before override it in order to expose the error of ParseImpl.
I started a try job on this changelist, just for the sake of it. It is not as thorough as it could be, but let's take care of the following first: https://codereview.chromium.org/23445013/diff/18001/chrome/common/extensions/... File chrome/common/extensions/command.cc (right): https://codereview.chromium.org/23445013/diff/18001/chrome/common/extensions/... chrome/common/extensions/command.cc:484: if (error->empty()) { Can you elaborate on why this is needed... I don't follow...
https://codereview.chromium.org/23445013/diff/18001/chrome/common/extensions/... File chrome/common/extensions/command.cc (right): https://codereview.chromium.org/23445013/diff/18001/chrome/common/extensions/... chrome/common/extensions/command.cc:484: if (error->empty()) { On 2013/08/29 14:46:28, Finnur wrote: > Can you elaborate on why this is needed... > I don't follow... If ParseImpl have some error, then the error message will be set. For example: "kInvalidKeyBindingUnknownPlatform". We check only the return accelerator, the key_code will be ui::VKEY_UNKNOWN no matter what error it encountered. Then we override the detailed error, so we can only see errors::kInvalidKeyBinding in this situation.
On 2013/08/29 14:46:28, Finnur wrote: > I started a try job on this changelist, just for the sake of it. It is not as > thorough as it could be, but let's take care of the following first: Thanks. Due to my change of checking error message before override it, maybe some test case will failed, because the error message changed.
I see. Hmmm, this means we've been masking other errors from the parser. Anyway, this is good. LGTM.
Ping sky@ for owner review: ui/base/*
The PM gave a green light on having MediaKeys support for non-global shortcuts, which means we need to change the docs to say that MediaKeys are now supported.
On 2013/08/30 10:40:47, Finnur wrote: > The PM gave a green light on having MediaKeys support for non-global shortcuts, > which means we need to change the docs to say that MediaKeys are now supported. Done. Please review.
https://codereview.chromium.org/23445013/diff/35001/chrome/common/extensions/... File chrome/common/extensions/docs/templates/intros/commands.html (right): https://codereview.chromium.org/23445013/diff/35001/chrome/common/extensions/... chrome/common/extensions/docs/templates/intros/commands.html:20: Media keys cannot be used in key combination, just use them singly.<p> Suggest rewording: Modifiers (such as Ctrl) can not be used in combination with the Media Keys. Hmm... Now I'm actually wondering if there's any need to restrict the Media keys to not have modifiers... :s Is there any technical reason why we shouldn't be able to detect Ctrl+MediaNextTrack, for example?
https://codereview.chromium.org/23445013/diff/35001/chrome/common/extensions/... File chrome/common/extensions/docs/templates/intros/commands.html (right): https://codereview.chromium.org/23445013/diff/35001/chrome/common/extensions/... chrome/common/extensions/docs/templates/intros/commands.html:20: Media keys cannot be used in key combination, just use them singly.<p> On 2013/08/30 11:39:28, Finnur wrote: > Suggest rewording: > Modifiers (such as Ctrl) can not be used in combination with the Media Keys. > > Hmm... Now I'm actually wondering if there's any need to restrict the Media keys > to not have modifiers... :s Is there any technical reason why we shouldn't be > able to detect Ctrl+MediaNextTrack, for example? Done. Hmm...I think we have the ability to detect key combinations like "Ctrl+MediaNextTrack", but AFAICS they are being used with modifiers rarely. And some of these keys layout are special. For example: http://forums.logitech.com/t5/Keyboard-Hardware/Logitech-Wave-Keyboard-Media-... You can search in google image with "media keys" to see some keyboard.
> but AFAICS they are being used with modifiers rarely. You can make the same argument with other keys (Arrow keys, Insert, etc), yet we support modifiers there. We don't need to do it now, just something to think about. Looks like there's no technical reason (that we know of) not support modifiers on MediaKeys. Still LGTM.
On 2013/08/30 12:46:45, Finnur wrote: > > but AFAICS they are being used with modifiers rarely. > > You can make the same argument with other keys (Arrow keys, Insert, etc), yet we > support modifiers there. > > We don't need to do it now, just something to think about. Looks like there's no > technical reason (that we know of) not support modifiers on MediaKeys. > > Still LGTM. Thanks very much. I will follow up if there are any requirement that we want to support modifiers on MediaKeys.
On 2013/08/30 12:46:45, Finnur wrote: > > but AFAICS they are being used with modifiers rarely. > > You can make the same argument with other keys (Arrow keys, Insert, etc), yet we > support modifiers there. > > We don't need to do it now, just something to think about. Looks like there's no > technical reason (that we know of) not support modifiers on MediaKeys. > > Still LGTM. Thanks very much. I will follow up if there are any requirement that we want to support modifiers on MediaKeys.
https://codereview.chromium.org/23445013/diff/39001/ui/base/strings/ui_string... File ui/base/strings/ui_strings.grd (right): https://codereview.chromium.org/23445013/diff/39001/ui/base/strings/ui_string... ui/base/strings/ui_strings.grd:1462: MediaNextTrack Is there a reason these AreOneWord?
https://codereview.chromium.org/23445013/diff/39001/ui/base/strings/ui_string... File ui/base/strings/ui_strings.grd (right): https://codereview.chromium.org/23445013/diff/39001/ui/base/strings/ui_string... ui/base/strings/ui_strings.grd:1462: MediaNextTrack On 2013/08/30 17:08:29, sky wrote: > Is there a reason these AreOneWord? Done. Nit. I have change it for better readability.
https://codereview.chromium.org/23445013/diff/32001/ui/base/strings/ui_string... File ui/base/strings/ui_strings.grd (right): https://codereview.chromium.org/23445013/diff/32001/ui/base/strings/ui_string... ui/base/strings/ui_strings.grd:1462: Media Next Track I don't think these should be title case either, eg 'Media next track'
https://codereview.chromium.org/23445013/diff/32001/ui/base/strings/ui_string... File ui/base/strings/ui_strings.grd (right): https://codereview.chromium.org/23445013/diff/32001/ui/base/strings/ui_string... ui/base/strings/ui_strings.grd:1462: Media Next Track All of these are upper case. See above (Left Arrow, Up Arrow, PgUp, etc).
LGTM https://codereview.chromium.org/23445013/diff/32001/ui/base/strings/ui_string... File ui/base/strings/ui_strings.grd (right): https://codereview.chromium.org/23445013/diff/32001/ui/base/strings/ui_string... ui/base/strings/ui_strings.grd:1462: Media Next Track On 2013/09/03 16:04:29, Finnur wrote: > All of these are upper case. See above (Left Arrow, Up Arrow, PgUp, etc). > Oh the irony. All the ones after this aren't. I guess the are a different type though.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/zhchbin@gmail.com/23445013/32001
Message was sent while issue was closed.
Change committed as 221143 |
