This change enables Chrome to load locale information for the extension. It d...

This change enables Chrome to load locale information for the extension. It detects default locale, and filters out all locales not supported
by Chrome or with invalid names/missing messages.

It also checks for folders that start with _ and are not in the reserved list.

We don't validate messages file with this CL.

Added support for loading supplied locale information to the extension_file_util, and detecting default locale.
Added new constants to extension class (_locales directory name, messages filename).
Added new error messages to _constants.
Added new unittests.

BUG=12131
TEST=There should be no visible changes, except in case of error when loading extension (e.g. create empty _locales folder and try loading).

[Nebojša Ćirić, 2009-08-14 22:22:04]

First of many i18n changes. I tried to keep it as small as possible, but there
was lots to unittest.

Fixed few old lint errors too...

[Aaron Boodman, 2009-08-17 18:48:43]

http://codereview.chromium.org/170015/diff/1/7
File chrome/browser/extensions/extension_file_util.cc (right):

http://codereview.chromium.org/170015/diff/1/7#newcode289
Line 289: 
Since you are adding the first case of a special name that starts with
underscore, can you also add a check that there are no other files or
directories in the extension root that start with underscore?

We basically want to reserve this namespace for use by the system.

http://codereview.chromium.org/170015/diff/1/4
File chrome/browser/extensions/extension_l10n_util.cc (right):

http://codereview.chromium.org/170015/diff/1/4#newcode18
Line 18: bool FindDefaultLocale(const std::string& default_locale,
I know this is what we talked about, but seeing this logic in black and white
makes me realize it is over complicated.

Localization isn't required right? So how about if you use localization, you
*must* specify a default locale. As easy as that.

That would mean adding a key to the manifest like default_locale.

http://codereview.chromium.org/170015/diff/1/4#newcode50
Line 50: if (file_util::PathExists(locale_folder)) {
Don't you know that this folder exists since it was found by iterating the
parent directory?

Also, is there going to be more than one localization resource per locale? Or
would a single file work?

http://codereview.chromium.org/170015/diff/1/4#newcode73
Line 73: if (!AddLocale(chrome_locales, locale_folder, extension, &locale_name))
{
Do the list of Chromium-supported locales change frequently? Usually we try not
to silently fail on developer errors like this. If the list of valid locales
changes infrequently, I would prefer that having an invalid one was an error.

In that case, pass a std::string* error to this function and set it with details
of the bad locale.

http://codereview.chromium.org/170015/diff/1/6
File chrome/browser/extensions/extension_l10n_util_unittest.cc (right):

http://codereview.chromium.org/170015/diff/1/6#newcode66
Line 66: EXPECT_TRUE(extension->supported_locales().empty());
This is another example of a developer error. All of these should ideally be
reported as error messages.

http://codereview.chromium.org/170015/diff/1/11
File chrome/common/extensions/extension.cc (right):

http://codereview.chromium.org/170015/diff/1/11#newcode70
Line 70: const char Extension::kLocaleFolder[] = "_locale";
Nit: Can this please be called "_locales"?

[Aaron Boodman, 2009-08-17 18:50:15]

Also, sorry for taking so long to review this. Please poke me next
time I don't respond within a day.

- a

On Mon, Aug 17, 2009 at 11:48 AM, <aa@chromium.org> wrote:
>
> http://codereview.chromium.org/170015/diff/1/7
> File chrome/browser/extensions/extension_file_util.cc (right):
>
> http://codereview.chromium.org/170015/diff/1/7#newcode289
> Line 289:
> Since you are adding the first case of a special name that starts with
> underscore, can you also add a check that there are no other files or
> directories in the extension root that start with underscore?
>
> We basically want to reserve this namespace for use by the system.
>
> http://codereview.chromium.org/170015/diff/1/4
> File chrome/browser/extensions/extension_l10n_util.cc (right):
>
> http://codereview.chromium.org/170015/diff/1/4#newcode18
> Line 18: bool FindDefaultLocale(const std::string& default_locale,
> I know this is what we talked about, but seeing this logic in black and
> white makes me realize it is over complicated.
>
> Localization isn't required right? So how about if you use localization,
> you *must* specify a default locale. As easy as that.
>
> That would mean adding a key to the manifest like default_locale.
>
> http://codereview.chromium.org/170015/diff/1/4#newcode50
> Line 50: if (file_util::PathExists(locale_folder)) {
> Don't you know that this folder exists since it was found by iterating
> the parent directory?
>
> Also, is there going to be more than one localization resource per
> locale? Or would a single file work?
>
> http://codereview.chromium.org/170015/diff/1/4#newcode73
> Line 73: if (!AddLocale(chrome_locales, locale_folder, extension,
> &locale_name)) {
> Do the list of Chromium-supported locales change frequently? Usually we
> try not to silently fail on developer errors like this. If the list of
> valid locales changes infrequently, I would prefer that having an
> invalid one was an error.
>
> In that case, pass a std::string* error to this function and set it with
> details of the bad locale.
>
> http://codereview.chromium.org/170015/diff/1/6
> File chrome/browser/extensions/extension_l10n_util_unittest.cc (right):
>
> http://codereview.chromium.org/170015/diff/1/6#newcode66
> Line 66: EXPECT_TRUE(extension->supported_locales().empty());
> This is another example of a developer error. All of these should
> ideally be reported as error messages.
>
> http://codereview.chromium.org/170015/diff/1/11
> File chrome/common/extensions/extension.cc (right):
>
> http://codereview.chromium.org/170015/diff/1/11#newcode70
> Line 70: const char Extension::kLocaleFolder[] = "_locale";
> Nit: Can this please be called "_locales"?
>
> http://codereview.chromium.org/170015
>

[Nebojša Ćirić, 2009-08-17 20:24:52]

:), I don't expect people to work on weekends, and this was sent Friday
afternoon, so you are right on time.
I've noticed that one has to restart Chrome to change locale/language. Since
LoadExtension code already deals with JSON parsing, and loading files, I
think we could load/parse message catalogs at that time too, not wait for
load on demand. What do you think?

I left TODO(cira) next to FIleEnumerator to draw attention :). Erik left a
comment with FileEnumerator that warns against its use in the main thread. I
am not sure if LoadExtension is executed on the main or not.

Will work on your comments, but answers to these two questions would help
re-organize code somewhat.

Cira

On Mon, Aug 17, 2009 at 11:48 AM, <aa@chromium.org> wrote:

>
> http://codereview.chromium.org/170015/diff/1/7
> File chrome/browser/extensions/extension_file_util.cc (right):
>
> http://codereview.chromium.org/170015/diff/1/7#newcode289
> Line 289:
> Since you are adding the first case of a special name that starts with
> underscore, can you also add a check that there are no other files or
> directories in the extension root that start with underscore?
>
> We basically want to reserve this namespace for use by the system.
>
> http://codereview.chromium.org/170015/diff/1/4
> File chrome/browser/extensions/extension_l10n_util.cc (right):
>
> http://codereview.chromium.org/170015/diff/1/4#newcode18
> Line 18: bool FindDefaultLocale(const std::string& default_locale,
> I know this is what we talked about, but seeing this logic in black and
> white makes me realize it is over complicated.
>
> Localization isn't required right? So how about if you use localization,
> you *must* specify a default locale. As easy as that.
>
> That would mean adding a key to the manifest like default_locale.
>
> http://codereview.chromium.org/170015/diff/1/4#newcode50
> Line 50: if (file_util::PathExists(locale_folder)) {
> Don't you know that this folder exists since it was found by iterating
> the parent directory?
>
> Also, is there going to be more than one localization resource per
> locale? Or would a single file work?
>
> http://codereview.chromium.org/170015/diff/1/4#newcode73
> Line 73: if (!AddLocale(chrome_locales, locale_folder, extension,
> &locale_name)) {
> Do the list of Chromium-supported locales change frequently? Usually we
> try not to silently fail on developer errors like this. If the list of
> valid locales changes infrequently, I would prefer that having an
> invalid one was an error.
>
> In that case, pass a std::string* error to this function and set it with
> details of the bad locale.
>
> http://codereview.chromium.org/170015/diff/1/6
> File chrome/browser/extensions/extension_l10n_util_unittest.cc (right):
>
> http://codereview.chromium.org/170015/diff/1/6#newcode66
> Line 66: EXPECT_TRUE(extension->supported_locales().empty());
> This is another example of a developer error. All of these should
> ideally be reported as error messages.
>
> http://codereview.chromium.org/170015/diff/1/11
> File chrome/common/extensions/extension.cc (right):
>
> http://codereview.chromium.org/170015/diff/1/11#newcode70
> Line 70: const char Extension::kLocaleFolder[] = "_locale";
> Nit: Can this please be called "_locales"?
>
>
> http://codereview.chromium.org/170015
>

[Aaron Boodman, 2009-08-18 00:59:36]

On 2009/08/17 20:24:52, Nebojša Ćirić wrote:
> :), I don't expect people to work on weekends, and this was sent Friday
> afternoon, so you are right on time.
> I've noticed that one has to restart Chrome to change locale/language. Since
> LoadExtension code already deals with JSON parsing, and loading files, I
> think we could load/parse message catalogs at that time too, not wait for
> load on demand. What do you think?

sgtm

> I left TODO(cira) next to FIleEnumerator to draw attention :). Erik left a
> comment with FileEnumerator that warns against its use in the main thread. I
> am not sure if LoadExtension is executed on the main or not.

All the functions in extension_file_util are only called on the file thread, so
you are good. ExtensionsServiceBackend::LoadExtension is also only called on the
file thread.

A convention in Chrome is that things called 'backend' are on the file thread,
but generally the class comments will tell you for sure what threads an object
deals with.

- a

[Nebojša Ćirić, 2009-08-18 17:13:41]

Ready for the next round.

I haven't implemented loading/parsing of message catalogs in this CL - it would
make it much bigger and harder to review.

Once we are done here, I'll start working on that.

[Aaron Boodman, 2009-08-18 23:47:12]

http://codereview.chromium.org/170015/diff/4001/5004
File chrome/browser/extensions/extension_file_util.cc (right):

http://codereview.chromium.org/170015/diff/4001/5004#newcode278
Line 278: if (!extension_l10n_util::SetDefaultLocale(manifest, extension.get()))
{
I think it would be a little more clear to phrase this as "validating" the
default locale setting, rather than "setting" it.

So, parse default_locale() in Extension just like everything else. Then here,
just check it exists. You'll end up with an invalid Extension, but that's OK, we
do this same pattern for many other parts of extensions.

http://codereview.chromium.org/170015/diff/4001/5004#newcode283
Line 283: 
Right here, we should iterate all the children of the root extension directory
and if any of them start with "_" and aren't "_locales", that should be an
error. We need to reserve the namespace of entries that start with "_" for
future use by Chrome.

http://codereview.chromium.org/170015/diff/4001/5002
File chrome/browser/extensions/extension_l10n_util.h (right):

http://codereview.chromium.org/170015/diff/4001/5002#newcode34
Line 34: // 1. Do nothing if _locale directory is missing (not an error).
Nit: _locales

http://codereview.chromium.org/170015/diff/4001/5002#newcode36
Line 36: // 3. Enumerate all subdirectories of _locale directory.
Same here.

[Nebojša Ćirić, 2009-08-19 02:28:24]

I think everything is covered now. Updated some tests (and added few lines to
extension_unittest).

On 2009/08/18 23:47:12, Aaron Boodman wrote:
> http://codereview.chromium.org/170015/diff/4001/5004
> File chrome/browser/extensions/extension_file_util.cc (right):
> 
> http://codereview.chromium.org/170015/diff/4001/5004#newcode278
> Line 278: if (!extension_l10n_util::SetDefaultLocale(manifest,
extension.get()))
> {
> I think it would be a little more clear to phrase this as "validating" the
> default locale setting, rather than "setting" it.
> 
> So, parse default_locale() in Extension just like everything else. Then here,
> just check it exists. You'll end up with an invalid Extension, but that's OK,
we
> do this same pattern for many other parts of extensions.

Done.
 
> http://codereview.chromium.org/170015/diff/4001/5004#newcode283
> Line 283: 
> Right here, we should iterate all the children of the root extension directory
> and if any of them start with "_" and aren't "_locales", that should be an
> error. We need to reserve the namespace of entries that start with "_" for
> future use by Chrome.

Done. I thought you were talking about _locales subitems only.
 
> http://codereview.chromium.org/170015/diff/4001/5002
> File chrome/browser/extensions/extension_l10n_util.h (right):
> 
> http://codereview.chromium.org/170015/diff/4001/5002#newcode34
> Line 34: // 1. Do nothing if _locale directory is missing (not an error).
> Nit: _locales

Done.

> http://codereview.chromium.org/170015/diff/4001/5002#newcode36
> Line 36: // 3. Enumerate all subdirectories of _locale directory.
> Same here.

Done.

[Nebojša Ćirić, 2009-08-19 02:29:49]

Btw. why are all try servers constantly chocking on this CL? Log tells me that
some chunks failed to upload, but that doesn't tell me much.

[Aaron Boodman, 2009-08-19 03:00:49]

LGTM with one more nit and a question.

http://codereview.chromium.org/170015/diff/5016/5020
File chrome/browser/extensions/extension_file_util.cc (right):

http://codereview.chromium.org/170015/diff/5016/5020#newcode389
Line 389: *error = StringPrintf("Illegal file/directory name %s", filename);
Can you make this error a bit more developer friendly, like:

Cannot load extension with file or directory name "/_blah.png". Filenames
starting with "_" are reserved for use by the system.

http://codereview.chromium.org/170015/diff/5016/5024
File chrome/common/extensions/extension.cc (right):

http://codereview.chromium.org/170015/diff/5016/5024#newcode71
Line 71: const char Extension::kMessagesFilename[] = "messages";
I asked this in a previous review, but it looks to me like presently, the file
heirarchy is:

/_locales
  /en_us
    messages

Is that right? Out of curiosity, I'm wondering what other files you might think
would go in each locale folder.