Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
553243002:
Track the active ExtensionKeybindingRegistry and make it available to EventRewriter. (Closed)
|
Created:
6 years, 3 months ago by David Tseng Modified:
6 years, 3 months ago CC:
chromium-reviews, extensions-reviews_chromium.org, nkostylev+watch_chromium.org, tfarina, oshima+watch_chromium.org, chromium-apps-reviews_chromium.org, stevenjb+watch_chromium.org, davemoore+watch_chromium.org, dmazzoni, Peter Lundblad Base URL:
https://chromium.googlesource.com/chromium/src.git@lkcr Project:
chromium Visibility:
Public. |
DescriptionTrack the active ExtensionKeybindingRegistry and make it available to EventRewriter.
Add the ability to query globally if a shortcut
is registered by an extension and use that to avoid unnecessarily rewriting
Chrome OS keys.
BUG=410944
Committed: https://crrev.com/4264d2c4b5d217e1c5dbc2ae80ab6ce595f9b6d3
Cr-Commit-Position: refs/heads/master@{#295284}
Patch Set 1 #
Total comments: 9
Patch Set 2 : Add test. #Patch Set 3 : Fix Mac. #Patch Set 4 : Fix bool vs BOOL. #
Total comments: 3
Patch Set 5 : Address sky's comment. #
Total comments: 18
Patch Set 6 : #Patch Set 7 : Address comments. #Patch Set 8 : #
Total comments: 1
Patch Set 9 : Rename/move accesser. #Patch Set 10 : Remove tracker class. #
Total comments: 11
Patch Set 11 : Address more comments. #
Total comments: 5
Patch Set 12 : Correct logic. #Patch Set 13 : Comment. #
Total comments: 21
Patch Set 14 : Address finnur's comments. #Patch Set 15 : IsRegisteredForActiveWindow. #Patch Set 16 : Rebase. #Patch Set 17 : Fix tests (caught NULL ExtensionCommandsGlobalRegistry). #Patch Set 18 : Try meta_l rather than super_l #
Total comments: 7
Patch Set 19 : Address test failures; sky's comments. #
Total comments: 3
Patch Set 20 : IsRegistered #
Total comments: 5
Patch Set 21 : Simplify logic. #Patch Set 22 : Remove OnWidgetDestroyed. #Messages
Total messages: 51 (9 generated)
dtseng@chromium.org changed reviewers: + finnur@chromium.org
W.I.P. @finnur, is this what you had in mind? Cl doesn't check the global registry; just wanted to check this is actually ok to plumb through. A little more invasive than I'd like particularly with the changes to EventRewriter. I don't see any other alternative to this besides doing the manual conversions I suggested.
Yeah, this is about what I expected (along with the global command object). I think the appearance of invasiveness could be reduced a bit by refactoring the new code into a helper function. https://codereview.chromium.org/553243002/diff/1/chrome/browser/chromeos/even... File chrome/browser/chromeos/events/event_rewriter.cc (right): https://codereview.chromium.org/553243002/diff/1/chrome/browser/chromeos/even... chrome/browser/chromeos/events/event_rewriter.cc:162: // Check the extensions system didn't bind any of these keys. I would do a few things here. 1) Add a helper function, IsExtensionCommandRegistered(event) and move almost all of the new logic to that. 2) Change this function to read: if ((event.type() == ui::ET_KEY_PRESSED) || (event.type() == ui::ET_KEY_RELEASED)) { // Some keyboard events for ChromeOS get rewritten, such as: // Search+Shift+Left gets converted to Shift+Home (BeginDocument). // This doesn't make sense if the user has assigned that shortcut // to an extension. Because: // 1) The extension would, upon seeing a request for Ctrl+Shift+Home have // to register for Shift+Home, instead. // 2) The conversion is unnecessary, because Shift+Home (BeginDocument) isn't // going to be executed. // Therefore, we skip converting the accelerator if an extension has // registered for this shortcut. if (IsExtensionCommandRegistered(event) return ui::EVENT_REWRITE_CONTINUE; return RewriteKeyEvent(static_cast<const ui::KeyEvent&>(event), } 3) Make sure I got the conversion correctly (Ctrl+Shift+Left is Shift+Home, which is BeginDocument). https://codereview.chromium.org/553243002/diff/1/chrome/browser/ui/browser_wi... File chrome/browser/ui/browser_window.h (right): https://codereview.chromium.org/553243002/diff/1/chrome/browser/ui/browser_wi... chrome/browser/ui/browser_window.h:406: // Returns if the given |accelerator| is registered by an extension. nit: s/if/whether/ https://codereview.chromium.org/553243002/diff/1/chrome/browser/ui/views/fram... File chrome/browser/ui/views/frame/browser_view.h (right): https://codereview.chromium.org/553243002/diff/1/chrome/browser/ui/views/fram... chrome/browser/ui/views/frame/browser_view.h:379: nit: Delete line. https://codereview.chromium.org/553243002/diff/1/chrome/browser/ui/views/tool... File chrome/browser/ui/views/toolbar/browser_actions_container.h (right): https://codereview.chromium.org/553243002/diff/1/chrome/browser/ui/views/tool... chrome/browser/ui/views/toolbar/browser_actions_container.h:190: // Returns if the given |accelerator| is registered by an extension. nit: s/if/whether/ https://codereview.chromium.org/553243002/diff/1/chrome/browser/ui/views/tool... File chrome/browser/ui/views/toolbar/toolbar_view.h (right): https://codereview.chromium.org/553243002/diff/1/chrome/browser/ui/views/tool... chrome/browser/ui/views/toolbar/toolbar_view.h:97: // Returns if the given |accelerator| is registered by an extension. nit: s/if/whether/
dtseng@chromium.org changed reviewers: + sky@chromium.org
+ sky for OWNERS. FYI @dmazzoni, I have the change you made to ui_controls_factory_aurax11.cc patched in here. I'll try to land it as part of this change or if you could land first, I'll rebase. Either way, I'm blocked on that change for this cl. https://codereview.chromium.org/553243002/diff/1/chrome/browser/ui/browser_wi... File chrome/browser/ui/browser_window.h (right): https://codereview.chromium.org/553243002/diff/1/chrome/browser/ui/browser_wi... chrome/browser/ui/browser_window.h:406: // Returns if the given |accelerator| is registered by an extension. On 2014/09/10 10:15:16, Finnur wrote: > nit: s/if/whether/ Done. https://codereview.chromium.org/553243002/diff/1/chrome/browser/ui/views/fram... File chrome/browser/ui/views/frame/browser_view.h (right): https://codereview.chromium.org/553243002/diff/1/chrome/browser/ui/views/fram... chrome/browser/ui/views/frame/browser_view.h:379: On 2014/09/10 10:15:16, Finnur wrote: > nit: Delete line. Done. https://codereview.chromium.org/553243002/diff/1/chrome/browser/ui/views/tool... File chrome/browser/ui/views/toolbar/browser_actions_container.h (right): https://codereview.chromium.org/553243002/diff/1/chrome/browser/ui/views/tool... chrome/browser/ui/views/toolbar/browser_actions_container.h:190: // Returns if the given |accelerator| is registered by an extension. On 2014/09/10 10:15:16, Finnur wrote: > nit: s/if/whether/ Done. https://codereview.chromium.org/553243002/diff/1/chrome/browser/ui/views/tool... File chrome/browser/ui/views/toolbar/toolbar_view.h (right): https://codereview.chromium.org/553243002/diff/1/chrome/browser/ui/views/tool... chrome/browser/ui/views/toolbar/toolbar_view.h:97: // Returns if the given |accelerator| is registered by an extension. On 2014/09/10 10:15:16, Finnur wrote: > nit: s/if/whether/ Done.
https://codereview.chromium.org/553243002/diff/60001/chrome/browser/ui/browse... File chrome/browser/ui/browser_window.h (right): https://codereview.chromium.org/553243002/diff/60001/chrome/browser/ui/browse... chrome/browser/ui/browser_window.h:407: virtual bool IsExtensionCommandRegistered( This feels like something profile specific, and not per browser window. It seems like you've wired it this way as ExtensionKeybindingRegistry is created per window. Is there some other way to get this information through the profile?
https://codereview.chromium.org/553243002/diff/60001/chrome/browser/ui/browse... File chrome/browser/ui/browser_window.h (right): https://codereview.chromium.org/553243002/diff/60001/chrome/browser/ui/browse... chrome/browser/ui/browser_window.h:407: virtual bool IsExtensionCommandRegistered( On 2014/09/10 23:55:14, sky wrote: > This feels like something profile specific, and not per browser window. It seems > like you've wired it this way as ExtensionKeybindingRegistry is created per > window. Is there some other way to get this information through the profile? In the case of global commands, the registry is associated with a profile. Unfortunately, the non-global registry lives in a window, so I don't think there's any other way to get at the owner (BrowserView -> ToolbarView -> BrowserActionsContainer). If there's some other way of getting a BrowserActionsContainer based on a profile or BrowserContext, I'd be happy to do that, but I don't think that's available. @finnur, why was the non-global registry placed in BrowserActionsContainer?
On Wed, Sep 10, 2014 at 5:05 PM, <dtseng@chromium.org> wrote: > > https://codereview.chromium.org/553243002/diff/60001/chrome/browser/ui/browse... > File chrome/browser/ui/browser_window.h (right): > > https://codereview.chromium.org/553243002/diff/60001/chrome/browser/ui/browse... > chrome/browser/ui/browser_window.h:407: virtual bool > IsExtensionCommandRegistered( > On 2014/09/10 23:55:14, sky wrote: >> >> This feels like something profile specific, and not per browser > > window. It seems >> >> like you've wired it this way as ExtensionKeybindingRegistry is > > created per >> >> window. Is there some other way to get this information through the > > profile? > > In the case of global commands, the registry is associated with a > profile. Unfortunately, the non-global registry lives in a window, so I > don't think there's any other way to get at the owner (BrowserView -> > ToolbarView -> BrowserActionsContainer). If there's some other way of > getting a BrowserActionsContainer based on a profile or BrowserContext, > I'd be happy to do that, but I don't think that's available. I certainly get that there needs to be some per window state, but it seems like that information could be tracked separately so that to determine what you want you don't need to go through browserwindow. -Scott > > @finnur, why was the non-global registry placed in > BrowserActionsContainer? > > https://codereview.chromium.org/553243002/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
I think mainly due to the need to handle browser actions and page actions, the ExtensionKeybindingRegistry lives where it does. I guess I can come up with a few ways to do what I want without going through a browser window - make EventRewriter a |ExtensionKeybindingRegistryObserver| which receives calls whenever a registry gets created/destroyed/active browser window changes / registry changes. Not sure if we'd miss object creation/have ordering issues here. - make it a BrowserContextKeyedAPI (might have some issues with multiple objects per context though). Any preferences/other suggestions? As is, it does mirror the way we handle ExecuteExtensionCommand. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
finnur@chromium.org changed reviewers: + rdevlin.cronin@chromium.org
https://codereview.chromium.org/553243002/diff/60001/chrome/browser/ui/browse... File chrome/browser/ui/browser_window.h (right): https://codereview.chromium.org/553243002/diff/60001/chrome/browser/ui/browse... chrome/browser/ui/browser_window.h:407: virtual bool IsExtensionCommandRegistered( It has been quite a while, but as I recall it is because it has the same lifetime as the BrowserActionsContainer, is conceptually related to it (the BAC shows extensions and the Registry is handling shortcuts for them). And it also needs access to the focus manager for that window in order to register accelerators. I do have an urge to loop in Devlin, though, as he's been revamping this area and may have good ideas.
PTAL. @sky, removed all the plumbing through BrowserWindow. @finnur, added a class to track the active non-global registry. From someone relatively new to the ownership relationships here, it seems like a per profile owner rather than a window owner makes sense here (i.e. each non-global registry seems to hold the same keybindings right?).
Patchset #7 (id:120001) has been deleted
Patchset #6 (id:100001) has been deleted
Adding myself, if you don't mind. :) https://codereview.chromium.org/553243002/diff/80001/chrome/browser/extension... File chrome/browser/extensions/extension_keybinding_registry_tracker.h (right): https://codereview.chromium.org/553243002/diff/80001/chrome/browser/extension... chrome/browser/extensions/extension_keybinding_registry_tracker.h:1: // Copyright (c) 2013 The Chromium Authors. All rights reserved. no (c), and not 2013 anymore :) https://codereview.chromium.org/553243002/diff/80001/chrome/browser/extension... chrome/browser/extensions/extension_keybinding_registry_tracker.h:19: class ExtensionKeybindingRegistryTracker : public BrowserContextKeyedAPI { After seeing the whole of this patch, I think this is actually quite overarchitected. ExtensionCommandsGlobalRegistry is itself a BrowserContextKeyedAPI, so couldn't we just expose a Get/SetActiveRegistry on that? https://codereview.chromium.org/553243002/diff/80001/chrome/browser/extension... chrome/browser/extensions/extension_keybinding_registry_tracker.h:26: // profile. nit: s/profile/browser context. https://codereview.chromium.org/553243002/diff/80001/chrome/browser/extension... chrome/browser/extensions/extension_keybinding_registry_tracker.h:36: explicit ExtensionKeybindingRegistryTracker(content::BrowserContext* context); nit: Prefer these at the top (right below public:) https://codereview.chromium.org/553243002/diff/80001/chrome/browser/ui/views/... File chrome/browser/ui/views/toolbar/browser_actions_container.cc (right): https://codereview.chromium.org/553243002/diff/80001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/browser_actions_container.cc:958: if (GetWidget()->IsActive()) This is probably not the best point for this. This method is called when a BrowserAction's visibility (might have) changed, but there's no guarantee that when a window changes, a browser action will become visible/invisible. I think a better solution is to listen for when the widget becomes active/inactive. If you wanted to rely on ToolbarView (which is already a WidgetObserver) to do that, I'd say it's fine (but I'll leave that up to sky@). https://codereview.chromium.org/553243002/diff/80001/chrome/browser/ui/views/... File chrome/browser/ui/views/toolbar/browser_actions_container.h (right): https://codereview.chromium.org/553243002/diff/80001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/browser_actions_container.h:1: // Copyright 2014 The Chromium Authors. All rights reserved. nit: These, on the other hand, _shouldn't_ be updated. https://codereview.chromium.org/553243002/diff/80001/chrome/test/data/extensi... File chrome/test/data/extensions/api_test/keybinding/chromeos_conversions/manifest.json (right): https://codereview.chromium.org/553243002/diff/80001/chrome/test/data/extensi... chrome/test/data/extensions/api_test/keybinding/chromeos_conversions/manifest.json:16: "Search-Shift-Up": { nit: indentation
https://codereview.chromium.org/553243002/diff/80001/chrome/browser/extension... File chrome/browser/extensions/extension_keybinding_registry_tracker.h (right): https://codereview.chromium.org/553243002/diff/80001/chrome/browser/extension... chrome/browser/extensions/extension_keybinding_registry_tracker.h:19: class ExtensionKeybindingRegistryTracker : public BrowserContextKeyedAPI { On 2014/09/11 18:41:01, Devlin wrote: > After seeing the whole of this patch, I think this is actually quite > overarchitected. ExtensionCommandsGlobalRegistry is itself a > BrowserContextKeyedAPI, so couldn't we just expose a Get/SetActiveRegistry on > that? I'd prefer to keep these separated since I think we want to keep the distinction between global and non-global registry, but can change this if you feel strongly. Maybe GetActiveNonglobalRegistry? Seems confusing to me.
https://codereview.chromium.org/553243002/diff/80001/chrome/browser/extension... File chrome/browser/extensions/extension_keybinding_registry_tracker.h (right): https://codereview.chromium.org/553243002/diff/80001/chrome/browser/extension... chrome/browser/extensions/extension_keybinding_registry_tracker.h:1: // Copyright (c) 2013 The Chromium Authors. All rights reserved. On 2014/09/11 18:41:01, Devlin wrote: > no (c), and not 2013 anymore :) Done. https://codereview.chromium.org/553243002/diff/80001/chrome/browser/extension... chrome/browser/extensions/extension_keybinding_registry_tracker.h:26: // profile. On 2014/09/11 18:41:01, Devlin wrote: > nit: s/profile/browser context. Done. https://codereview.chromium.org/553243002/diff/80001/chrome/browser/extension... chrome/browser/extensions/extension_keybinding_registry_tracker.h:36: explicit ExtensionKeybindingRegistryTracker(content::BrowserContext* context); On 2014/09/11 18:41:01, Devlin wrote: > nit: Prefer these at the top (right below public:) Done. https://codereview.chromium.org/553243002/diff/80001/chrome/browser/ui/views/... File chrome/browser/ui/views/toolbar/browser_actions_container.cc (right): https://codereview.chromium.org/553243002/diff/80001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/browser_actions_container.cc:958: if (GetWidget()->IsActive()) On 2014/09/11 18:41:01, Devlin wrote: > This is probably not the best point for this. This method is called when a > BrowserAction's visibility (might have) changed, but there's no guarantee that > when a window changes, a browser action will become visible/invisible. I think > a better solution is to listen for when the widget becomes active/inactive. If > you wanted to rely on ToolbarView (which is already a WidgetObserver) to do > that, I'd say it's fine (but I'll leave that up to sky@). Done. (sky@ feel free to suggest alternatives). https://codereview.chromium.org/553243002/diff/80001/chrome/browser/ui/views/... File chrome/browser/ui/views/toolbar/browser_actions_container.h (right): https://codereview.chromium.org/553243002/diff/80001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/browser_actions_container.h:1: // Copyright 2014 The Chromium Authors. All rights reserved. On 2014/09/11 18:41:01, Devlin wrote: > nit: These, on the other hand, _shouldn't_ be updated. Done. https://codereview.chromium.org/553243002/diff/80001/chrome/test/data/extensi... File chrome/test/data/extensions/api_test/keybinding/chromeos_conversions/manifest.json (right): https://codereview.chromium.org/553243002/diff/80001/chrome/test/data/extensi... chrome/test/data/extensions/api_test/keybinding/chromeos_conversions/manifest.json:16: "Search-Shift-Up": { On 2014/09/11 18:41:01, Devlin wrote: > nit: indentation Done.
https://codereview.chromium.org/553243002/diff/80001/chrome/browser/extension... File chrome/browser/extensions/extension_keybinding_registry_tracker.h (right): https://codereview.chromium.org/553243002/diff/80001/chrome/browser/extension... chrome/browser/extensions/extension_keybinding_registry_tracker.h:19: class ExtensionKeybindingRegistryTracker : public BrowserContextKeyedAPI { On 2014/09/11 18:58:50, David Tseng wrote: > On 2014/09/11 18:41:01, Devlin wrote: > > After seeing the whole of this patch, I think this is actually quite > > overarchitected. ExtensionCommandsGlobalRegistry is itself a > > BrowserContextKeyedAPI, so couldn't we just expose a Get/SetActiveRegistry on > > that? > > I'd prefer to keep these separated since I think we want to keep the distinction > between global and non-global registry, but can change this if you feel > strongly. Maybe GetActiveNonglobalRegistry? Seems confusing to me. I don't think that making an accessor to an active instance is making the global and non-global any less separated. I'd say put on the accessor. (I'd also weakly opt against saying Nonglobal in the name, since the class is named ExtensionKeybindingRegistry vs NonGlobalExtensionKeybindingRegistry; I think that a method comment is sufficient to differentiate. But if you feel strongly, I could be persuaded.) https://codereview.chromium.org/553243002/diff/180001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/browser_actions_container.h (right): https://codereview.chromium.org/553243002/diff/180001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/browser_actions_container.h:252: extensions::ExtensionKeybindingRegistry* GetExtensionKeybindingRegistry(); This should go with the other simple accessors. Normally, this would be at the bottom (though above testing code), but in this class they're at the top. So this should go around line 155, and should also be named simply "extension_keybinding_registry()" (and inlined here in the .h).
https://codereview.chromium.org/553243002/diff/80001/chrome/browser/extension... File chrome/browser/extensions/extension_keybinding_registry_tracker.h (right): https://codereview.chromium.org/553243002/diff/80001/chrome/browser/extension... chrome/browser/extensions/extension_keybinding_registry_tracker.h:19: class ExtensionKeybindingRegistryTracker : public BrowserContextKeyedAPI { On 2014/09/11 19:48:48, Devlin wrote: > On 2014/09/11 18:58:50, David Tseng wrote: > > On 2014/09/11 18:41:01, Devlin wrote: > > > After seeing the whole of this patch, I think this is actually quite > > > overarchitected. ExtensionCommandsGlobalRegistry is itself a > > > BrowserContextKeyedAPI, so couldn't we just expose a Get/SetActiveRegistry > on > > > that? > > > > I'd prefer to keep these separated since I think we want to keep the > distinction > > between global and non-global registry, but can change this if you feel > > strongly. Maybe GetActiveNonglobalRegistry? Seems confusing to me. > > I don't think that making an accessor to an active instance is making the global > and non-global any less separated. I'd say put on the accessor. (I'd also > weakly opt against saying Nonglobal in the name, since the class is named > ExtensionKeybindingRegistry vs NonGlobalExtensionKeybindingRegistry; Example of the confusion here...you meant ExtensionCommandsGlobalRegistry rather than ExtensionKeybindingRegistry? The latter isn't a BrowserContextKeyedAPI So, if we added ExtensionCommandsGlobalRegistry::GetActiveRegistry() ExtensionCommandsGlobalRegistry* registry = ...; ExtensionKeybindingRegistry* registry2 = registry->GetActiveRegistry(); it actually refers to another registry (not the instance referred to by the caller). Is this what you meant? I think > that a method comment is sufficient to differentiate. But if you feel strongly, > I could be persuaded.) If you're not convinced by the above points, then I'll change it :).
https://codereview.chromium.org/553243002/diff/80001/chrome/browser/extension... File chrome/browser/extensions/extension_keybinding_registry_tracker.h (right): https://codereview.chromium.org/553243002/diff/80001/chrome/browser/extension... chrome/browser/extensions/extension_keybinding_registry_tracker.h:19: class ExtensionKeybindingRegistryTracker : public BrowserContextKeyedAPI { On 2014/09/11 20:38:41, David Tseng wrote: > On 2014/09/11 19:48:48, Devlin wrote: > > On 2014/09/11 18:58:50, David Tseng wrote: > > > On 2014/09/11 18:41:01, Devlin wrote: > > > > After seeing the whole of this patch, I think this is actually quite > > > > overarchitected. ExtensionCommandsGlobalRegistry is itself a > > > > BrowserContextKeyedAPI, so couldn't we just expose a Get/SetActiveRegistry > > on > > > > that? > > > > > > I'd prefer to keep these separated since I think we want to keep the > > distinction > > > between global and non-global registry, but can change this if you feel > > > strongly. Maybe GetActiveNonglobalRegistry? Seems confusing to me. > > > > I don't think that making an accessor to an active instance is making the > global > > and non-global any less separated. I'd say put on the accessor. (I'd also > > weakly opt against saying Nonglobal in the name, since the class is named > > ExtensionKeybindingRegistry vs NonGlobalExtensionKeybindingRegistry; > > Example of the confusion here...you meant ExtensionCommandsGlobalRegistry rather > than ExtensionKeybindingRegistry? The latter isn't a BrowserContextKeyedAPI > > So, if we added > ExtensionCommandsGlobalRegistry::GetActiveRegistry() > > ExtensionCommandsGlobalRegistry* registry = ...; > ExtensionKeybindingRegistry* registry2 = registry->GetActiveRegistry(); > > it actually refers to another registry (not the instance referred to by the > caller). > > Is this what you meant? > > > I think > > that a method comment is sufficient to differentiate. But if you feel > strongly, > > I could be persuaded.) > > > If you're not convinced by the above points, then I'll change it :). > I think I named them all correctly in my comments :) (I did say ExtensionCommandsGlobalRegistry was the BCKS). And actually, I think the implementation is fine: ExtensionKeybindingRegistry* active_registry = ExtensionCommandsGlobalRegistry::Get(browser_context)->GetActiveRegistry(); Even conceptually, this makes sense - you're asking the GlobalRegistry to return whatever the currently-active one is. I think that's quite a bit simpler than trying to remember a whole new object (ExtensionKeybindingRegistryTracker). If the argument is that you could have two "registry" variables, then there's a larger problem - what about all our XTabHelpers? :)
On 2014/09/11 20:52:46, Devlin wrote: > https://codereview.chromium.org/553243002/diff/80001/chrome/browser/extension... > File chrome/browser/extensions/extension_keybinding_registry_tracker.h (right): > > https://codereview.chromium.org/553243002/diff/80001/chrome/browser/extension... > chrome/browser/extensions/extension_keybinding_registry_tracker.h:19: class > ExtensionKeybindingRegistryTracker : public BrowserContextKeyedAPI { > On 2014/09/11 20:38:41, David Tseng wrote: > > On 2014/09/11 19:48:48, Devlin wrote: > > > On 2014/09/11 18:58:50, David Tseng wrote: > > > > On 2014/09/11 18:41:01, Devlin wrote: > > > > > After seeing the whole of this patch, I think this is actually quite > > > > > overarchitected. ExtensionCommandsGlobalRegistry is itself a > > > > > BrowserContextKeyedAPI, so couldn't we just expose a > Get/SetActiveRegistry > > > on > > > > > that? > > > > > > > > I'd prefer to keep these separated since I think we want to keep the > > > distinction > > > > between global and non-global registry, but can change this if you feel > > > > strongly. Maybe GetActiveNonglobalRegistry? Seems confusing to me. > > > > > > I don't think that making an accessor to an active instance is making the > > global > > > and non-global any less separated. I'd say put on the accessor. (I'd also > > > weakly opt against saying Nonglobal in the name, since the class is named > > > ExtensionKeybindingRegistry vs NonGlobalExtensionKeybindingRegistry; > > > > Example of the confusion here...you meant ExtensionCommandsGlobalRegistry > rather > > than ExtensionKeybindingRegistry? The latter isn't a BrowserContextKeyedAPI > > > > So, if we added > > ExtensionCommandsGlobalRegistry::GetActiveRegistry() > > > > ExtensionCommandsGlobalRegistry* registry = ...; > > ExtensionKeybindingRegistry* registry2 = registry->GetActiveRegistry(); > > > > it actually refers to another registry (not the instance referred to by the > > caller). > > > > Is this what you meant? > > > > > > I think > > > that a method comment is sufficient to differentiate. But if you feel > > strongly, > > > I could be persuaded.) > > > > > > If you're not convinced by the above points, then I'll change it :). > > > > I think I named them all correctly in my comments :) (I did say > ExtensionCommandsGlobalRegistry was the BCKS). And actually, I think the > implementation is fine: > ExtensionKeybindingRegistry* active_registry = > ExtensionCommandsGlobalRegistry::Get(browser_context)->GetActiveRegistry(); > > Even conceptually, this makes sense - you're asking the GlobalRegistry to return > whatever the currently-active one is. I think that's quite a bit simpler than > trying to remember a whole new object (ExtensionKeybindingRegistryTracker). > > If the argument is that you could have two "registry" variables, then there's a > larger problem - what about all our XTabHelpers? :) Conceptually, I differentiated between the global and non-global registry; you can have many of the latter, but only one of the former. I thought it strange to ask the global registry for an instance of the non-global. However, I can see the points you made and certainly appreciate having less code, so, done.
https://codereview.chromium.org/553243002/diff/220001/chrome/browser/extensio... File chrome/browser/extensions/extension_commands_global_registry.h (right): https://codereview.chromium.org/553243002/diff/220001/chrome/browser/extensio... chrome/browser/extensions/extension_commands_global_registry.h:36: // Sets the active ExtensionKeybindingRegistry. nit: No comments on getters/setters. https://codereview.chromium.org/553243002/diff/220001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/browser_actions_container.h (right): https://codereview.chromium.org/553243002/diff/220001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/browser_actions_container.h:12: #include "chrome/browser/ui/views/extensions/extension_keybinding_registry_views.h" Shouldn't need to include this. https://codereview.chromium.org/553243002/diff/220001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/toolbar_view.cc (right): https://codereview.chromium.org/553243002/diff/220001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_view.cc:277: extensions::ExtensionCommandsGlobalRegistry::Get(browser_->profile()) We also need to unset this, either in ToolbarView or BrowserActionsContainer, when the BrowserActionsContainer gets destroyed. It would probably make the most sense to do in a OnWidgetDestroyed() method.
https://codereview.chromium.org/553243002/diff/220001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/toolbar_view.cc (right): https://codereview.chromium.org/553243002/diff/220001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_view.cc:277: extensions::ExtensionCommandsGlobalRegistry::Get(browser_->profile()) On 2014/09/11 21:38:13, Devlin wrote: > We also need to unset this, either in ToolbarView or BrowserActionsContainer, > when the BrowserActionsContainer gets destroyed. It would probably make the > most sense to do in a OnWidgetDestroyed() method. And also when activation is lost, and not necessarily sent to another window with a toolbar (like, e.g., an app window). So also just an else clause to this part.
https://codereview.chromium.org/553243002/diff/220001/chrome/browser/extensio... File chrome/browser/extensions/extension_commands_global_registry.h (right): https://codereview.chromium.org/553243002/diff/220001/chrome/browser/extensio... chrome/browser/extensions/extension_commands_global_registry.h:36: // Sets the active ExtensionKeybindingRegistry. On 2014/09/11 21:38:13, Devlin wrote: > nit: No comments on getters/setters. Done. https://codereview.chromium.org/553243002/diff/220001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/browser_actions_container.h (right): https://codereview.chromium.org/553243002/diff/220001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/browser_actions_container.h:12: #include "chrome/browser/ui/views/extensions/extension_keybinding_registry_views.h" On 2014/09/11 21:38:13, Devlin wrote: > Shouldn't need to include this. I actually do due to the accesser which calls extension_keybinding_registry.get(); it's also a toss up whether that's still considered a trivial accesser (and therefore qualifies for the unix style naming). https://codereview.chromium.org/553243002/diff/220001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/toolbar_view.cc (right): https://codereview.chromium.org/553243002/diff/220001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_view.cc:277: extensions::ExtensionCommandsGlobalRegistry::Get(browser_->profile()) On 2014/09/11 21:38:13, Devlin wrote: > We also need to unset this, either in ToolbarView or BrowserActionsContainer, > when the BrowserActionsContainer gets destroyed. It would probably make the > most sense to do in a OnWidgetDestroyed() method. Yeah, I wasn't sure about this one. Will add to OnWidgetDestroyed. https://codereview.chromium.org/553243002/diff/220001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_view.cc:277: extensions::ExtensionCommandsGlobalRegistry::Get(browser_->profile()) On 2014/09/11 21:40:00, Devlin wrote: > On 2014/09/11 21:38:13, Devlin wrote: > > We also need to unset this, either in ToolbarView or BrowserActionsContainer, > > when the BrowserActionsContainer gets destroyed. It would probably make the > > most sense to do in a OnWidgetDestroyed() method. > > And also when activation is lost, and not necessarily sent to another window > with a toolbar (like, e.g., an app window). So also just an else clause to this > part. That's what I considered originally...but are we guaranteed this widget looses visibility before the new widget gains/is notified of its visibility change?
https://codereview.chromium.org/553243002/diff/220001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/browser_actions_container.h (right): https://codereview.chromium.org/553243002/diff/220001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/browser_actions_container.h:12: #include "chrome/browser/ui/views/extensions/extension_keybinding_registry_views.h" On 2014/09/11 23:57:09, David Tseng wrote: > On 2014/09/11 21:38:13, Devlin wrote: > > Shouldn't need to include this. > > I actually do due to the accesser which calls > extension_keybinding_registry.get(); it's also a toss up whether that's still > considered a trivial accesser (and therefore qualifies for the unix style > naming). A forward-declaration is sufficient, because you don't need to know any information about the type for the get() command - get() is actually a method of the scoped pointer (and is perfectly trivial for a unix_hacker_style accesssor). Though, since you brought it up, this probably *should* #include base/memory/scoped_ptr.h https://codereview.chromium.org/553243002/diff/220001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/toolbar_view.cc (right): https://codereview.chromium.org/553243002/diff/220001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_view.cc:277: extensions::ExtensionCommandsGlobalRegistry::Get(browser_->profile()) On 2014/09/11 23:57:09, David Tseng wrote: > On 2014/09/11 21:40:00, Devlin wrote: > > On 2014/09/11 21:38:13, Devlin wrote: > > > We also need to unset this, either in ToolbarView or > BrowserActionsContainer, > > > when the BrowserActionsContainer gets destroyed. It would probably make the > > > most sense to do in a OnWidgetDestroyed() method. > > > > And also when activation is lost, and not necessarily sent to another window > > with a toolbar (like, e.g., an app window). So also just an else clause to > this > > part. > > That's what I considered originally...but are we guaranteed this widget looses > visibility before the new widget gains/is notified of its visibility change? Sky knows best here, so I definitely defer to him. :) But a cursory look through the code implied that no, we won't get a visibility change prior to a deletion (so we need OnWidgetDestroyed()). https://codereview.chromium.org/553243002/diff/240001/chrome/browser/extensio... File chrome/browser/extensions/extension_commands_global_registry.h (right): https://codereview.chromium.org/553243002/diff/240001/chrome/browser/extensio... chrome/browser/extensions/extension_commands_global_registry.h:79: // The active ExtensionKeybindingRegistry. I might be a bit more specific here and say something like "The ExtensionKeybindingRegistry that belongs to the active window. Only valid if using TOOLKIT_VIEWS." Whether or not we should #ifdef this code out for non-toolkit code... I don't mind it being here (since it will be NULL otherwise, which is a logical value), but I'll leave it to Finnur. https://codereview.chromium.org/553243002/diff/240001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/toolbar_view.cc (right): https://codereview.chromium.org/553243002/diff/240001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_view.cc:283: ->set_active_registry(registry); Not quite. We should only set this to NULL if we *were* the active registry. Something like: ExtensionCommandsGlobalRegistry* global_command_registry = ExtensionCommandsGlobalRegistry::Get(browser_->profile()); if (visible) { extension_message_bubble_factory_->MaybeShow(app_menu_); global_command_registry->set_active_registry( browser_actions_->extension_keybinding_registry()); } else if (global_command_registry->active_registry() == browser_actions_->extension_keybinding_registry()) { global_command_registry->set_active_registry(NULL); } The point is to set the registry to NULL if and only if this *was* the active registry to begin with (otherwise, the currently-active registry will still be valid). Same goes for OnWidgetDestroyed() code.
https://codereview.chromium.org/553243002/diff/220001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/browser_actions_container.h (right): https://codereview.chromium.org/553243002/diff/220001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/browser_actions_container.h:12: #include "chrome/browser/ui/views/extensions/extension_keybinding_registry_views.h" On 2014/09/12 00:10:33, Devlin wrote: > On 2014/09/11 23:57:09, David Tseng wrote: > > On 2014/09/11 21:38:13, Devlin wrote: > > > Shouldn't need to include this. > > > > I actually do due to the accesser which calls > > extension_keybinding_registry.get(); it's also a toss up whether that's still > > considered a trivial accesser (and therefore qualifies for the unix style > > naming). > > A forward-declaration is sufficient, because you don't need to know any > information about the type for the get() command - get() is actually a method of > the scoped pointer (and is perfectly trivial for a unix_hacker_style accesssor). > > Though, since you brought it up, this probably *should* #include > base/memory/scoped_ptr.h I misspoke perhaps, but the cast to ExtensionKeybindingRegistry does require the header...I could return an ExtensionKeybindingsViews, but then would need to up cast everywhere I'm trying to set.
https://codereview.chromium.org/553243002/diff/240001/chrome/browser/extensio... File chrome/browser/extensions/extension_commands_global_registry.h (right): https://codereview.chromium.org/553243002/diff/240001/chrome/browser/extensio... chrome/browser/extensions/extension_commands_global_registry.h:79: // The active ExtensionKeybindingRegistry. On 2014/09/12 00:10:33, Devlin wrote: > I might be a bit more specific here and say something like "The > ExtensionKeybindingRegistry that belongs to the active window. Only valid if > using TOOLKIT_VIEWS." > > Whether or not we should #ifdef this code out for non-toolkit code... I don't > mind it being here (since it will be NULL otherwise, which is a logical value), > but I'll leave it to Finnur. Done (the comment part). https://codereview.chromium.org/553243002/diff/240001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/toolbar_view.cc (right): https://codereview.chromium.org/553243002/diff/240001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_view.cc:283: ->set_active_registry(registry); On 2014/09/12 00:10:33, Devlin wrote: > Not quite. We should only set this to NULL if we *were* the active registry. > Something like: > > ExtensionCommandsGlobalRegistry* global_command_registry = > ExtensionCommandsGlobalRegistry::Get(browser_->profile()); > if (visible) { > extension_message_bubble_factory_->MaybeShow(app_menu_); > global_command_registry->set_active_registry( > browser_actions_->extension_keybinding_registry()); > } else if (global_command_registry->active_registry() == > browser_actions_->extension_keybinding_registry()) { > global_command_registry->set_active_registry(NULL); > } > > The point is to set the registry to NULL if and only if this *was* the active > registry to begin with (otherwise, the currently-active registry will still be > valid). > > Same goes for OnWidgetDestroyed() code. Yup; you're totally right. Done.
I like where this is heading. https://codereview.chromium.org/553243002/diff/80001/chrome/browser/extension... File chrome/browser/extensions/extension_keybinding_registry_tracker.h (right): https://codereview.chromium.org/553243002/diff/80001/chrome/browser/extension... chrome/browser/extensions/extension_keybinding_registry_tracker.h:19: class ExtensionKeybindingRegistryTracker : public BrowserContextKeyedAPI { Thanks Devlin, for weighing in. It was good to get a fresh perspective on this. https://codereview.chromium.org/553243002/diff/240001/chrome/browser/extensio... File chrome/browser/extensions/extension_commands_global_registry.h (right): https://codereview.chromium.org/553243002/diff/240001/chrome/browser/extensio... chrome/browser/extensions/extension_commands_global_registry.h:79: // The active ExtensionKeybindingRegistry. Rather than if-def'ing I'm actually more interested in getting this to work on other platforms, as I think this is useful going forward. But that's probably beyond the scope of this. https://codereview.chromium.org/553243002/diff/280001/chrome/browser/chromeos... File chrome/browser/chromeos/events/event_rewriter.cc (right): https://codereview.chromium.org/553243002/diff/280001/chrome/browser/chromeos... chrome/browser/chromeos/events/event_rewriter.cc:125: ->active_registry(); This checks the active window's registry. But what conclusion did we reach with global shortcuts? Do we not need to do anything for those because they don't pass through the keyboard stack in the normal way (and therefore don't get rewritten)? Or should we could call IsAcceleratorRegistered on the global registry also? If so, then one thing we could do is add an IsRegistered(accelerator) method on the global registry and call that on line 141 instead of mucking with the active_registry here. In that new method (IsRegistered) call IsAcceleratorRegistered on both the active and the global registry. Then you also don't have to make IsAcceleratorRegistered public, I believe. In any case, would be good to add a test for global shortcut. https://codereview.chromium.org/553243002/diff/280001/chrome/browser/chromeos... chrome/browser/chromeos/events/event_rewriter.cc:143: return false; nit: prefer: return foo || bar; over if (foo || bar) return true; return false; https://codereview.chromium.org/553243002/diff/280001/chrome/browser/extensio... File chrome/browser/extensions/extension_commands_global_registry.h (right): https://codereview.chromium.org/553243002/diff/280001/chrome/browser/extensio... chrome/browser/extensions/extension_commands_global_registry.h:36: ExtensionKeybindingRegistry* active_registry() { return active_registry_; } I'd argue that documenting this function is even more important than documenting the variable below. I'm also a little bit worried that this is ambiguous (because there is also an active *global* commands registry, which this function does *not* return). I propose a name change: // Returns which non-global command registry is active (belonging // to the currently active window). registry_for_active_window() set_registry_for_active_window() and ExtensionKeybindingRegistry* registry_for_active_window_; https://codereview.chromium.org/553243002/diff/280001/chrome/browser/extensio... chrome/browser/extensions/extension_commands_global_registry.h:40: } Also, shouldn't these two functions be below the ctor and dtor? https://codereview.chromium.org/553243002/diff/280001/chrome/browser/extensio... chrome/browser/extensions/extension_commands_global_registry.h:79: // The ExtensionKeybindingRegistry belonging to the active window. Only valid I'd make the distinction non-global and global clear here, with something like: // The global commands registry not only keeps track of global commands // registered, but also of which non-global command registry is active // (belonging to the currently active window). Only valid for TOOLKIT_VIEWS // and NULL otherwise. https://codereview.chromium.org/553243002/diff/280001/chrome/browser/extensio... File chrome/browser/extensions/extension_keybinding_apitest.cc (right): https://codereview.chromium.org/553243002/diff/280001/chrome/browser/extensio... chrome/browser/extensions/extension_keybinding_apitest.cc:746: // Send all expected keys. Please document what keys those are (Search+Shift+Left), etc. https://codereview.chromium.org/553243002/diff/280001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/toolbar_view.cc (right): https://codereview.chromium.org/553243002/diff/280001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_view.cc:279: registry->set_active_registry( Prefer line break above -- these code blocks are not related. https://codereview.chromium.org/553243002/diff/280001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/keybinding/chromeos_conversions/manifest.json (right): https://codereview.chromium.org/553243002/diff/280001/chrome/test/data/extens... chrome/test/data/extensions/api_test/keybinding/chromeos_conversions/manifest.json:2: "name": "An extension to test shortcuts propagation", s/shortcuts propagation/non-conversion of Chrome OS keys/
https://codereview.chromium.org/553243002/diff/280001/chrome/browser/chromeos... File chrome/browser/chromeos/events/event_rewriter.cc (right): https://codereview.chromium.org/553243002/diff/280001/chrome/browser/chromeos... chrome/browser/chromeos/events/event_rewriter.cc:125: ->active_registry(); On 2014/09/12 11:25:44, Finnur wrote: > This checks the active window's registry. But what conclusion did we reach with > global shortcuts? Do we not need to do anything for those because they don't > pass through the keyboard stack in the normal way (and therefore don't get > rewritten)? EventRewriter hooks in pretty early so it does rewrite both global and nonglobal accelerators (which is why we check both below on ln 141). > > Or should we could call IsAcceleratorRegistered on the global registry also? If > so, then one thing we could do is add an IsRegistered(accelerator) method on the > global registry and call that on line 141 instead of mucking with the > active_registry here. In that new method (IsRegistered) call > IsAcceleratorRegistered on both the active and the global registry. Then you > also don't have to make IsAcceleratorRegistered public, I believe. > Again, I'm kind of against the mixing of global and non-global. Do we not really care about distinguishing between the two? Only I see EventRewriter as a special case. It wants to deal with both global and non global commands in the same way. If you're ok with mixing the two concepts, then I'll add the new IsRegistered, but think it will cause all kinds of confusion down the line. I'm actually more in favor of placing IsRegistered on ExtensionKeybinding which would check both the global registry and the current instance. That makes more sense to me. > In any case, would be good to add a test for global shortcut. I do agree with this. Can mirror the current extension but make each command global. Actually, just tried doing this and turns out it won't work because of the restrictions placed on global commands being ctrl+shift+[0-9]. Punting on this one for another cl, but will leave the helper test function in for now. https://codereview.chromium.org/553243002/diff/280001/chrome/browser/chromeos... chrome/browser/chromeos/events/event_rewriter.cc:143: return false; On 2014/09/12 11:25:44, Finnur wrote: > nit: prefer: > > return foo || bar; > > over > > if (foo || bar) > return true; > return false; Done. https://codereview.chromium.org/553243002/diff/280001/chrome/browser/extensio... File chrome/browser/extensions/extension_commands_global_registry.h (right): https://codereview.chromium.org/553243002/diff/280001/chrome/browser/extensio... chrome/browser/extensions/extension_commands_global_registry.h:36: ExtensionKeybindingRegistry* active_registry() { return active_registry_; } On 2014/09/12 11:25:44, Finnur wrote: > I'd argue that documenting this function is even more important than documenting > the variable below. I'm also a little bit worried that this is ambiguous > (because there is also an active *global* commands registry, which this function > does *not* return). Right, this was my concern as well. But, considering I didn't have a strong opinion on the tracker strategy (though I'm now rethinking that), we went with this somewhat confusing approach. > > I propose a name change: > > > // Returns which non-global command registry is active (belonging > // to the currently active window). > registry_for_active_window() > Apparently, trivial accessers don't require comments, but in this case, would be helpful yes. > set_registry_for_active_window() > > and > > ExtensionKeybindingRegistry* registry_for_active_window_; Ok. Made this change. https://codereview.chromium.org/553243002/diff/280001/chrome/browser/extensio... chrome/browser/extensions/extension_commands_global_registry.h:40: } On 2014/09/12 11:25:44, Finnur wrote: > Also, shouldn't these two functions be below the ctor and dtor? Done. https://codereview.chromium.org/553243002/diff/280001/chrome/browser/extensio... chrome/browser/extensions/extension_commands_global_registry.h:79: // The ExtensionKeybindingRegistry belonging to the active window. Only valid On 2014/09/12 11:25:44, Finnur wrote: > I'd make the distinction non-global and global clear here, with something like: > > // The global commands registry not only keeps track of global commands > // registered, but also of which non-global command registry is active > // (belonging to the currently active window). Only valid for TOOLKIT_VIEWS > // and NULL otherwise. Done. https://codereview.chromium.org/553243002/diff/280001/chrome/browser/extensio... File chrome/browser/extensions/extension_keybinding_apitest.cc (right): https://codereview.chromium.org/553243002/diff/280001/chrome/browser/extensio... chrome/browser/extensions/extension_keybinding_apitest.cc:746: // Send all expected keys. On 2014/09/12 11:25:44, Finnur wrote: > Please document what keys those are (Search+Shift+Left), etc. That seems unnecessary since the keys are listed below? If you find it more readable, I suppose, but seems pretty clear from the SendKeyPressSync calls. https://codereview.chromium.org/553243002/diff/280001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/toolbar_view.cc (right): https://codereview.chromium.org/553243002/diff/280001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_view.cc:279: registry->set_active_registry( On 2014/09/12 11:25:44, Finnur wrote: > Prefer line break above -- these code blocks are not related. Done. https://codereview.chromium.org/553243002/diff/280001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/keybinding/chromeos_conversions/manifest.json (right): https://codereview.chromium.org/553243002/diff/280001/chrome/test/data/extens... chrome/test/data/extensions/api_test/keybinding/chromeos_conversions/manifest.json:2: "name": "An extension to test shortcuts propagation", On 2014/09/12 11:25:44, Finnur wrote: > s/shortcuts propagation/non-conversion of Chrome OS keys/ Done.
The CL description seems a bit mechanical. I would go with something like: Add the ability to query globally if a shortcut is registered by an extension and use that to avoid unnecessarily rewriting Chrome OS keys. ... or something. https://codereview.chromium.org/553243002/diff/280001/chrome/browser/chromeos... File chrome/browser/chromeos/events/event_rewriter.cc (right): https://codereview.chromium.org/553243002/diff/280001/chrome/browser/chromeos... chrome/browser/chromeos/events/event_rewriter.cc:125: ->active_registry(); > (which is why we check both below on ln 141). Oh, woops. I overlooked the fact we are checking both local and global. Sorry. :) > If you're ok with mixing the two concepts, then I'll add the > new IsRegistered Unless we have a more elegant approach, I think we should go with it. The alternatives I've seen so far just add complex elsewhere. With good function naming and good comments we can alleviate potential confusion. > I'm actually more in favor of placing IsRegistered on > ExtensionKeybinding which would check both the global registry > and the current instance. I would argue the opposite is simpler. As a consumer, I would be wanting to know if a shortcut is in use anywhere (i.e. globally within Chrome). It would be weirder to me to have to ask a local object for that info as opposed to the global object. It also would complicate the code here, because instead of accessing and querying just the global object, you'd have to start querying which window is active, etc. > Actually, just tried doing this and turns out it won't work > because of the restrictions placed on global commands being > ctrl+shift+[0-9]. You had a CL that relaxes restrictions. Maybe it is time to extend that to tests? I can see that being useful in the future as well. I'm ok with you doing that in a follow-up CL to this one, if you prefer. https://codereview.chromium.org/553243002/diff/280001/chrome/browser/extensio... File chrome/browser/extensions/extension_keybinding_apitest.cc (right): https://codereview.chromium.org/553243002/diff/280001/chrome/browser/extensio... chrome/browser/extensions/extension_keybinding_apitest.cc:746: // Send all expected keys. I do find it more readable. I can never remember what all those booleans mean (well, I know they mean Command, Control, Alt, and Shift, but I always have to look up the order to be sure). https://codereview.chromium.org/553243002/diff/280001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/keybinding/chromeos_conversions/manifest.json (right): https://codereview.chromium.org/553243002/diff/280001/chrome/test/data/extens... chrome/test/data/extensions/api_test/keybinding/chromeos_conversions/manifest.json:8: "permissions": [ "tabs" ], Good catch.
https://codereview.chromium.org/553243002/diff/280001/chrome/browser/chromeos... File chrome/browser/chromeos/events/event_rewriter.cc (right): https://codereview.chromium.org/553243002/diff/280001/chrome/browser/chromeos... chrome/browser/chromeos/events/event_rewriter.cc:125: ->active_registry(); On 2014/09/15 10:48:42, Finnur wrote: > > (which is why we check both below on ln 141). > > Oh, woops. I overlooked the fact we are checking both local and global. Sorry. > :) > > > If you're ok with mixing the two concepts, then I'll add the > > new IsRegistered > > Unless we have a more elegant approach, I think we should go with it. The > alternatives I've seen so far just add complex elsewhere. With good function > naming and good comments we can alleviate potential confusion. > > > I'm actually more in favor of placing IsRegistered on > > ExtensionKeybinding which would check both the global registry > > and the current instance. > > I would argue the opposite is simpler. As a consumer, I would be wanting to know > if a shortcut is in use anywhere (i.e. globally within Chrome). It would be > weirder to me to have to ask a local object for that info as opposed to the > global object. It also would complicate the code here, because instead of > accessing and querying just the global object, you'd have to start querying > which window is active, etc. > > > Actually, just tried doing this and turns out it won't work > > because of the restrictions placed on global commands being > > ctrl+shift+[0-9]. > > You had a CL that relaxes restrictions. Maybe it is time to extend that to > tests? I can see that being useful in the future as well. I'm ok with you doing > that in a follow-up CL to this one, if you prefer. Added IsRegisteredForActiveWindow to GlobalCommandsRegistry as suggested. Unfortunately, we still need to make IsAcceleratorRegistered public since we need to call it on the registry for active window. Will add a test for the global case in a follow up due to the relaxation piece. https://codereview.chromium.org/553243002/diff/280001/chrome/browser/extensio... File chrome/browser/extensions/extension_keybinding_apitest.cc (right): https://codereview.chromium.org/553243002/diff/280001/chrome/browser/extensio... chrome/browser/extensions/extension_keybinding_apitest.cc:746: // Send all expected keys. On 2014/09/15 10:48:42, Finnur wrote: > I do find it more readable. I can never remember what all those booleans mean > (well, I know they mean Command, Control, Alt, and Shift, but I always have to > look up the order to be sure). Done.
PTAL sky@ for OWNERS.
https://codereview.chromium.org/553243002/diff/400001/chrome/browser/chromeos... File chrome/browser/chromeos/events/event_rewriter.cc (right): https://codereview.chromium.org/553243002/diff/400001/chrome/browser/chromeos... chrome/browser/chromeos/events/event_rewriter.cc:124: if (key_event.IsShiftDown()) Isn't this the same as looking at flags(), eg flags = key_event.flags() & (modifiers you care about)? https://codereview.chromium.org/553243002/diff/400001/chrome/browser/chromeos... chrome/browser/chromeos/events/event_rewriter.cc:134: return extensions::ExtensionCommandsGlobalRegistry::Get(profile) Is the global registry always non-NULL? https://codereview.chromium.org/553243002/diff/400001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/toolbar_view.cc (right): https://codereview.chromium.org/553243002/diff/400001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_view.cc:282: if (visible) { Isn't it the active state and not the visibility you care about?
https://codereview.chromium.org/553243002/diff/400001/chrome/browser/chromeos... File chrome/browser/chromeos/events/event_rewriter.cc (right): https://codereview.chromium.org/553243002/diff/400001/chrome/browser/chromeos... chrome/browser/chromeos/events/event_rewriter.cc:134: return extensions::ExtensionCommandsGlobalRegistry::Get(profile) On 2014/09/16 02:34:48, sky wrote: > Is the global registry always non-NULL? Turned out the ProfileManager above can be NULL in a unit test, so Profile* can be NULL and likely ExtensionCommandsGlobalRegistry. Checking them all now. https://codereview.chromium.org/553243002/diff/400001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/toolbar_view.cc (right): https://codereview.chromium.org/553243002/diff/400001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_view.cc:282: if (visible) { On 2014/09/16 02:34:48, sky wrote: > Isn't it the active state and not the visibility you care about? You're right. Added WidgetObserver override.
Patchset #18 (id:380001) has been deleted
LGTM, with one nit. https://codereview.chromium.org/553243002/diff/420001/chrome/browser/extensio... File chrome/browser/extensions/extension_commands_global_registry.h (right): https://codereview.chromium.org/553243002/diff/420001/chrome/browser/extensio... chrome/browser/extensions/extension_commands_global_registry.h:62: bool IsRegisteredForActiveWindow(const ui::Accelerator& accelerator); This should just be IsRegistered(). We're not just checking the active window. https://codereview.chromium.org/553243002/diff/420001/chrome/browser/extensio... File chrome/browser/extensions/extension_keybinding_apitest.cc (right): https://codereview.chromium.org/553243002/diff/420001/chrome/browser/extensio... chrome/browser/extensions/extension_keybinding_apitest.cc:732: // TODO(dtseng): Test times out on Chrome OS debug. See http://crbug.com/412456. I presume this isn't directly because of this changelist, and as such: Thank you for investigating.
https://codereview.chromium.org/553243002/diff/420001/chrome/browser/extensio... File chrome/browser/extensions/extension_commands_global_registry.h (right): https://codereview.chromium.org/553243002/diff/420001/chrome/browser/extensio... chrome/browser/extensions/extension_commands_global_registry.h:62: bool IsRegisteredForActiveWindow(const ui::Accelerator& accelerator); On 2014/09/16 10:29:14, Finnur wrote: > This should just be IsRegistered(). We're not just checking the active window. Done. Comment should suffice to disambiguate IsRegistered vs IsAcceleratorRegistered I suppose.
https://codereview.chromium.org/553243002/diff/400001/chrome/browser/chromeos... File chrome/browser/chromeos/events/event_rewriter.cc (right): https://codereview.chromium.org/553243002/diff/400001/chrome/browser/chromeos... chrome/browser/chromeos/events/event_rewriter.cc:124: if (key_event.IsShiftDown()) On 2014/09/16 02:34:48, sky wrote: > Isn't this the same as looking at flags(), eg > flags = key_event.flags() & (modifiers you care about)? It doesn't seem like you addressed this comment. https://codereview.chromium.org/553243002/diff/440001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/toolbar_view.cc (right): https://codereview.chromium.org/553243002/diff/440001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_view.cc:295: void ToolbarView::OnWidgetDestroyed(views::Widget* widget) { Is this needed? Won't you get OnWidgetActivationChanged before OnWidgetDestroyed?
https://codereview.chromium.org/553243002/diff/400001/chrome/browser/chromeos... File chrome/browser/chromeos/events/event_rewriter.cc (right): https://codereview.chromium.org/553243002/diff/400001/chrome/browser/chromeos... chrome/browser/chromeos/events/event_rewriter.cc:124: if (key_event.IsShiftDown()) On 2014/09/16 02:34:48, sky wrote: > Isn't this the same as looking at flags(), eg > flags = key_event.flags() & (modifiers you care about)? That would indeed simplify this logic. Done. https://codereview.chromium.org/553243002/diff/440001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/toolbar_view.cc (right): https://codereview.chromium.org/553243002/diff/440001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_view.cc:295: void ToolbarView::OnWidgetDestroyed(views::Widget* widget) { On 2014/09/16 16:44:35, sky wrote: > Is this needed? Won't you get OnWidgetActivationChanged before > OnWidgetDestroyed? On digging through some of the views code, I don't know if this is guaranteed.
https://codereview.chromium.org/553243002/diff/440001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/toolbar_view.cc (right): https://codereview.chromium.org/553243002/diff/440001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_view.cc:295: void ToolbarView::OnWidgetDestroyed(views::Widget* widget) { On 2014/09/16 20:42:57, David Tseng wrote: > On 2014/09/16 16:44:35, sky wrote: > > Is this needed? Won't you get OnWidgetActivationChanged before > > OnWidgetDestroyed? > > On digging through some of the views code, I don't know if this is guaranteed. What case do you think this isn't guaranteed?
https://codereview.chromium.org/553243002/diff/440001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/toolbar_view.cc (right): https://codereview.chromium.org/553243002/diff/440001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_view.cc:295: void ToolbarView::OnWidgetDestroyed(views::Widget* widget) { On 2014/09/16 22:05:49, sky wrote: > On 2014/09/16 20:42:57, David Tseng wrote: > > On 2014/09/16 16:44:35, sky wrote: > > > Is this needed? Won't you get OnWidgetActivationChanged before > > > OnWidgetDestroyed? > > > > On digging through some of the views code, I don't know if this is guaranteed. > > What case do you think this isn't guaranteed? What happens if I close a browser window? Would I get a de-activation before destruction call? Seems like this varies by platform. I removed the call since I'll be the first to admit I have only a naive understanding of when these methods get called on the various platforms and reading the code didn't help that much. Hope this looks ok now.
LGTM https://codereview.chromium.org/553243002/diff/440001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/toolbar_view.cc (right): https://codereview.chromium.org/553243002/diff/440001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_view.cc:295: void ToolbarView::OnWidgetDestroyed(views::Widget* widget) { On 2014/09/17 01:01:53, David Tseng wrote: > On 2014/09/16 22:05:49, sky wrote: > > On 2014/09/16 20:42:57, David Tseng wrote: > > > On 2014/09/16 16:44:35, sky wrote: > > > > Is this needed? Won't you get OnWidgetActivationChanged before > > > > OnWidgetDestroyed? > > > > > > On digging through some of the views code, I don't know if this is > guaranteed. > > > > What case do you think this isn't guaranteed? > > What happens if I close a browser window? Would I get a de-activation before > destruction call? Seems like this varies by platform. Your certainly should. > I removed the call since I'll be the first to admit I have only a naive > understanding of when these methods get called on the various platforms and > reading the code didn't help that much. > > Hope this looks ok now. >
The CQ bit was checked by dtseng@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patchset/553243002/480001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_swarming on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by dtseng@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patchset/553243002/480001
Message was sent while issue was closed.
Committed patchset #22 (id:480001) as 62804f56ccee0c56e7d7f91f60dee3e48e3cab9d
Message was sent while issue was closed.
Patchset 22 (id:??) landed as https://crrev.com/4264d2c4b5d217e1c5dbc2ae80ab6ce595f9b6d3 Cr-Commit-Position: refs/heads/master@{#295284}
Message was sent while issue was closed.
A revert of this CL (patchset #22 id:480001) has been created in https://codereview.chromium.org/575093003/ by erg@chromium.org. The reason for reverting is: The new tests fail on Linux Chromeos (ozone). http://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%2.... |
