Move default_locale out of Extension class

Move default_locale out of Extension class.

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

[Devlin, 2013-01-18 21:08:46]

https://codereview.chromium.org/12025010/diff/1/chrome/browser/extensions/api...
File chrome/browser/extensions/api/i18n/i18n_api.cc (right):

https://codereview.chromium.org/12025010/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/i18n/i18n_api.cc:67: new DefaultLocaleHandler);
Hmm.  We've been shifting to registering functions in the API, when it's
initialized, but the I18n function is registered in a generated file
(generated_api.h). Yoyo, do we want to keep the registration there, or move it
into the API, like the others?

https://codereview.chromium.org/12025010/diff/1/chrome/common/extensions/api/...
File chrome/common/extensions/api/i18n/default_locale_handler.h (right):

https://codereview.chromium.org/12025010/diff/1/chrome/common/extensions/api/...
chrome/common/extensions/api/i18n/default_locale_handler.h:18: std::string
default_locale_;
nit: structs don't use the trailing underscore for variables (since they aren't
private).

https://codereview.chromium.org/12025010/diff/1/chrome/common/extensions/exte...
File chrome/common/extensions/extension_file_util_unittest.cc (right):

https://codereview.chromium.org/12025010/diff/1/chrome/common/extensions/exte...
chrome/common/extensions/extension_file_util_unittest.cc:29:
extensions::ManifestHandler::Register(keys::kDefaultLocale,
Call the super's SetUp function as well (even if it doesn't do anything yet).

https://codereview.chromium.org/12025010/diff/1/chrome/common/extensions/mani...
File
chrome/common/extensions/manifest_tests/extension_manifests_default_locale_unittest.cc
(right):

https://codereview.chromium.org/12025010/diff/1/chrome/common/extensions/mani...
chrome/common/extensions/manifest_tests/extension_manifests_default_locale_unittest.cc:1:
// Copyright (c) 2013 The Chromium Authors. All rights reserved.
This file should be with the handler. To keep with how we've been naming them,
this one should be c/c/e/api/i18n/default_locale_manifest_unittest.cc

https://codereview.chromium.org/12025010/diff/1/chrome/common/extensions/mani...
chrome/common/extensions/manifest_tests/extension_manifests_default_locale_unittest.cc:14:
extensions::ManifestHandler::Register(keys::kDefaultLocale,
This whole file should probably be in the extensions namespace.

https://codereview.chromium.org/12025010/diff/1/chrome/common/extensions/unpa...
File chrome/common/extensions/unpacker_unittest.cc (right):

https://codereview.chromium.org/12025010/diff/1/chrome/common/extensions/unpa...
chrome/common/extensions/unpacker_unittest.cc:34: virtual void SetUp() OVERRIDE
{
Call the super's SetUp().

[Joe Thomas, 2013-01-18 22:16:57]

Thanks for the review. Patch updated.

https://codereview.chromium.org/12025010/diff/1/chrome/browser/extensions/api...
File chrome/browser/extensions/api/i18n/i18n_api.cc (right):

https://codereview.chromium.org/12025010/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/i18n/i18n_api.cc:67: new DefaultLocaleHandler);
On 2013/01/18 21:08:46, D Cronin wrote:
> Hmm.  We've been shifting to registering functions in the API, when it's
> initialized, but the I18n function is registered in a generated file
> (generated_api.h). Yoyo, do we want to keep the registration there, or move it
> into the API, like the others?

I will wait for Yoyo's decision on this.

https://codereview.chromium.org/12025010/diff/1/chrome/common/extensions/api/...
File chrome/common/extensions/api/i18n/default_locale_handler.h (right):

https://codereview.chromium.org/12025010/diff/1/chrome/common/extensions/api/...
chrome/common/extensions/api/i18n/default_locale_handler.h:18: std::string
default_locale_;
On 2013/01/18 21:08:46, D Cronin wrote:
> nit: structs don't use the trailing underscore for variables (since they
aren't
> private).

Done.

https://codereview.chromium.org/12025010/diff/1/chrome/common/extensions/exte...
File chrome/common/extensions/extension_file_util_unittest.cc (right):

https://codereview.chromium.org/12025010/diff/1/chrome/common/extensions/exte...
chrome/common/extensions/extension_file_util_unittest.cc:29:
extensions::ManifestHandler::Register(keys::kDefaultLocale,
On 2013/01/18 21:08:46, D Cronin wrote:
> Call the super's SetUp function as well (even if it doesn't do anything yet).

Done.

https://codereview.chromium.org/12025010/diff/1/chrome/common/extensions/mani...
File
chrome/common/extensions/manifest_tests/extension_manifests_default_locale_unittest.cc
(right):

https://codereview.chromium.org/12025010/diff/1/chrome/common/extensions/mani...
chrome/common/extensions/manifest_tests/extension_manifests_default_locale_unittest.cc:1:
// Copyright (c) 2013 The Chromium Authors. All rights reserved.
On 2013/01/18 21:08:46, D Cronin wrote:
> This file should be with the handler. To keep with how we've been naming them,
> this one should be c/c/e/api/i18n/default_locale_manifest_unittest.cc

Done.

https://codereview.chromium.org/12025010/diff/1/chrome/common/extensions/mani...
chrome/common/extensions/manifest_tests/extension_manifests_default_locale_unittest.cc:14:
extensions::ManifestHandler::Register(keys::kDefaultLocale,
On 2013/01/18 21:08:46, D Cronin wrote:
> This whole file should probably be in the extensions namespace.

Done.

https://codereview.chromium.org/12025010/diff/1/chrome/common/extensions/unpa...
File chrome/common/extensions/unpacker_unittest.cc (right):

https://codereview.chromium.org/12025010/diff/1/chrome/common/extensions/unpa...
chrome/common/extensions/unpacker_unittest.cc:34: virtual void SetUp() OVERRIDE
{
On 2013/01/18 21:08:46, D Cronin wrote:
> Call the super's SetUp().

Done.

[Devlin, 2013-01-18 23:30:33]

lgtm

[Yoyo Zhou, 2013-01-22 20:15:56]

https://codereview.chromium.org/12025010/diff/5002/chrome/common/extensions/e...
File chrome/common/extensions/extension_file_util.cc (right):

https://codereview.chromium.org/12025010/diff/5002/chrome/common/extensions/e...
chrome/common/extensions/extension_file_util.cc:567:
extensions::LocaleInfo::GetDefaultLocale(&extension);
This validation is done in the utility process so you'll need to register the
handler in ChromeContentUtilityClient.

[Joe Thomas, 2013-01-23 20:01:08]

Added ManifestHandler registration in ChromeContentUtilityClient. Also rebased
the patch.

https://chromiumcodereview.appspot.com/12025010/diff/5002/chrome/common/exten...
File chrome/common/extensions/extension_file_util.cc (right):

https://chromiumcodereview.appspot.com/12025010/diff/5002/chrome/common/exten...
chrome/common/extensions/extension_file_util.cc:567:
extensions::LocaleInfo::GetDefaultLocale(&extension);
On 2013/01/22 20:15:56, Yoyo Zhou wrote:
> This validation is done in the utility process so you'll need to register the
> handler in ChromeContentUtilityClient.

Done.

[Yoyo Zhou, 2013-01-23 22:45:47]

LGTM

[Joe Thomas, 2013-01-23 23:12:19]

+ben for c/b/renderer_host/, chrome/utility/ & gyp changes
+mirandac for profiles/

[Miranda Callahan, 2013-01-24 14:07:20]

On 2013/01/23 23:12:19, Joe Thomas wrote:
> +ben for c/b/renderer_host/, chrome/utility/ & gyp changes
> +mirandac for profiles/

profile_dependency_manager LGTM

[Ben Goodger (Google), 2013-01-28 17:37:04]

lgtm

[commit-bot: I haz the power, 2013-01-28 17:39:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/MHX348@motorola.com/12025010/14001

[commit-bot: I haz the power, 2013-01-28 18:17:42]

Sorry for I got bad news for ya.
Compile failed with a clobber build on android_clang_dbg.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[Joe Thomas, 2013-01-28 22:33:38]

Yoyo@,

It looks like I don't have the permission to add trybots. It's giving me
"failure -1" error. 

Can you please help me to add it to Android bots?

[Yoyo Zhou, 2013-01-28 22:42:45]

On 2013/01/28 22:33:38, Joe Thomas wrote:
> Yoyo@,
> 
> It looks like I don't have the permission to add trybots. It's giving me
> "failure -1" error. 
> 
> Can you please help me to add it to Android bots?

I guess only committers have permission to use those checkboxes. I added some
android bots for you.

[Joe Thomas, 2013-01-29 00:01:00]

Yoyo@

Could you please add the android bot again? The previous patch failed to apply
for some reason. I rebased the patch again.

Thanks, Joe

[Yoyo Zhou, 2013-01-29 00:07:34]

On 2013/01/29 00:01:00, Joe Thomas wrote:
> Yoyo@
> 
> Could you please add the android bot again? The previous patch failed to apply
> for some reason. I rebased the patch again.
> 
> Thanks, Joe

I think the "try more bots" tool patches atop LKGR, which is somewhat behind the
head of the tree. If it fails again we might have to wait for it to catch up, or
I can go and try to patch Rietveld to support patching on a different revision
number.

[Joe Thomas, 2013-01-29 00:13:13]

On 2013/01/29 00:07:34, Yoyo Zhou wrote:
> On 2013/01/29 00:01:00, Joe Thomas wrote:
> > Yoyo@
> > 
> > Could you please add the android bot again? The previous patch failed to
apply
> > for some reason. I rebased the patch again.
> > 
> > Thanks, Joe
> 
> I think the "try more bots" tool patches atop LKGR, which is somewhat behind
the
> head of the tree. If it fails again we might have to wait for it to catch up,
or
> I can go and try to patch Rietveld to support patching on a different revision
> number.

It failed again. In think we can try again later (I don't want to give you more
trouble :-)).

[Joe Thomas, 2013-01-29 19:51:32]

Yoyo@,
Can you please try adding the Android bots again?  Thanks.

[Joe Thomas, 2013-01-29 22:24:44]

+ nileshagrawal@ for android gypi changes.

Yoyo@
Fixed the unittests in sandboxed_unpacker_unittest.cc &
extension_manifests_initvalue_unittest.cc

[Yoyo Zhou, 2013-01-29 22:33:47]

LGTM

[nilesh, 2013-01-29 22:47:58]

On 2013/01/29 22:24:44, Joe Thomas wrote:
> + nileshagrawal@ for android gypi changes.

LGTM

[Joe Thomas, 2013-01-30 01:08:55]

Yoyo@,

Please take a look again.

Using the new ManifestHandler interface (default_locale_handler.cc/h)
Fixed couple of nits in manifest_handler.cc.

Thanks

[Yoyo Zhou, 2013-01-30 01:10:59]

On 2013/01/30 01:08:55, Joe Thomas wrote:
> Yoyo@,
> 
> Please take a look again.
> 
> Using the new ManifestHandler interface (default_locale_handler.cc/h)
> Fixed couple of nits in manifest_handler.cc.
> 
> Thanks

LGTM

[commit-bot: I haz the power, 2013-01-30 01:14:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/MHX348@motorola.com/12025010/34006

[commit-bot: I haz the power, 2013-01-30 03:55:16]

Retried try job too often on mac_rel for step(s) unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[Joe Thomas, 2013-01-30 20:01:25]

Yoyo@ 

FYI - Fixed a unittest in extension_service_unittest.cc

[commit-bot: I haz the power, 2013-01-30 20:06:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/MHX348@motorola.com/12025010/29012

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

Change committed as 179711