Fix extension command service to check if user has modified key before overwrite on update.

chrome/browser/extensions/api/commands/command_service.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/extensions/api/commands/command_service.h"
 
 #include <vector>
 
 #include "base/lazy_instance.h"
 #include "base/prefs/scoped_user_pref_update.h"
@@ skipping 333 matching lines @@
                                           const std::string& command) const {
   const base::DictionaryValue* bindings =
       profile_->GetPrefs()->GetDictionary(prefs::kExtensionCommands);
   for (base::DictionaryValue::Iterator it(*bindings); !it.IsAtEnd();
        it.Advance()) {
     const base::DictionaryValue* item = NULL;
     it.value().GetAsDictionary(&item);
 
     std::string extension;
     item->GetString(kExtension, &extension);
+

[Finnur, 2014/10/22 09:43:03]

nit: This line break seems to be breaking up code that logically fits together.
I would remove it.

     if (extension != extension_id)
       continue;
     std::string command_name;
     item->GetString(kCommandName, &command_name);
     if (command != command_name)
       continue;
     // Format stored in Preferences is: "Platform:Shortcut[:ExtensionId]".
     std::string shortcut = it.key();
     if (!IsForCurrentPlatform(shortcut))
       continue;
@@ skipping 179 matching lines @@
     AddKeybindingPref(page_action_command->accelerator(),
                       extension->id(),
                       page_action_command->command_name(),
                       false,   // Overwriting not allowed.
                       false);  // Not global.
   }
 }
 
 bool CommandService::CanAutoAssign(const Command &command,
                                    const Extension* extension) {
-  // Media Keys are non-exclusive, so allow auto-assigning them.
-  if (Command::IsMediaKey(command.accelerator()))
+

[Finnur, 2014/10/22 09:43:03]

Don't add line breaks at the start of a function.

+  // Media Keys are non-exclusive, so allow auto-assigning them if the user
+  // has not changed them for the current extension.
+  if (Command::IsMediaKey(command.accelerator())) {
+    if (IsCommandShortcutUserModified(extension, command.command_name()))
+      return false;
     return true;

[Finnur, 2014/10/22 09:43:03]

nit: Prefer:

  return !foo();

over

  if (foo())
    return false;
  return true;

But why not just leave the IsMediKey check as-is but move it below
IsCommandShortcutUserModified check? As in:

// Extensions are allowed to auto-assign updated keys if the user has not
// changed from the previous value.
if (IsCommandShortcutUserModified(extension, command.command_name()))
  return false;

// Media Keys are non-exclusive, so allow auto-assigning them.
if (Command::IsMediaKey(command.accelerator()))
  return true;

+  }
 
   // Extensions are allowed to auto-assign updated keys if the user has not
   // changed from the previous value.
   if (IsCommandShortcutUserModified(extension, command.command_name()))
     return false;
 
   if (command.global()) {
     using namespace extensions;
     if (command.command_name() == manifest_values::kBrowserActionCommandEvent ||
         command.command_name() == manifest_values::kPageActionCommandEvent)
@@ skipping 297 matching lines @@
   return true;
 }
 
 template <>
 void
 BrowserContextKeyedAPIFactory<CommandService>::DeclareFactoryDependencies() {
   DependsOn(ExtensionCommandsGlobalRegistry::GetFactoryInstance());
 }
 
 }  // namespace extensions