Enable user assign media keys in chrome://extensions page.

Enable user assign media keys in chrome://extensions page.

BUG=131612
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=221360

[zhchbin, 2013-09-04 06:05:56]

Please review.

[Finnur, 2013-09-04 11:28:12]

https://codereview.chromium.org/23523019/diff/1/chrome/browser/resources/exte...
File chrome/browser/resources/extensions/extension_command_list.js (right):

https://codereview.chromium.org/23523019/diff/1/chrome/browser/resources/exte...
chrome/browser/resources/extensions/extension_command_list.js:32: /** @const */
var keyMediaPlayPause = 179;
The list is alphabetical so this should not be at the bottom.

https://codereview.chromium.org/23523019/diff/1/chrome/browser/resources/exte...
chrome/browser/resources/extensions/extension_command_list.js:58: keyCode ==
keyMediaPlayPause ||
Same here.

https://codereview.chromium.org/23523019/diff/1/chrome/browser/resources/exte...
chrome/browser/resources/extensions/extension_command_list.js:120: output +=
'MediaPlayPause'; break;
Same here.

https://codereview.chromium.org/23523019/diff/1/chrome/browser/resources/exte...
chrome/browser/resources/extensions/extension_command_list.js:135: function
isMediaKey(keyCode) {
I'd like to make this function a bit more general, so that if we add more keys
it is easy to just add to this matrix. It also helps make the code below a bit
more general and readable. So, I would do this:

function modifiers(keyCode) {
  switch (keycode) {
   case keyMediaNextTrack:
   case keyMediaPrevTrack:
   case keyMediaStop:
   case keyMediaPlayPause:
     return areNotAllowed;
   default:
     return areRequired;
  }
}

areAllowed and areRequired can be enums (consts) at the top.

Remember to fix the comment for this function too.

https://codereview.chromium.org/23523019/diff/1/chrome/browser/resources/exte...
chrome/browser/resources/extensions/extension_command_list.js:367: }
I think this is a bit more readable:

function hasModifier(event, countShiftAsModifier) {
  return event.ctrlKey || event.altKey || (cr.isMac && event.metaKey) ||
         (countShiftAsModifier && event.shiftKey);
}

if (modifiers(event.keyCode) == areRequired && !hasModifier(event, false)) {
  // Ctrl or Alt (or Cmd on Mac) is a must for most shortcuts.
  return;
}

if (modifiers(event.keyCode) == areNotAllowed && hasModifier(event, true))
  return;

You can even (for extra readability) create an enum for "ignoreShift" and "all"
instead of using a bool for countShiftAsModifier.

[zhchbin, 2013-09-04 12:50:02]

Please review again.

https://codereview.chromium.org/23523019/diff/1/chrome/browser/resources/exte...
File chrome/browser/resources/extensions/extension_command_list.js (right):

https://codereview.chromium.org/23523019/diff/1/chrome/browser/resources/exte...
chrome/browser/resources/extensions/extension_command_list.js:32: /** @const */
var keyMediaPlayPause = 179;
On 2013/09/04 11:28:12, Finnur wrote:
> The list is alphabetical so this should not be at the bottom.

Done.

https://codereview.chromium.org/23523019/diff/1/chrome/browser/resources/exte...
chrome/browser/resources/extensions/extension_command_list.js:58: keyCode ==
keyMediaPlayPause ||
On 2013/09/04 11:28:12, Finnur wrote:
> Same here.

Done.

https://codereview.chromium.org/23523019/diff/1/chrome/browser/resources/exte...
chrome/browser/resources/extensions/extension_command_list.js:120: output +=
'MediaPlayPause'; break;
On 2013/09/04 11:28:12, Finnur wrote:
> Same here.

Done.

https://codereview.chromium.org/23523019/diff/1/chrome/browser/resources/exte...
chrome/browser/resources/extensions/extension_command_list.js:135: function
isMediaKey(keyCode) {
On 2013/09/04 11:28:12, Finnur wrote:
> I'd like to make this function a bit more general, so that if we add more keys
> it is easy to just add to this matrix. It also helps make the code below a bit
> more general and readable. So, I would do this:
> 
> function modifiers(keyCode) {
>   switch (keycode) {
>    case keyMediaNextTrack:
>    case keyMediaPrevTrack:
>    case keyMediaStop:
>    case keyMediaPlayPause:
>      return areNotAllowed;
>    default:
>      return areRequired;
>   }
> }
> 
> areAllowed and areRequired can be enums (consts) at the top.
> 
> Remember to fix the comment for this function too.

Done with using a enum:

  /**
   * Enum for whether we require modifiers of a keycode.
   * @enum {number}
   */
  var Modifiers = {
    ARE_NOT_REQUIRED: 0,
    ARE_REQUIRED: 1
  };

https://codereview.chromium.org/23523019/diff/1/chrome/browser/resources/exte...
chrome/browser/resources/extensions/extension_command_list.js:367: }
On 2013/09/04 11:28:12, Finnur wrote:
> I think this is a bit more readable:
> 
> function hasModifier(event, countShiftAsModifier) {
>   return event.ctrlKey || event.altKey || (cr.isMac && event.metaKey) ||
>          (countShiftAsModifier && event.shiftKey);
> }
> 
> if (modifiers(event.keyCode) == areRequired && !hasModifier(event, false)) {
>   // Ctrl or Alt (or Cmd on Mac) is a must for most shortcuts.
>   return;
> }
> 
> if (modifiers(event.keyCode) == areNotAllowed && hasModifier(event, true))
>   return;
> 
> You can even (for extra readability) create an enum for "ignoreShift" and
"all"
> instead of using a bool for countShiftAsModifier.

Done with still using a boolean for countShiftAsModifier.

[Finnur, 2013-09-04 13:53:05]

https://codereview.chromium.org/23523019/diff/6001/chrome/browser/resources/e...
File chrome/browser/resources/extensions/extension_command_list.js (right):

https://codereview.chromium.org/23523019/diff/6001/chrome/browser/resources/e...
chrome/browser/resources/extensions/extension_command_list.js:150: return
Modifiers.ARE_NOT_REQUIRED;
This is incorrect. Eventually, we might have keys where modifiers are allowed
but not required. That's not the case for MediaKeys today. This should be
Modifiers.ARE_NOT_ALLOWED.

https://codereview.chromium.org/23523019/diff/6001/chrome/browser/resources/e...
chrome/browser/resources/extensions/extension_command_list.js:158: * modifiers:
'Ctrl', 'Alt', 'Cmd' on Mac, 'Shift' when the
Nit: Replace
", 'Shift'"
with
"and 'Shift'"

https://codereview.chromium.org/23523019/diff/6001/chrome/browser/resources/e...
chrome/browser/resources/extensions/extension_command_list.js:160: * @param
{Event} event The keyboard event to convert.
s/convert/consider/

https://codereview.chromium.org/23523019/diff/6001/chrome/browser/resources/e...
chrome/browser/resources/extensions/extension_command_list.js:389: if
(modifiers(event.keyCode) == Modifiers.ARE_NOT_REQUIRED &&
Same here: ARE_NOT_ALLOWED.

[zhchbin, 2013-09-04 14:24:27]

https://codereview.chromium.org/23523019/diff/6001/chrome/browser/resources/e...
File chrome/browser/resources/extensions/extension_command_list.js (right):

https://codereview.chromium.org/23523019/diff/6001/chrome/browser/resources/e...
chrome/browser/resources/extensions/extension_command_list.js:150: return
Modifiers.ARE_NOT_REQUIRED;
On 2013/09/04 13:53:05, Finnur wrote:
> This is incorrect. Eventually, we might have keys where modifiers are allowed
> but not required. That's not the case for MediaKeys today. This should be
> Modifiers.ARE_NOT_ALLOWED.

Done. I got your idea now!! Sorry for my mistake.

https://codereview.chromium.org/23523019/diff/6001/chrome/browser/resources/e...
chrome/browser/resources/extensions/extension_command_list.js:158: * modifiers:
'Ctrl', 'Alt', 'Cmd' on Mac, 'Shift' when the
On 2013/09/04 13:53:05, Finnur wrote:
> Nit: Replace
> ", 'Shift'"
> with
> "and 'Shift'"

Done.

https://codereview.chromium.org/23523019/diff/6001/chrome/browser/resources/e...
chrome/browser/resources/extensions/extension_command_list.js:160: * @param
{Event} event The keyboard event to convert.
On 2013/09/04 13:53:05, Finnur wrote:
> s/convert/consider/

Done.

https://codereview.chromium.org/23523019/diff/6001/chrome/browser/resources/e...
chrome/browser/resources/extensions/extension_command_list.js:389: if
(modifiers(event.keyCode) == Modifiers.ARE_NOT_REQUIRED &&
On 2013/09/04 13:53:05, Finnur wrote:
> Same here: ARE_NOT_ALLOWED.

Done.

[Finnur, 2013-09-04 15:03:17]

LGTM with one nit.

https://codereview.chromium.org/23523019/diff/14001/chrome/browser/resources/...
File chrome/browser/resources/extensions/extension_command_list.js (right):

https://codereview.chromium.org/23523019/diff/14001/chrome/browser/resources/...
chrome/browser/resources/extensions/extension_command_list.js:140: * allowed to
be used without any modifier.
nit: s/allowed/required/

[zhchbin, 2013-09-05 01:30:50]

https://codereview.chromium.org/23523019/diff/14001/chrome/browser/resources/...
File chrome/browser/resources/extensions/extension_command_list.js (right):

https://codereview.chromium.org/23523019/diff/14001/chrome/browser/resources/...
chrome/browser/resources/extensions/extension_command_list.js:140: * allowed to
be used without any modifier.
On 2013/09/04 15:03:17, Finnur wrote:
> nit: s/allowed/required/

Done.

[commit-bot: I haz the power, 2013-09-05 01:31:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zhchbin@gmail.com/23523019/16001

[commit-bot: I haz the power, 2013-09-05 02:37:57]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_dbg_si...

[commit-bot: I haz the power, 2013-09-05 02:46:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zhchbin@gmail.com/23523019/16001

[commit-bot: I haz the power, 2013-09-05 03:13:02]

Retried try job too often on ios_dbg_simulator for step(s) ui_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_dbg_si...

[commit-bot: I haz the power, 2013-09-05 07:22:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zhchbin@gmail.com/23523019/16001

[commit-bot: I haz the power, 2013-09-05 07:23:16]

Change committed as 221360