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

mac: 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.

This is the Mac version of the fix. Views was fixed at:
https://codereview.chromium.org/360423002.

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

[erikchen, 2014-07-14 23:00:19]

wittman: Please review.

I tried to write this CL in a way that was most compatible with the stated
definition of ui::AcceleratorManager::HandlerPriority.

The new code now:
1. Attempts to process high priority extension accelerators.
2. Gives web content a chance to process accelerator.
3. Attempts to process normal priority extension accelerators.

[Mike Wittman, 2014-07-15 00:20:38]

Thanks Erik!

https://codereview.chromium.org/388313002/diff/120001/chrome/test/data/extens...
File
chrome/test/data/extensions/api_test/keybinding/overwrite_bookmark_shortcut/manifest.json
(right):

https://codereview.chromium.org/388313002/diff/120001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/keybinding/overwrite_bookmark_shortcut/manifest.json:18:
"mac": "Command+D"
Is this necessary? The docs say that Ctrl is automatically converted to Command
on Mac: https://developer.chrome.com/apps/commands

[erikchen, 2014-07-15 02:41:13]

wittman: PTAL

https://codereview.chromium.org/388313002/diff/120001/chrome/test/data/extens...
File
chrome/test/data/extensions/api_test/keybinding/overwrite_bookmark_shortcut/manifest.json
(right):

https://codereview.chromium.org/388313002/diff/120001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/keybinding/overwrite_bookmark_shortcut/manifest.json:18:
"mac": "Command+D"
On 2014/07/15 00:20:38, Mike Wittman wrote:
> Is this necessary? The docs say that Ctrl is automatically converted to
Command
> on Mac: https://developer.chrome.com/apps/commands

Nope. removed.

[Mike Wittman, 2014-07-15 02:55:39]

lgtm

[erikchen, 2014-07-15 17:11:23]

yoz: Looking for OWNER review of chrome/browser/extensions/
shess: Looking for OWNER review of chrome/browser/ui/cocoa/

[Scott Hess - ex-Googler, 2014-07-15 19:20:59]

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

https://codereview.chromium.org/388313002/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/extension_keybinding_apitest.cc:357: #endif  //
defined(OS_MACOSX)
AFAICT, these only differ on "Command+D" vs "Ctrl+D".  Maybe better to have the
#ifdef around a local constant with a single function call.  [Mostly my
complaint is around having the two bits of code look very different.]

https://codereview.chromium.org/388313002/diff/140001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/apps/native_app_window_cocoa.h (right):

https://codereview.chromium.org/388313002/diff/140001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/apps/native_app_window_cocoa.h:46:
(ui::AcceleratorManager::HandlerPriority)priority;
Rather than split the line I'd just relax the colons-must-line-up rule.  The
point of lining them up is to make it easy to read down the parameter list, but
in this case that isn't going to happen no matter what you do.

https://codereview.chromium.org/388313002/diff/140001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/apps/native_app_window_cocoa.mm (right):

https://codereview.chromium.org/388313002/diff/140001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/apps/native_app_window_cocoa.mm:196:
(ui::AcceleratorManager::HandlerPriority)priority {
Here too.

https://codereview.chromium.org/388313002/diff/140001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/browser_window_controller.h (right):

https://codereview.chromium.org/388313002/diff/140001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/browser_window_controller.h:315:
(ui::AcceleratorManager::HandlerPriority)priority;
And here.

https://codereview.chromium.org/388313002/diff/140001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/browser_window_controller.mm (right):

https://codereview.chromium.org/388313002/diff/140001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/browser_window_controller.mm:1231:
(ui::AcceleratorManager::HandlerPriority)priority {
Sigh.

[Yoyo Zhou, 2014-07-15 19:46:38]

extensions LGTM

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

https://codereview.chromium.org/388313002/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/extension_keybinding_apitest.cc:357: #endif  //
defined(OS_MACOSX)
On 2014/07/15 19:20:59, Scott Hess wrote:
> AFAICT, these only differ on "Command+D" vs "Ctrl+D".  Maybe better to have
the
> #ifdef around a local constant with a single function call.  [Mostly my
> complaint is around having the two bits of code look very different.]

Agreed.

https://codereview.chromium.org/388313002/diff/140001/chrome/test/data/extens...
File chrome/test/data/extensions/test_file_with_ctrl-d_keybinding.html (right):

https://codereview.chromium.org/388313002/diff/140001/chrome/test/data/extens...
chrome/test/data/extensions/test_file_with_ctrl-d_keybinding.html:5: if
(e.keyCode == 17 || e.keyCode == 91 || e.keyCode == 93)
It would be nice to comment on what these values mean.

[erikchen, 2014-07-15 20:48:40]

shess: PTAL

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

https://codereview.chromium.org/388313002/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/extension_keybinding_apitest.cc:357: #endif  //
defined(OS_MACOSX)
On 2014/07/15 19:46:37, Yoyo Zhou wrote:
> On 2014/07/15 19:20:59, Scott Hess wrote:
> > AFAICT, these only differ on "Command+D" vs "Ctrl+D".  Maybe better to have
> the
> > #ifdef around a local constant with a single function call.  [Mostly my
> > complaint is around having the two bits of code look very different.]
> 
> Agreed.

Done.

https://codereview.chromium.org/388313002/diff/140001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/apps/native_app_window_cocoa.h (right):

https://codereview.chromium.org/388313002/diff/140001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/apps/native_app_window_cocoa.h:46:
(ui::AcceleratorManager::HandlerPriority)priority;
On 2014/07/15 19:20:59, Scott Hess wrote:
> Rather than split the line I'd just relax the colons-must-line-up rule.  The
> point of lining them up is to make it easy to read down the parameter list,
but
> in this case that isn't going to happen no matter what you do.

I dislike formatting that conflicts with both Chromium style guides and
clang-format. That being said, you seem to care more about this than I do so I
went ahead and made your suggested change.

https://codereview.chromium.org/388313002/diff/140001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/apps/native_app_window_cocoa.mm (right):

https://codereview.chromium.org/388313002/diff/140001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/apps/native_app_window_cocoa.mm:196:
(ui::AcceleratorManager::HandlerPriority)priority {
On 2014/07/15 19:20:59, Scott Hess wrote:
> Here too.

Done.

https://codereview.chromium.org/388313002/diff/140001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/browser_window_controller.h (right):

https://codereview.chromium.org/388313002/diff/140001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/browser_window_controller.h:315:
(ui::AcceleratorManager::HandlerPriority)priority;
On 2014/07/15 19:20:59, Scott Hess wrote:
> And here.

Done.

https://codereview.chromium.org/388313002/diff/140001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/browser_window_controller.mm (right):

https://codereview.chromium.org/388313002/diff/140001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/browser_window_controller.mm:1231:
(ui::AcceleratorManager::HandlerPriority)priority {
On 2014/07/15 19:20:59, Scott Hess wrote:
> Sigh.

Done.

https://codereview.chromium.org/388313002/diff/140001/chrome/test/data/extens...
File chrome/test/data/extensions/test_file_with_ctrl-d_keybinding.html (right):

https://codereview.chromium.org/388313002/diff/140001/chrome/test/data/extens...
chrome/test/data/extensions/test_file_with_ctrl-d_keybinding.html:5: if
(e.keyCode == 17 || e.keyCode == 91 || e.keyCode == 93)
On 2014/07/15 19:46:37, Yoyo Zhou wrote:
> It would be nice to comment on what these values mean.

Done.

[Scott Hess - ex-Googler, 2014-07-15 21:30:44]

lgtm

https://codereview.chromium.org/388313002/diff/140001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/apps/native_app_window_cocoa.h (right):

https://codereview.chromium.org/388313002/diff/140001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/apps/native_app_window_cocoa.h:46:
(ui::AcceleratorManager::HandlerPriority)priority;
On 2014/07/15 20:48:40, erikchen1 wrote:
> On 2014/07/15 19:20:59, Scott Hess wrote:
> > Rather than split the line I'd just relax the colons-must-line-up rule.  The
> > point of lining them up is to make it easy to read down the parameter list,
> but
> > in this case that isn't going to happen no matter what you do.
> 
> I dislike formatting that conflicts with both Chromium style guides and
> clang-format. That being said, you seem to care more about this than I do so I
> went ahead and made your suggested change.

Yeah, I'll probably pay for my opinion.  "clang-format does it this way" would
have been sufficient defense, since there is not a possibility that this will be
formatted cleanly, so it's just choosing which thing to be annoyed by.  Thanks
ObjC!

[erikchen, 2014-07-15 21:31:31]

The CQ bit was checked by erikchen@chromium.org

[commit-bot: I haz the power, 2014-07-15 21:35:31]

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

[commit-bot: I haz the power, 2014-07-16 05:48:28]

Change committed as 283366