Implemented the rest of loading/parsing logic for extension i18n:...

CL is the same as http://codereview.chromium.org/173487, but had to be moved to new CL number because I switched machines.

Implemented the rest of loading/parsing logic for extension i18n:

1. Loading message catalogs for default and application locale.
2. Parsing JSON and replacing placeholders with actual content within a message.
3. Creating unified dictionary (union of default and application dictionaries,
where application dict. has priority for common messages).

New class ExtensionMessageBundle holds new dictionary, and parses data. It's
injected into Extension.

ExtensionMessageHandler::ReplaceVariablesInString can replace both
$placeholders$ and __MSG_messages__ in given string (HTML, manifest, actual
message string...).

Implemented actual manifest name/description replacement too, as an example.

Extension is now pretty agnostic about localization, and this makes it easier to use message bundles
with things that are not extensions...

BUG=12131

[Nebojša Ćirić, 2009-09-11 23:26:40]

Ready for another pass.

[Nebojša Ćirić, 2009-09-15 17:44:59]

Ping... I promise to send next one to somebody else :)

On 2009/09/11 23:26:40, Nebojša Ćirić wrote:
> Ready for another pass.

[Nebojša Ćirić, 2009-09-16 18:17:09]

Erik, Aaron, could you point somebody in the group who can do the review? I
would be glad to come over and give some background to it...

[Erik does not do reviews, 2009-09-16 20:57:43]

Sorry, I didn't notice that I was on the review list for this one.  It
looks like Aaron is looking at this one.  Feel free to send the next
one to me.

Erik


On Wed, Sep 16, 2009 at 11:17 AM,  <cira@chromium.org> wrote:
> Erik, Aaron, could you point somebody in the group who can do the review? I
> would be glad to come over and give some background to it...
>
> http://codereview.chromium.org/202063
>

[Aaron Boodman, 2009-09-16 22:37:39]

Sorry for slow review. I sux.

http://codereview.chromium.org/202063/diff/2031/2042
File chrome/browser/extensions/extension_file_util.cc (right):

http://codereview.chromium.org/202063/diff/2031/2042#newcode313
Line 313: if (file_util::PathExists(locale_path)) {
Reverse this check and return early.

http://codereview.chromium.org/202063/diff/2031/2044
File chrome/browser/extensions/extension_message_bundle.cc (right):

http://codereview.chromium.org/202063/diff/2031/2044#newcode34
Line 34: const DictionaryValue& current_locale_catalog,
I would change the signature to:

::Create(const DictionaryValue& catalog, const DictionaryValue& default_catalog,
std::string* error)

The fact that one of the locales is the "current" locale active in Chrome is
irrelevant to this class. It just cares about the two dictionaries it receives:
the one that is primary and the one that is the default.

Also, I don't think that default_locale and current_locale are needed per
previous comment.

http://codereview.chromium.org/202063/diff/2031/2044#newcode60
Line 60: DictionaryValue::key_iterator key_it =
current_locale_catalog.begin_keys();
Might want a quick one line comment here explaining that we are smooshing these
two dictionaries together.

http://codereview.chromium.org/202063/diff/2031/2044#newcode63
Line 63: if (!IsValidName(key))
If IsValidName() is only ever be used with wide chars, maybe changes its
signature to not require this conversion?

http://codereview.chromium.org/202063/diff/2031/2044#newcode76
Line 76: // Add only messages that are not provided by app_catalog.
General question: are names case-sensitive? Should they be?

http://codereview.chromium.org/202063/diff/2031/2044#newcode108
Line 108: if (!GetPlaceholders(*name_tree, key, &placeholders, error)) {
Braces not needed for this conditional.

http://codereview.chromium.org/202063/diff/2031/2044#newcode187
Line 187: while (true) {
Some blank lines in here to break up thoughts would help readability.

http://codereview.chromium.org/202063/diff/2031/2039
File chrome/browser/extensions/extension_message_bundle.h (right):

http://codereview.chromium.org/202063/diff/2031/2039#newcode19
Line 19: // Holds locale information (default locale, current locale, valid
locales).
Comments should be more about the what than the how. Something more like the
following would help new readers understand what this class does, I think:

Contains localized extension messages for one locale. Any messages that the
locale does not provide are pulled from the default locale.

http://codereview.chromium.org/202063/diff/2031/2039#newcode46
Line 46: // locale of the browser into one dictionary.
Again, comments should focus on what more than how. So in this case, maybe
something like:

Initializes the instance from the contents of two catalogs. If a key is not
present in current_locale_catalog, the value from default_local_catalog is used
instead.

Also, can this method be private?

http://codereview.chromium.org/202063/diff/2031/2039#newcode58
Line 58: // Get message with key |name|.
Maybe say something in the comment about any static placeholders being resolved?

http://codereview.chromium.org/202063/diff/2031/2039#newcode62
Line 62: // Dictionary size.
Comment nit: maybe: number of messages in the catalog? Also, out of curiosity,
why do external consumers of this class need this? Is it just for testing?

http://codereview.chromium.org/202063/diff/2031/2039#newcode66
Line 66: // dictionary_.
Again, little nit: the comments should avoid referring to implementation details
like |dictionary_|.

Replaces all __MSG_message__ with localized values from the catalog.

Won't comment on this anymore in the code review.

http://codereview.chromium.org/202063/diff/2031/2039#newcode91
Line 91: const std::string& current_locale() const { return current_locale_; }
Out of curiosity: are these values used outside this class? It seems odd that
they would be. And since this class is flattening the two dictionary on
creation, it seems like the class itself wouldn't use them either.

http://codereview.chromium.org/202063/diff/2031/2039#newcode128
Line 128: std::string default_locale_;
None of default_locale_, current_locale_, or supported_locales_ are used inside
this class.

Since this class is really about the messages for one locale (not necessarily
the default one), it makes sense to me that at least default_locale_ and
supported_locales_ should be moved to Extension.

As for current_locale_, it seems like this duplicates state that is already
available in other classes outside the extension system, so maybe we shouldn't
store it at all?

http://codereview.chromium.org/202063/diff/2031/2041
File chrome/browser/extensions/extension_ui_unittest.cc (left):

http://codereview.chromium.org/202063/diff/2031/2041#oldcode61
Line 61: class ExtensionUITest : public testing::Test {
Did you mean to remove this?

http://codereview.chromium.org/202063/diff/2031/2049
File chrome/chrome.gyp (right):

http://codereview.chromium.org/202063/diff/2031/2049#newcode852
Line 852: 'browser/chromeos/gview_request_interceptor.h',
Undo this change.

http://codereview.chromium.org/202063/diff/2031/2047
File chrome/common/extensions/extension.cc (right):

http://codereview.chromium.org/202063/diff/2031/2047#newcode618
Line 618: // Initialize & localize name.
It makes me uncomfortable be storing the localized versions of strings as data
in the program. I feel like in general, we want to localize at the last possible
moment.

I was talking to Erik about this and it seems like a nice way to do this would
be to have certain well-known keys. like chrome.extension-name. If your catalog
contains these keys, it overrides whatever is in the manifest.

That way we don't end up with tons of manifests IRL that look like:

"name": "__MSG_extension_name__".

WDYT?

[Nebojša Ćirić, 2009-09-17 16:33:46]

> http://codereview.chromium.org/202063/diff/2031/2047#newcode618
> Line 618: // Initialize & localize name.
> It makes me uncomfortable be storing the localized versions of strings as data
> in the program. I feel like in general, we want to localize at the last
possible
> moment.
> 
> I was talking to Erik about this and it seems like a nice way to do this would
> be to have certain well-known keys. like chrome.extension-name. If your
catalog
> contains these keys, it overrides whatever is in the manifest.
> 
> That way we don't end up with tons of manifests IRL that look like:
> 
> "name": "__MSG_extension_name__".
> 
> WDYT?

I feel they may forget to localize the name/description since it's a special
case (or not know how to do that). If somebody doesn't want to localize at all,
he won't have any messages defined, and his manifest is going to look "normal".

If you do feel strongly about this, what do you think about
__MSG_chrome_extension_name__, __MSG_chrome_extension_description__?

Code change is trivial (add kExtensionName="__MSG..." and then match on
extension InitFromValue).

[Erik does not do reviews, 2009-09-17 16:53:11]

On Thu, Sep 17, 2009 at 9:33 AM,  <cira@chromium.org> wrote:
>
>> http://codereview.chromium.org/202063/diff/2031/2047#newcode618
>> Line 618: // Initialize & localize name.
>> It makes me uncomfortable be storing the localized versions of strings as
>> data
>> in the program. I feel like in general, we want to localize at the last
>
> possible
>>
>> moment.
>
>> I was talking to Erik about this and it seems like a nice way to do this
>> would
>> be to have certain well-known keys. like chrome.extension-name. If your
>
> catalog
>>
>> contains these keys, it overrides whatever is in the manifest.
>
>> That way we don't end up with tons of manifests IRL that look like:
>
>> "name": "__MSG_extension_name__".
>
>> WDYT?
>
> I feel they may forget to localize the name/description since it's a special
> case (or not know how to do that). If somebody doesn't want to localize at
> all,
> he won't have any messages defined, and his manifest is going to look
> "normal".

I don't see how adding substitution into the manifest makes it any
less likely for them to forget.

In any event, we can handle this kind of error at package time.  If
they have localization data, we could require the name and description
keys be localized and that those keys not exist in the manifest.

Think about it like this instead: the default implementation is that
there are simply required name and description keys in the
localization data files, but we have a special case for developers
with no localization that allows them to add a name and description to
the manifest.


> If you do feel strongly about this, what do you think about
> __MSG_chrome_extension_name__, __MSG_chrome_extension_description__?
>
> Code change is trivial (add kExtensionName="__MSG..." and then match on
> extension InitFromValue).
>
> http://codereview.chromium.org/202063
>

[Nebojša Ćirić, 2009-09-17 20:54:17]

Uploading new version...

http://codereview.chromium.org/202063/diff/2031/2042
File chrome/browser/extensions/extension_file_util.cc (right):

http://codereview.chromium.org/202063/diff/2031/2042#newcode313
Line 313: if (file_util::PathExists(locale_path)) {
On 2009/09/16 22:37:40, Aaron Boodman wrote:
> Reverse this check and return early.

Done.

http://codereview.chromium.org/202063/diff/2031/2044
File chrome/browser/extensions/extension_message_bundle.cc (right):

http://codereview.chromium.org/202063/diff/2031/2044#newcode34
Line 34: const DictionaryValue& current_locale_catalog,
On 2009/09/16 22:37:40, Aaron Boodman wrote:
> I would change the signature to:
> 
> ::Create(const DictionaryValue& catalog, const DictionaryValue&
default_catalog,
> std::string* error)
> 
> The fact that one of the locales is the "current" locale active in Chrome is
> irrelevant to this class. It just cares about the two dictionaries it
receives:
> the one that is primary and the one that is the default.
> 
> Also, I don't think that default_locale and current_locale are needed per
> previous comment.

Done.

http://codereview.chromium.org/202063/diff/2031/2044#newcode60
Line 60: DictionaryValue::key_iterator key_it =
current_locale_catalog.begin_keys();
On 2009/09/16 22:37:40, Aaron Boodman wrote:
> Might want a quick one line comment here explaining that we are smooshing
these
> two dictionaries together.

Done.

http://codereview.chromium.org/202063/diff/2031/2044#newcode63
Line 63: if (!IsValidName(key))
It's actually used for std::strings, but I made it template just for kicks - so
it can work with wstrings too.

On 2009/09/16 22:37:40, Aaron Boodman wrote:
> If IsValidName() is only ever be used with wide chars, maybe changes its
> signature to not require this conversion?

http://codereview.chromium.org/202063/diff/2031/2044#newcode76
Line 76: // Add only messages that are not provided by app_catalog.
Added code to ignore caseness. So __MSG_name__ is equal to __MSG_NAmE__. You
still have to get __MSG_ part all upper case.

On 2009/09/16 22:37:40, Aaron Boodman wrote:
> General question: are names case-sensitive? Should they be?

http://codereview.chromium.org/202063/diff/2031/2044#newcode108
Line 108: if (!GetPlaceholders(*name_tree, key, &placeholders, error)) {
On 2009/09/16 22:37:40, Aaron Boodman wrote:
> Braces not needed for this conditional.

Done.

http://codereview.chromium.org/202063/diff/2031/2044#newcode187
Line 187: while (true) {
On 2009/09/16 22:37:40, Aaron Boodman wrote:
> Some blank lines in here to break up thoughts would help readability.

Done.

http://codereview.chromium.org/202063/diff/2031/2039
File chrome/browser/extensions/extension_message_bundle.h (right):

http://codereview.chromium.org/202063/diff/2031/2039#newcode19
Line 19: // Holds locale information (default locale, current locale, valid
locales).
On 2009/09/16 22:37:40, Aaron Boodman wrote:
> Comments should be more about the what than the how. Something more like the
> following would help new readers understand what this class does, I think:
> 
> Contains localized extension messages for one locale. Any messages that the
> locale does not provide are pulled from the default locale.

Done.

http://codereview.chromium.org/202063/diff/2031/2039#newcode46
Line 46: // locale of the browser into one dictionary.
On 2009/09/16 22:37:40, Aaron Boodman wrote:
> Again, comments should focus on what more than how. So in this case, maybe
> something like:
> 
> Initializes the instance from the contents of two catalogs. If a key is not
> present in current_locale_catalog, the value from default_local_catalog is
used
> instead.
> 
> Also, can this method be private?

Done.

http://codereview.chromium.org/202063/diff/2031/2039#newcode58
Line 58: // Get message with key |name|.
On 2009/09/16 22:37:40, Aaron Boodman wrote:
> Maybe say something in the comment about any static placeholders being
resolved?

Done.

http://codereview.chromium.org/202063/diff/2031/2039#newcode62
Line 62: // Dictionary size.
Done. Added comment about test only.

On 2009/09/16 22:37:40, Aaron Boodman wrote:
> Comment nit: maybe: number of messages in the catalog? Also, out of curiosity,
> why do external consumers of this class need this? Is it just for testing?

http://codereview.chromium.org/202063/diff/2031/2039#newcode66
Line 66: // dictionary_.
On 2009/09/16 22:37:40, Aaron Boodman wrote:
> Again, little nit: the comments should avoid referring to implementation
details
> like |dictionary_|.
> 
> Replaces all __MSG_message__ with localized values from the catalog.
> 
> Won't comment on this anymore in the code review.

Done.

http://codereview.chromium.org/202063/diff/2031/2039#newcode91
Line 91: const std::string& current_locale() const { return current_locale_; }
See below - removed all.

On 2009/09/16 22:37:40, Aaron Boodman wrote:
> Out of curiosity: are these values used outside this class? It seems odd that
> they would be. And since this class is flattening the two dictionary on
> creation, it seems like the class itself wouldn't use them either.

http://codereview.chromium.org/202063/diff/2031/2039#newcode128
Line 128: std::string default_locale_;
I actually think we don't need them at all. Once the catalogs are flattened we
can just get messages.

If for any reason we decide that default, current and supported locales are
needed, we can easily add them later on to extension.h 

As for current locale - the only reason to cache that info, is to avoid costly
operation of getting it.

On 2009/09/16 22:37:40, Aaron Boodman wrote:
> None of default_locale_, current_locale_, or supported_locales_ are used
inside
> this class.
> 
> Since this class is really about the messages for one locale (not necessarily
> the default one), it makes sense to me that at least default_locale_ and
> supported_locales_ should be moved to Extension.
> 
> As for current_locale_, it seems like this duplicates state that is already
> available in other classes outside the extension system, so maybe we shouldn't
> store it at all?

http://codereview.chromium.org/202063/diff/2031/2041
File chrome/browser/extensions/extension_ui_unittest.cc (left):

http://codereview.chromium.org/202063/diff/2031/2041#oldcode61
Line 61: class ExtensionUITest : public testing::Test {
I was fixing ui tests, and found out this file. Yes, I removed it since it's not
used. There are no TEST_F blocks in the code.

On 2009/09/16 22:37:40, Aaron Boodman wrote:
> Did you mean to remove this?

http://codereview.chromium.org/202063/diff/2031/2049
File chrome/chrome.gyp (right):

http://codereview.chromium.org/202063/diff/2031/2049#newcode852
Line 852: 'browser/chromeos/gview_request_interceptor.h',
On 2009/09/16 22:37:40, Aaron Boodman wrote:
> Undo this change.

Done.

http://codereview.chromium.org/202063/diff/2031/2047
File chrome/common/extensions/extension.cc (right):

http://codereview.chromium.org/202063/diff/2031/2047#newcode618
Line 618: // Initialize & localize name.
On 2009/09/16 22:37:40, Aaron Boodman wrote:
> It makes me uncomfortable be storing the localized versions of strings as data
> in the program. I feel like in general, we want to localize at the last
possible
> moment.
> 
> I was talking to Erik about this and it seems like a nice way to do this would
> be to have certain well-known keys. like chrome.extension-name. If your
catalog
> contains these keys, it overrides whatever is in the manifest.
> 
> That way we don't end up with tons of manifests IRL that look like:
> 
> "name": "__MSG_extension_name__".
> 
> WDYT?

Done.

[Aaron Boodman, 2009-09-17 21:34:17]

lgtm!

[Aaron Boodman, 2009-10-07 01:33:17]

If this issue is dead, can we close it please?