Fixes component extension commands assignment on reload.

Fixes component extension commands assignment on reload.

Component extensions, when added/removed, trigger the following calls to ExtensionRegistryObserver:
Add:
OnExtensionWillBeInstalled
OnExtensionInstalled
OnExtensionLoaded
Remove:
OnExtensionUnloaded
OnExtensionUninstalled
Add (2nd call)
OnExtensionLoaded
Removed (2nd call):
OnExtensionUnloaded
OnExtensionUninstalled

In the non-component case, the 2nd calls are identical to the 1st. To work around this issue but still support whatever observers depend on this behavior, an uninstall reason component has been added and checked for explicitly in CommandService::OnExtensionUninstalled.

This has the positive side effect of removing the special casing in OnExtensionWillBeInstalled for component extensions in CommandService.

BUG=457350, 458612
TEST=interactive_ui_tests --gtest_filter=CommandsApiTest.AddRemoveAddComponentExtension

Committed: https://crrev.com/9fb3d5b62a6466053aeee3a26c0d0a898428f4eb
Cr-Commit-Position: refs/heads/master@{#317602}

[David Tseng, 2015-02-06 18:26:41]

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

[Finnur, 2015-02-09 09:35:22]

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

[Finnur, 2015-02-09 09:37:59]

Mike should probably be involved, since this both augments the changes he made
and the current CL fails on the two tests he added.

[Mike Wittman, 2015-02-10 19:30:58]

It's not clear to me what the behavior at issue is here. Can you please write a
bug with specific repro steps, as well as a test that fails with the current
code?

I tried patching just your command_service_browsertest.cc change into my
checkout and the test still passes against a recent master.

[David Tseng, 2015-02-10 19:43:11]

Here's a bug:
crbug.com/457350
<https://code.google.com/p/chromium/issues/detail?id=457350>

I'll try to write a test that demonstrates the issue.

On Tue, Feb 10, 2015 at 11:30 AM, <wittman@chromium.org> wrote:

> It's not clear to me what the behavior at issue is here. Can you please
> write a
> bug with specific repro steps, as well as a test that fails with the
> current
> code?
>
> I tried patching just your command_service_browsertest.cc change into my
> checkout and the test still passes against a recent master.
>
> https://codereview.chromium.org/904943003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[David Tseng, 2015-02-10 21:36:48]

I went ahead and also added a test (extension_keybinding_apitest.cc).

On Tue, Feb 10, 2015 at 11:43 AM, David Tseng <dtseng@chromium.org> wrote:

> Here's a bug:
> crbug.com/457350
> <https://code.google.com/p/chromium/issues/detail?id=457350>
>
> I'll try to write a test that demonstrates the issue.
>
> On Tue, Feb 10, 2015 at 11:30 AM, <wittman@chromium.org> wrote:
>
>> It's not clear to me what the behavior at issue is here. Can you please
>> write a
>> bug with specific repro steps, as well as a test that fails with the
>> current
>> code?
>>
>> I tried patching just your command_service_browsertest.cc change into my
>> checkout and the test still passes against a recent master.
>>
>> https://codereview.chromium.org/904943003/
>>
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Mike Wittman, 2015-02-10 22:44:48]

On 2015/02/10 21:36:48, David Tseng wrote:
> I went ahead and also added a test (extension_keybinding_apitest.cc).
> 
> On Tue, Feb 10, 2015 at 11:43 AM, David Tseng <mailto:dtseng@chromium.org>
wrote:
> 
> > Here's a bug:
> > crbug.com/457350
> > <https://code.google.com/p/chromium/issues/detail?id=457350>
> >
> > I'll try to write a test that demonstrates the issue.
> >
> > On Tue, Feb 10, 2015 at 11:30 AM, <mailto:wittman@chromium.org> wrote:
> >
> >> It's not clear to me what the behavior at issue is here. Can you please
> >> write a
> >> bug with specific repro steps, as well as a test that fails with the
> >> current
> >> code?
> >>
> >> I tried patching just your command_service_browsertest.cc change into my
> >> checkout and the test still passes against a recent master.
> >>
> >> https://codereview.chromium.org/904943003/

Great, thanks! So the issue is that keybindings are not reclaimed on install of
the extension after remove?

If that's the case, maybe we should just remove the "was_assigned" preference on
extension remove. The purpose of it is solely to make decisions about whether
the extension should automatically update its keybinding on extension update and
it doesn't need to be kept around when the extension is not present. As long as
we can distinguish extension remove from update and only remove the preference
in the former case, that may fix the problem.

[David Tseng, 2015-02-10 23:06:45]

PTAL at the newest patchset which does what you're saying.

The issue is that component extensions never get uninstalled. We now reset
the kSuggestedKeyWasAssigned for each command to false in
OnExtensionUnloaded (only for component extensions).

It wasn't clear to me how this pref was being used since it gets assigned
to true after an extension first gets installed, but it was being used to
check for in IsCommandsShortcutUserModified. Would be great to rename that
constant if the latter is actually the real intent of the pref.


On Tue, Feb 10, 2015 at 2:44 PM, <wittman@chromium.org> wrote:

> On 2015/02/10 21:36:48, David Tseng wrote:
>
>> I went ahead and also added a test (extension_keybinding_apitest.cc).
>>
>
>  On Tue, Feb 10, 2015 at 11:43 AM, David Tseng <mailto:dtseng@chromium.org
>> >
>>
> wrote:
>
>  > Here's a bug:
>> > crbug.com/457350
>> > <https://code.google.com/p/chromium/issues/detail?id=457350>
>> >
>> > I'll try to write a test that demonstrates the issue.
>> >
>> > On Tue, Feb 10, 2015 at 11:30 AM, <mailto:wittman@chromium.org> wrote:
>> >
>> >> It's not clear to me what the behavior at issue is here. Can you please
>> >> write a
>> >> bug with specific repro steps, as well as a test that fails with the
>> >> current
>> >> code?
>> >>
>> >> I tried patching just your command_service_browsertest.cc change into
>> my
>> >> checkout and the test still passes against a recent master.
>> >>
>> >> https://codereview.chromium.org/904943003/
>>
>
> Great, thanks! So the issue is that keybindings are not reclaimed on
> install of
> the extension after remove?
>
> If that's the case, maybe we should just remove the "was_assigned"
> preference on
> extension remove. The purpose of it is solely to make decisions about
> whether
> the extension should automatically update its keybinding on extension
> update and
> it doesn't need to be kept around when the extension is not present. As
> long as
> we can distinguish extension remove from update and only remove the
> preference
> in the former case, that may fix the problem.
>
>
> https://codereview.chromium.org/904943003/
>
> To unsubscribe from this group and stop receiving emails from it, send an
> email to chromium-reviews+unsubscribe@chromium.org.
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[David Tseng, 2015-02-12 16:21:34]

Friendly ping.

On 2015/02/10 23:06:45, David Tseng wrote:
> PTAL at the newest patchset which does what you're saying.
> 
> The issue is that component extensions never get uninstalled. We now reset
> the kSuggestedKeyWasAssigned for each command to false in
> OnExtensionUnloaded (only for component extensions).
> 
> It wasn't clear to me how this pref was being used since it gets assigned
> to true after an extension first gets installed, but it was being used to
> check for in IsCommandsShortcutUserModified. Would be great to rename that
> constant if the latter is actually the real intent of the pref.
> 
> 
> On Tue, Feb 10, 2015 at 2:44 PM, <mailto:wittman@chromium.org> wrote:
> 
> > On 2015/02/10 21:36:48, David Tseng wrote:
> >
> >> I went ahead and also added a test (extension_keybinding_apitest.cc).
> >>
> >
> >  On Tue, Feb 10, 2015 at 11:43 AM, David Tseng <mailto:dtseng@chromium.org
> >> >
> >>
> > wrote:
> >
> >  > Here's a bug:
> >> > crbug.com/457350
> >> > <https://code.google.com/p/chromium/issues/detail?id=457350>
> >> >
> >> > I'll try to write a test that demonstrates the issue.
> >> >
> >> > On Tue, Feb 10, 2015 at 11:30 AM, <mailto:wittman@chromium.org> wrote:
> >> >
> >> >> It's not clear to me what the behavior at issue is here. Can you please
> >> >> write a
> >> >> bug with specific repro steps, as well as a test that fails with the
> >> >> current
> >> >> code?
> >> >>
> >> >> I tried patching just your command_service_browsertest.cc change into
> >> my
> >> >> checkout and the test still passes against a recent master.
> >> >>
> >> >> https://codereview.chromium.org/904943003/
> >>
> >
> > Great, thanks! So the issue is that keybindings are not reclaimed on
> > install of
> > the extension after remove?
> >
> > If that's the case, maybe we should just remove the "was_assigned"
> > preference on
> > extension remove. The purpose of it is solely to make decisions about
> > whether
> > the extension should automatically update its keybinding on extension
> > update and
> > it doesn't need to be kept around when the extension is not present. As
> > long as
> > we can distinguish extension remove from update and only remove the
> > preference
> > in the former case, that may fix the problem.
> >
> >
> > https://codereview.chromium.org/904943003/
> >
> > To unsubscribe from this group and stop receiving emails from it, send an
> > email to mailto:chromium-reviews+unsubscribe@chromium.org.
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[Mike Wittman, 2015-02-12 20:02:43]

On 2015/02/12 16:21:34, David Tseng wrote:
> Friendly ping.
> 
> On 2015/02/10 23:06:45, David Tseng wrote:
> > PTAL at the newest patchset which does what you're saying.
> > 
> > The issue is that component extensions never get uninstalled. We now reset
> > the kSuggestedKeyWasAssigned for each command to false in
> > OnExtensionUnloaded (only for component extensions).
> > 
> > It wasn't clear to me how this pref was being used since it gets assigned
> > to true after an extension first gets installed, but it was being used to
> > check for in IsCommandsShortcutUserModified. Would be great to rename that
> > constant if the latter is actually the real intent of the pref.

Sorry for the delay... somehow I never received your previous reply.

So removing the was_assigned preference in OnExtensionUninstalled does not fix
the original issue in the bug? If not, then it sounds like there are two
different issues here: the extension remove case, and whatever is happening in
the Chrome OS Linux OOBE case.

Can you explain the sequence of extension events that triggers the problem in
the Chrome OS Linux OOBE case? I don't think we should remove the preference on
extension disable since that event doesn't alter the keybinding assignment.

Although used to determine if the user has modified the key, the pref is not
intended to encode that state directly. Maintaining that state would require a
different implementation in CommandService.

[David Tseng, 2015-02-12 20:27:39]

On Thu, Feb 12, 2015 at 12:02 PM, <wittman@chromium.org> wrote:
On 2015/02/12 16:21:34, David Tseng wrote:
Friendly ping.

On 2015/02/10 23:06:45, David Tseng wrote:
> PTAL at the newest patchset which does what you're saying.
>
> The issue is that component extensions never get uninstalled. We now reset
> the kSuggestedKeyWasAssigned for each command to false in
> OnExtensionUnloaded (only for component extensions).
>
> It wasn't clear to me how this pref was being used since it gets assigned
> to true after an extension first gets installed, but it was being used to
> check for in IsCommandsShortcutUserModified. Would be great to rename that
> constant if the latter is actually the real intent of the pref.

Sorry for the delay... somehow I never received your previous reply.

So removing the was_assigned preference in OnExtensionUninstalled does not
fix
the original issue in the bug?

Component extensions never get uninstalled, so that particular logic never
gets triggered.


If not, then it sounds like there are two
different issues here: the extension remove case, and whatever is happening
in
the Chrome OS Linux OOBE case.

Can you explain the sequence of extension events that triggers the problem
in
the Chrome OS Linux OOBE case? I don't think we should remove the
preference on
extension disable since that event doesn't alter the keybinding assignment.

Although used to determine if the user has modified the key, the pref is not
intended to encode that state directly. Maintaining that state would
require a
different implementation in CommandService.

The issue with component extensions is that we only get
OnExtensionLoaded/OnExtensionUnloaded events when an extension is added or
removed respectively. UpdateKeybindings I think was meant to be called on
install only so some amount of cleanup should be done for component
extensions in the OnExtensionUnloaded case.

See the latest patchset for a test case. It will fail without the change to
remove the was_assigned pref on unload.
That test just runs a component extension test (which adds the component
extension), manually removes the component extension, then runs it again
triggering a second add.
From CommandService's perspective, we get OnExtensionLoaded twice.

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[David Tseng, 2015-02-12 21:30:57]

On Thu, Feb 12, 2015 at 12:27 PM, David Tseng <dtseng@chromium.org> wrote:

>
>
> On Thu, Feb 12, 2015 at 12:02 PM, <wittman@chromium.org> wrote:
> On 2015/02/12 16:21:34, David Tseng wrote:
> Friendly ping.
>
> On 2015/02/10 23:06:45, David Tseng wrote:
> > PTAL at the newest patchset which does what you're saying.
> >
> > The issue is that component extensions never get uninstalled. We now
> reset
> > the kSuggestedKeyWasAssigned for each command to false in
> > OnExtensionUnloaded (only for component extensions).
> >
> > It wasn't clear to me how this pref was being used since it gets assigned
> > to true after an extension first gets installed, but it was being used to
> > check for in IsCommandsShortcutUserModified. Would be great to rename
> that
> > constant if the latter is actually the real intent of the pref.
>
> Sorry for the delay... somehow I never received your previous reply.
>
> So removing the was_assigned preference in OnExtensionUninstalled does not
> fix
> the original issue in the bug?
>
> Component extensions never get uninstalled, so that particular logic never
> gets triggered.
>
>
> If not, then it sounds like there are two
> different issues here: the extension remove case, and whatever is
> happening in
> the Chrome OS Linux OOBE case.
>
> Can you explain the sequence of extension events that triggers the problem
> in
> the Chrome OS Linux OOBE case? I don't think we should remove the
> preference on
> extension disable since that event doesn't alter the keybinding assignment.
>
> Although used to determine if the user has modified the key, the pref is
> not
> intended to encode that state directly. Maintaining that state would
> require a
> different implementation in CommandService.
>
> The issue with component extensions is that we only get
> OnExtensionLoaded/OnExtensionUnloaded events when an extension is added or
> removed respectively. UpdateKeybindings I think was meant to be called on
> install only so some amount of cleanup should be done for component
> extensions in the OnExtensionUnloaded case.
>
> See the latest patchset for a test case. It will fail without the change
> to remove the was_assigned pref on unload.
> That test just runs a component extension test (which adds the component
> extension), manually removes the component extension, then runs it again
> triggering a second add.
> From CommandService's perspective, we get OnExtensionLoaded twice.
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Mike Wittman, 2015-02-13 00:15:16]

On 2015/02/12 21:30:57, David Tseng wrote:
> On Thu, Feb 12, 2015 at 12:27 PM, David Tseng <mailto:dtseng@chromium.org>
wrote:
> > On Thu, Feb 12, 2015 at 12:02 PM, <mailto:wittman@chromium.org> wrote:
> > So removing the was_assigned preference in OnExtensionUninstalled does not
> > fix
> > the original issue in the bug?
> >
> Component extensions never get uninstalled, so that particular logic never
> gets triggered.

OK, so then the test, while validating a legitimate issue, is actually testing
something different than what is being experienced in the Chrome OS Linux OOBE
case. I think the proper fix for the issue in the test is removing the
was_assigned preference in OnExtensionUninstalled. I think we should make this
change independent of the resolution to the Chrome OS Linux OOBE case.

> The issue with component extensions is that we only get
> OnExtensionLoaded/OnExtensionUnloaded events when an extension is added or
> removed respectively. UpdateKeybindings I think was meant to be called on
> install only so some amount of cleanup should be done for component
> extensions in the OnExtensionUnloaded case.

Can we determine when a component extension has been updated and do
OnExtensionInstalled in that case? Finnur? That would keep things a lot simpler
and avoid a lot of special-casing for component extensions vs. everything else.
This keybinding behavior is already pretty tricky, and it seems unlikely to be
the only extension behavior (current and future) that assumes
OnExtensionInstalled will be called on update.

If I'm understanding what you're trying to do after looking through your
previous CL (https://crrev.com/788973002), maybe you should address the
component case by simply allowing component extensions to update keybindings by
short-circuiting the IsCommandShortcutUserModified check in CanAutoAssign. This
would allow component extensions to silently overwrite custom user keybindings,
but since component keybindings are hidden from the UI by default it shouldn't
be a big deal.

[David Tseng, 2015-02-13 01:19:35]

Maybe I'm missing something here, but removing a component extension didn't
cause OnExtensionUninstalled to get called, but I'll verify that again to
be sure and ping this thread with my findings.

My aim in the original cl attempt was to try and preserve the user defined
keybindings. The issue I ran into was that active keybindings were somehow
already set to empty shortcuts, was_assigned was set to true, so the logic
that compares previous bindings to active bindings kicked in and rejects
the previous bindings. This wasn't what I wanted. I could just check if the
extension is a component extension and always return false in
IsCommandsShortcutUserAssigned.

I suspect there's some user synced prefs at work in the OOBE case, so
whatever assumptions made by CommandService don't hold up when you sign in
and prefs get synced. Flipping on a component extension when first logging
in generates an OnExtensionInstalled/OnExtensionLoaded possibly with
was_assigned already populated.

Anyhow, I think the only way for me to move forward with this is to write
tests to demonstrate issues and fix them.

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[David Tseng, 2015-02-13 01:21:18]

On Thu, Feb 12, 2015 at 5:19 PM, David Tseng <dtseng@chromium.org> wrote:

> Maybe I'm missing something here, but removing a component extension
> didn't cause OnExtensionUninstalled to get called, but I'll verify that
> again to be sure and ping this thread with my findings.
>
> My aim in the original cl attempt was to try and preserve the user defined
> keybindings. The issue I ran into was that active keybindings were somehow
> already set to empty shortcuts, was_assigned was set to true, so the logic
> that compares previous bindings to active bindings kicked in and rejects
> the previous bindings. This wasn't what I wanted. I could just check if the
> extension is a component extension and always return false in
> IsCommandsShortcutUserAssigned.
>
> I suspect there's some user synced prefs at work in the OOBE case, so
> whatever assumptions made by CommandService don't hold up when you sign in
> and prefs get synced. Flipping on a component extension when first logging
> in generates an OnExtensionInstalled/OnExtensionLoaded possibly with
> was_assigned already populated.
>
> Anyhow, I think the only way for me to move forward with this is to write
> tests to demonstrate issues and fix them.
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Finnur, 2015-02-13 15:06:35]

> Can we determine when a component extension has been updated and do
> OnExtensionInstalled in that case? Finnur?

As I recall, I mentioned exploring this avenue some time back in order to fix
another issue with keybindings. It would be nice not to have to have
special-case for component extensions. But I just don't feel like I know well
enough how component extensions differ in this regard or what is possible here.

[David Tseng, 2015-02-13 18:30:28]

PTAL.

After moving clearing of the pref was_assigned, CommandsApiTest.Component
still passes. Here's what I got when logging add/remove of a component
extension (done three times):
OnExtensionWillBeInstalled
OnExtensionUninstalled
OnExtensionUninstalled
OnExtensionUninstalled

The asymmetry above has probably been at the root of all of the confusion
and the need to listen to the load event and breaks other reasonable
assumptions in CommandService.

Anyhow, I guess a root fix would be to make component extensions fire will
be installed when added. This would allow component extensions to have user
defined keybindings.

I'd, however, like to get this cl in as a intermediate step just to get
keybindings to work when a component extension gets toggled. We do this for
some extensions in Chrome OS (e.g. ChromeVox) via natively bound hotkeys.

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[David Tseng, 2015-02-13 18:36:24]

dtseng@chromium.org changed reviewers:
+ kalman@chromium.org

[David Tseng, 2015-02-13 18:36:25]

+ kalman. Are you aware of this issue with component extension add/removed not
triggering OnExtensionWillBeInstalled after the first add/remove?

[not at google - send to devlin, 2015-02-13 18:51:26]

On 2015/02/13 18:36:25, David Tseng wrote:
> + kalman. Are you aware of this issue with component extension add/removed not
> triggering OnExtensionWillBeInstalled after the first add/remove?

I didn't know component extensions could even be added or removed. I'm unaware
of this issue because I didn't even know it could arise. If there's a good
reason to, it sounds like a bug.

[not at google - send to devlin, 2015-02-13 18:51:34]

it = not firing the event.

[David Tseng, 2015-02-13 19:06:28]

On 2015/02/13 18:51:26, kalman wrote:
> On 2015/02/13 18:36:25, David Tseng wrote:
> > + kalman. Are you aware of this issue with component extension add/removed
not
> > triggering OnExtensionWillBeInstalled after the first add/remove?
> 
> I didn't know component extensions could even be added or removed. I'm unaware
> of this issue because I didn't even know it could arise. If there's a good
> reason to, it sounds like a bug.

We (ChromeVox) do it all the time apparently; on each press of ctrl+alt+z on
Chrome OS.

This seems like a larger discussion we should have on a bug. I'll file it but
think it's not in scope for this cl.

[Mike Wittman, 2015-02-13 22:11:21]

https://codereview.chromium.org/904943003/diff/120001/chrome/browser/extensio...
File chrome/browser/extensions/api/commands/command_service.cc (right):

https://codereview.chromium.org/904943003/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/api/commands/command_service.cc:808: std::string
command;
indentation (+ next three lines)

https://codereview.chromium.org/904943003/diff/120001/chrome/browser/extensio...
chrome/browser/extensions/api/commands/command_service.cc:820:
command_keys->SetBoolean(kSuggestedKeyWasAssigned, false);
I'd prefer to remove this key from the preferences entirely since it is
meaningless when the extension is not installed.

https://codereview.chromium.org/904943003/diff/120001/chrome/test/data/extens...
File chrome/test/data/extensions/api_test/keybinding/component/background.js
(right):

https://codereview.chromium.org/904943003/diff/120001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/keybinding/component/background.js:9:
chrome.test.assertEq(platformBinding, commands[0].shortcut);
Can you move the keybinding checks into C++ to match the other tests in
extension_keybinding_apitest.cc? If so, you may be able to reuse one of the
existing keybinding test extensions (e.g. keybindings/basics) rather than
introducing a new one.

[David Tseng, 2015-02-17 18:42:37]

Looking at this a little more this morning and
ExtensionService::RemoveComponentExtension triggers
ExtensionRegistryObserver::OnExtensionUninstalled while the install path
uses non-component extension specific logic (i.e.
ExtensionService::AddNewOrUpdatedExtension or AddExtension). The install
codepath for component extension should probably try and reuse the normal
install path for ordinary extensions and not special case version checking
(in ExtensionService::AddComponentExtension).

PTAL. The latest patchset adds a new uninstall reason (component) and
checkcs for it in command service. No longer messing with prefs as I think
this is a deeper issue with add/remove of component extensions.

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Mike Wittman, 2015-02-17 19:17:36]

On 2015/02/17 18:42:37, David Tseng wrote:
> Looking at this a little more this morning and
> ExtensionService::RemoveComponentExtension triggers
> ExtensionRegistryObserver::OnExtensionUninstalled while the install path
> uses non-component extension specific logic (i.e.
> ExtensionService::AddNewOrUpdatedExtension or AddExtension). The install
> codepath for component extension should probably try and reuse the normal
> install path for ordinary extensions and not special case version checking
> (in ExtensionService::AddComponentExtension).
> 
> PTAL. The latest patchset adds a new uninstall reason (component) and
> checkcs for it in command service. No longer messing with prefs as I think
> this is a deeper issue with add/remove of component extensions.

Seems like a better approach, but also outside my area of expertise. I think
Finnur or Benjamin will need to review the component lifecycle changes.

[David Tseng, 2015-02-17 19:18:14]

Sounds good; thanks for helping out.

On Tue, Feb 17, 2015 at 11:17 AM, <wittman@chromium.org> wrote:

> On 2015/02/17 18:42:37, David Tseng wrote:
>
>> Looking at this a little more this morning and
>> ExtensionService::RemoveComponentExtension triggers
>> ExtensionRegistryObserver::OnExtensionUninstalled while the install path
>> uses non-component extension specific logic (i.e.
>> ExtensionService::AddNewOrUpdatedExtension or AddExtension). The install
>> codepath for component extension should probably try and reuse the normal
>> install path for ordinary extensions and not special case version checking
>> (in ExtensionService::AddComponentExtension).
>>
>
>  PTAL. The latest patchset adds a new uninstall reason (component) and
>> checkcs for it in command service. No longer messing with prefs as I think
>> this is a deeper issue with add/remove of component extensions.
>>
>
> Seems like a better approach, but also outside my area of expertise. I
> think
> Finnur or Benjamin will need to review the component lifecycle changes.
>
>
>
> https://codereview.chromium.org/904943003/
>
> To unsubscribe from this group and stop receiving emails from it, send an
> email to chromium-reviews+unsubscribe@chromium.org.
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Finnur, 2015-02-18 11:51:31]

https://codereview.chromium.org/904943003/diff/180001/chrome/browser/extensio...
File chrome/browser/extensions/api/commands/command_service.cc (left):

https://codereview.chromium.org/904943003/diff/180001/chrome/browser/extensio...
chrome/browser/extensions/api/commands/command_service.cc:313: if
(extension->location() != Manifest::COMPONENT)
It is good to see these special-cases go away.

https://codereview.chromium.org/904943003/diff/180001/chrome/browser/extensio...
File chrome/browser/extensions/api/commands/command_service.cc (right):

https://codereview.chromium.org/904943003/diff/180001/chrome/browser/extensio...
chrome/browser/extensions/api/commands/command_service.cc:319: return;
A casual reader would benefit from an explanation here.

But am I correct in assuming that if we just get the OnExtensionWillBeInstalled
event to reliably fire for component extensions, then all this would just work
without special-casing? (including lines 318-319)

Can we dig a bit into why that event is not reliably sent for component
extensions? Or do you have a good sense as to what is happening behind the
scenes there?

I suspect if we can get to the bottom of that, then we don't even need the
uninstall reason, right?

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

https://codereview.chromium.org/904943003/diff/180001/chrome/browser/extensio...
chrome/browser/extensions/extension_keybinding_apitest.cc:961:
IN_PROC_BROWSER_TEST_F(CommandsApiTest, Component) {
This test would benefit from a comment explaining what we are testing.

https://codereview.chromium.org/904943003/diff/180001/chrome/browser/extensio...
chrome/browser/extensions/extension_keybinding_apitest.cc:970:
"pkplfbidichfdicaijlchgnapepdginl");
These six lines of code can probably be shrunk down to two, since you don't use
the extension_service variable beyond line 969.

https://codereview.chromium.org/904943003/diff/180001/extensions/browser/unin...
File extensions/browser/uninstall_reason.h (right):

https://codereview.chromium.org/904943003/diff/180001/extensions/browser/unin...
extensions/browser/uninstall_reason.h:27: // repeatedly. crbug.com/458612.
This comment seems out of place. Moving it to where you early-return would
probably help.

Also swap out:
"repeatedly. "
... for 
"repeatedly, see:http://"

https://codereview.chromium.org/904943003/diff/180001/extensions/browser/unin...
extensions/browser/uninstall_reason.h:28: UNINSTALL_REASON_COMPONENT
nit: Add trailing comma.

[David Tseng, 2015-02-18 18:24:40]

dtseng@chromium.org changed reviewers:
+ tapted@chromium.org

[David Tseng, 2015-02-18 18:24:41]

+ tapted. Please see bug crbug.com/458612 and your change (3 years ago) to
ExtensionService::AddComponentExtension for context. Thanks.

https://codereview.chromium.org/904943003/diff/180001/chrome/browser/extensio...
File chrome/browser/extensions/api/commands/command_service.cc (right):

https://codereview.chromium.org/904943003/diff/180001/chrome/browser/extensio...
chrome/browser/extensions/api/commands/command_service.cc:319: return;
On 2015/02/18 11:51:31, Finnur wrote:
> A casual reader would benefit from an explanation here.
> 
> But am I correct in assuming that if we just get the
OnExtensionWillBeInstalled
> event to reliably fire for component extensions, then all this would just work
> without special-casing? (including lines 318-319)

Correct.

> 
> Can we dig a bit into why that event is not reliably sent for component
> extensions? Or do you have a good sense as to what is happening behind the
> scenes there?

I do. See ExtensionService::AddComponentExtension. There's some special casing
for versioning. ExtensionService::AddExtension doesn't trigger the *Instal*
observer calls while ExtensionService::AddNewOrUpdatedExtension does.

> 
> I suspect if we can get to the bottom of that, then we don't even need the
> uninstall reason, right?


It pretty much comes down to why that logic was added in the first place. git
bblame shows tapted@ added it three years ago. I'll add him to this review.

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

https://codereview.chromium.org/904943003/diff/180001/chrome/browser/extensio...
chrome/browser/extensions/extension_keybinding_apitest.cc:961:
IN_PROC_BROWSER_TEST_F(CommandsApiTest, Component) {
On 2015/02/18 11:51:31, Finnur wrote:
> This test would benefit from a comment explaining what we are testing.

Done.

https://codereview.chromium.org/904943003/diff/180001/chrome/browser/extensio...
chrome/browser/extensions/extension_keybinding_apitest.cc:970:
"pkplfbidichfdicaijlchgnapepdginl");
On 2015/02/18 11:51:31, Finnur wrote:
> These six lines of code can probably be shrunk down to two, since you don't
use
> the extension_service variable beyond line 969.

Done.

https://codereview.chromium.org/904943003/diff/180001/extensions/browser/unin...
File extensions/browser/uninstall_reason.h (right):

https://codereview.chromium.org/904943003/diff/180001/extensions/browser/unin...
extensions/browser/uninstall_reason.h:27: // repeatedly. crbug.com/458612.
On 2015/02/18 11:51:31, Finnur wrote:
> This comment seems out of place. Moving it to where you early-return would
> probably help.
> 
> Also swap out:
> "repeatedly. "
> ... for 
> "repeatedly, see:http://"

Done.

https://codereview.chromium.org/904943003/diff/180001/extensions/browser/unin...
extensions/browser/uninstall_reason.h:28: UNINSTALL_REASON_COMPONENT
On 2015/02/18 11:51:31, Finnur wrote:
> nit: Add trailing comma.

Is this convention? This wasn't done before, so leaving as is.

[tapted, 2015-02-18 23:10:55]

https://codereview.chromium.org/904943003/diff/180001/chrome/browser/extensio...
File chrome/browser/extensions/api/commands/command_service.cc (right):

https://codereview.chromium.org/904943003/diff/180001/chrome/browser/extensio...
chrome/browser/extensions/api/commands/command_service.cc:319: return;
On 2015/02/18 18:24:40, David Tseng wrote:
> On 2015/02/18 11:51:31, Finnur wrote:
> > A casual reader would benefit from an explanation here.
> > 
> > But am I correct in assuming that if we just get the
> OnExtensionWillBeInstalled
> > event to reliably fire for component extensions, then all this would just
work
> > without special-casing? (including lines 318-319)
> 
> Correct.
> 
> > 
> > Can we dig a bit into why that event is not reliably sent for component
> > extensions? Or do you have a good sense as to what is happening behind the
> > scenes there?
> 
> I do. See ExtensionService::AddComponentExtension. There's some special casing
> for versioning. ExtensionService::AddExtension doesn't trigger the *Instal*
> observer calls while ExtensionService::AddNewOrUpdatedExtension does.
> 
> > 
> > I suspect if we can get to the bottom of that, then we don't even need the
> > uninstall reason, right?
> 
> 
> It pretty much comes down to why that logic was added in the first place. git
> bblame shows tapted@ added it three years ago. I'll add him to this review.

Wow - those were some of my first patches on Chrome :)

There's a good description/rationale in the original bug --
http://crbug.com/157717


The problem is (or was back then..) that component extensions only have their
lazy background pages re-parsed on a browser upgrade. A lot of the time this
isn't a problem. But if a component extension is enabled via a chrome://flag or
finch experiment it means that the lazy event listeners registered in the
Preferences could be those for an old version of the component extension.

regarding why OnExtensionWillBeInstalled itself doesn't trigger for component
extensions -- you can see in my first comment on that bug, that this was the
case back then already (and I wasn't sure back then if it was intentional).

I have a vague recollection that firing "OnExtensionWillBeInstalled" would have
had unwanted side-effects for component extensions. Like, extra manifest
validation that component extensions would fail. But the landscape has probably
changed a lot since then.

[David Tseng, 2015-02-18 23:22:26]

https://codereview.chromium.org/904943003/diff/180001/chrome/browser/extensio...
File chrome/browser/extensions/api/commands/command_service.cc (right):

https://codereview.chromium.org/904943003/diff/180001/chrome/browser/extensio...
chrome/browser/extensions/api/commands/command_service.cc:319: return;
On 2015/02/18 23:10:55, tapted wrote:
> On 2015/02/18 18:24:40, David Tseng wrote:
> > On 2015/02/18 11:51:31, Finnur wrote:
> > > A casual reader would benefit from an explanation here.
> > > 
> > > But am I correct in assuming that if we just get the
> > OnExtensionWillBeInstalled
> > > event to reliably fire for component extensions, then all this would just
> work
> > > without special-casing? (including lines 318-319)
> > 
> > Correct.
> > 
> > > 
> > > Can we dig a bit into why that event is not reliably sent for component
> > > extensions? Or do you have a good sense as to what is happening behind the
> > > scenes there?
> > 
> > I do. See ExtensionService::AddComponentExtension. There's some special
casing
> > for versioning. ExtensionService::AddExtension doesn't trigger the *Instal*
> > observer calls while ExtensionService::AddNewOrUpdatedExtension does.
> > 
> > > 
> > > I suspect if we can get to the bottom of that, then we don't even need the
> > > uninstall reason, right?
> > 
> > 
> > It pretty much comes down to why that logic was added in the first place.
git
> > bblame shows tapted@ added it three years ago. I'll add him to this review.
> 
> Wow - those were some of my first patches on Chrome :)
> 
> There's a good description/rationale in the original bug --
> http://crbug.com/157717
> 
> 
> The problem is (or was back then..) that component extensions only have their
> lazy background pages re-parsed on a browser upgrade. A lot of the time this
> isn't a problem. But if a component extension is enabled via a chrome://flag
or
> finch experiment it means that the lazy event listeners registered in the
> Preferences could be those for an old version of the component extension.
> 
> regarding why OnExtensionWillBeInstalled itself doesn't trigger for component
> extensions -- you can see in my first comment on that bug, that this was the
> case back then already (and I wasn't sure back then if it was intentional).

It does get triggered, but only on a version upgrade. The confusing part is the
decision, perhaps not intentional, to trigger OnExtensionUninstalled every time
in ComponentLoader::Remove leading to the unbalanced calls. Someone trying to
implement an extension API, like the commands API, need then to treat component
extensions differently.


> 
> I have a vague recollection that firing "OnExtensionWillBeInstalled" would
have
> had unwanted side-effects for component extensions. Like, extra manifest
> validation that component extensions would fail. But the landscape has
probably
> changed a lot since then.

I think it has a little as mentioned above. The commands API would not work at
all without triggering OnExtensionWillBeInstalled.

As I see it now, it seems like we just shouldn't trigger OnExtensionUninstalled
since component extensions can't really be uninstalled. The rationale for
triggering the *Install* calls, is to ensure various APIs properly initialize
the extension's prefs and do whatever special setup necessary only on install
(as oppose to load/unload).

Finnur, I think perhaps the above should be a separate discussion; would like to
get this cl in so commands will work when reloading an component extension.

[Finnur, 2015-02-20 10:31:49]

Kalman: See comment at bottom of this message).

Was travelling yesterday, so I didn't see your message until now.

This CL represents a net reduction in hacks (-1) to work around this problem,
which is positive. The only thing I'm hesitant about is the effect of adding a
new uninstall reason (and no longer using INTERNAL_MANAGEMENT when removing
component extension). Any qualms about that, Ben?

I'll give my LGTM based on my comments above (so you don't have to wait on me),
but would like a second opinion from Ben (Kalman) before checking in, just to be
sure.

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

https://codereview.chromium.org/904943003/diff/180001/chrome/browser/extensio...
chrome/browser/extensions/extension_keybinding_apitest.cc:961:
IN_PROC_BROWSER_TEST_F(CommandsApiTest, Component) {
Actually, I was hoping you'd flesh it out a bit more, something similar to:
Make sure a component extension retains its commands upon removal and re-adding.

https://codereview.chromium.org/904943003/diff/180001/extensions/browser/unin...
File extensions/browser/uninstall_reason.h (right):

https://codereview.chromium.org/904943003/diff/180001/extensions/browser/unin...
extensions/browser/uninstall_reason.h:28: UNINSTALL_REASON_COMPONENT
Yes, it is convention to add a trailing comma to prevent churn when checking in
new enums (don't have to modify the line before the one you add, just to add a
trailing comma).

[not at google - send to devlin, 2015-02-20 16:14:49]

kalman@chromium.org changed reviewers:
+ amistry@chromium.org

[not at google - send to devlin, 2015-02-20 16:14:50]

Sorry for not keeping up with this code review, these questions are probably
covered, and I have many it turns out:

1. Adding a new uninstall reason seems fine if it's an important distinction to
be made, however, at a high level what's special about the commands API?

2. The old enum this used for this, UNINSTALL_REASON_INTERNAL_MANAGEMENT, is a
pretty weird name but also only used in a single place... when uninstalling the
hotwording extension... which I had thought was a component extension anyway.
Argh what is going on.

3. Where is the -1 hack you mention Finnur?

4. Would it be less hacky to plumb through the |external_install| boolean from
here?
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...
(perhaps fixing the TODO that Devlin mentions above). Answer depends on answer
to (1).

5. All that said, it would be nice to fix the unbalanced behavior since it's
clearly wrong to get a unbalanced number of install and uninstall events - if
that fixes your problem at all. I would have assumed the same problem exists for
any external install/uninstall flow?

https://codereview.chromium.org/904943003/diff/200001/chrome/browser/extensio...
File chrome/browser/extensions/api/commands/command_service.cc (right):

https://codereview.chromium.org/904943003/diff/200001/chrome/browser/extensio...
chrome/browser/extensions/api/commands/command_service.cc:311:
UpdateKeybindings(extension);
For the commands case specifically, Finnur, why do we update the key bindings
both for install and for loaded? If I'm to guess... it's so that extensions that
are disabled or whatever still claim the keybindings?

https://codereview.chromium.org/904943003/diff/200001/chrome/browser/extensio...
chrome/browser/extensions/api/commands/command_service.cc:319: // repeatedly,
see http://crbug.com/458612.
Could you still mention why? The bug describes the sequence of steps but it'd be
good to mention something extra here. I can't suggest anything because I don't
know.

https://codereview.chromium.org/904943003/diff/200001/extensions/browser/unin...
File extensions/browser/uninstall_reason.h (right):

https://codereview.chromium.org/904943003/diff/200001/extensions/browser/unin...
extensions/browser/uninstall_reason.h:26: UNINSTALL_REASON_COMPONENT
"COMPONENT" doesn't sound like an uninstall reason. Would it be accurate to call
it "COMPONENT_REMOVED"?

[Finnur, 2015-02-20 16:40:13]

Got 6 mins until my train leaves to write this email. Hence brief replies. Hope
they make sense.

On 2015/02/20 16:14:50, kalman wrote:
> Sorry for not keeping up with this code review, these questions are probably
> covered, and I have many it turns out:
> 
> 1. Adding a new uninstall reason seems fine if it's an important distinction
to
> be made, however, at a high level what's special about the commands API?
> 

Nothing. But it relies on a balanced set of events (install and uninstall),
which Chrome is unfortunately not providing for component extensions (a bug in
my opinion).

> 2. The old enum this used for this, UNINSTALL_REASON_INTERNAL_MANAGEMENT, is a
> pretty weird name but also only used in a single place... when uninstalling
the
> hotwording extension... which I had thought was a component extension anyway.
> Argh what is going on.

Not sure. Can't look it up at the moment.

> 
> 3. Where is the -1 hack you mention Finnur?

Two hacks removed from command_service.cc and one new added.

> 
> 4. Would it be less hacky to plumb through the |external_install| boolean from
> here?
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...
> (perhaps fixing the TODO that Devlin mentions above). Answer depends on answer
> to (1).

I'll leave it to David to investigate.

> 
> 5. All that said, it would be nice to fix the unbalanced behavior since it's
> clearly wrong to get a unbalanced number of install and uninstall events - if
> that fixes your problem at all. I would have assumed the same problem exists
for
> any external install/uninstall flow?
> 
>
https://codereview.chromium.org/904943003/diff/200001/chrome/browser/extensio...
> File chrome/browser/extensions/api/commands/command_service.cc (right):
> 
>
https://codereview.chromium.org/904943003/diff/200001/chrome/browser/extensio...
> chrome/browser/extensions/api/commands/command_service.cc:311:
> UpdateKeybindings(extension);
> For the commands case specifically, Finnur, why do we update the key bindings
> both for install and for loaded? If I'm to guess... it's so that extensions
that
> are disabled or whatever still claim the keybindings?
> 
>
https://codereview.chromium.org/904943003/diff/200001/chrome/browser/extensio...
> chrome/browser/extensions/api/commands/command_service.cc:319: // repeatedly,
> see http://crbug.com/458612.
> Could you still mention why? The bug describes the sequence of steps but it'd
be
> good to mention something extra here. I can't suggest anything because I don't
> know.
> 
>
https://codereview.chromium.org/904943003/diff/200001/extensions/browser/unin...
> File extensions/browser/uninstall_reason.h (right):
> 
>
https://codereview.chromium.org/904943003/diff/200001/extensions/browser/unin...
> extensions/browser/uninstall_reason.h:26: UNINSTALL_REASON_COMPONENT
> "COMPONENT" doesn't sound like an uninstall reason. Would it be accurate to
call
> it "COMPONENT_REMOVED"?

[David Tseng, 2015-02-20 17:55:51]

New patchsets have been uploaded after l-g-t-m from finnur@chromium.org

[David Tseng, 2015-02-20 18:01:10]

With regard to fixing the underlying issue, I think we need to resolve the
question of what it means to add or remove a component extension.

Does adding mean install followed by load? It seems like the current behavior is
just about right with install only getting triggered on first load and
subsequent version increases. Does this match the behavior of ordinary
extensions?

Removal is a little more ambiguous to me. When should a removal trigger
uninstall? Since a user can't uninstall the component extension, perhaps it
should never trigger at all? I think this would match the behavior of ordinary
extensions more closely than triggering uninstall every time we remove a
component extension.

https://codereview.chromium.org/904943003/diff/200001/chrome/browser/extensio...
File chrome/browser/extensions/api/commands/command_service.cc (right):

https://codereview.chromium.org/904943003/diff/200001/chrome/browser/extensio...
chrome/browser/extensions/api/commands/command_service.cc:311:
UpdateKeybindings(extension);
On 2015/02/20 16:14:50, kalman wrote:
> For the commands case specifically, Finnur, why do we update the key bindings
> both for install and for loaded? If I'm to guess... it's so that extensions
that
> are disabled or whatever still claim the keybindings?

The loaded case is removed in this cl.

This was part of the original hack to work around what turned out to be the
unbalanced install/uninstall issue. Since component extensions only ever got
install the first time a component extension was added in a new profile, we had
to update keybindings for component extensions on loaded as well.

https://codereview.chromium.org/904943003/diff/200001/chrome/browser/extensio...
chrome/browser/extensions/api/commands/command_service.cc:319: // repeatedly,
see http://crbug.com/458612.
On 2015/02/20 16:14:50, kalman wrote:
> Could you still mention why? The bug describes the sequence of steps but it'd
be
> good to mention something extra here. I can't suggest anything because I don't
> know.

Done.

https://codereview.chromium.org/904943003/diff/200001/extensions/browser/unin...
File extensions/browser/uninstall_reason.h (right):

https://codereview.chromium.org/904943003/diff/200001/extensions/browser/unin...
extensions/browser/uninstall_reason.h:26: UNINSTALL_REASON_COMPONENT
On 2015/02/20 16:14:50, kalman wrote:
> "COMPONENT" doesn't sound like an uninstall reason. Would it be accurate to
call
> it "COMPONENT_REMOVED"?

Done.

[not at google - send to devlin, 2015-02-20 18:12:12]

On 2015/02/20 18:01:10, David Tseng wrote:
> With regard to fixing the underlying issue, I think we need to resolve the
> question of what it means to add or remove a component extension.
> 
> Does adding mean install followed by load? It seems like the current behavior
is
> just about right with install only getting triggered on first load and
> subsequent version increases. Does this match the behavior of ordinary
> extensions?
> 
> Removal is a little more ambiguous to me. When should a removal trigger
> uninstall? Since a user can't uninstall the component extension, perhaps it
> should never trigger at all? I think this would match the behavior of ordinary
> extensions more closely than triggering uninstall every time we remove a
> component extension.

Yeah, seems like we should never be firing an uninstall for component
extensions, and just fire load/unload events when they're added/removed.

[not at google - send to devlin, 2015-02-20 18:12:32]

however, lgtm, let's get this patch in for now to fix the bug before branch.

[David Tseng, 2015-02-20 18:12:45]

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

https://codereview.chromium.org/904943003/diff/180001/chrome/browser/extensio...
chrome/browser/extensions/extension_keybinding_apitest.cc:961:
IN_PROC_BROWSER_TEST_F(CommandsApiTest, Component) {
On 2015/02/20 10:31:49, Finnur wrote:
> Actually, I was hoping you'd flesh it out a bit more, something similar to:
> Make sure a component extension retains its commands upon removal and
re-adding.

Got it; done.

https://codereview.chromium.org/904943003/diff/180001/extensions/browser/unin...
File extensions/browser/uninstall_reason.h (right):

https://codereview.chromium.org/904943003/diff/180001/extensions/browser/unin...
extensions/browser/uninstall_reason.h:28: UNINSTALL_REASON_COMPONENT
On 2015/02/20 10:31:49, Finnur wrote:
> Yes, it is convention to add a trailing comma to prevent churn when checking
in
> new enums (don't have to modify the line before the one you add, just to add a
> trailing comma).

Makes sense. Done.

[David Tseng, 2015-02-20 18:14:25]

New patchsets have been uploaded after l-g-t-m from kalman@chromium.org

[Mike Wittman, 2015-02-20 18:59:30]

On my part, this change looks good but this comment from patch set 7 is still
relevant:
 
On 2015/02/13 22:11:21, Mike Wittman wrote:
https://codereview.chromium.org/904943003/diff/120001/chrome/test/data/extens...
> File chrome/test/data/extensions/api_test/keybinding/component/background.js
> (right):
> 
>
https://codereview.chromium.org/904943003/diff/120001/chrome/test/data/extens...
> chrome/test/data/extensions/api_test/keybinding/component/background.js:9:
> chrome.test.assertEq(platformBinding, commands[0].shortcut);
> Can you move the keybinding checks into C++ to match the other tests in
> extension_keybinding_apitest.cc? If so, you may be able to reuse one of the
> existing keybinding test extensions (e.g. keybindings/basics) rather than
> introducing a new one.

I'll be OOO for the next few days, so don't wait for approval from me on this
CL.

[David Tseng, 2015-02-20 19:13:37]

https://codereview.chromium.org/904943003/diff/120001/chrome/test/data/extens...
File chrome/test/data/extensions/api_test/keybinding/component/background.js
(right):

https://codereview.chromium.org/904943003/diff/120001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/keybinding/component/background.js:9:
chrome.test.assertEq(platformBinding, commands[0].shortcut);
On 2015/02/13 22:11:20, Mike Wittman wrote:
> Can you move the keybinding checks into C++ to match the other tests in
> extension_keybinding_apitest.cc? If so, you may be able to reuse one of the
> existing keybinding test extensions (e.g. keybindings/basics) rather than
> introducing a new one.

Unfortunately, I can't reuse because component extensions require a "key"
attribute in their manifest. I guess I could always add it to basics, but it
seems nice to keep things separate. I guess I could name it basics_component to
be clearer? It also seems nice to exercise the entire commands codepath from js.

[Mike Wittman, 2015-02-20 19:21:11]

lgtm

https://codereview.chromium.org/904943003/diff/120001/chrome/test/data/extens...
File chrome/test/data/extensions/api_test/keybinding/component/background.js
(right):

https://codereview.chromium.org/904943003/diff/120001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/keybinding/component/background.js:9:
chrome.test.assertEq(platformBinding, commands[0].shortcut);
On 2015/02/20 19:13:37, David Tseng wrote:
> On 2015/02/13 22:11:20, Mike Wittman wrote:
> > Can you move the keybinding checks into C++ to match the other tests in
> > extension_keybinding_apitest.cc? If so, you may be able to reuse one of the
> > existing keybinding test extensions (e.g. keybindings/basics) rather than
> > introducing a new one.
> 
> Unfortunately, I can't reuse because component extensions require a "key"
> attribute in their manifest. I guess I could always add it to basics, but it
> seems nice to keep things separate. I guess I could name it basics_component
to
> be clearer? It also seems nice to exercise the entire commands codepath from
js.

Sounds reasonable. I'm OK with the name as-is.

[Anand Mistry (off Chromium), 2015-02-22 23:50:35]

On 2015/02/20 16:14:50, kalman wrote:
> 2. The old enum this used for this, UNINSTALL_REASON_INTERNAL_MANAGEMENT, is a
> pretty weird name but also only used in a single place... when uninstalling
the
> hotwording extension... which I had thought was a component extension anyway.
> Argh what is going on.

The new hotword code (M41+) is a component extension, and the old hotword code
will be removed after M41 goes stable. We do use a shared module for the
language files and NaCl module, which we need to uninstall/reinstall on language
change, but that's not classed as a "component" in any way.

[David Tseng, 2015-02-23 16:21:00]

The CQ bit was checked by dtseng@chromium.org

[commit-bot: I haz the power, 2015-02-23 16:21:56]

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

[commit-bot: I haz the power, 2015-02-23 17:24:28]

Committed patchset #13 (id:240001)

[commit-bot: I haz the power, 2015-02-23 17:24:58]

Patchset 13 (id:??) landed as
https://crrev.com/9fb3d5b62a6466053aeee3a26c0d0a898428f4eb
Cr-Commit-Position: refs/heads/master@{#317602}