Allow extensions to remove and override the bookmark shortcut key

Allow extensions to remove and override the bookmark shortcut key

This feature is enabled for dev behind the
--enable-override-bookmarks-ui=1 feature flag, and for all releases for
internal bookmarks extensions.

Implements the shortcut key aspect of the
Remove Bookmark Shortcut Chrome API proposal:
https://docs.google.com/a/chromium.org/document/d/1C2Mle92O9uGlji5y5gGDM5tNJ_tVE1Vb-2xgsZPNDTk

BUG=335655
R=erg@chromium.org, finnur@chromium.org, shess@chromium.org, sky@chromium.org

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

[Mike Wittman, 2014-02-04 19:57:14]

PTAL

kalman: chrome/{browser,commmon}/extensions
sky: chrome/browser/ui, chrome/browser/ui/views
shess: chrome/browser/ui/cocoa
erg: chrome/browser/ui/gtk

[not at google - send to devlin, 2014-02-04 20:31:22]

I'll let finnur take this one.

[sky, 2014-02-04 22:52:53]

https://codereview.chromium.org/143493005/diff/1/chrome/browser/ui/views/acce...
File chrome/browser/ui/views/accelerator_utils_views.cc (right):

https://codereview.chromium.org/143493005/diff/1/chrome/browser/ui/views/acce...
chrome/browser/ui/views/accelerator_utils_views.cc:28: ui::Accelerator
GetChromeAcceleratorForCommandId(int command_id) {
Accelerators handled in ash aren't covered here. Also some commands are not
handled via the accelerator table on windows but through app commands. See
BrowserView::GetCommandIDForAppCommandID.

[Scott Hess - ex-Googler, 2014-02-04 23:03:16]

LGTM for cocoa/.

[Elliot Glaysher, 2014-02-04 23:05:12]

gtk lgtm

[not at google - send to devlin, 2014-02-04 23:05:13]

(though the manifest stuff l g t m)

[Finnur, 2014-02-05 13:37:40]

https://codereview.chromium.org/143493005/diff/1/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/commands/command_service.cc (right):

https://codereview.chromium.org/143493005/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/commands/command_service.cc:92: if
(SettingsOverrides::Get(extension)->RemovesBookmarkShortcut() &&
I mentioned this in the design doc, but worth repeating here: It is a bit
confusing to be able to assign a shortcut based on the fact that a
remove_shortcut permission is granted. This should really be
AllowBookmarkShortcutOverride(), or something.

https://codereview.chromium.org/143493005/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/commands/command_service.cc:95:
extensions::APIPermission::kBookmarkManagerPrivate) ||
You mentioned you didn't want a dependency on the bookmarks permission, but you
have a dependency on the bookmarkManager permission instead. Should that be
listed in the design doc?

https://codereview.chromium.org/143493005/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/commands/command_service.cc:397: false,  //
Overwriting not allowed.
Note: If the user has reassigned Ctrl+D to something else, then the bookmark
extension won't be allowed to assign it (since this is false). I'm leaning
towards that being correct, because the user should be in control of the
shortcuts, but could be convinced otherwise as in this case it is a bit of a
gray area since they just installed an extension that said it is going to take
over the shortcut and even has a special permission to do so. The counterpoint
is that the shortcut is no longer assigned to the bookmark feature...

https://codereview.chromium.org/143493005/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/commands/command_service.cc:398:
iter->second.global());
Commands can be global/non-global in scope and a global command works even when
Chrome does not have focus (it is considered system-wide). 

Before your change, Ctrl+D was reserved and could not be overwritten by an
extension (only through user interaction). Now, however, the only thing stopping
the extension from making Ctrl+D available globally (which means Ctrl+D will
stop working in other apps since your extension eats it) is the fact that we are
super restrictive about the things you can auto-assign globally (see
IsWhitelistedGlobalShortcut). 

Initially I thought we should perhaps pass in bool global to
IsReservedChromeAccelerator and only consult the overwrite bookmarks flag if
global == false. But then I thought, this is probably (and should be) covered by
IsWhitelistedGlobalShortcut. If we decide to relax the restriction on
auto-assigning global shortcuts, then we should be able to auto-assign Ctrl+D,
even globally.

Perhaps the best course of action is simply to add a comment at the top of
IsWhitelistedGlobalShortcut, saying something like:

"If the restrictions on auto-assigning global shortcuts are ever relaxed in the
future, then keys such as the bookmarking shortcut (IDC_BOOKMARK_PAGE) need to
be evaluated on a case-by-case basis. Such keys can be auto-assigned by
extensions (with the right permission), even though the shortcut is already in
use by Chrome".

https://codereview.chromium.org/143493005/diff/1/chrome/browser/ui/accelerato...
File chrome/browser/ui/accelerator_utils.h (right):

https://codereview.chromium.org/143493005/diff/1/chrome/browser/ui/accelerato...
chrome/browser/ui/accelerator_utils.h:21: ui::Accelerator
GetChromeAcceleratorForCommandId(int command_id);
Your design doc talks about not showing the Bookmark feature in the menu if an
extension is overwriting it. This CL does not do that, though. Is that
intentional?

https://codereview.chromium.org/143493005/diff/1/chrome/common/extensions/man...
File chrome/common/extensions/manifest_handlers/settings_overrides_handler.h
(right):

https://codereview.chromium.org/143493005/diff/1/chrome/common/extensions/man...
chrome/common/extensions/manifest_handlers/settings_overrides_handler.h:19:
struct SettingsOverrides : public Extension::ManifestData {
This isn't purely about data anymore - Why is this still a struct?

[Finnur, 2014-02-05 15:32:05]

Also, this setting should be documented (see src\chrome\common\extensions\docs).


Is it possible to write a test for this feature?

[Mike Wittman, 2014-02-05 18:10:45]

https://codereview.chromium.org/143493005/diff/1/chrome/browser/ui/views/acce...
File chrome/browser/ui/views/accelerator_utils_views.cc (right):

https://codereview.chromium.org/143493005/diff/1/chrome/browser/ui/views/acce...
chrome/browser/ui/views/accelerator_utils_views.cc:28: ui::Accelerator
GetChromeAcceleratorForCommandId(int command_id) {
On 2014/02/04 22:52:54, sky wrote:
> Accelerators handled in ash aren't covered here. Also some commands are not
> handled via the accelerator table on windows but through app commands. See
> BrowserView::GetCommandIDForAppCommandID.

I've added a check for Ash accelerators (and standard accelerators, and moved
the function to the _aura.cc file).

All of the command IDs associated with the app commands also have standard
keyboard accelerators, which would be returned by the function. I renamed the
function to GetPrimaryChromeAcceleratorForCommandId to emphasize that there may
be alternate accelerator keys.

[Mike Wittman, 2014-02-05 19:17:06]

https://codereview.chromium.org/143493005/diff/1/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/commands/command_service.cc (right):

https://codereview.chromium.org/143493005/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/commands/command_service.cc:92: if
(SettingsOverrides::Get(extension)->RemovesBookmarkShortcut() &&
On 2014/02/05 13:37:40, Finnur wrote:
> I mentioned this in the design doc, but worth repeating here: It is a bit
> confusing to be able to assign a shortcut based on the fact that a
> remove_shortcut permission is granted. This should really be
> AllowBookmarkShortcutOverride(), or something.

I'll update this as necessary pending agreement in the design doc.

https://codereview.chromium.org/143493005/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/commands/command_service.cc:95:
extensions::APIPermission::kBookmarkManagerPrivate) ||
On 2014/02/05 13:37:40, Finnur wrote:
> You mentioned you didn't want a dependency on the bookmarks permission, but
you
> have a dependency on the bookmarkManager permission instead. Should that be
> listed in the design doc?

The check for kBookmarkManagerPrivate API permission is done as a means of
whitelisting Chrome-internal bookmarks extensions to use this functionality. It
has no effect on authors of general extensions, so I don't see a reason to
mention it in the design doc.

https://codereview.chromium.org/143493005/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/commands/command_service.cc:397: false,  //
Overwriting not allowed.
On 2014/02/05 13:37:40, Finnur wrote:
> Note: If the user has reassigned Ctrl+D to something else, then the bookmark
> extension won't be allowed to assign it (since this is false). I'm leaning
> towards that being correct, because the user should be in control of the
> shortcuts, but could be convinced otherwise as in this case it is a bit of a
> gray area since they just installed an extension that said it is going to take
> over the shortcut and even has a special permission to do so. The counterpoint
> is that the shortcut is no longer assigned to the bookmark feature...

I tend to agree that user-created shortcuts should not be automatically
overridden by extensions with this permission. Presumably if the user has taken
the effort to reassign the key once, they will be able to reassign again to this
extension if they want.

https://codereview.chromium.org/143493005/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/commands/command_service.cc:398:
iter->second.global());
On 2014/02/05 13:37:40, Finnur wrote:
> Commands can be global/non-global in scope and a global command works even
when
> Chrome does not have focus (it is considered system-wide). 
> 
> Before your change, Ctrl+D was reserved and could not be overwritten by an
> extension (only through user interaction). Now, however, the only thing
stopping
> the extension from making Ctrl+D available globally (which means Ctrl+D will
> stop working in other apps since your extension eats it) is the fact that we
are
> super restrictive about the things you can auto-assign globally (see
> IsWhitelistedGlobalShortcut). 
> 
> Initially I thought we should perhaps pass in bool global to
> IsReservedChromeAccelerator and only consult the overwrite bookmarks flag if
> global == false. But then I thought, this is probably (and should be) covered
by
> IsWhitelistedGlobalShortcut. If we decide to relax the restriction on
> auto-assigning global shortcuts, then we should be able to auto-assign Ctrl+D,
> even globally.
> 
> Perhaps the best course of action is simply to add a comment at the top of
> IsWhitelistedGlobalShortcut, saying something like:
> 
> "If the restrictions on auto-assigning global shortcuts are ever relaxed in
the
> future, then keys such as the bookmarking shortcut (IDC_BOOKMARK_PAGE) need to
> be evaluated on a case-by-case basis. Such keys can be auto-assigned by
> extensions (with the right permission), even though the shortcut is already in
> use by Chrome".

SGTM. I've added the comment.

https://codereview.chromium.org/143493005/diff/1/chrome/browser/ui/accelerato...
File chrome/browser/ui/accelerator_utils.h (right):

https://codereview.chromium.org/143493005/diff/1/chrome/browser/ui/accelerato...
chrome/browser/ui/accelerator_utils.h:21: ui::Accelerator
GetChromeAcceleratorForCommandId(int command_id);
On 2014/02/05 13:37:40, Finnur wrote:
> Your design doc talks about not showing the Bookmark feature in the menu if an
> extension is overwriting it. This CL does not do that, though. Is that
> intentional?

Yes. I will address the menu aspects in a separate CL.

https://codereview.chromium.org/143493005/diff/1/chrome/common/extensions/man...
File chrome/common/extensions/manifest_handlers/settings_overrides_handler.h
(right):

https://codereview.chromium.org/143493005/diff/1/chrome/common/extensions/man...
chrome/common/extensions/manifest_handlers/settings_overrides_handler.h:19:
struct SettingsOverrides : public Extension::ManifestData {
On 2014/02/05 13:37:40, Finnur wrote:
> This isn't purely about data anymore - Why is this still a struct?

Mainly for consistency with the other subclasses of Extension::ManifestData.
I've changed the members to static functions to live within the style
guidelines.

[Mike Wittman, 2014-02-06 02:29:57]

On 2014/02/05 15:32:05, Finnur wrote:
> Also, this setting should be documented (see
src\chrome\common\extensions\docs).
> 
> 
> Is it possible to write a test for this feature?

Added documentation and tests.

[sky, 2014-02-06 16:03:15]

https://codereview.chromium.org/143493005/diff/1/chrome/browser/ui/views/acce...
File chrome/browser/ui/views/accelerator_utils_views.cc (right):

https://codereview.chromium.org/143493005/diff/1/chrome/browser/ui/views/acce...
chrome/browser/ui/views/accelerator_utils_views.cc:28: ui::Accelerator
GetChromeAcceleratorForCommandId(int command_id) {
On 2014/02/05 18:10:46, Mike Wittman wrote:
> On 2014/02/04 22:52:54, sky wrote:
> > Accelerators handled in ash aren't covered here. Also some commands are not
> > handled via the accelerator table on windows but through app commands. See
> > BrowserView::GetCommandIDForAppCommandID.
> 
> I've added a check for Ash accelerators (and standard accelerators, and moved
> the function to the _aura.cc file).
> 
> All of the command IDs associated with the app commands also have standard
> keyboard accelerators, which would be returned by the function. I renamed the
> function to GetPrimaryChromeAcceleratorForCommandId to emphasize that there
may
> be alternate accelerator keys.

Ah, ok. Good point.

https://codereview.chromium.org/143493005/diff/410017/chrome/browser/ui/views...
File chrome/browser/ui/views/accelerator_utils_aura.cc (right):

https://codereview.chromium.org/143493005/diff/410017/chrome/browser/ui/views...
chrome/browser/ui/views/accelerator_utils_aura.cc:41: if
(GetStandardAcceleratorForCommandId(command_id, &accelerator))
nit: I would likely combine this if and 43.

https://codereview.chromium.org/143493005/diff/410017/chrome/browser/ui/views...
chrome/browser/ui/views/accelerator_utils_aura.cc:44:
chrome::GetActiveDesktop(),
Using activedesktop like this is a bit dicey. Is there no way to pass in the
hostdesktop?

[Mike Wittman, 2014-02-06 19:35:41]

https://codereview.chromium.org/143493005/diff/410017/chrome/browser/ui/views...
File chrome/browser/ui/views/accelerator_utils_aura.cc (right):

https://codereview.chromium.org/143493005/diff/410017/chrome/browser/ui/views...
chrome/browser/ui/views/accelerator_utils_aura.cc:44:
chrome::GetActiveDesktop(),
On 2014/02/06 16:03:16, sky wrote:
> Using activedesktop like this is a bit dicey. Is there no way to pass in the
> hostdesktop?

This is invoked from the extension command service, which operates independent
of any particular browser as far as I can tell.

Taking a step back, this function probably should identify Ash accelerators if
it's possible to have an Ash desktop on the platform, independent of whether the
Ash desktop is currently in use. I'll update accordingly.

A side node: on Windows the Ash accelerators are duplicated in the general
kAcceleratorMap table, so this function would produce the same results
regardless of how (or if) GetAshAcceleratorForCommandId is invoked. On ChromeOS
it is necessary to pick up those accelerators via GetAshAcceleratorForCommandId.

[Finnur, 2014-02-10 10:14:17]

LGTM, with the caveat that I'd like the question resolved in the design doc. I
don't think we need to hold up this CL for that, though, as long as you are
willing to submit a followup CL to address things that might result from that
discussion.

https://codereview.chromium.org/143493005/diff/550001/chrome/browser/extensio...
File chrome/browser/extensions/extension_keybinding_apitest.cc (right):

https://codereview.chromium.org/143493005/diff/550001/chrome/browser/extensio...
chrome/browser/extensions/extension_keybinding_apitest.cc:209: // Activate the
bookmark shortcut (Ctrl+D) to make page green (should not work
nit: the page

https://codereview.chromium.org/143493005/diff/550001/chrome/common/extension...
File chrome/common/extensions/api/manifest_types.json (right):

https://codereview.chromium.org/143493005/diff/550001/chrome/common/extension...
chrome/common/extensions/api/manifest_types.json:60: },
Usually, we restrict new APIs to the dev channel until we gain confidence in
them. This, however, isn't restrict-able to channels, so it will go straight to
stable...  Right?

https://codereview.chromium.org/143493005/diff/550001/chrome/common/extension...
File chrome/common/extensions/docs/templates/articles/settings_override.html
(right):

https://codereview.chromium.org/143493005/diff/550001/chrome/common/extension...
chrome/common/extensions/docs/templates/articles/settings_override.html:22:
</style>
Where does this css snippet come from? It doesn't seem to be used below...

https://codereview.chromium.org/143493005/diff/550001/chrome/common/extension...
chrome/common/extensions/docs/templates/articles/settings_override.html:30: An
extension can override the following properties:
Suggest: ... override one or more ...

https://codereview.chromium.org/143493005/diff/550001/chrome/common/extension...
chrome/common/extensions/docs/templates/articles/settings_override.html:37:
Bookmark button: the "star" button that appears on the right side of the
Heads up: Generally, it is better to not assume UI elements will be in a certain
fixed location. For example, we're looking into moving the star button out of
the Omnibox, so this comment might get out of date sooner than you think. :)

https://codereview.chromium.org/143493005/diff/550001/chrome/common/extension...
chrome/common/extensions/docs/templates/articles/settings_override.html:67: <b> 
"chrome_settings_overrides" : {
nit: I would leave <b> on its own line, so that the indentation for the rest of
this line is easier to verify for correctness.

[Finnur, 2014-02-11 09:35:00]

https://codereview.chromium.org/143493005/diff/550001/chrome/browser/extensio...
File chrome/browser/extensions/api/commands/command_service.cc (right):

https://codereview.chromium.org/143493005/diff/550001/chrome/browser/extensio...
chrome/browser/extensions/api/commands/command_service.cc:88: return false;
Ooops! Not LGTM.

This is incorrect for Mac at the moment. As issue 342484 points out,
we need to fix IsChromeAccelerator on Mac before we can check this in.

[Finnur, 2014-02-11 09:50:05]

https://codereview.chromium.org/143493005/diff/550001/chrome/browser/extensio...
File chrome/browser/extensions/api/commands/command_service.cc (right):

https://codereview.chromium.org/143493005/diff/550001/chrome/browser/extensio...
chrome/browser/extensions/api/commands/command_service.cc:105: }
We need to flip this logic. Instead of current implementation of (pseudo):

if (!reserved(accelerator))
  return false;
if (hasRemoveBookmarkPermission())
  return accelerator != BOOKMARK
return true;

... we need something like ...

if (accelerator == BOOKMARK)
  return !hasRemoveBookmarkPermission();
return reserved(accelerator);

This will ensure that if reserved() is incorrectly implemented (like it is now)
then still only extensions with permission to override the key can do so.

[Mike Wittman, 2014-02-11 23:09:44]

https://codereview.chromium.org/143493005/diff/550001/chrome/browser/extensio...
File chrome/browser/extensions/api/commands/command_service.cc (right):

https://codereview.chromium.org/143493005/diff/550001/chrome/browser/extensio...
chrome/browser/extensions/api/commands/command_service.cc:88: return false;
On 2014/02/11 09:35:01, Finnur wrote:
> Ooops! Not LGTM.
> 
> This is incorrect for Mac at the moment. As issue 342484 points out,
> we need to fix IsChromeAccelerator on Mac before we can check this in.

OK. I will ping this review when the Mac changes are in.

https://codereview.chromium.org/143493005/diff/550001/chrome/browser/extensio...
chrome/browser/extensions/api/commands/command_service.cc:105: }
On 2014/02/11 09:50:05, Finnur wrote:
> We need to flip this logic. Instead of current implementation of (pseudo):
> 
> if (!reserved(accelerator))
>   return false;
> if (hasRemoveBookmarkPermission())
>   return accelerator != BOOKMARK
> return true;
> 
> ... we need something like ...
> 
> if (accelerator == BOOKMARK)
>   return !hasRemoveBookmarkPermission();
> return reserved(accelerator);
> 
> This will ensure that if reserved() is incorrectly implemented (like it is
now)
> then still only extensions with permission to override the key can do so.

Done.

https://codereview.chromium.org/143493005/diff/550001/chrome/browser/extensio...
File chrome/browser/extensions/extension_keybinding_apitest.cc (right):

https://codereview.chromium.org/143493005/diff/550001/chrome/browser/extensio...
chrome/browser/extensions/extension_keybinding_apitest.cc:209: // Activate the
bookmark shortcut (Ctrl+D) to make page green (should not work
On 2014/02/10 10:14:18, Finnur wrote:
> nit: the page

Done.

https://codereview.chromium.org/143493005/diff/550001/chrome/common/extension...
File chrome/common/extensions/api/manifest_types.json (right):

https://codereview.chromium.org/143493005/diff/550001/chrome/common/extension...
chrome/common/extensions/api/manifest_types.json:60: },
On 2014/02/10 10:14:18, Finnur wrote:
> Usually, we restrict new APIs to the dev channel until we gain confidence in
> them. This, however, isn't restrict-able to channels, so it will go straight
to
> stable...  Right?

No, it's restricted to dev via _manifest_features.json. The top-level
chrome_settings_overrides is limited to dev channel for general use, and stable
for internal bookmarks extensions.

https://codereview.chromium.org/143493005/diff/550001/chrome/common/extension...
File chrome/common/extensions/docs/templates/articles/settings_override.html
(right):

https://codereview.chromium.org/143493005/diff/550001/chrome/common/extension...
chrome/common/extensions/docs/templates/articles/settings_override.html:22:
</style>
On 2014/02/10 10:14:18, Finnur wrote:
> Where does this css snippet come from? It doesn't seem to be used below...

Removed. This was originally from the Override Pages documentation.

https://codereview.chromium.org/143493005/diff/550001/chrome/common/extension...
chrome/common/extensions/docs/templates/articles/settings_override.html:30: An
extension can override the following properties:
On 2014/02/10 10:14:18, Finnur wrote:
> Suggest: ... override one or more ...

Done.

https://codereview.chromium.org/143493005/diff/550001/chrome/common/extension...
chrome/common/extensions/docs/templates/articles/settings_override.html:37:
Bookmark button: the "star" button that appears on the right side of the
On 2014/02/10 10:14:18, Finnur wrote:
> Heads up: Generally, it is better to not assume UI elements will be in a
certain
> fixed location. For example, we're looking into moving the star button out of
> the Omnibox, so this comment might get out of date sooner than you think. :)

Updated to reference the button by function.

https://codereview.chromium.org/143493005/diff/550001/chrome/common/extension...
chrome/common/extensions/docs/templates/articles/settings_override.html:67: <b> 
"chrome_settings_overrides" : {
On 2014/02/10 10:14:18, Finnur wrote:
> nit: I would leave <b> on its own line, so that the indentation for the rest
of
> this line is easier to verify for correctness.

Done.

[Finnur, 2014-02-12 12:51:13]

https://codereview.chromium.org/143493005/diff/550001/chrome/browser/extensio...
File chrome/browser/extensions/api/commands/command_service.cc (right):

https://codereview.chromium.org/143493005/diff/550001/chrome/browser/extensio...
chrome/browser/extensions/api/commands/command_service.cc:88: return false;
Thank you.

https://codereview.chromium.org/143493005/diff/760001/chrome/browser/extensio...
File chrome/browser/extensions/api/commands/command_service.cc (right):

https://codereview.chromium.org/143493005/diff/760001/chrome/browser/extensio...
chrome/browser/extensions/api/commands/command_service.cc:98:
FeatureSwitch::enable_override_bookmarks_ui()->IsEnabled()));
It is hard to wrap one's head around verifying the correctness of allowing
extensions to override shortcuts. The ! here doesn't help, but neither does the
logic being split up into checks in three different places (here,
IsWhitelistedGlobalShortcut and IsMediaKey way down this file).

More on this in my next comment.

https://codereview.chromium.org/143493005/diff/760001/chrome/browser/extensio...
chrome/browser/extensions/api/commands/command_service.cc:126: }
I just realized (coming in this morning with a clear head) that the thing that
is really bothering me with this is that we don't have a single funnel for
determining whether we can override a key. Instead we have three functions that
decide whether you can override the shortcut or not (pre-existing condition, as
they'd call it).

I'm therefore proposing we delete the comment you added to this function and
simply merge the two functions above into what I've written below (haven't
compiled it, but I think this should do what we want):

bool CanAutoAssign(const extensions::Command& command,
                   const Extension* extension,
                   Profile* profile,
                   bool is_named_command) {
  // Media Keys are non-exclusive, so allow auto-assigning them.
  if (extensions::CommandService::IsMediaKey(command.accelerator())
    return true;

  if (command.global()) {
    if (!is_named_command)
      return false;  // Browser and page actions are not global in nature.

    // Global shortcuts are restricted to (Ctrl|Command)+Shift+[0-9].
#if defined OS_MACOSX
    if (!command.accelerator().IsCmdDown())
      return false;
#else
    if (!command.accelerator().IsCtrlDown())
      return false;
#endif
    if (!command.accelerator().IsShiftDown())
      return false;
    return (command.accelerator().key_code() >= ui::VKEY_0 &&
            command.accelerator().key_code() <= ui::VKEY_9);
  } else {
    // Not a global command, check if Chrome shortcut and whether
    // we can override it.
    if (command.accelerator() ==
        chrome::GetPrimaryChromeAcceleratorForCommandId(IDC_BOOKMARK_PAGE)) {
      using extensions::SettingsOverrides;
      using extensions::FeatureSwitch;
      const SettingsOverrides* settings_overrides =
          SettingsOverrides::Get(extension);
      if (settings_overrides &&
          SettingsOverrides::RemovesBookmarkShortcut(*settings_overrides) &&
          (extensions::PermissionsData::HasAPIPermission(
              extension,
              extensions::APIPermission::kBookmarkManagerPrivate) ||
           FeatureSwitch::enable_override_bookmarks_ui()->IsEnabled())) {
        // If this check fails it either means we have an API to override a
        // key that isn't a ChromeAccelerator (and the API can therefore be
        // deprecated) or the IsChromeAccelerator isn't consistently
        // returning true for all accelerators.
        DCHECK(chrome::IsChromeAccelerator(command.accelerator(), profile));
        return true;
      }
    }
  
    return !chrome::IsChromeAccelerator(command.accelerator(), profile);
  }
}

https://codereview.chromium.org/143493005/diff/760001/chrome/browser/extensio...
chrome/browser/extensions/api/commands/command_service.cc:398:
IsWhitelistedGlobalShortcut(iter->second)) ||
This would turn into one simple if statement:
  if (CanOverride(iter->second, extension, profile, true))
... and same applies to below, except false for last param (for both).

[Mike Wittman, 2014-02-12 19:55:57]

https://codereview.chromium.org/143493005/diff/760001/chrome/browser/extensio...
File chrome/browser/extensions/api/commands/command_service.cc (right):

https://codereview.chromium.org/143493005/diff/760001/chrome/browser/extensio...
chrome/browser/extensions/api/commands/command_service.cc:98:
FeatureSwitch::enable_override_bookmarks_ui()->IsEnabled()));
On 2014/02/12 12:51:14, Finnur wrote:
> It is hard to wrap one's head around verifying the correctness of allowing
> extensions to override shortcuts. The ! here doesn't help, but neither does
the
> logic being split up into checks in three different places (here,
> IsWhitelistedGlobalShortcut and IsMediaKey way down this file).
> 
> More on this in my next comment.

Agreed.

https://codereview.chromium.org/143493005/diff/760001/chrome/browser/extensio...
chrome/browser/extensions/api/commands/command_service.cc:126: }
On 2014/02/12 12:51:14, Finnur wrote:
> I just realized (coming in this morning with a clear head) that the thing that
> is really bothering me with this is that we don't have a single funnel for
> determining whether we can override a key. Instead we have three functions
that
> decide whether you can override the shortcut or not (pre-existing condition,
as
> they'd call it).
> 
> I'm therefore proposing we delete the comment you added to this function and
> simply merge the two functions above into what I've written below (haven't
> compiled it, but I think this should do what we want):
> 
> bool CanAutoAssign(const extensions::Command& command,
>                    const Extension* extension,
>                    Profile* profile,
>                    bool is_named_command) {
>   // Media Keys are non-exclusive, so allow auto-assigning them.
>   if (extensions::CommandService::IsMediaKey(command.accelerator())
>     return true;
> 
>   if (command.global()) {
>     if (!is_named_command)
>       return false;  // Browser and page actions are not global in nature.
> 
>     // Global shortcuts are restricted to (Ctrl|Command)+Shift+[0-9].
> #if defined OS_MACOSX
>     if (!command.accelerator().IsCmdDown())
>       return false;
> #else
>     if (!command.accelerator().IsCtrlDown())
>       return false;
> #endif
>     if (!command.accelerator().IsShiftDown())
>       return false;
>     return (command.accelerator().key_code() >= ui::VKEY_0 &&
>             command.accelerator().key_code() <= ui::VKEY_9);
>   } else {
>     // Not a global command, check if Chrome shortcut and whether
>     // we can override it.
>     if (command.accelerator() ==
>         chrome::GetPrimaryChromeAcceleratorForCommandId(IDC_BOOKMARK_PAGE)) {
>       using extensions::SettingsOverrides;
>       using extensions::FeatureSwitch;
>       const SettingsOverrides* settings_overrides =
>           SettingsOverrides::Get(extension);
>       if (settings_overrides &&
>           SettingsOverrides::RemovesBookmarkShortcut(*settings_overrides) &&
>           (extensions::PermissionsData::HasAPIPermission(
>               extension,
>               extensions::APIPermission::kBookmarkManagerPrivate) ||
>            FeatureSwitch::enable_override_bookmarks_ui()->IsEnabled())) {
>         // If this check fails it either means we have an API to override a
>         // key that isn't a ChromeAccelerator (and the API can therefore be
>         // deprecated) or the IsChromeAccelerator isn't consistently
>         // returning true for all accelerators.
>         DCHECK(chrome::IsChromeAccelerator(command.accelerator(), profile));
>         return true;
>       }
>     }
>   
>     return !chrome::IsChromeAccelerator(command.accelerator(), profile);
>   }
> }

SGTM. This is clearer, and I believe implements the proper checking logic. I've
changed the interface to be a bit clearer and more explicit (IMO).

https://codereview.chromium.org/143493005/diff/760001/chrome/browser/extensio...
chrome/browser/extensions/api/commands/command_service.cc:398:
IsWhitelistedGlobalShortcut(iter->second)) ||
On 2014/02/12 12:51:14, Finnur wrote:
> This would turn into one simple if statement:
>   if (CanOverride(iter->second, extension, profile, true))
> ... and same applies to below, except false for last param (for both).

Done.

[Mike Wittman, 2014-02-12 22:15:30]

The Mac fix for IsChromeAccelerator is now committed in r250793. Can you approve
this CL, Finnur?

[Finnur, 2014-02-12 22:54:44]

LGTM, one nit and a comment.

https://codereview.chromium.org/143493005/diff/850001/chrome/browser/extensio...
File chrome/browser/extensions/api/commands/command_service.cc (right):

https://codereview.chromium.org/143493005/diff/850001/chrome/browser/extensio...
chrome/browser/extensions/api/commands/command_service.cc:106: const
ui::Accelerator& accelerator,
nit: I started with CanOverride but changed it because you aren't necessarily
overriding something (the shortcut might not be taken). It seemed like
CanAutoAssign was a better fit. CanAssign could also be used, if you find that
more palatable. I also thought command (accelerator in your case) should be the
first param because that's what is being assigned/overwritten (and not the
extension). I'll leave this up to you, though.

https://codereview.chromium.org/143493005/diff/850001/chrome/browser/extensio...
chrome/browser/extensions/api/commands/command_service.cc:424: const
extensions::Command command = iter->second;
Like this.

[Mike Wittman, 2014-02-12 23:57:03]

https://codereview.chromium.org/143493005/diff/850001/chrome/browser/extensio...
File chrome/browser/extensions/api/commands/command_service.cc (right):

https://codereview.chromium.org/143493005/diff/850001/chrome/browser/extensio...
chrome/browser/extensions/api/commands/command_service.cc:106: const
ui::Accelerator& accelerator,
On 2014/02/12 22:54:48, Finnur wrote:
> nit: I started with CanOverride but changed it because you aren't necessarily
> overriding something (the shortcut might not be taken). It seemed like
> CanAutoAssign was a better fit. CanAssign could also be used, if you find that
> more palatable. I also thought command (accelerator in your case) should be
the
> first param because that's what is being assigned/overwritten (and not the
> extension). I'll leave this up to you, though.

CanAutoAssign is fine with me, and I think your original parameter ordering
works best with that name.

[Mike Wittman, 2014-02-13 00:03:40]

sky: can I get an lgtm on c/b/ui?

[sky, 2014-02-13 01:31:31]

LGTM

[Mike Wittman, 2014-02-13 01:45:11]

The CQ bit was checked by wittman@chromium.org

[commit-bot: I haz the power, 2014-02-13 01:46:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wittman@chromium.org/143493005/1010001

[commit-bot: I haz the power, 2014-02-13 03:39:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-13 03:39:52]

Retried try job too often on mac_rel for step(s) app_list_unittests,
cacheinvalidation_unittests, cc_unittests, check_deps, check_deps2git,
chromedriver_unittests, components_unittests, content_browsertests,
content_unittests, crypto_unittests, google_apis_unittests, gpu_unittests,
ipc_tests, jingle_unittests, media_unittests, message_center_unittests,
nacl_integration, ppapi_unittests, printing_unittests, remoting_unittests,
sql_unittests, sync_integration_tests, sync_unit_tests,
telemetry_perf_unittests, telemetry_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[Mike Wittman, 2014-02-13 07:25:06]

The CQ bit was checked by wittman@chromium.org

[commit-bot: I haz the power, 2014-02-13 07:25:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wittman@chromium.org/143493005/1010001

[commit-bot: I haz the power, 2014-02-13 09:40:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-13 09:40:07]

Retried try job too often on mac_rel for step(s) interactive_ui_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[erikchen, 2014-02-13 19:54:53]

There are 2 reasons that the tests are failing on mac:

1. SendKeyPressSync() needs to be invoked with the parameter command=true, not
control=true.
2. The Accelerator operator==() override is incorrect in its current form. The
logic should be:
  a. If the native key_code/modifier/types match, return true.
  b. If the platform_accelerators match, return true.
  c. Otherwise, return false.

The current logic is:
  a. If either Accelerator has a platform accelerator, return whether the
platform accelerators match.
  b. Otherwise, return whether the native key_code/modifier/types match.

The Accelerator created by the extension_keybinding_registry is cross-platform,
and has no associated platform_accelerator.
The Accelerator for Bookmark This Page(cmd-d), when created on Mac, has a filled
out platform_accelerator, and as well as a key_code/modifier/types.
(Prior to my change in crrev.com/152643007, the Accelerator on Mac only had a
platform_accelerator, which made comparison between the two accelerators
impossible).

[Mike Wittman, 2014-02-13 20:23:49]

The CQ bit was checked by wittman@chromium.org

[commit-bot: I haz the power, 2014-02-13 20:24:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wittman@chromium.org/143493005/1480001

[commit-bot: I haz the power, 2014-02-13 20:52:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-13 20:52:45]

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

[Mike Wittman, 2014-02-13 22:17:02]

The CQ bit was checked by wittman@chromium.org

[commit-bot: I haz the power, 2014-02-13 22:17:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wittman@chromium.org/143493005/1480001

[commit-bot: I haz the power, 2014-02-13 22:36:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-13 22:36:35]

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

[Mike Wittman, 2014-02-13 23:24:17]

The CQ bit was checked by wittman@chromium.org

[commit-bot: I haz the power, 2014-02-13 23:27:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wittman@chromium.org/143493005/1480001

[commit-bot: I haz the power, 2014-02-14 03:03:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-14 03:03:54]

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

[erikchen, 2014-02-14 03:07:20]

The CQ bit was checked by erikchen@chromium.org

[commit-bot: I haz the power, 2014-02-14 03:10:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wittman@chromium.org/143493005/1480001

[commit-bot: I haz the power, 2014-02-14 06:31:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-14 06:31:11]

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

[Finnur, 2014-02-14 10:53:40]

https://codereview.chromium.org/143493005/diff/1480001/chrome/browser/extensi...
File chrome/browser/extensions/extension_keybinding_apitest.cc (right):

https://codereview.chromium.org/143493005/diff/1480001/chrome/browser/extensi...
chrome/browser/extensions/extension_keybinding_apitest.cc:212: browser(),
ui::VKEY_D, true, false, false, false));
This also needs the if-def, otherwise this negative test will not test anything
on Mac.

[Mike Wittman, 2014-02-14 17:03:52]

https://codereview.chromium.org/143493005/diff/1480001/chrome/browser/extensi...
File chrome/browser/extensions/extension_keybinding_apitest.cc (right):

https://codereview.chromium.org/143493005/diff/1480001/chrome/browser/extensi...
chrome/browser/extensions/extension_keybinding_apitest.cc:212: browser(),
ui::VKEY_D, true, false, false, false));
On 2014/02/14 10:53:41, Finnur wrote:
> This also needs the if-def, otherwise this negative test will not test
anything
> on Mac.

Done.

[Mike Wittman, 2014-02-14 18:06:18]

Committed patchset #11 manually as r251368 (presubmit successful).