Observe OnExtensionLoaded to trigger update of chrome.commands keybindings for component extensions.

chrome/browser/extensions/extension_keybinding_registry.cc

Index: chrome/browser/extensions/extension_keybinding_registry.cc
diff --git a/chrome/browser/extensions/extension_keybinding_registry.cc b/chrome/browser/extensions/extension_keybinding_registry.cc
index dae15fd8b0561f973cf6181f7880a041c2bd3566..2ba2e94f4f36e1b64d031448591b5326511776a1 100644
--- a/chrome/browser/extensions/extension_keybinding_registry.cc
+++ b/chrome/browser/extensions/extension_keybinding_registry.cc
@@ -130,12 +130,23 @@ void ExtensionKeybindingRegistry::AddEventTarget(
     const ui::Accelerator& accelerator,
     const std::string& extension_id,
     const std::string& command_name) {
-  event_targets_[accelerator].push_back(
-      std::make_pair(extension_id, command_name));
   // Shortcuts except media keys have only one target in the list. See comment
   // about |event_targets_|.
-  if (!extensions::Command::IsMediaKey(accelerator))
-    DCHECK_EQ(1u, event_targets_[accelerator].size());
+  if (!extensions::Command::IsMediaKey(accelerator) &&
+      ((event_targets_[accelerator].size() == 1 &&
+        !event_targets_[accelerator].begin()->second.empty() &&
+        event_targets_[accelerator].begin()->second != command_name) ||
+       event_targets_[accelerator].size() > 1))
+    NOTREACHED();
+
+  // No need to add the same command again if not a media key.
+  if (!extensions::Command::IsMediaKey(accelerator) &&
+      event_targets_[accelerator].size() > 0 &&
+      event_targets_[accelerator].begin()->second == command_name)
+    return;

[Finnur, 2014/12/19 15:26:52]

Hmm... I'm not sure I like all this. I'm sure it gets the tests to pass but I'm
worried we are papering over a problem.

Originally, we had a simple DCHECK:
   DCHECK_EQ(1u, event_targets_[accelerator].size());
... which was designed to catch when we were adding another target for the same
accelerator, because that should never happen.

Then MediaKeys were introduced, and we realized that they should be
non-exclusive. Because such keys can have more than one target and the code
makes sure to let all consumers know when that key is struck (as we designed).
So we made an exception when IsMediaKey, because it suppports multiple targets.

Now, however, looking at the NOTREACHED above, I'm not even sure when it will
hit or what it is guarding now... 

I think we need to figure out why the same accelerator is being added twice and
go back to the DCHECK as it was. At least I'd like to know why it is occurring
before relaxing the restriction.

[David Tseng, 2014/12/19 19:41:01]

I was seeing multiple event targets getting added, but only on a try bot; I
never did manage to reproduce locally. Any thoughts? 

According to my logging, the same command was sometimes being added twice and
sometimes, a command with an empty name was being added.

I can see if the change to OnExtensionUninstalled makes any difference.

[Finnur, 2014/12/22 15:20:08]

My recommendation would be to revert back to the simpler DCHECK. Then, if it
passes locally, run it through the try bot and see if the change to
OnExtensionUninstalled does the trick.

If it fails locally, then you should be able to see the two callstacks that lead
to the event being added (you can run the test on its own and make it crash when
the first request comes in -- the DCHECK should already have shown you the
second time the request comes in, I suppose, otherwise you can just make it
crash the second time the request comes in).

You might even be able to do the same trick on the bots, if it doesn't repro
locally.

+
+  event_targets_[accelerator].push_back(
+      std::make_pair(extension_id, command_name));
 }
 
 bool ExtensionKeybindingRegistry::GetFirstTarget(