Conflict detection for Extension Keybinding.

Conflict detection for Extension Keybinding.

At install time, the extension will now try to make its suggested keybindings active. If the keybinding is free, then the extension is successful (before this CL the last registered extension would win). If the keybinding is not free, the keybinding becomes inactive. I've introduced a pref that keeps track of which keybinding is active, which will become the basis for a config UI.

This changelist also fixes a DCHECK on close of Chrome if an extension with a keybinding has been uninstalled (we were unregistering twice).

BUG=121419
TEST=Covered in automated test. (Install two extensions with the same keybinding. Only the first should get its keybinding activated).
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=135115

[Aaron Boodman, 2012-04-25 19:14:38]

http://codereview.chromium.org/10201016/diff/10004/chrome/browser/extensions/...
File chrome/browser/extensions/extension_keybinding_registry.cc (right):

http://codereview.chromium.org/10201016/diff/10004/chrome/browser/extensions/...
chrome/browser/extensions/extension_keybinding_registry.cc:56:
PrefService::UNSYNCABLE_PREF);
This seems like something we'd want to sync.

http://codereview.chromium.org/10201016/diff/10004/chrome/browser/extensions/...
chrome/browser/extensions/extension_keybinding_registry.cc:64: const
Extension::ExtensionKeybinding* command =
In a separate change, uou should move ExtensionKeybindingRegistry to
extensions::KeybindingRegistry. Sorry for not noticing this earlier.

http://codereview.chromium.org/10201016/diff/10004/chrome/browser/extensions/...
chrome/browser/extensions/extension_keybinding_registry.cc:65:
profile->GetExtensionService()->GetExtensionById(
If there is no such extension_id, we crash. Is that OK? Do we know for sure at
the times this is called that the extension ID is legit?

If the assumption is that only enabled extension_id's are passed, store the
returned const Extension* and CHECK that it is non-null.

Otherwise when someone inevitably calls with with a bad extension ID we will go
off into lala land for a little while before crashing because
extension->browser_action_command() and command->accelerator() are both
non-virtual.

http://codereview.chromium.org/10201016/diff/10004/chrome/browser/extensions/...
chrome/browser/extensions/extension_keybinding_registry.cc:69: return
ExtensionKeybindingRegistry::KeybindingActive(
Nit: Since this function call is so long, I would prefer to see it as if/else
rather than a ternary operator.

http://codereview.chromium.org/10201016/diff/10004/chrome/browser/extensions/...
chrome/browser/extensions/extension_keybinding_registry.cc:69: return
ExtensionKeybindingRegistry::KeybindingActive(
Do not need the "ExtensionKeybindingRegistry::" qualifier here. Same in other
places in this file.

http://codereview.chromium.org/10201016/diff/10004/chrome/browser/extensions/...
chrome/browser/extensions/extension_keybinding_registry.cc:161:
DCHECK(!extension_id.empty());
I prefer CHECK to DCHECK

http://codereview.chromium.org/10201016/diff/10004/chrome/browser/extensions/...
chrome/browser/extensions/extension_keybinding_registry.cc:170: DictionaryValue*
value;
= NULL

http://codereview.chromium.org/10201016/diff/10004/chrome/browser/extensions/...
chrome/browser/extensions/extension_keybinding_registry.cc:208: void
ExtensionKeybindingRegistry::RemoveKeybindingPref(
RemoveKeybindingPrefs (plural) since this can remove multiple keybindings?

http://codereview.chromium.org/10201016/diff/10004/chrome/browser/extensions/...
File chrome/browser/extensions/extension_keybinding_registry.h (right):

http://codereview.chromium.org/10201016/diff/10004/chrome/browser/extensions/...
chrome/browser/extensions/extension_keybinding_registry.h:40: static const
Extension::ExtensionKeybinding* GetActiveBrowserActionCommand(
The names being used here are making this code more confusing that it needs to
be.

What do you think about changing Extension::ExtensionKeybinding to
Extension::Command? That makes more sense since it more represent an abstract
named command than a keybinding.

Also, is Extension::Keybinding::accelerator the assigned accelerator, or only
the suggested accelerator, from the manifest. If the latter, renaming it
suggested or default accelerator would be more clear.

Then the comment would be updated to something like:

Gets the command for |extension_id|'s browser action if it has an active
keybinding. Otherwise, returns NULL.

The one for command map would be:

Gets the commands for |extension_id| that have active keybindings. If the
extension has commands that do not have active keybindings, those commands are
not included in the returned map.

All of these naming cleanup changes can be done in a separate CL.

http://codereview.chromium.org/10201016/diff/10004/chrome/browser/extensions/...
chrome/browser/extensions/extension_keybinding_registry.h:62: static void
ResolveKeyBindings(PrefService* user_prefs,
You are using Keybinding (not KeyBinding) everywhere else.

http://codereview.chromium.org/10201016/diff/10004/chrome/browser/extensions/...
chrome/browser/extensions/extension_keybinding_registry.h:62: static void
ResolveKeyBindings(PrefService* user_prefs,
AssignInitialKeybindings?

http://codereview.chromium.org/10201016/diff/10004/chrome/browser/extensions/...
chrome/browser/extensions/extension_keybinding_registry.h:68: static bool
KeybindingActive(PrefService* user_prefs,
Verbify: IsKeybindingActive

http://codereview.chromium.org/10201016/diff/10004/chrome/browser/extensions/...
File chrome/browser/extensions/extension_service.cc (right):

http://codereview.chromium.org/10201016/diff/10004/chrome/browser/extensions/...
chrome/browser/extensions/extension_service.cc:813:
ExtensionKeybindingRegistry::RemoveKeybindingPref(profile_->GetPrefs(),
Please avoid adding more glue code here. Instead, make KeybindingRegistry listen
to EXTENSION_INSATLLED and EXTENSION_UNINSTALLED and do this work itself.

http://codereview.chromium.org/10201016/diff/10004/chrome/common/extensions/e...
File chrome/common/extensions/extension.h (right):

http://codereview.chromium.org/10201016/diff/10004/chrome/common/extensions/e...
chrome/common/extensions/extension.h:589: // The browser action command
keybinding that the extension wants to use,
Sigh. Bummer to carry this information around in every process when it isn't
even correct.

Can you make these getters instead, and pull the information dynamically out of
manifest_?

It still stinks to carry around the manifest, but that is a different problem.

This can be a separate change.

[Finnur, 2012-04-26 13:12:35]

PTAL

http://codereview.chromium.org/10201016/diff/10004/chrome/browser/extensions/...
File chrome/browser/extensions/extension_keybinding_registry.cc (right):

http://codereview.chromium.org/10201016/diff/10004/chrome/browser/extensions/...
chrome/browser/extensions/extension_keybinding_registry.cc:56:
PrefService::UNSYNCABLE_PREF);
Yes. I've now changed this to SYNCABLE_PREF. I was initially planning on
delaying it a bit because we need to sync this value with care so as to not
clobber keybindings on one platform with the keybindings with another. However,
I've made it so that the keybinding key we store it under takes platform into
account, so that should no longer be an issue.

On 2012/04/25 19:14:38, Aaron Boodman wrote:
> This seems like something we'd want to sync.

http://codereview.chromium.org/10201016/diff/10004/chrome/browser/extensions/...
chrome/browser/extensions/extension_keybinding_registry.cc:64: const
Extension::ExtensionKeybinding* command =
Yup. Sounds good.

On 2012/04/25 19:14:38, Aaron Boodman wrote:
> In a separate change, uou should move ExtensionKeybindingRegistry to
> extensions::KeybindingRegistry. Sorry for not noticing this earlier.

http://codereview.chromium.org/10201016/diff/10004/chrome/browser/extensions/...
chrome/browser/extensions/extension_keybinding_registry.cc:65:
profile->GetExtensionService()->GetExtensionById(
Callers are always holding what I assume to be valid Extension pointers when
they call these functions. If they are not then we have a problem (elsewhere) I
think. So, I've added a CHECK.

On 2012/04/25 19:14:38, Aaron Boodman wrote:
> If there is no such extension_id, we crash. Is that OK? Do we know for sure at
> the times this is called that the extension ID is legit?
> 
> If the assumption is that only enabled extension_id's are passed, store the
> returned const Extension* and CHECK that it is non-null.
> 
> Otherwise when someone inevitably calls with with a bad extension ID we will
go
> off into lala land for a little while before crashing because
> extension->browser_action_command() and command->accelerator() are both
> non-virtual.

http://codereview.chromium.org/10201016/diff/10004/chrome/browser/extensions/...
chrome/browser/extensions/extension_keybinding_registry.cc:69: return
ExtensionKeybindingRegistry::KeybindingActive(
Both done.

On 2012/04/25 19:14:38, Aaron Boodman wrote:
> Do not need the "ExtensionKeybindingRegistry::" qualifier here. Same in other
> places in this file.

http://codereview.chromium.org/10201016/diff/10004/chrome/browser/extensions/...
File chrome/browser/extensions/extension_keybinding_registry.h (right):

http://codereview.chromium.org/10201016/diff/10004/chrome/browser/extensions/...
chrome/browser/extensions/extension_keybinding_registry.h:40: static const
Extension::ExtensionKeybinding* GetActiveBrowserActionCommand(
Yup. Sounds good.

On 2012/04/25 19:14:38, Aaron Boodman wrote:
> The names being used here are making this code more confusing that it needs to
> be.
> 
> What do you think about changing Extension::ExtensionKeybinding to
> Extension::Command? That makes more sense since it more represent an abstract
> named command than a keybinding.
> 
> Also, is Extension::Keybinding::accelerator the assigned accelerator, or only
> the suggested accelerator, from the manifest. If the latter, renaming it
> suggested or default accelerator would be more clear.
> 
> Then the comment would be updated to something like:
> 
> Gets the command for |extension_id|'s browser action if it has an active
> keybinding. Otherwise, returns NULL.
> 
> The one for command map would be:
> 
> Gets the commands for |extension_id| that have active keybindings. If the
> extension has commands that do not have active keybindings, those commands are
> not included in the returned map.
> 
> All of these naming cleanup changes can be done in a separate CL.

http://codereview.chromium.org/10201016/diff/10004/chrome/browser/extensions/...
File chrome/browser/extensions/extension_service.cc (right):

http://codereview.chromium.org/10201016/diff/10004/chrome/browser/extensions/...
chrome/browser/extensions/extension_service.cc:813:
ExtensionKeybindingRegistry::RemoveKeybindingPref(profile_->GetPrefs(),
I thought about that. The problem is that the ExtensionKeybindingRegistry class
is a base class for common Keybinding functionality across platforms. On Views,
for example, the ExtensionKeybindingRegistryViews is tied to a window because it
needs to communicate with the FocusManager for that window.

Long story short: There can be multiple ExtensionKeybindingRegistry objects
around (if you have multiple windows), which would then all listen for that
uninstall event and all try to remove the prefs. Not a big problem, but seemed
less wasteful to do this once here with a static function than to do it multiple
times from each observer.

I can make that change though, if you feel strongly about it. I also toyed with
the idea of adding this to the extension_prefs_->OnExtensionUninstalled() call
on the line below (since this is after all dealing with Extension Prefs). Is
that more acceptable?

On 2012/04/25 19:14:38, Aaron Boodman wrote:
> Please avoid adding more glue code here. Instead, make KeybindingRegistry
listen
> to EXTENSION_INSATLLED and EXTENSION_UNINSTALLED and do this work itself.

http://codereview.chromium.org/10201016/diff/10004/chrome/browser/extensions/...
chrome/browser/extensions/extension_service.cc:2266: extension);
Same question here. Move it to OnExtensionInstalled (see line 2252)?

http://codereview.chromium.org/10201016/diff/10004/chrome/common/extensions/e...
File chrome/common/extensions/extension.h (right):

http://codereview.chromium.org/10201016/diff/10004/chrome/common/extensions/e...
chrome/common/extensions/extension.h:589: // The browser action command
keybinding that the extension wants to use,
Yup. Sounds good.

[Finnur, 2012-04-26 13:29:39]

Also, need OWNERS LG from:
erg: chrome/browser/ui/gtk
sky: chrome/browser/ui/views

[sky, 2012-04-26 16:07:23]

LGTM

[Aaron Boodman, 2012-04-26 19:29:30]

http://codereview.chromium.org/10201016/diff/10004/chrome/browser/extensions/...
File chrome/browser/extensions/extension_service.cc (right):

http://codereview.chromium.org/10201016/diff/10004/chrome/browser/extensions/...
chrome/browser/extensions/extension_service.cc:813:
ExtensionKeybindingRegistry::RemoveKeybindingPref(profile_->GetPrefs(),
On 2012/04/26 13:12:35, Finnur wrote:
> I thought about that. The problem is that the ExtensionKeybindingRegistry
class
> is a base class for common Keybinding functionality across platforms. On
Views,
> for example, the ExtensionKeybindingRegistryViews is tied to a window because
it
> needs to communicate with the FocusManager for that window.
> 
> Long story short: There can be multiple ExtensionKeybindingRegistry objects
> around (if you have multiple windows), which would then all listen for that
> uninstall event and all try to remove the prefs. Not a big problem, but seemed
> less wasteful to do this once here with a static function than to do it
multiple
> times from each observer.

I see. In that case, I suggest a small KeybindingGlue object (needs a better
name!) that has Profile lifetime and which is owned by ExtensionSystem. This
object would listen to EXTENSION_(UN)INSTALLED and do the appropriate
registrations.

It also has the advantage of being easily unit-testable.

[Finnur, 2012-04-26 21:06:06]

Is there a similar object in the codebase that I can model it after, to make
sure I understand what you are asking for?

On 2012/04/26 19:29:30, Aaron Boodman wrote:
>
http://codereview.chromium.org/10201016/diff/10004/chrome/browser/extensions/...
> File chrome/browser/extensions/extension_service.cc (right):
> 
>
http://codereview.chromium.org/10201016/diff/10004/chrome/browser/extensions/...
> chrome/browser/extensions/extension_service.cc:813:
> ExtensionKeybindingRegistry::RemoveKeybindingPref(profile_->GetPrefs(),
> On 2012/04/26 13:12:35, Finnur wrote:
> > I thought about that. The problem is that the ExtensionKeybindingRegistry
> class
> > is a base class for common Keybinding functionality across platforms. On
> Views,
> > for example, the ExtensionKeybindingRegistryViews is tied to a window
because
> it
> > needs to communicate with the FocusManager for that window.
> > 
> > Long story short: There can be multiple ExtensionKeybindingRegistry objects
> > around (if you have multiple windows), which would then all listen for that
> > uninstall event and all try to remove the prefs. Not a big problem, but
seemed
> > less wasteful to do this once here with a static function than to do it
> multiple
> > times from each observer.
> 
> I see. In that case, I suggest a small KeybindingGlue object (needs a better
> name!) that has Profile lifetime and which is owned by ExtensionSystem. This
> object would listen to EXTENSION_(UN)INSTALLED and do the appropriate
> registrations.
> 
> It also has the advantage of being easily unit-testable.

[Aaron Boodman, 2012-04-26 22:46:39]

On Thu, Apr 26, 2012 at 2:06 PM,  <finnur@chromium.org> wrote:
> Is there a similar object in the codebase that I can model it after, to make
> sure I understand what you are asking for?

Actually, after talking it over with Yoyo, I think we want to
completely decouple the implementation of keyboard shortcuts from the
core extension system infrastructure. This will be the wave of the
future!

The way to do this would be to create a ProfileKeyedService
(chrome/browser/profiles/profile_keyed_service.h) for keyboard
shortcuts. This object's whole point in life would be to listen for
EXTENSION_INSTALLED and EXTENSION_UNINSTALLED and then to update these
two preferences. Take a look at any other class that implements this
interface for an example of how to use it. You will want to depend
upon the ExtensionSystem PKS so that you are loaded after it. I can't
remember how to express this dependency, but you should be able to
find it in other implementations.

The background is that it is likely that in the next few quarters, we
will split the extension system into at least a couple smaller pieces:

- Core extensions infrastructure (crx packing, unpacking,
ExtensionFunction and friends, ExtensionSystem and friends, Extension,
etc)
- Implementations of shared features using the extension system
(background pages, i18n, etc)
- Implementations of chrome-specific features (bookmarks, history,
keyboard shortcuts)
- Implementations of apps-specific features

Since keyboard shortcuts are a Chrome-specific feature, the core part
of extensions should not directly depend on them.

- a

[Yoyo Zhou, 2012-04-27 03:33:23]

On Thu, Apr 26, 2012 at 3:46 PM, Aaron Boodman <aa@chromium.org> wrote:
> On Thu, Apr 26, 2012 at 2:06 PM,  <finnur@chromium.org> wrote:
>> Is there a similar object in the codebase that I can model it after, to make
>> sure I understand what you are asking for?
>
> Actually, after talking it over with Yoyo, I think we want to
> completely decouple the implementation of keyboard shortcuts from the
> core extension system infrastructure. This will be the wave of the
> future!
>
> The way to do this would be to create a ProfileKeyedService
> (chrome/browser/profiles/profile_keyed_service.h) for keyboard
> shortcuts. This object's whole point in life would be to listen for
> EXTENSION_INSTALLED and EXTENSION_UNINSTALLED and then to update these
> two preferences. Take a look at any other class that implements this
> interface for an example of how to use it. You will want to depend
> upon the ExtensionSystem PKS so that you are loaded after it. I can't
> remember how to express this dependency, but you should be able to
> find it in other implementations.

DependsOn(ExtensionSystemFactory::GetInstance());

See
http://code.google.com/codesearch#OAMlx_jo-ck/src/chrome/browser/extensions/e...
for an example.

> The background is that it is likely that in the next few quarters, we
> will split the extension system into at least a couple smaller pieces:
>
> - Core extensions infrastructure (crx packing, unpacking,
> ExtensionFunction and friends, ExtensionSystem and friends, Extension,
> etc)
> - Implementations of shared features using the extension system
> (background pages, i18n, etc)
> - Implementations of chrome-specific features (bookmarks, history,
> keyboard shortcuts)
> - Implementations of apps-specific features
>
> Since keyboard shortcuts are a Chrome-specific feature, the core part
> of extensions should not directly depend on them.
>
> - a

[Finnur, 2012-04-27 16:23:24]

OK, I've done what I think you wanted. Please review the factory code carefully.
It seems to work fine, but this is the first such ProfileKeyedServiceFactory I
write, so I might be missing something.

The only snag I hit was the the factory creates the service lazily, but since we
want it to listen for install/uninstall events we want it to start as soon as
possible. Hence my changes to ExtensionKeybindingRegistry.cc. If you have a
better idea on how to do it, please let me know...

[Yoyo Zhou, 2012-04-27 16:54:55]

On 2012/04/27 16:23:24, Finnur wrote:
> OK, I've done what I think you wanted. Please review the factory code
carefully.
> It seems to work fine, but this is the first such ProfileKeyedServiceFactory I
> write, so I might be missing something.
> 
> The only snag I hit was the the factory creates the service lazily, but since
we
> want it to listen for install/uninstall events we want it to start as soon as
> possible. Hence my changes to ExtensionKeybindingRegistry.cc. If you have a
> better idea on how to do it, please let me know...

There's a comment in profile_dependency_manager.h that advises:
  // (If you want your ProfileKeyedService to be started
  // with the Profile, override ProfileKeyedBaseFactory::
  // ServiceIsCreatedWithProfile() to return true.)

[Finnur, 2012-04-30 09:28:37]

PTAL.

https://chromiumcodereview.appspot.com/10201016/diff/42002/chrome/browser/ext...
File chrome/browser/extensions/extension_command_service.cc (right):

https://chromiumcodereview.appspot.com/10201016/diff/42002/chrome/browser/ext...
chrome/browser/extensions/extension_command_service.cc:199:
RemoveKeybindingPrefs(*content::Details<std::string>(details).ptr());
I made an error here earlier and discovered it when I was testing the other
changes in this CL. Don't remember how it got introduced and why it didn't
surface before, since I tested this multiple times.

https://chromiumcodereview.appspot.com/10201016/diff/42002/chrome/browser/ext...
File chrome/browser/extensions/extension_command_service_factory.cc (right):

https://chromiumcodereview.appspot.com/10201016/diff/42002/chrome/browser/ext...
chrome/browser/extensions/extension_command_service_factory.cc:24: }
Thanks for the tip, Yoyo.

This change means there are no changes remaining for
extension_keybinding_registry.cc, so it was removed from the CL.

[Finnur, 2012-04-30 09:30:53]

> PTAL.

To elaborate:

aa & yoyo: PTAL.
sky: Thanks for your LG.
erg: Can you give OWNERS LG for GTK?

[Aaron Boodman, 2012-04-30 17:48:12]

https://chromiumcodereview.appspot.com/10201016/diff/42002/chrome/browser/ext...
File chrome/browser/extensions/extension_command_service.h (right):

https://chromiumcodereview.appspot.com/10201016/diff/42002/chrome/browser/ext...
chrome/browser/extensions/extension_command_service.h:1: // Copyright (c) 2012
The Chromium Authors. All rights reserved.
Can we move all the command-related files into browser/extensions/api/commands?

https://chromiumcodereview.appspot.com/10201016/diff/42002/chrome/browser/ext...
chrome/browser/extensions/extension_command_service.h:43: const
Extension::ExtensionKeybinding* GetActiveBrowserActionCommand(
Nice idea moving this stuff into the PKS.

https://chromiumcodereview.appspot.com/10201016/diff/42002/chrome/browser/ext...
File chrome/browser/extensions/extension_command_service_factory.h (right):

https://chromiumcodereview.appspot.com/10201016/diff/42002/chrome/browser/ext...
chrome/browser/extensions/extension_command_service_factory.h:14: // Singleton
that owns all ExtensionCommandService and associates them with
I don't think there's an ownership relationship... the PKS infrastructure ends
up owning the ExtensionCommandService instances. This is just the factory.

https://chromiumcodereview.appspot.com/10201016/diff/42002/chrome/browser/ext...
File chrome/browser/extensions/extension_system_factory.cc (right):

https://chromiumcodereview.appspot.com/10201016/diff/42002/chrome/browser/ext...
chrome/browser/extensions/extension_system_factory.cc:36:
DependsOn(ExtensionCommandServiceFactory::GetInstance());
This doesn't seem right. You want it the other way 'round. CommandServiceFactory
should depend on ExtensionSystem.

[Yoyo Zhou, 2012-05-01 01:51:30]

http://codereview.chromium.org/10201016/diff/42002/chrome/browser/extensions/...
File chrome/browser/extensions/extension_command_service.cc (right):

http://codereview.chromium.org/10201016/diff/42002/chrome/browser/extensions/...
chrome/browser/extensions/extension_command_service.cc:39:
content::Source<Profile>(profile->GetOriginalProfile()));
Since it's redirected in incognito, the profile you get built with can only be
the original profile.

http://codereview.chromium.org/10201016/diff/42002/chrome/browser/extensions/...
chrome/browser/extensions/extension_command_service.cc:51:
profile_->GetExtensionService()->GetExtensionById(extension_id, false);
Nit: eventually we'd like to get rid of GetExtensionService. You can rewrite
this as
ExtensionSystem::Get(profile_)->extensions()->GetByID(extension_id)

http://codereview.chromium.org/10201016/diff/42002/chrome/browser/extensions/...
chrome/browser/extensions/extension_command_service.cc:124: if
(!bindings->GetDictionary(key, &value) || !value)
I don't think you need the extra null check.

http://codereview.chromium.org/10201016/diff/42002/chrome/browser/extensions/...
chrome/browser/extensions/extension_command_service.cc:129: return false;  //
Already taken by another extension.
Comments are misleading. Actually it's "not taken by this extension", i.e. it
could also be not taken by any extension.

http://codereview.chromium.org/10201016/diff/42002/chrome/browser/extensions/...
chrome/browser/extensions/extension_command_service.cc:145:
iter->second.command_name(),
(sorry since I haven't seen the previous keybinding CLs, but) Is it detected if
an extension uses duplicate command names?

http://codereview.chromium.org/10201016/diff/42002/chrome/browser/extensions/...
File chrome/browser/extensions/extension_command_service.h (right):

http://codereview.chromium.org/10201016/diff/42002/chrome/browser/extensions/...
chrome/browser/extensions/extension_command_service.h:41: // |profile|) to see
if the keybinding is active. Returns NULL if the
Nit: change |profile| to either profile or |profile_| (I would prefer the
former, since confusion about which profile it is is unlikely).

http://codereview.chromium.org/10201016/diff/42002/chrome/browser/extensions/...
chrome/browser/extensions/extension_command_service.h:71: void
AssignInitialKeybindings(const Extension* extension);
Does this function need to be public?

http://codereview.chromium.org/10201016/diff/42002/chrome/browser/extensions/...
File chrome/browser/extensions/extension_command_service_factory.cc (right):

http://codereview.chromium.org/10201016/diff/42002/chrome/browser/extensions/...
chrome/browser/extensions/extension_command_service_factory.cc:28:
ProfileDependencyManager::GetInstance()) {
DependsOn would go here.

http://codereview.chromium.org/10201016/diff/42002/chrome/browser/extensions/...
File chrome/browser/extensions/extension_system_factory.cc (right):

http://codereview.chromium.org/10201016/diff/42002/chrome/browser/extensions/...
chrome/browser/extensions/extension_system_factory.cc:36:
DependsOn(ExtensionCommandServiceFactory::GetInstance());
On 2012/04/30 17:48:12, Aaron Boodman wrote:
> This doesn't seem right. You want it the other way 'round.
CommandServiceFactory
> should depend on ExtensionSystem.

Indeed (and you want to depend on ExtensionSystemFactory rather than
ExtensionSystemSharedFactory, I think).

http://codereview.chromium.org/10201016/diff/42002/chrome/common/extensions/e...
File chrome/common/extensions/extension.h (right):

http://codereview.chromium.org/10201016/diff/42002/chrome/common/extensions/e...
chrome/common/extensions/extension.h:598: // which is not necessarily the one it
can use, as it might be inacive.
nit: typo. %s/inacive/inactive/. Also delete the period after inactive.

[Yoyo Zhou, 2012-05-01 01:52:48]

http://codereview.chromium.org/10201016/diff/42002/chrome/browser/profiles/pr...
File chrome/browser/profiles/profile_dependency_manager.cc (right):

http://codereview.chromium.org/10201016/diff/42002/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_dependency_manager.cc:179:
ExtensionCommandServiceFactory::GetInstance();
Comment for erg:
I realize that I never added ExtensionSystemFactory here. Should it be here?

If so, finnur, do you mind adding it?

[Elliot Glaysher, 2012-05-01 17:42:50]

gtk lgtm

http://codereview.chromium.org/10201016/diff/42002/chrome/browser/profiles/pr...
File chrome/browser/profiles/profile_dependency_manager.cc (right):

http://codereview.chromium.org/10201016/diff/42002/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_dependency_manager.cc:179:
ExtensionCommandServiceFactory::GetInstance();
On 2012/05/01 01:52:49, Yoyo Zhou wrote:
> Comment for erg:
> I realize that I never added ExtensionSystemFactory here. Should it be here?
> 
> If so, finnur, do you mind adding it?

Yes, it should be here for registration purposes.

[Finnur, 2012-05-02 13:50:45]

erg: Thanks for the review.
aa & yoyo: PTAL. I believe I've addressed all comments with a change or a
comment of my own.

Aaron, I sync'ed to latest and tested this locally (with Matt's change for the
event order) and it seems to work fine.

Tip: If you view the deltas from CL9 you'll see my latest changes (and only my
changes). On the other hand, if you compare against CL8 you'll see unrelated
changes from gclient sync.

http://codereview.chromium.org/10201016/diff/42002/chrome/browser/extensions/...
File chrome/browser/extensions/extension_command_service.cc (right):

http://codereview.chromium.org/10201016/diff/42002/chrome/browser/extensions/...
chrome/browser/extensions/extension_command_service.cc:51:
profile_->GetExtensionService()->GetExtensionById(extension_id, false);
extensions() isn't defined in ExtensionSystem. We can do:

ExtensionSystem::Get(profile_)->extensions_service()->extensions()->GetByID(extension_id)

... but I don't see what that buys here...

On 2012/05/01 01:51:30, Yoyo Zhou wrote:
> Nit: eventually we'd like to get rid of GetExtensionService. You can rewrite
> this as
> ExtensionSystem::Get(profile_)->extensions()->GetByID(extension_id)

http://codereview.chromium.org/10201016/diff/42002/chrome/browser/extensions/...
chrome/browser/extensions/extension_command_service.cc:145:
iter->second.command_name(),
No, but it cannot happen because command names are keys in the manifest json
file.

On 2012/05/01 01:51:30, Yoyo Zhou wrote:
> (sorry since I haven't seen the previous keybinding CLs, but) Is it detected
if
> an extension uses duplicate command names?

http://codereview.chromium.org/10201016/diff/42002/chrome/browser/extensions/...
File chrome/browser/extensions/extension_command_service.h (right):

http://codereview.chromium.org/10201016/diff/42002/chrome/browser/extensions/...
chrome/browser/extensions/extension_command_service.h:1: // Copyright (c) 2012
The Chromium Authors. All rights reserved.
Absolutely. Mind if I do that in a followup changelist?
It will make reviewing of this changelist less messy, for one.

On 2012/04/30 17:48:12, Aaron Boodman wrote:
> Can we move all the command-related files into
browser/extensions/api/commands?

http://codereview.chromium.org/10201016/diff/42002/chrome/browser/extensions/...
chrome/browser/extensions/extension_command_service.h:71: void
AssignInitialKeybindings(const Extension* extension);
Nope, not any more. Good catch.

On 2012/05/01 01:51:30, Yoyo Zhou wrote:
> Does this function need to be public?

http://codereview.chromium.org/10201016/diff/42002/chrome/browser/extensions/...
File chrome/browser/extensions/extension_command_service_factory.h (right):

http://codereview.chromium.org/10201016/diff/42002/chrome/browser/extensions/...
chrome/browser/extensions/extension_command_service_factory.h:14: // Singleton
that owns all ExtensionCommandService and associates them with
Yeah, I used another factory as a template and did search-replace to add
ExtensionCommandService. Then forgot to revise the comment.

On 2012/04/30 17:48:12, Aaron Boodman wrote:
> I don't think there's an ownership relationship... the PKS infrastructure ends
> up owning the ExtensionCommandService instances. This is just the factory.

http://codereview.chromium.org/10201016/diff/42002/chrome/browser/extensions/...
File chrome/browser/extensions/extension_system_factory.cc (right):

http://codereview.chromium.org/10201016/diff/42002/chrome/browser/extensions/...
chrome/browser/extensions/extension_system_factory.cc:36:
DependsOn(ExtensionCommandServiceFactory::GetInstance());
On 2012/05/01 01:51:30, Yoyo Zhou wrote:
> On 2012/04/30 17:48:12, Aaron Boodman wrote:
> > This doesn't seem right. You want it the other way 'round.
> CommandServiceFactory
> > should depend on ExtensionSystem.
> 
> Indeed (and you want to depend on ExtensionSystemFactory rather than
> ExtensionSystemSharedFactory, I think).

Done.

http://codereview.chromium.org/10201016/diff/42002/chrome/browser/profiles/pr...
File chrome/browser/profiles/profile_dependency_manager.cc (right):

http://codereview.chromium.org/10201016/diff/42002/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_dependency_manager.cc:179:
ExtensionCommandServiceFactory::GetInstance();
On 2012/05/01 17:42:50, Elliot Glaysher wrote:
> On 2012/05/01 01:52:49, Yoyo Zhou wrote:
> > Comment for erg:
> > I realize that I never added ExtensionSystemFactory here. Should it be here?
> > 
> > If so, finnur, do you mind adding it?
> 
> Yes, it should be here for registration purposes.

Done.

[Yoyo Zhou, 2012-05-02 19:12:49]

LGTM

http://codereview.chromium.org/10201016/diff/42002/chrome/browser/extensions/...
File chrome/browser/extensions/extension_command_service.cc (right):

http://codereview.chromium.org/10201016/diff/42002/chrome/browser/extensions/...
chrome/browser/extensions/extension_command_service.cc:51:
profile_->GetExtensionService()->GetExtensionById(extension_id, false);
On 2012/05/02 13:50:45, Finnur wrote:
> extensions() isn't defined in ExtensionSystem. We can do:
> 
>
ExtensionSystem::Get(profile_)->extensions_service()->extensions()->GetByID(extension_id)
> 
> ... but I don't see what that buys here...

Eventually we'll want to deprecate profile_->GetExtensionService, so just
changing that part to
ExtensionSystem::Get(profile_)->extensions_service() is fine.

> On 2012/05/01 01:51:30, Yoyo Zhou wrote:
> > Nit: eventually we'd like to get rid of GetExtensionService. You can rewrite
> > this as
> > ExtensionSystem::Get(profile_)->extensions()->GetByID(extension_id)
>

[Aaron Boodman, 2012-05-02 20:06:48]

LGTM (2)

On Wed, May 2, 2012 at 12:12 PM,  <yoz@chromium.org> wrote:
> LGTM
>
>
>
>
http://codereview.chromium.org/10201016/diff/42002/chrome/browser/extensions/...
> File chrome/browser/extensions/extension_command_service.cc (right):
>
>
http://codereview.chromium.org/10201016/diff/42002/chrome/browser/extensions/...
> chrome/browser/extensions/extension_command_service.cc:51:
> profile_->GetExtensionService()->GetExtensionById(extension_id, false);
> On 2012/05/02 13:50:45, Finnur wrote:
>>
>> extensions() isn't defined in ExtensionSystem. We can do:
>
>
>
>
ExtensionSystem::Get(profile_)->extensions_service()->extensions()->GetByID(extension_id)
>
>> ... but I don't see what that buys here...
>
>
> Eventually we'll want to deprecate profile_->GetExtensionService, so
> just changing that part to
> ExtensionSystem::Get(profile_)->extensions_service() is fine.
>
>
>> On 2012/05/01 01:51:30, Yoyo Zhou wrote:
>> > Nit: eventually we'd like to get rid of GetExtensionService. You can
>
> rewrite
>>
>> > this as
>> > ExtensionSystem::Get(profile_)->extensions()->GetByID(extension_id)
>
>
>
> http://codereview.chromium.org/10201016/

[Finnur, 2012-05-03 10:22:18]

http://codereview.chromium.org/10201016/diff/42002/chrome/browser/extensions/...
File chrome/browser/extensions/extension_command_service.cc (right):

http://codereview.chromium.org/10201016/diff/42002/chrome/browser/extensions/...
chrome/browser/extensions/extension_command_service.cc:51:
profile_->GetExtensionService()->GetExtensionById(extension_id, false);
I see.

Done!