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

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

Worked for all other keys except for the media keys where CanAutoAssign would ALWAYS return true even if key was modified by user. Now no commands should be overwritten on update if they have been changed by a user.

BUG=421482
R=finnur@chromium.org

Committed: https://crrev.com/26df1b6ef65f42d581c2d8d5e0e2807d67602f2b
Cr-Commit-Position: refs/heads/master@{#301309}

[Finnur, 2014-10-22 09:43:02]

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

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

Adding Mike as FYI, in case he has any objections.

To me this looks fine, thanks for adding a test. Nits below.

https://codereview.chromium.org/667833004/diff/1/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/commands/command_service.cc (right):

https://codereview.chromium.org/667833004/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/commands/command_service.cc:354: 
nit: This line break seems to be breaking up code that logically fits together.
I would remove it.

https://codereview.chromium.org/667833004/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/commands/command_service.cc:554: 
Don't add line breaks at the start of a function.

https://codereview.chromium.org/667833004/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/commands/command_service.cc:560: return true;
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;

https://codereview.chromium.org/667833004/diff/1/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_keybinding_apitest.cc (right):

https://codereview.chromium.org/667833004/diff/1/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_keybinding_apitest.cc:6: #include
"base/strings/stringprintf.h"
Remove?

https://codereview.chromium.org/667833004/diff/1/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_keybinding_apitest.cc:655: 
nit: remove this linebreak.

https://codereview.chromium.org/667833004/diff/1/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_keybinding_apitest.cc:681:
MediaKeyShortcutChangedOnUpdateAfterBeingReassignedByUser) {
nit: Adhere to the 80 cols rule.

https://codereview.chromium.org/667833004/diff/1/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_keybinding_apitest.cc:710: 
nit: Same here -- don't need the line break.

https://codereview.chromium.org/667833004/diff/1/chrome/test/data/extensions/...
File chrome/test/data/extensions/api_test/keybinding/update/mk_v1/background.js
(right):

https://codereview.chromium.org/667833004/diff/1/chrome/test/data/extensions/...
chrome/test/data/extensions/api_test/keybinding/update/mk_v1/background.js:1: //
Copyright (c) 2012 The Chromium Authors. All rights reserved.
Drop the (c) and use 2014 as the year.

https://codereview.chromium.org/667833004/diff/1/chrome/test/data/extensions/...
File chrome/test/data/extensions/api_test/keybinding/update/mk_v1/manifest.json
(right):

https://codereview.chromium.org/667833004/diff/1/chrome/test/data/extensions/...
chrome/test/data/extensions/api_test/keybinding/update/mk_v1/manifest.json:2:
"name": "A test for checking that user keybings persist after update",
This name is a bit weird. I'd go with:
A test for making sure media keys that get reassigned don't get reinstated
during update.

https://codereview.chromium.org/667833004/diff/1/chrome/test/data/extensions/...
File chrome/test/data/extensions/api_test/keybinding/update/mk_v2/background.js
(right):

https://codereview.chromium.org/667833004/diff/1/chrome/test/data/extensions/...
chrome/test/data/extensions/api_test/keybinding/update/mk_v2/background.js:1: //
Copyright (c) 2012 The Chromium Authors. All rights reserved.
Drop the (c) and use 2014 as the year.

https://codereview.chromium.org/667833004/diff/1/extensions/common/manifest_c...
File extensions/common/manifest_constants.h (right):

https://codereview.chromium.org/667833004/diff/1/extensions/common/manifest_c...
extensions/common/manifest_constants.h:238: extern const char
kMediaKeyTestCommand[];
You only seem to use this in the test file, so I wouldn't add it here. Just
define it locally at the top of the test .cc file (in the anonymous namespace).

[Mike Wittman, 2014-10-22 18:15:38]

Looks good other than Finnur's comments and the test data change.

Sorry for the breakage!

https://codereview.chromium.org/667833004/diff/1/chrome/test/data/extensions/...
File chrome/test/data/extensions/api_test/keybinding/update/mk_v2/manifest.json
(right):

https://codereview.chromium.org/667833004/diff/1/chrome/test/data/extensions/...
chrome/test/data/extensions/api_test/keybinding/update/mk_v2/manifest.json:3:
"version": "1.0",
This should be "version": "2.0" in order to trigger the extension update path.

[Finnur, 2014-10-22 21:02:41]

Wait? What? ... You are apologizing for the "breakage"? Really? LOL. :)

You improved on what we had and I could just as well apologize for failing to
spot what was missing. :)

But not to worry, aegabriel makes up for our shortcomings. :)

[aegabriel, 2014-10-23 02:51:39]

On 2014/10/22 21:02:41, Finnur wrote:
> Wait? What? ... You are apologizing for the "breakage"? Really? LOL. :)
> 
> You improved on what we had and I could just as well apologize for failing to
> spot what was missing. :)
> 
> But not to worry, aegabriel makes up for our shortcomings. :)

Awesome, thanks for the thorough review's guys! I have updated the code as per
your suggestions.

Thanks for adding this feature in the first place Mike! :)

[Finnur, 2014-10-23 09:09:54]

Just a few more nits.

https://codereview.chromium.org/667833004/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/api/commands/command_service.cc (right):

https://codereview.chromium.org/667833004/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/commands/command_service.cc:558: // Media Keys are
non-exclusive, so allow auto-assigning them
nit: End your comments with some kind of punctuation (period in this case).

https://codereview.chromium.org/667833004/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/extension_keybinding_apitest.cc (right):

https://codereview.chromium.org/667833004/diff/20001/chrome/browser/extension...
chrome/browser/extensions/extension_keybinding_apitest.cc:682:
MediaKeyShortcutChangedOnUpdateAfterBeingReassignedByUser) {
style: Indentation is off here, should be 4 spaces after beginning of line.

https://codereview.chromium.org/667833004/diff/20001/chrome/test/data/extensi...
File chrome/test/data/extensions/api_test/keybinding/update/mk_v1/manifest.json
(right):

https://codereview.chromium.org/667833004/diff/20001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/keybinding/update/mk_v1/manifest.json:3:
during update.",
nit: this looks a bit weird, is there a line break in the wrong location?

https://codereview.chromium.org/667833004/diff/20001/chrome/test/data/extensi...
File chrome/test/data/extensions/api_test/keybinding/update/mk_v2/manifest.json
(right):

https://codereview.chromium.org/667833004/diff/20001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/keybinding/update/mk_v2/manifest.json:3:
during update.",
Same question here.

[aegabriel, 2014-10-23 14:43:42]

Added the style fixes and cleanup in.

[Finnur, 2014-10-23 15:08:27]

LGTM

[aegabriel, 2014-10-24 16:03:45]

The CQ bit was checked by minilogo@gmail.com

[commit-bot: I haz the power, 2014-10-24 16:05:08]

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

[commit-bot: I haz the power, 2014-10-24 16:10:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-24 16:10:44]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[aegabriel, 2014-10-27 01:20:38]

The CQ bit was checked by minilogo@gmail.com

[commit-bot: I haz the power, 2014-10-27 01:21:52]

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

[commit-bot: I haz the power, 2014-10-27 01:24:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-27 01:24:27]

Try jobs failed on following builders:
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/73741)
  android_arm64_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...)
  linux_chromium_chromeos_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_32 on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel/...)

[aegabriel, 2014-10-27 01:42:45]

The CQ bit was checked by minilogo@gmail.com

[commit-bot: I haz the power, 2014-10-27 01:43:45]

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

[commit-bot: I haz the power, 2014-10-27 01:46:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-27 01:46:33]

Try jobs failed on following builders:
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/73743)
  android_arm64_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
  win_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel/...)

[aegabriel, 2014-10-27 01:55:40]

The CQ bit was checked by minilogo@gmail.com

[commit-bot: I haz the power, 2014-10-27 01:56:06]

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

[commit-bot: I haz the power, 2014-10-27 02:50:06]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2014-10-27 02:50:51]

Patchset 6 (id:??) landed as
https://crrev.com/26df1b6ef65f42d581c2d8d5e0e2807d67602f2b
Cr-Commit-Position: refs/heads/master@{#301309}