Move Commands from Extension to CommandsHandler

Move Commands from Extension to CommandsHandler

TBR=thakis@chromium.org for c/b/ui/cocoa
BUG=159265
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=178485

[Devlin, 2012-12-21 20:36:21]

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

https://codereview.chromium.org/11662013/diff/2001/chrome/browser/extensions/...
chrome/browser/extensions/api/commands/command_service.cc:47:
ManifestHandler::Register(extension_manifest_keys::kCommands,
I don't think we want to create an additional CommandsAPI, when CommandService
does a lot of the same setup (but I could be wrong).  That being said, do we
want to rename CommandService to CommandsAPI for consistency?

[Yoyo Zhou, 2012-12-22 00:38:32]

LGTM

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

https://codereview.chromium.org/11662013/diff/2001/chrome/browser/extensions/...
chrome/browser/extensions/api/commands/command_service.cc:47:
ManifestHandler::Register(extension_manifest_keys::kCommands,
On 2012/12/21 20:36:21, D Cronin wrote:
> I don't think we want to create an additional CommandsAPI, when CommandService
> does a lot of the same setup (but I could be wrong).  That being said, do we
> want to rename CommandService to CommandsAPI for consistency?

No, it doesn't need to be renamed.

https://codereview.chromium.org/11662013/diff/2001/chrome/common/extensions/a...
File chrome/common/extensions/api/commands/commands_handler.cc (right):

https://codereview.chromium.org/11662013/diff/2001/chrome/common/extensions/a...
chrome/common/extensions/api/commands/commands_handler.cc:14: namespace
extensions {
add newline

[Devlin, 2012-12-27 20:39:12]

https://codereview.chromium.org/11662013/diff/2001/chrome/common/extensions/a...
File chrome/common/extensions/api/commands/commands_handler.cc (right):

https://codereview.chromium.org/11662013/diff/2001/chrome/common/extensions/a...
chrome/common/extensions/api/commands/commands_handler.cc:14: namespace
extensions {
On 2012/12/22 00:38:33, Yoyo Zhou wrote:
> add newline

Done.

[Devlin, 2013-01-23 00:14:57]

+ben@ and nileshagrawal@ for added exceptions to gypis.
This doesn't add much new code to be compiled, since it is almost all ripped out
of the Extension class.

Let me know if it's okay to TBR these in the future (most gypi changes are, but
since these add exceptions, figured I'd double check).

[nilesh, 2013-01-23 00:45:28]

On 2013/01/23 00:14:57, D Cronin wrote:
> +ben@ and nileshagrawal@ for added exceptions to gypis.
> This doesn't add much new code to be compiled, since it is almost all ripped
out
> of the Extension class.
> 
> Let me know if it's okay to TBR these in the future (most gypi changes are,
but
> since these add exceptions, figured I'd double check).

It looks like both command_service.cc and omnibox_handler.cc are excluded on
android, and you need to include command_handler.cc just for the unittests. I
would rather exclude the unittest from the build.(extension_unittests.cc)
ExtensionsTest.* is already disabled on Android.

[Devlin, 2013-01-23 18:15:50]

On 2013/01/23 00:45:28, nileshagrawal1 wrote:
> On 2013/01/23 00:14:57, D Cronin wrote:
> > +ben@ and nileshagrawal@ for added exceptions to gypis.
> > This doesn't add much new code to be compiled, since it is almost all ripped
> out
> > of the Extension class.
> > 
> > Let me know if it's okay to TBR these in the future (most gypi changes are,
> but
> > since these add exceptions, figured I'd double check).
> 
> It looks like both command_service.cc and omnibox_handler.cc are excluded on
> android, and you need to include command_handler.cc just for the unittests. I
> would rather exclude the unittest from the build.(extension_unittests.cc)
> ExtensionsTest.* is already disabled on Android.

Done.

[nilesh, 2013-01-23 18:27:54]

https://chromiumcodereview.appspot.com/11662013/diff/31015/chrome/chrome_test...
File chrome/chrome_tests_unit.gypi (right):

https://chromiumcodereview.appspot.com/11662013/diff/31015/chrome/chrome_test...
chrome/chrome_tests_unit.gypi:1769:
'browser/extensions/permissions_updater_unittest.cc',
Please exclude the test here (in enable_extentions==0)

https://chromiumcodereview.appspot.com/11662013/diff/31015/chrome/chrome_test...
chrome/chrome_tests_unit.gypi:2194: ['exclude',
'^common/extensions/extension_unittest.cc'],
Please exclude this file using the "sources!" list in the enable_extensions==0
block.

[Devlin, 2013-01-23 20:21:17]

https://chromiumcodereview.appspot.com/11662013/diff/31015/chrome/chrome_test...
File chrome/chrome_tests_unit.gypi (right):

https://chromiumcodereview.appspot.com/11662013/diff/31015/chrome/chrome_test...
chrome/chrome_tests_unit.gypi:1769:
'browser/extensions/permissions_updater_unittest.cc',
On 2013/01/23 18:27:54, nileshagrawal1 wrote:
> Please exclude the test here (in enable_extentions==0)

Done.  Sorry about that; I'm not very familiar with gypis.

https://chromiumcodereview.appspot.com/11662013/diff/31015/chrome/chrome_test...
chrome/chrome_tests_unit.gypi:2194: ['exclude',
'^common/extensions/extension_unittest.cc'],
On 2013/01/23 18:27:54, nileshagrawal1 wrote:
> Please exclude this file using the "sources!" list in the enable_extensions==0
> block.

Done.

[nilesh, 2013-01-23 20:52:12]

On 2013/01/23 00:14:57, D Cronin wrote:
> +ben@ and nileshagrawal@ for added exceptions to gypis.

LGTM

[Ben Goodger (Google), 2013-01-23 22:34:57]

lgtm

[commit-bot: I haz the power, 2013-01-23 23:30:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rdevlin.cronin@chromium.org/11662013/3...

[commit-bot: I haz the power, 2013-01-24 02:30:36]

Change committed as 178485