Allow WebContents key handling to supplant extension overrides of the bookmark shortcut

Allow WebContents key handling to supplant extension overrides of the bookmark shortcut

Use the same prioritization for accelerator processing for the bookmark
shortcut when overridden by an extension as for when it is built-in to
the browser. Namely, allow WebContents key processing to take place
before extension accelerator processing.

Two changes are required:

1. Register the keybinding with normal priority rather than high
priority, so that FocusManager::OnKeyEvent does not process the
accelerator before the WebContents.

2. Don't process the accelerator for the extension in the
AcceleratorTarget subclasses. Instead, use the existing browser command
processing in BookmarkCurrentPage which delegates to the extension. This
is necessary so that the extension accelerator processing does not get
invoked while command execution is otherwise blocked in
BrowserView::PreHandleKeyboardEvent.

This change is for Views. Mac will be addressed in a separate CL.

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

[Mike Wittman, 2014-07-01 22:30:43]

Finnur, please take a look.

[Finnur, 2014-07-02 11:56:16]

https://codereview.chromium.org/360423002/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/extension_keybinding_registry.h (right):

https://codereview.chromium.org/360423002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/extension_keybinding_registry.h:124:
content::BrowserContext* browser_context() const { return browser_context_; }
nit: doc.

https://codereview.chromium.org/360423002/diff/20001/chrome/browser/ui/views/...
File chrome/browser/ui/views/extensions/extension_keybinding_registry_views.cc
(right):

https://codereview.chromium.org/360423002/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/extensions/extension_keybinding_registry_views.cc:70: }
I'd rather have a utility function on the Command object (see similar function
IsMediaKey) that takes an accelerator and returns its priority. As in:

ui::AcceleratorManager::HandlerPriority
Command::GetPriority(const ui::Accelerator& accelerator,
                     const extensions::Extension* extension) {
  // Extensions overriding the bookmark shortcut need normal priority to
  // preserve the built-in processing order of the key and not override
  // WebContents key handling.
  if (accelerator ==
chrome::GetPrimaryChromeAcceleratorForCommandId(IDC_BOOKMARK_PAGE) &&
      extensions::UIOverrides::RemovesBookmarkShortcut(extension))
    return ui::AcceleratorManager::kNormalPriority;

  return ui::AcceleratorManager::kHighPriority;
}

Also note there's no braces on single line if and else clauses (a multiline
conditional does not imply braces for if/else -- a common misunderstanding of
the style guide).

https://codereview.chromium.org/360423002/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/extensions/extension_keybinding_registry_views.cc:72:
priority,
Then this becomes:
  Command::GetPriority(iter->second.accelerator(), extension)

https://codereview.chromium.org/360423002/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/extensions/extension_keybinding_registry_views.cc:104: }
and this becomes:
if (Command::GetPriority(accelerator, extension) ==
ui::AcceleratorManager::kNormalPriority)
  return false;

https://codereview.chromium.org/360423002/diff/20001/chrome/browser/ui/views/...
File chrome/browser/ui/views/location_bar/page_action_image_view.cc (right):

https://codereview.chromium.org/360423002/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/location_bar/page_action_image_view.cc:81: priority,
Same here, and below. Lots of opportunities to simplify.

https://codereview.chromium.org/360423002/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/location_bar/page_action_image_view.cc:200:
enabled_extensions().GetByID(page_action()->extension_id());
Actually, I do wonder if we should have two Command::GetPriority functions, one
that accepts an extension_id and the other an extension pointer (both would
accept accelerator and the first could call the latter after looking up the
extension pointer). I think that would potentially simplify even more of this
patch.

[Mike Wittman, 2014-07-02 21:56:20]

https://codereview.chromium.org/360423002/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/extension_keybinding_registry.h (right):

https://codereview.chromium.org/360423002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/extension_keybinding_registry.h:124:
content::BrowserContext* browser_context() const { return browser_context_; }
On 2014/07/02 11:56:16, Finnur wrote:
> nit: doc.

Done.

https://codereview.chromium.org/360423002/diff/20001/chrome/browser/ui/views/...
File chrome/browser/ui/views/extensions/extension_keybinding_registry_views.cc
(right):

https://codereview.chromium.org/360423002/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/extensions/extension_keybinding_registry_views.cc:70: }
On 2014/07/02 11:56:16, Finnur wrote:
> I'd rather have a utility function on the Command object (see similar function
> IsMediaKey) that takes an accelerator and returns its priority. As in:
> 
> ui::AcceleratorManager::HandlerPriority
> Command::GetPriority(const ui::Accelerator& accelerator,
>                      const extensions::Extension* extension) {
>   // Extensions overriding the bookmark shortcut need normal priority to
>   // preserve the built-in processing order of the key and not override
>   // WebContents key handling.
>   if (accelerator ==
> chrome::GetPrimaryChromeAcceleratorForCommandId(IDC_BOOKMARK_PAGE) &&
>       extensions::UIOverrides::RemovesBookmarkShortcut(extension))
>     return ui::AcceleratorManager::kNormalPriority;
> 
>   return ui::AcceleratorManager::kHighPriority;
> }
> 
> Also note there's no braces on single line if and else clauses (a multiline
> conditional does not imply braces for if/else -- a common misunderstanding of
> the style guide).

We can do the utility function, but putting it in Command violates dependency
ordering -- it needs to include from c/apps, c/b/ui, and extension/browser. I
put it in c/b/ui/extensions which seemed reasonable, but suggestions for other
locations welcome.

https://codereview.chromium.org/360423002/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/extensions/extension_keybinding_registry_views.cc:72:
priority,
On 2014/07/02 11:56:16, Finnur wrote:
> Then this becomes:
>   Command::GetPriority(iter->second.accelerator(), extension)

Done.

https://codereview.chromium.org/360423002/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/extensions/extension_keybinding_registry_views.cc:104: }
On 2014/07/02 11:56:16, Finnur wrote:
> and this becomes:
> if (Command::GetPriority(accelerator, extension) ==
> ui::AcceleratorManager::kNormalPriority)
>   return false;

Done.

https://codereview.chromium.org/360423002/diff/20001/chrome/browser/ui/views/...
File chrome/browser/ui/views/location_bar/page_action_image_view.cc (right):

https://codereview.chromium.org/360423002/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/location_bar/page_action_image_view.cc:81: priority,
On 2014/07/02 11:56:16, Finnur wrote:
> Same here, and below. Lots of opportunities to simplify.

Done.

https://codereview.chromium.org/360423002/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/location_bar/page_action_image_view.cc:200:
enabled_extensions().GetByID(page_action()->extension_id());
On 2014/07/02 11:56:16, Finnur wrote:
> Actually, I do wonder if we should have two Command::GetPriority functions,
one
> that accepts an extension_id and the other an extension pointer (both would
> accept accelerator and the first could call the latter after looking up the
> extension pointer). I think that would potentially simplify even more of this
> patch.

Done.

[Finnur, 2014-07-03 11:09:59]

LGTM (with nits), as long as I'm understanding things correctly (see question
below).

https://codereview.chromium.org/360423002/diff/20001/chrome/browser/ui/views/...
File chrome/browser/ui/views/extensions/extension_keybinding_registry_views.cc
(right):

https://codereview.chromium.org/360423002/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/extensions/extension_keybinding_registry_views.cc:70: }
Ah, I see. That's probably fine then.

https://codereview.chromium.org/360423002/diff/60001/chrome/browser/extension...
File chrome/browser/extensions/extension_keybinding_apitest.cc (right):

https://codereview.chromium.org/360423002/diff/60001/chrome/browser/extension...
chrome/browser/extensions/extension_keybinding_apitest.cc:351:
ASSERT_TRUE(RunExtensionTest("keybinding/basics"))
You should probably set the --enable-override-bookmarks-ui flag here, to make
sure that the absence of that flag isn't causing this test to pass.

https://codereview.chromium.org/360423002/diff/60001/chrome/browser/ui/extens...
File chrome/browser/ui/extensions/accelerator_priority.cc (right):

https://codereview.chromium.org/360423002/diff/60001/chrome/browser/ui/extens...
chrome/browser/ui/extensions/accelerator_priority.cc:22: return
ui::AcceleratorManager::kNormalPriority;
Nit: single line, no braces.

https://codereview.chromium.org/360423002/diff/60001/chrome/browser/ui/extens...
chrome/browser/ui/extensions/accelerator_priority.cc:23: }
Just to be sure this is working as I expect it:

When a user sets Ctrl+D to a non-bookmark-related extension, it will always be
high priority. 

The only possible weirdness would be if a bookmark-overriding extension
*doesn't* override Ctrl+D, but the user opts to set Ctrl+D to something *else*
in that extension. But that's pretty contrived example... and the only downside
is that the shortcut isn't guaranteed to be high-priority, so the web page gets
first shot at it.

Right?

https://codereview.chromium.org/360423002/diff/60001/chrome/browser/ui/extens...
File chrome/browser/ui/extensions/accelerator_priority.h (right):

https://codereview.chromium.org/360423002/diff/60001/chrome/browser/ui/extens...
chrome/browser/ui/extensions/accelerator_priority.h:32:
ui::AcceleratorManager::HandlerPriority GetAcceleratorPriority(
nit: Functions which only differ by signatures (parameters) are only allowed as
exceptions (like when the difference is char vs string16). Suggest renaming this
GetAcceleratorPriorityById.

[Mike Wittman, 2014-07-07 18:43:58]

https://codereview.chromium.org/360423002/diff/60001/chrome/browser/extension...
File chrome/browser/extensions/extension_keybinding_apitest.cc (right):

https://codereview.chromium.org/360423002/diff/60001/chrome/browser/extension...
chrome/browser/extensions/extension_keybinding_apitest.cc:351:
ASSERT_TRUE(RunExtensionTest("keybinding/basics"))
On 2014/07/03 11:09:59, Finnur (OOO - back Jul 14th) wrote:
> You should probably set the --enable-override-bookmarks-ui flag here, to make
> sure that the absence of that flag isn't causing this test to pass.

Done.

https://codereview.chromium.org/360423002/diff/60001/chrome/browser/ui/extens...
File chrome/browser/ui/extensions/accelerator_priority.cc (right):

https://codereview.chromium.org/360423002/diff/60001/chrome/browser/ui/extens...
chrome/browser/ui/extensions/accelerator_priority.cc:22: return
ui::AcceleratorManager::kNormalPriority;
On 2014/07/03 11:09:59, Finnur (OOO - back Jul 14th) wrote:
> Nit: single line, no braces.

Done.

https://codereview.chromium.org/360423002/diff/60001/chrome/browser/ui/extens...
chrome/browser/ui/extensions/accelerator_priority.cc:23: }
On 2014/07/03 11:09:59, Finnur (OOO - back Jul 14th) wrote:
> Just to be sure this is working as I expect it:
> 
> When a user sets Ctrl+D to a non-bookmark-related extension, it will always be
> high priority. 
> 
> The only possible weirdness would be if a bookmark-overriding extension
> *doesn't* override Ctrl+D, but the user opts to set Ctrl+D to something *else*
> in that extension. But that's pretty contrived example... and the only
downside
> is that the shortcut isn't guaranteed to be high-priority, so the web page
gets
> first shot at it.
> 
> Right?

Yes, that's correct.

https://codereview.chromium.org/360423002/diff/60001/chrome/browser/ui/extens...
File chrome/browser/ui/extensions/accelerator_priority.h (right):

https://codereview.chromium.org/360423002/diff/60001/chrome/browser/ui/extens...
chrome/browser/ui/extensions/accelerator_priority.h:32:
ui::AcceleratorManager::HandlerPriority GetAcceleratorPriority(
On 2014/07/03 11:09:59, Finnur (OOO - back Jul 14th) wrote:
> nit: Functions which only differ by signatures (parameters) are only allowed
as
> exceptions (like when the difference is char vs string16). Suggest renaming
this
> GetAcceleratorPriorityById.

Done.

[Mike Wittman, 2014-07-07 18:45:13]

Peter, can you do OWNERS review of c/b/ui?

[Peter Kasting, 2014-07-07 23:47:11]

https://codereview.chromium.org/360423002/diff/80001/chrome/browser/ui/extens...
File chrome/browser/ui/extensions/accelerator_priority.cc (right):

https://codereview.chromium.org/360423002/diff/80001/chrome/browser/ui/extens...
chrome/browser/ui/extensions/accelerator_priority.cc:24: return
ui::AcceleratorManager::kHighPriority;
It seems surprising to me that IDC_BOOKMARK_PAGE is the only shortcut we want to
special-case here.  What about other extension-overridable shortcuts that we
normally allow web pages to handle?  Shouldn't we also be returning
kNormalPriority for them?  I'd expect that we'd want "normal priority" in
general, rather than high.

https://codereview.chromium.org/360423002/diff/80001/chrome/browser/ui/views/...
File chrome/browser/ui/views/extensions/extension_keybinding_registry_views.cc
(right):

https://codereview.chromium.org/360423002/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/extensions/extension_keybinding_registry_views.cc:57: if
(!IsAcceleratorRegistered(iter->second.accelerator())) {
Nit: Shorter:

    const ui::Accelerator& accelerator = iter->second.accelerator();
    if (!IsAcceleratorRegistered(accelerator)) {
      focus_manager_->RegisterAccelerator(
          accelerator, GetAcceleratorPriority(accelerator, extension), this);
    }

    AddEventTarget(accelerator, extension->id(), iter->second.command_name());

https://codereview.chromium.org/360423002/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/extensions/extension_keybinding_registry_views.cc:82: //
processing to be effective.
Nit: What does "to be effective" mean here?  This phrase seems somewhat
ambiguous to a reader; I suggest finding a clearer formulation.  (3 different
files)

https://codereview.chromium.org/360423002/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/extensions/extension_keybinding_registry_views.cc:83: if
(GetAcceleratorPriorityById(accelerator,
Nit: Shorter:

  const ui::AcceleratorManager::HandlerPriority priority =
      GetAcceleratorPriorityById(accelerator, extension_id, browser_context());
  return (priority == ui::AcceleratorManager::kHighPriority) &&
      ExtensionKeybindingRegistry::NotifyEventTargets(accelerator);

https://codereview.chromium.org/360423002/diff/80001/chrome/browser/ui/views/...
File chrome/browser/ui/views/location_bar/page_action_image_view.cc (right):

https://codereview.chromium.org/360423002/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/location_bar/page_action_image_view.cc:183: if
(GetAcceleratorPriorityById(accelerator,
Nit: Shorter:

  const ui::AcceleratorManager::HandlerPriority priority =
      GetAcceleratorPriorityById(accelerator, extension_id, browser_context());
  if (priority == ui::AcceleratorManager::kHighPriority)
    ExecuteAction(ExtensionPopup::SHOW);
  return priority == ui::AcceleratorManager::kHighPriority;

[Finnur, 2014-07-08 10:35:03]

https://codereview.chromium.org/360423002/diff/80001/chrome/browser/ui/extens...
File chrome/browser/ui/extensions/accelerator_priority.cc (right):

https://codereview.chromium.org/360423002/diff/80001/chrome/browser/ui/extens...
chrome/browser/ui/extensions/accelerator_priority.cc:24: return
ui::AcceleratorManager::kHighPriority;
The intent here is to allow extensions to override the bookmarking system as a
whole, and the shortcut Ctrl+D as part of that, while at the same time
preserving how the bookmarking shortcut works now (not break Ctrl+D handling in
web pages just because the bookmarking system is implemented by an extension).
This is currently the only shortcut that we allow an extension to steal (take
away even if already in use)-- and only if the extension requests the privilege
to override the bookmarking system.

This is not the same as a user assigning Ctrl+D to an extension action. That
shortcut would have high priority. I can (when not on vacation) :) look up why
we elected to use High priority for all extension shortcuts. We can always
revisit that decision, but it would be beyond the scope of this CL.

[Mike Wittman, 2014-07-08 19:29:01]

https://codereview.chromium.org/360423002/diff/80001/chrome/browser/ui/views/...
File chrome/browser/ui/views/extensions/extension_keybinding_registry_views.cc
(right):

https://codereview.chromium.org/360423002/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/extensions/extension_keybinding_registry_views.cc:57: if
(!IsAcceleratorRegistered(iter->second.accelerator())) {
On 2014/07/07 23:47:10, Peter Kasting wrote:
> Nit: Shorter:
> 
>     const ui::Accelerator& accelerator = iter->second.accelerator();
>     if (!IsAcceleratorRegistered(accelerator)) {
>       focus_manager_->RegisterAccelerator(
>           accelerator, GetAcceleratorPriority(accelerator, extension), this);
>     }
> 
>     AddEventTarget(accelerator, extension->id(), iter->second.command_name());

Done.

https://codereview.chromium.org/360423002/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/extensions/extension_keybinding_registry_views.cc:82: //
processing to be effective.
On 2014/07/07 23:47:10, Peter Kasting wrote:
> Nit: What does "to be effective" mean here?  This phrase seems somewhat
> ambiguous to a reader; I suggest finding a clearer formulation.  (3 different
> files)

Done.

https://codereview.chromium.org/360423002/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/extensions/extension_keybinding_registry_views.cc:83: if
(GetAcceleratorPriorityById(accelerator,
On 2014/07/07 23:47:10, Peter Kasting wrote:
> Nit: Shorter:
> 
>   const ui::AcceleratorManager::HandlerPriority priority =
>       GetAcceleratorPriorityById(accelerator, extension_id,
browser_context());
>   return (priority == ui::AcceleratorManager::kHighPriority) &&
>       ExtensionKeybindingRegistry::NotifyEventTargets(accelerator);

Done.

https://codereview.chromium.org/360423002/diff/80001/chrome/browser/ui/views/...
File chrome/browser/ui/views/location_bar/page_action_image_view.cc (right):

https://codereview.chromium.org/360423002/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/location_bar/page_action_image_view.cc:183: if
(GetAcceleratorPriorityById(accelerator,
On 2014/07/07 23:47:10, Peter Kasting wrote:
> Nit: Shorter:
> 
>   const ui::AcceleratorManager::HandlerPriority priority =
>       GetAcceleratorPriorityById(accelerator, extension_id,
browser_context());
>   if (priority == ui::AcceleratorManager::kHighPriority)
>     ExecuteAction(ExtensionPopup::SHOW);
>   return priority == ui::AcceleratorManager::kHighPriority;

Done, although requires an extra line for extension_id.

[Peter Kasting, 2014-07-08 19:30:36]

LGTM

https://codereview.chromium.org/360423002/diff/80001/chrome/browser/ui/extens...
File chrome/browser/ui/extensions/accelerator_priority.cc (right):

https://codereview.chromium.org/360423002/diff/80001/chrome/browser/ui/extens...
chrome/browser/ui/extensions/accelerator_priority.cc:24: return
ui::AcceleratorManager::kHighPriority;
On 2014/07/08 10:35:02, Finnur (OOO - back Jul 14th) wrote:
> The intent here is to allow extensions to override the bookmarking system as a
> whole, and the shortcut Ctrl+D as part of that, while at the same time
> preserving how the bookmarking shortcut works now (not break Ctrl+D handling
in
> web pages just because the bookmarking system is implemented by an extension).
> This is currently the only shortcut that we allow an extension to steal (take
> away even if already in use)-- and only if the extension requests the
privilege
> to override the bookmarking system.
> 
> This is not the same as a user assigning Ctrl+D to an extension action. That
> shortcut would have high priority. I can (when not on vacation) :) look up why
> we elected to use High priority for all extension shortcuts. We can always
> revisit that decision, but it would be beyond the scope of this CL.

Cool.  This is helpful info.  Consider putting some or all of it into comments
here and/or in the header.

[Mike Wittman, 2014-07-08 22:15:21]

https://codereview.chromium.org/360423002/diff/80001/chrome/browser/ui/extens...
File chrome/browser/ui/extensions/accelerator_priority.cc (right):

https://codereview.chromium.org/360423002/diff/80001/chrome/browser/ui/extens...
chrome/browser/ui/extensions/accelerator_priority.cc:24: return
ui::AcceleratorManager::kHighPriority;
On 2014/07/08 19:30:36, Peter Kasting wrote:
> On 2014/07/08 10:35:02, Finnur (OOO - back Jul 14th) wrote:
> > The intent here is to allow extensions to override the bookmarking system as
a
> > whole, and the shortcut Ctrl+D as part of that, while at the same time
> > preserving how the bookmarking shortcut works now (not break Ctrl+D handling
> in
> > web pages just because the bookmarking system is implemented by an
extension).
> > This is currently the only shortcut that we allow an extension to steal
(take
> > away even if already in use)-- and only if the extension requests the
> privilege
> > to override the bookmarking system.
> > 
> > This is not the same as a user assigning Ctrl+D to an extension action. That
> > shortcut would have high priority. I can (when not on vacation) :) look up
why
> > we elected to use High priority for all extension shortcuts. We can always
> > revisit that decision, but it would be beyond the scope of this CL.
> 
> Cool.  This is helpful info.  Consider putting some or all of it into comments
> here and/or in the header.

Added a comment in the header.

[Mike Wittman, 2014-07-08 22:15:38]

The CQ bit was checked by wittman@chromium.org

[commit-bot: I haz the power, 2014-07-08 22:17:50]

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

[Mike Wittman, 2014-07-08 22:30:21]

The CQ bit was checked by wittman@chromium.org

[commit-bot: I haz the power, 2014-07-08 22:31:33]

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

[commit-bot: I haz the power, 2014-07-09 05:13:27]

Change committed as 281946