MacViews: Fix duplicate definition of ExtensionKeyBindingRegistry::SetShortcutHandlingSuspended

MacViews: Fix duplicate definition of ExtensionKeyBindingRegistry::SetShortcutHandlingSuspended

ExtensionKeyBindingRegistry::SetShortcutHandlingSuspended currently has 2
definitions (Cocoa and Views).
Consolidate them into one definition that just sets a flag.
Now we can't easily tell FocusManager that shortcuts are disabled, so we
make ExtensionKeyBindingRegistryViews install a pre-target handler on the
window and consume accelerator events while recording extension shortcuts.

BUG=425229
Committed: https://crrev.com/de66e360e2a2944bdf71608726b19b3bf2a2a6ff
Cr-Commit-Position: refs/heads/master@{#312220}

[Andre, 2015-01-08 19:56:45]

andresantoso@chromium.org changed reviewers:
+ sky@chromium.org

[Andre, 2015-01-08 19:56:46]

sky@ please review.

[sky, 2015-01-08 23:39:24]

sky@chromium.org changed reviewers:
+ rdevlin.cronin@chromium.org

[sky, 2015-01-08 23:39:26]

+rdevlin for extension changes.

https://codereview.chromium.org/838253004/diff/1/ash/accelerators/focus_manag...
File ash/accelerators/focus_manager_factory.h (right):

https://codereview.chromium.org/838253004/diff/1/ash/accelerators/focus_manag...
ash/accelerators/focus_manager_factory.h:17: class AshFocusManagerFactory :
public views::FocusManagerFactory,
Having a class called FocusManagerFactory implement AcceleratorProcess is very
misleading. Can you follow a similar pattern to old and have the
AcceleratorProcess be implemented on a separate object that is handed to the
focus manager?

https://codereview.chromium.org/838253004/diff/1/chrome/browser/ui/views/fram...
File chrome/browser/ui/views/frame/browser_view.cc (right):

https://codereview.chromium.org/838253004/diff/1/chrome/browser/ui/views/fram...
chrome/browser/ui/views/frame/browser_view.cc:1379: if
(extensions::ExtensionKeybindingRegistry::IsShortcutHandlingSuspended())
Could this be implemented by as a PreTargetHandler on env? My general desire is
to avoid the dependency from BrowserView onto the extension classes.

https://codereview.chromium.org/838253004/diff/1/ui/base/accelerators/acceler...
File ui/base/accelerators/accelerator.h (right):

https://codereview.chromium.org/838253004/diff/1/ui/base/accelerators/acceler...
ui/base/accelerators/accelerator.h:121: class UI_BASE_EXPORT
AcceleratorProcessor {
Is there a reason this isn't in its own header?

https://codereview.chromium.org/838253004/diff/1/ui/base/accelerators/acceler...
ui/base/accelerators/accelerator.h:132: // keyboard accelerator, or NULL if no
view is registered for that keyboard
view->target

https://codereview.chromium.org/838253004/diff/1/ui/base/accelerators/acceler...
File ui/base/accelerators/accelerator_manager.h (right):

https://codereview.chromium.org/838253004/diff/1/ui/base/accelerators/acceler...
ui/base/accelerators/accelerator_manager.h:1: 
fix this.

https://codereview.chromium.org/838253004/diff/1/ui/views/focus/focus_manager.cc
File ui/views/focus/focus_manager.cc (right):

https://codereview.chromium.org/838253004/diff/1/ui/views/focus/focus_manager...
ui/views/focus/focus_manager.cc:563: break;
nit: make this return target and move 559 into for loop.

https://codereview.chromium.org/838253004/diff/1/ui/views/focus/focus_manager.h
File ui/views/focus/focus_manager.h (right):

https://codereview.chromium.org/838253004/diff/1/ui/views/focus/focus_manager...
ui/views/focus/focus_manager.h:150: FocusManager(Widget* widget);
explicit

https://codereview.chromium.org/838253004/diff/1/ui/views/focus/focus_manager...
ui/views/focus/focus_manager.h:271: // Adds/removes an accelerator processor.
How about test coverage of some of this new functionality?

https://codereview.chromium.org/838253004/diff/1/ui/views/focus/focus_manager...
ui/views/focus/focus_manager.h:275: void
AddAcceleratorPreProcessor(ui::AcceleratorProcessor*);
style guide says you should have names here.

[Andre, 2015-01-09 00:06:15]

+cc finnur

https://codereview.chromium.org/838253004/diff/1/ash/accelerators/focus_manag...
File ash/accelerators/focus_manager_factory.h (right):

https://codereview.chromium.org/838253004/diff/1/ash/accelerators/focus_manag...
ash/accelerators/focus_manager_factory.h:17: class AshFocusManagerFactory :
public views::FocusManagerFactory,
On 2015/01/08 23:39:25, sky wrote:
> Having a class called FocusManagerFactory implement AcceleratorProcess is very
> misleading. Can you follow a similar pattern to old and have the
> AcceleratorProcess be implemented on a separate object that is handed to the
> focus manager?

Done.
The asan builder was complaining that the object leaked, since the Delegate
object is never released
in the old pattern.

https://codereview.chromium.org/838253004/diff/1/ui/base/accelerators/acceler...
File ui/base/accelerators/accelerator.h (right):

https://codereview.chromium.org/838253004/diff/1/ui/base/accelerators/acceler...
ui/base/accelerators/accelerator.h:121: class UI_BASE_EXPORT
AcceleratorProcessor {
On 2015/01/08 23:39:26, sky wrote:
> Is there a reason this isn't in its own header?

Done, moved to its own header.

https://codereview.chromium.org/838253004/diff/1/ui/base/accelerators/acceler...
ui/base/accelerators/accelerator.h:132: // keyboard accelerator, or NULL if no
view is registered for that keyboard
On 2015/01/08 23:39:26, sky wrote:
> view->target

Done.

https://codereview.chromium.org/838253004/diff/1/ui/base/accelerators/acceler...
File ui/base/accelerators/accelerator_manager.h (right):

https://codereview.chromium.org/838253004/diff/1/ui/base/accelerators/acceler...
ui/base/accelerators/accelerator_manager.h:1: 
On 2015/01/08 23:39:26, sky wrote:
> fix this.

Done.

https://codereview.chromium.org/838253004/diff/1/ui/views/focus/focus_manager.cc
File ui/views/focus/focus_manager.cc (right):

https://codereview.chromium.org/838253004/diff/1/ui/views/focus/focus_manager...
ui/views/focus/focus_manager.cc:563: break;
On 2015/01/08 23:39:26, sky wrote:
> nit: make this return target and move 559 into for loop.

Done.

https://codereview.chromium.org/838253004/diff/1/ui/views/focus/focus_manager.h
File ui/views/focus/focus_manager.h (right):

https://codereview.chromium.org/838253004/diff/1/ui/views/focus/focus_manager...
ui/views/focus/focus_manager.h:150: FocusManager(Widget* widget);
On 2015/01/08 23:39:26, sky wrote:
> explicit

Done.

https://codereview.chromium.org/838253004/diff/1/ui/views/focus/focus_manager...
ui/views/focus/focus_manager.h:275: void
AddAcceleratorPreProcessor(ui::AcceleratorProcessor*);
On 2015/01/08 23:39:26, sky wrote:
> style guide says you should have names here.

Done.

[Andre, 2015-01-09 00:15:50]

Accidentally hit published too early. Still looking into PreTargetHandler and
tests.

[Andre, 2015-01-09 01:04:01]

Patchset #2 (id:20001) has been deleted

[Devlin, 2015-01-09 17:09:26]

https://codereview.chromium.org/838253004/diff/1/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_keybinding_registry.h (right):

https://codereview.chromium.org/838253004/diff/1/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_keybinding_registry.h:61: static void
SetShortcutHandlingSuspended(bool suspended);
nit: Now that these are implemented here, please make them unix_hacker_style.

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

https://codereview.chromium.org/838253004/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/extensions/extension_keybinding_registry_views.cc:23:
focus_manager->AddAcceleratorPreProcessor(this);
This is somewhat less definitive than the old implementation, since if another
object is added as a pre-processor, it could process the accelerator before this
one can suppress it.  If we currently only have this as a pre-processor, could
we establish the pattern that pre-processors are only designed to suppress,
rather than handle, accelerators?  This would avoid racy behavior.

On that note, I'm not entirely sure that I like the pre-post processor
vernacular, since it (to me) implies that one does work before the processing of
the accelerator, and one does work after the processing - which isn't true in
either case (they both just have the option to "process" them).  If Scott feels
differently, go with his preference, but I think I prefer something more like
"AcceleratorSuppressor" which has the chance to suppress them (and only ever
consumes events) and "AcceleratorProcessor" (which actually takes an action on
the event) - it makes the purposes seem more clear to me.

[Andre, 2015-01-10 00:48:11]

sky, devlin, PTAL at the latest patch.

https://codereview.chromium.org/838253004/diff/1/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_keybinding_registry.h (right):

https://codereview.chromium.org/838253004/diff/1/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_keybinding_registry.h:61: static void
SetShortcutHandlingSuspended(bool suspended);
On 2015/01/09 17:09:26, Devlin wrote:
> nit: Now that these are implemented here, please make them unix_hacker_style.

The style guide is not specific about this, but my interpretation is that the
rule does not apply to static methods.
It would also be weird if it doesn't match
GlobalShortcutListener::SetShortcutHandlingSuspended, since they
are called back to back in CommandHandler.

https://codereview.chromium.org/838253004/diff/1/chrome/browser/ui/views/fram...
File chrome/browser/ui/views/frame/browser_view.cc (right):

https://codereview.chromium.org/838253004/diff/1/chrome/browser/ui/views/fram...
chrome/browser/ui/views/frame/browser_view.cc:1379: if
(extensions::ExtensionKeybindingRegistry::IsShortcutHandlingSuspended())
On 2015/01/08 23:39:25, sky wrote:
> Could this be implemented by as a PreTargetHandler on env? My general desire
is
> to avoid the dependency from BrowserView onto the extension classes.

Thanks for the idea!
PTAL at patch #5, using EventMonitor to achieve the same effect in a simpler
way.

[Finnur, 2015-01-12 11:09:45]

finnur@chromium.org changed reviewers:
+ finnur@chromium.org

[Finnur, 2015-01-12 11:09:46]

General note: Might want to review the CL description, if only to remove the
redundant word 'we' in one location.

https://codereview.chromium.org/838253004/diff/100001/chrome/browser/ui/views...
File chrome/browser/ui/views/extensions/extension_keybinding_registry_views.cc
(left):

https://codereview.chromium.org/838253004/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/extensions/extension_keybinding_registry_views.cc:17:
views::FocusManager::set_shortcut_handling_suspended(suspended);
Isn't this the last remaining caller for
FocusManager::set_shortcut_handling_suspended (so that function can be removed
now)?

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

https://codereview.chromium.org/838253004/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/extensions/extension_keybinding_registry_views.cc:28:
widget->AddObserver(this);
Shouldn't we remove this observer in the dtor?

https://codereview.chromium.org/838253004/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/extensions/extension_keybinding_registry_views.cc:84:
event->SetHandled();
Curious as to why we need to let some go unhandled?

[Andre, 2015-01-12 19:32:03]

Patchset #6 (id:120001) has been deleted

[Andre, 2015-01-12 23:27:15]

https://codereview.chromium.org/838253004/diff/100001/chrome/browser/ui/views...
File chrome/browser/ui/views/extensions/extension_keybinding_registry_views.cc
(left):

https://codereview.chromium.org/838253004/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/extensions/extension_keybinding_registry_views.cc:17:
views::FocusManager::set_shortcut_handling_suspended(suspended);
On 2015/01/12 11:09:46, Finnur wrote:
> Isn't this the last remaining caller for
> FocusManager::set_shortcut_handling_suspended (so that function can be removed
> now)?

Ah yes, I forgot to remove this when I started over with the new patch.
Removed.

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

https://codereview.chromium.org/838253004/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/extensions/extension_keybinding_registry_views.cc:28:
widget->AddObserver(this);
On 2015/01/12 11:09:46, Finnur wrote:
> Shouldn't we remove this observer in the dtor?

Done.

https://codereview.chromium.org/838253004/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/extensions/extension_keybinding_registry_views.cc:84:
event->SetHandled();
On 2015/01/12 11:09:46, Finnur wrote:
> Curious as to why we need to let some go unhandled?

SetHandled means we consume the event here to prevent FocusManager from
processing it.
I've added a comment to explain.

[Finnur, 2015-01-13 11:16:00]

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

https://codereview.chromium.org/838253004/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/extensions/extension_keybinding_registry_views.cc:84:
event->SetHandled();
You missed my point. I was asking why we don't call SetHandled on all? I'm not
saying this code is wrong, I'm just trying to understand why we let some pass
through.

[Andre, 2015-01-14 01:38:26]

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

https://codereview.chromium.org/838253004/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/extensions/extension_keybinding_registry_views.cc:84:
event->SetHandled();
On 2015/01/13 11:16:00, Finnur wrote:
> You missed my point. I was asking why we don't call SetHandled on all? I'm not
> saying this code is wrong, I'm just trying to understand why we let some pass
> through.

The intention was that if shortcut handling is not suspended, we don't call
SetHandled() so that the event dispatching happens normally (FocusManager
would process it later).

I found that shortcut recording is not working on ChromeOS with this patch, so
this doesn't look like the right approach. Looking into it.

[Finnur, 2015-01-14 12:26:46]

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

https://codereview.chromium.org/838253004/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/extensions/extension_keybinding_registry_views.cc:84:
event->SetHandled();
> The intention was that if shortcut handling is not suspended, we don't call
> SetHandled() so that the event dispatching happens normally (FocusManager
> would process it later).

I think you are referring to the if statement on line 79 and that part I
understand. 

I was referring to the if statement on line 83. Why is that needed?

> I found that shortcut recording is not working on ChromeOS with this patch, so
> this doesn't look like the right approach. Looking into it.

OK.

[Andre, 2015-01-14 21:24:55]

Devlin, Finnur, PTAL at the latest patch.

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

https://codereview.chromium.org/838253004/diff/100001/chrome/browser/ui/views...
chrome/browser/ui/views/extensions/extension_keybinding_registry_views.cc:84:
event->SetHandled();
On 2015/01/14 12:26:46, Finnur wrote:
> > The intention was that if shortcut handling is not suspended, we don't call
> > SetHandled() so that the event dispatching happens normally (FocusManager
> > would process it later).
> 
> I think you are referring to the if statement on line 79 and that part I
> understand. 
> 
> I was referring to the if statement on line 83. Why is that needed?
> 
> > I found that shortcut recording is not working on ChromeOS with this patch,
so
> > this doesn't look like the right approach. Looking into it.
> 
> OK.

Ah I see, sorry for the confusion.
The intent was for non-accelerator key events to not be consumed so that the
extension shortcut recording Web UI can receive it.
The problem is, the Web UI should also receive accelerator events even if
shortcut handling is disabled.

Patch #7 is another attempt at this using callbacks, PTAL.

[Devlin, 2015-01-14 21:55:18]

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

https://codereview.chromium.org/838253004/diff/150001/chrome/browser/ui/views...
chrome/browser/ui/views/extensions/extension_keybinding_registry_views.cc:25:
focus_manager_->set_shortcut_handling_suspended_callback(
Finnur and Scott know this code better, so I'll defer to them.  But to me, this
strikes me as a slightly less-than-ideal solution.  I wonder if this wouldn't
all be made easier by doing something like exposing a
GetExtensionKeybindingRegistry method on a BrowserWindow, and then just making
SetShortcutHandlingSuspended a non-static virtual method that the views and
cocoa implementations can each handle like they do now.  Would that be
objectionable for some reason?  Will it be possible to have multiple keybinding
registries, of both cocoa and views implementations?

https://codereview.chromium.org/838253004/diff/150001/ui/views/focus/focus_ma...
File ui/views/focus/focus_manager.cc (right):

https://codereview.chromium.org/838253004/diff/150001/ui/views/focus/focus_ma...
ui/views/focus/focus_manager.cc:511: if (!shortcut_handling_suspended_callback_)
nit: if this stays, it can be simplified to
return shortcut_handling_suspended_callback_ &&
    shortcut_handling_suspended_callback_->Run();

[Andre, 2015-01-14 22:01:59]

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

https://codereview.chromium.org/838253004/diff/150001/chrome/browser/ui/views...
chrome/browser/ui/views/extensions/extension_keybinding_registry_views.cc:25:
focus_manager_->set_shortcut_handling_suspended_callback(
On 2015/01/14 21:55:18, Devlin wrote:
> Finnur and Scott know this code better, so I'll defer to them.  But to me,
this
> strikes me as a slightly less-than-ideal solution.  I wonder if this wouldn't
> all be made easier by doing something like exposing a
> GetExtensionKeybindingRegistry method on a BrowserWindow, and then just making
> SetShortcutHandlingSuspended a non-static virtual method that the views and
> cocoa implementations can each handle like they do now.  Would that be
> objectionable for some reason?  Will it be possible to have multiple
keybinding
> registries, of both cocoa and views implementations?

Thanks!
I explored this idea earlier, but didn't know what to do with
CommandHandler::HandleSetShortcutHandlingSuspended.
How can we find the registry instances from there?

[Devlin, 2015-01-15 00:22:08]

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

https://codereview.chromium.org/838253004/diff/150001/chrome/browser/ui/views...
chrome/browser/ui/views/extensions/extension_keybinding_registry_views.cc:25:
focus_manager_->set_shortcut_handling_suspended_callback(
On 2015/01/14 22:01:58, Andre wrote:
> On 2015/01/14 21:55:18, Devlin wrote:
> > Finnur and Scott know this code better, so I'll defer to them.  But to me,
> this
> > strikes me as a slightly less-than-ideal solution.  I wonder if this
wouldn't
> > all be made easier by doing something like exposing a
> > GetExtensionKeybindingRegistry method on a BrowserWindow, and then just
making
> > SetShortcutHandlingSuspended a non-static virtual method that the views and
> > cocoa implementations can each handle like they do now.  Would that be
> > objectionable for some reason?  Will it be possible to have multiple
> keybinding
> > registries, of both cocoa and views implementations?
> 
> Thanks!
> I explored this idea earlier, but didn't know what to do with
> CommandHandler::HandleSetShortcutHandlingSuspended.
> How can we find the registry instances from there?

So, you don't need _all_ the registry instances - just one.  The body of the
SetShortcutHandlingSuspended could still, for the time being at least, disable
shortcuts everywhere with a static (Finnur might know if we could actually just
disable it for the one registry -- I *think* there's only one per window, and I
*think* that only the active window's registry can handle them - but that's more
"thinks" than I'm comfortable with asserting ;)).  To get that one, you can do a
little bit of fun plumbing.

Since CommandHandler is a WebUIMessageHandler...

chrome::FindBrowserWithWebContents(web_ui()->GetWebContents())->window()->
    GetExtensionKeybindingRegistry()->SetShortcutHandlingSuspended();

[Finnur, 2015-01-15 15:02:35]

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

https://codereview.chromium.org/838253004/diff/150001/chrome/browser/ui/views...
chrome/browser/ui/views/extensions/extension_keybinding_registry_views.cc:25:
focus_manager_->set_shortcut_handling_suspended_callback(
On 2015/01/15 00:22:08, Devlin wrote:
> On 2015/01/14 22:01:58, Andre wrote:
> > On 2015/01/14 21:55:18, Devlin wrote:
> > > Finnur and Scott know this code better, so I'll defer to them.  But to me,
> > this
> > > strikes me as a slightly less-than-ideal solution.  I wonder if this
> wouldn't
> > > all be made easier by doing something like exposing a
> > > GetExtensionKeybindingRegistry method on a BrowserWindow, and then just
> making
> > > SetShortcutHandlingSuspended a non-static virtual method that the views
and
> > > cocoa implementations can each handle like they do now.  Would that be
> > > objectionable for some reason?  Will it be possible to have multiple
> > keybinding
> > > registries, of both cocoa and views implementations?
> > 
> > Thanks!
> > I explored this idea earlier, but didn't know what to do with
> > CommandHandler::HandleSetShortcutHandlingSuspended.
> > How can we find the registry instances from there?
> 
> So, you don't need _all_ the registry instances - just one.  The body of the
> SetShortcutHandlingSuspended could still, for the time being at least, disable
> shortcuts everywhere with a static (Finnur might know if we could actually
just
> disable it for the one registry -- I *think* there's only one per window, and
I
> *think* that only the active window's registry can handle them - but that's
more
> "thinks" than I'm comfortable with asserting ;)).  

I *think* you are right. :)

The keys go through the active window only and you wouldn't even be able to
switch to another window without enabling shortcut handling again (it would get
reactivated as soon as the input field loses focus).

> Will it be possible to have multiple keybinding
> registries, of both cocoa and views implementations?

Not sure I grok the question. There's one registry per window and one global
one. So, in that regard you can definitely have many. Only one can be active
though (the global one is always passive, so to speak).

> To get that one, you can do a little bit of fun plumbing.
> 
> Since CommandHandler is a WebUIMessageHandler...
> 
> chrome::FindBrowserWithWebContents(web_ui()->GetWebContents())->window()->
>     GetExtensionKeybindingRegistry()->SetShortcutHandlingSuspended();

This should get you to the active registry. But the Global registry already
knows which is active: See registry_for_active_window().

Having the disabling only work for the active window should suffice. I *think*.
:) The way it works today disables shortcut handling even for other profiles in
Chrome, which is overkill. Shouldn't hurt, though, since only one Chrome window
can be active at a time, but still feels wrong.

I'd be interested in exploring if Devlin's solution would look better than the
current one. I had to make an effort to wrap my head around the current solution
("wait... you're handing out a callback to a function in another class... -- oh
I see..."). But that's the extent of my complaints. :)

[Devlin, 2015-01-15 16:36:51]

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

https://codereview.chromium.org/838253004/diff/150001/chrome/browser/ui/views...
chrome/browser/ui/views/extensions/extension_keybinding_registry_views.cc:25:
focus_manager_->set_shortcut_handling_suspended_callback(
On 2015/01/15 15:02:34, Finnur wrote:
> > Will it be possible to have multiple keybinding
> > registries, of both cocoa and views implementations?
> 
> Not sure I grok the question. There's one registry per window and one global
> one. So, in that regard you can definitely have many. Only one can be active
> though (the global one is always passive, so to speak).

Was more a question for Andre. :)  The ExtensionKeybindingRegistry is stored in
two different places on views/cocoa - in views, it's on the
BrowserActionsContainer, on cocoa, it's on the browser window.  Theoretically,
if we piecemeal-update the platform from cocoa to views, there could be two for
the window, maybe?  Presumably not. :)

> This should get you to the active registry. But the Global registry already
> knows which is active: See registry_for_active_window().

Oh, that helps a bunch.  So if we really wanted to be minimalistic here, we
could just change SetShortcutHandlingSuspended to be non-static and virtual,
call it on the registry_for_active_window(), and then leave the rest as-is. 
That said, as Finnur mentioned, probably better to just disable for the active
window if we can. :)

[Andre, 2015-01-15 22:26:12]

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

https://codereview.chromium.org/838253004/diff/150001/chrome/browser/ui/views...
chrome/browser/ui/views/extensions/extension_keybinding_registry_views.cc:25:
focus_manager_->set_shortcut_handling_suspended_callback(
On 2015/01/15 16:36:51, Devlin wrote:
> On 2015/01/15 15:02:34, Finnur wrote:
> > > Will it be possible to have multiple keybinding
> > > registries, of both cocoa and views implementations?
> > 
> > Not sure I grok the question. There's one registry per window and one global
> > one. So, in that regard you can definitely have many. Only one can be active
> > though (the global one is always passive, so to speak).
> 
> Was more a question for Andre. :)  The ExtensionKeybindingRegistry is stored
in
> two different places on views/cocoa - in views, it's on the
> BrowserActionsContainer, on cocoa, it's on the browser window.  Theoretically,
> if we piecemeal-update the platform from cocoa to views, there could be two
for
> the window, maybe?  Presumably not. :)
> 
> > This should get you to the active registry. But the Global registry already
> > knows which is active: See registry_for_active_window().
> 
> Oh, that helps a bunch.  So if we really wanted to be minimalistic here, we
> could just change SetShortcutHandlingSuspended to be non-static and virtual,
> call it on the registry_for_active_window(), and then leave the rest as-is. 
> That said, as Finnur mentioned, probably better to just disable for the active
> window if we can. :)

Thanks!
registry_for_active_window() is the missing piece I was looking for.
PTAL at patch #8 for the new implementation.

[Devlin, 2015-01-15 22:41:43]

I like this one better - it strikes me as much cleaner :)

https://codereview.chromium.org/838253004/diff/170001/chrome/browser/extensio...
File chrome/browser/extensions/extension_keybinding_registry.h (right):

https://codereview.chromium.org/838253004/diff/170001/chrome/browser/extensio...
chrome/browser/extensions/extension_keybinding_registry.h:61: virtual void
SetShortcutHandlingSuspended(bool suspended);
nit: Seeing how these are used, I might have a small preference for having a
non-virtual setter/getter, and a protected virtual (by default empty)
OnShortcutHandlingSuspended().  This way we don't have to call the super in the
subclasses.  I don't feel super strongly though.

https://codereview.chromium.org/838253004/diff/170001/chrome/browser/extensio...
chrome/browser/extensions/extension_keybinding_registry.h:62: bool
IsShortcutHandlingSuspended();
This should be hacker_style() now :)

https://codereview.chromium.org/838253004/diff/170001/chrome/browser/ui/views...
File chrome/browser/ui/views/extensions/extension_keybinding_registry_views.h
(right):

https://codereview.chromium.org/838253004/diff/170001/chrome/browser/ui/views...
chrome/browser/ui/views/extensions/extension_keybinding_registry_views.h:41:
void SetShortcutHandlingSuspended(bool suspended) override;
Is there a reason this can't go at line 48 with the rest of the
KeybindingRegistry functions?

https://codereview.chromium.org/838253004/diff/170001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/extensions/command_handler.cc (right):

https://codereview.chromium.org/838253004/diff/170001/chrome/browser/ui/webui...
chrome/browser/ui/webui/extensions/command_handler.cc:134:
global_registry->registry_for_active_window()->
nit: Should this maybe go into the ExtensionCommandsGlobalRegistry
implementation?  Is there any time we'd want to disable global, but not the
active window?

[Finnur, 2015-01-16 10:05:05]

On my way to work this morning, something was bothering me about what I
suggested (getting the active registry through the global one) and I confirmed
my suspicions this morning.

Please test this on a Mac because I think that's the only platform that doesn't
update the global registry with the active registry. Fortunately, (if I'm right
about this) this is easy to fix, and you can model it after what we do in
ToolbarView::OnWidgetActivationChanged.

https://codereview.chromium.org/838253004/diff/190001/chrome/browser/extensio...
File chrome/browser/extensions/extension_keybinding_registry.cc (right):

https://codereview.chromium.org/838253004/diff/190001/chrome/browser/extensio...
chrome/browser/extensions/extension_keybinding_registry.cc:47: void
ExtensionKeybindingRegistry::set_shortcut_handling_suspended(
nit: Should not be unix_hacker style (calls virtual function).

[Devlin, 2015-01-16 15:49:55]

https://codereview.chromium.org/838253004/diff/190001/chrome/browser/extensio...
File chrome/browser/extensions/extension_keybinding_registry.cc (right):

https://codereview.chromium.org/838253004/diff/190001/chrome/browser/extensio...
chrome/browser/extensions/extension_keybinding_registry.cc:47: void
ExtensionKeybindingRegistry::set_shortcut_handling_suspended(
On 2015/01/16 10:05:05, Finnur wrote:
> nit: Should not be unix_hacker style (calls virtual function).

I probably caused this confusion; I only meant that the accessor should be
hacker_style (related: it [the accessor] should also be defined in the header).

[Andre, 2015-01-16 18:31:51]

Patchset #10 (id:210001) has been deleted

[Andre, 2015-01-16 18:36:58]

Patchset #10 (id:230001) has been deleted

[Andre, 2015-01-16 18:39:49]

On 2015/01/16 10:05:05, Finnur wrote:
> On my way to work this morning, something was bothering me about what I
> suggested (getting the active registry through the global one) and I confirmed
> my suspicions this morning.
> 
> Please test this on a Mac because I think that's the only platform that
doesn't
> update the global registry with the active registry. Fortunately, (if I'm
right
> about this) this is easy to fix, and you can model it after what we do in
> ToolbarView::OnWidgetActivationChanged.
> 
>
https://codereview.chromium.org/838253004/diff/190001/chrome/browser/extensio...
> File chrome/browser/extensions/extension_keybinding_registry.cc (right):
> 
>
https://codereview.chromium.org/838253004/diff/190001/chrome/browser/extensio...
> chrome/browser/extensions/extension_keybinding_registry.cc:47: void
> ExtensionKeybindingRegistry::set_shortcut_handling_suspended(
> nit: Should not be unix_hacker style (calls virtual function).

You're right, this call was missing on Mac.
I've added it in the latest patch.

[Andre, 2015-01-16 18:40:15]

https://codereview.chromium.org/838253004/diff/170001/chrome/browser/extensio...
File chrome/browser/extensions/extension_keybinding_registry.h (right):

https://codereview.chromium.org/838253004/diff/170001/chrome/browser/extensio...
chrome/browser/extensions/extension_keybinding_registry.h:61: virtual void
SetShortcutHandlingSuspended(bool suspended);
On 2015/01/15 22:41:43, Devlin wrote:
> nit: Seeing how these are used, I might have a small preference for having a
> non-virtual setter/getter, and a protected virtual (by default empty)
> OnShortcutHandlingSuspended().  This way we don't have to call the super in
the
> subclasses.  I don't feel super strongly though.

Done.

https://codereview.chromium.org/838253004/diff/170001/chrome/browser/extensio...
chrome/browser/extensions/extension_keybinding_registry.h:62: bool
IsShortcutHandlingSuspended();
On 2015/01/15 22:41:43, Devlin wrote:
> This should be hacker_style() now :)

Done.

https://codereview.chromium.org/838253004/diff/170001/chrome/browser/ui/views...
File chrome/browser/ui/views/extensions/extension_keybinding_registry_views.h
(right):

https://codereview.chromium.org/838253004/diff/170001/chrome/browser/ui/views...
chrome/browser/ui/views/extensions/extension_keybinding_registry_views.h:41:
void SetShortcutHandlingSuspended(bool suspended) override;
On 2015/01/15 22:41:43, Devlin wrote:
> Is there a reason this can't go at line 48 with the rest of the
> KeybindingRegistry functions?

Done.

https://codereview.chromium.org/838253004/diff/170001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/extensions/command_handler.cc (right):

https://codereview.chromium.org/838253004/diff/170001/chrome/browser/ui/webui...
chrome/browser/ui/webui/extensions/command_handler.cc:134:
global_registry->registry_for_active_window()->
On 2015/01/15 22:41:43, Devlin wrote:
> nit: Should this maybe go into the ExtensionCommandsGlobalRegistry
> implementation?  Is there any time we'd want to disable global, but not the
> active window?

Done.

https://codereview.chromium.org/838253004/diff/190001/chrome/browser/extensio...
File chrome/browser/extensions/extension_keybinding_registry.cc (right):

https://codereview.chromium.org/838253004/diff/190001/chrome/browser/extensio...
chrome/browser/extensions/extension_keybinding_registry.cc:47: void
ExtensionKeybindingRegistry::set_shortcut_handling_suspended(
On 2015/01/16 10:05:05, Finnur wrote:
> nit: Should not be unix_hacker style (calls virtual function).

Done.

[Devlin, 2015-01-16 22:02:38]

I like it. Extension-y stuff lgtm.

[Finnur, 2015-01-19 14:33:02]

LGTM

[Andre, 2015-01-20 03:51:01]

Still need owner for c/b/ui/cocoa and ui/views.
sky@ please PTAL.

[sky, 2015-01-20 16:41:19]

ui/views LGTM - please get a more local owner for other changes.

[Andre, 2015-01-20 16:51:37]

andresantoso@chromium.org changed reviewers:
+ rsesek@chromium.org

[Andre, 2015-01-20 16:51:37]

+rsesek for cocoa.

[Robert Sesek, 2015-01-20 17:02:46]

cocoa/ LGTM

[Andre, 2015-01-20 17:40:18]

The CQ bit was checked by andresantoso@chromium.org

[commit-bot: I haz the power, 2015-01-20 17:40:54]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/838253004/250001

[commit-bot: I haz the power, 2015-01-20 17:58:43]

Committed patchset #10 (id:250001)

[commit-bot: I haz the power, 2015-01-20 17:59:23]

Patchset 10 (id:??) landed as
https://crrev.com/de66e360e2a2944bdf71608726b19b3bf2a2a6ff
Cr-Commit-Position: refs/heads/master@{#312220}