Make sure keybindings removed don't come back during extension update.

Make sure keybindings removed don't come back during extension update.

BUG=234947
TBR=yoz@chromium.org

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

[Yoyo Zhou, 2013-05-06 18:47:58]

https://codereview.chromium.org/14990002/diff/11001/chrome/browser/extensions...
File chrome/browser/extensions/api/commands/command_service.cc (right):

https://codereview.chromium.org/14990002/diff/11001/chrome/browser/extensions...
chrome/browser/extensions/api/commands/command_service.cc:237: if
(extension_prefs->InitialBindingsHaveBeenAssigned(extension->id()))
We now prefer to have less API-specific knowledge in ExtensionPrefs, so I'd
prefer something like
InitialBindingsHaveBeenAssigned(extension_prefs, extension->id())
and that could go here in an anonymous namespace.

https://codereview.chromium.org/14990002/diff/11001/chrome/browser/ui/views/b...
File chrome/browser/ui/views/browser_action_view.cc (right):

https://codereview.chromium.org/14990002/diff/11001/chrome/browser/ui/views/b...
chrome/browser/ui/views/browser_action_view.cc:410: keybinding_.reset(NULL);
Why is this views-only?

[Finnur, 2013-05-06 23:13:54]

https://codereview.chromium.org/14990002/diff/11001/chrome/browser/extensions...
File chrome/browser/extensions/api/commands/command_service.cc (right):

https://codereview.chromium.org/14990002/diff/11001/chrome/browser/extensions...
chrome/browser/extensions/api/commands/command_service.cc:237: if
(extension_prefs->InitialBindingsHaveBeenAssigned(extension->id()))
Do you have a CL I can take a look at to see how the latest and greatest truth
should be implemented? :)

https://codereview.chromium.org/14990002/diff/11001/chrome/browser/ui/views/b...
File chrome/browser/ui/views/browser_action_view.cc (right):

https://codereview.chromium.org/14990002/diff/11001/chrome/browser/ui/views/b...
chrome/browser/ui/views/browser_action_view.cc:410: keybinding_.reset(NULL);
Good question. This was something I discovered by the test I wrote for this CL.
If we don't reset this, then destroying the button will trigger us to try to
unregister this keybinding again and the FocusManager will DCHECK because the
keybinding isn't registered (naturally). That doesn't happen on other platforms
(as keybindings are handled differently on each platform).

[Yoyo Zhou, 2013-05-07 01:21:05]

https://codereview.chromium.org/14990002/diff/11001/chrome/browser/extensions...
File chrome/browser/extensions/api/commands/command_service.cc (right):

https://codereview.chromium.org/14990002/diff/11001/chrome/browser/extensions...
chrome/browser/extensions/api/commands/command_service.cc:237: if
(extension_prefs->InitialBindingsHaveBeenAssigned(extension->id()))
On 2013/05/06 23:13:54, Finnur wrote:
> Do you have a CL I can take a look at to see how the latest and greatest truth
> should be implemented? :)

Sure, there's a good example in

https://codereview.chromium.org/14699002/diff/35001/chrome/browser/extensions...

[Finnur, 2013-05-07 10:56:26]

Done, PTAL.

Also, TBR'ed Scott for trivial Views change (Browser Action Button).

[Yoyo Zhou, 2013-05-07 16:23:35]

LGTM

[Finnur, 2013-05-08 10:15:29]

Committed patchset #4 manually as r198873 (presubmit successful).