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

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 ExtensionMessageHandler 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.

BUG=12131

[Nebojša Ćirić, 2009-08-26 17:47:12]

Second part of the change.

This should cover infrastructure part of loading and parsing. Next CL will focus
on actually using the data...

[Nebojša Ćirić, 2009-08-28 18:32:44]

Btw. I have EDGE training Tue-Thu next week, so it would be good to finish this
review before that (so we don't have a break in between).

But no pressure :)

Cira

On 2009/08/26 17:47:12, Nebojša Ćirić wrote:
> Second part of the change.
> 
> This should cover infrastructure part of loading and parsing. Next CL will
focus
> on actually using the data...

[Nebojša Ćirić, 2009-08-31 18:49:25]

Ping (I can re-assign if you are busy).

On 2009/08/28 18:32:44, Nebojša Ćirić wrote:
> Btw. I have EDGE training Tue-Thu next week, so it would be good to finish
this
> review before that (so we don't have a break in between).
> 
> But no pressure :)
> 
> Cira
> 
> On 2009/08/26 17:47:12, Nebojša Ćirić wrote:
> > Second part of the change.
> > 
> > This should cover infrastructure part of loading and parsing. Next CL will
> focus
> > on actually using the data...

[Aaron Boodman, 2009-08-31 18:53:50]

I'm on it, sorry

[Aaron Boodman, 2009-08-31 21:42:55]

General comment: We spend a lot of time all over this project hand-validating
JSON.

It seems worthwhile to me to implement JSONSchema for C++. You could basically
just copy the JS implementation, here:

http://src.chromium.org/viewvc/chrome/trunk/src/chrome/renderer/resources/jso...

This would allow us to get rid of tons of manual error handling with probably
much more reliable code.

http://codereview.chromium.org/173487/diff/2001/3004
File chrome/browser/extensions/extension_file_util.cc (right):

http://codereview.chromium.org/173487/diff/2001/3004#newcode240
Line 240: static const std::string& app_locale =
l10n_util::GetApplicationLocale(L"");
This seems to check the prefs. Is it OK to do that on the file thread?

http://codereview.chromium.org/173487/diff/2001/3004#newcode248
Line 248: std::string value = extension->name();
Instead of reading and then re-writing the name, what about setting the message
handler before InitFromValue(). That way when Extension is init'ing itself, it
can just use the message handler to store the localized value from the
beginning.

http://codereview.chromium.org/173487/diff/2001/3001
File chrome/browser/extensions/extension_l10n_util.cc (right):

http://codereview.chromium.org/173487/diff/2001/3001#newcode102
Line 102: .AppendASCII(Extension::kMessagesFilename);
BTW: The message filenames should have the ".json" extension, so that they can
be associated with editors and so forth.

http://codereview.chromium.org/173487/diff/2001/3007
File chrome/browser/extensions/extension_message_handler.cc (right):

http://codereview.chromium.org/173487/diff/2001/3007#newcode24
Line 24: // Returns false and sets |error| to actual error message.
How about just returning the string since the return value is not used?

http://codereview.chromium.org/173487/diff/2001/3007#newcode31
Line 31: ExtensionMessageHandler::ExtensionMessageHandler() {
Nit: Since the constructor and destructors are not used, you don't need to
declare or define them -- the default ones are fine.

http://codereview.chromium.org/173487/diff/2001/3007#newcode37
Line 37: bool ExtensionMessageHandler::Init(const DictionaryValue*
default_catalog,
The naming of these parameters is a bit confusing to me as a newcomer to
localization.

What about: default_locale_catalog, current_locale_catalog?

You could use the same terminology in the comments for this method in the
header: "Merges the catalogs for the default locale of the extension and the
current locale of the browser into one dictionary..."

http://codereview.chromium.org/173487/diff/2001/3007#newcode49
Line 49: if (!GetMessageValue(*key_it, key, app_catalog, &value, error)) {
Nit: braces unnecessary here.

http://codereview.chromium.org/173487/diff/2001/3007#newcode52
Line 52: dictionary_.insert(make_pair(key, value));
Nit: I think the syntax dictionary[key] = value is more readable.

http://codereview.chromium.org/173487/diff/2001/3007#newcode77
Line 77: const DictionaryValue* main_catalog,
Nit: the "main_catalog" parameter seems misnamed. Just "catalog"?

http://codereview.chromium.org/173487/diff/2001/3007#newcode79
Line 79: std::string* error) const {
Add a DCHECK that the two keys are the same?

http://codereview.chromium.org/173487/diff/2001/3007#newcode169
Line 169: if (beg_index == message->npos) {
All these one line early exits can be braceless.

http://codereview.chromium.org/173487/diff/2001/3007#newcode189
Line 189: message->assign(var_name);
Did you mean to replace the entire input string with the variable name in this
case? That seems weird.

http://codereview.chromium.org/173487/diff/2001/3005
File chrome/browser/extensions/extension_message_handler.h (right):

http://codereview.chromium.org/173487/diff/2001/3005#newcode18
Line 18: class ExtensionMessageHandler {
This class seems mis-named. It isn't really "handling" anything. How about
ExtensionMessageBundle?

http://codereview.chromium.org/173487/diff/2001/3005#newcode20
Line 20: typedef base::hash_map<std::string, std::string> SSMap;
Avoid abbreviations. Can you come up with a descriptive, but still relatively
short name for this type? MessageMap? SubstitutionMap?

http://codereview.chromium.org/173487/diff/2001/3005#newcode42
Line 42: bool Init(const DictionaryValue* default_dictionary,
In cases where initialization can fail, we tend to prefer a private constructor
with a static Create() method that can return NULL. That makes it so that you
can never end up with an invalid instance.

For an example of this pattern, see RSAPrivateKey.

http://codereview.chromium.org/173487/diff/2001/3005#newcode48
Line 48: bool GetMessage(const std::string& name, std::string* message) const;
Nit: Can we just return std::string? It could be empty if the key doesn't exist.
If we ever find that we need to distinguish between empty string and
non-existent key, we could add a HasMessage() method.

http://codereview.chromium.org/173487/diff/2001/3005#newcode51
Line 51: size_t GetDictionarySize() const {
Is this just the number of messages? If so, maybe just size()? Or
num_messages()? The word "dictionary" seems out of place in the interface since
it isn't mentioned elsewhere.

http://codereview.chromium.org/173487/diff/2001/3005#newcode57
Line 57: bool ReplaceMessagesInString(std::string* text) const;
Nit: "InString" is unnecessary as that information is conveyed by the signature.

http://codereview.chromium.org/173487/diff/2001/3005#newcode67
Line 67: bool ReplaceVariablesInString(const SSMap& variables,
Looks like this should be static as it refers to no state in this class.

http://codereview.chromium.org/173487/diff/2001/3005#newcode68
Line 68: const std::string& var_begin,
Nit: maybe var_begin -> var_begin_delimiter? I thought this was a position from
its name alone.

http://codereview.chromium.org/173487/diff/2001/3005#newcode75
Line 75: bool IsValidName(const std::string& name) const;
Looks like this should be static.

http://codereview.chromium.org/173487/diff/2001/3005#newcode81
Line 81: bool GetMessageValue(const std::wstring& wkey,
It is really odd to have a method take two keys in different encodings. Looking
at the implementation I can see why you did that, but you should probably
mention in the comments that the two keys should be the same.

http://codereview.chromium.org/173487/diff/2001/3005#newcode83
Line 83: const DictionaryValue* main_catalog,
Chromium mainly uses const references for input params and non-const pointers
for output params.

http://codereview.chromium.org/173487/diff/2001/3008
File chrome/common/extensions/extension.h (right):

http://codereview.chromium.org/173487/diff/2001/3008#newcode269
Line 269: application_locale_ = application_locale;
Why do we need to store a duplicate copy of the application locale here? It
seems like we can get this from wherever it is stored when needed.

[Nebojša Ćirić, 2009-09-11 23:24:21]

All done. I will implement JSON validator at some other time (we talked about it
offline).

Since I got the new machine (and old one is gone) I can't easily append
changes/patches here, but I will link to the new CL from here - and to here from
the new CL) to preserve history).

New CL is http://codereview.chromium.org/202063/show.

On 2009/08/31 21:42:55, Aaron Boodman wrote:
> General comment: We spend a lot of time all over this project hand-validating
> JSON.
> 
> It seems worthwhile to me to implement JSONSchema for C++. You could basically
> just copy the JS implementation, here:
> 
>
http://src.chromium.org/viewvc/chrome/trunk/src/chrome/renderer/resources/jso...
> 
> This would allow us to get rid of tons of manual error handling with probably
> much more reliable code.
> 
> http://codereview.chromium.org/173487/diff/2001/3004
> File chrome/browser/extensions/extension_file_util.cc (right):
> 
> http://codereview.chromium.org/173487/diff/2001/3004#newcode240
> Line 240: static const std::string& app_locale =
> l10n_util::GetApplicationLocale(L"");
> This seems to check the prefs. Is it OK to do that on the file thread?
> 
> http://codereview.chromium.org/173487/diff/2001/3004#newcode248
> Line 248: std::string value = extension->name();
> Instead of reading and then re-writing the name, what about setting the
message
> handler before InitFromValue(). That way when Extension is init'ing itself, it
> can just use the message handler to store the localized value from the
> beginning.
> 
> http://codereview.chromium.org/173487/diff/2001/3001
> File chrome/browser/extensions/extension_l10n_util.cc (right):
> 
> http://codereview.chromium.org/173487/diff/2001/3001#newcode102
> Line 102: .AppendASCII(Extension::kMessagesFilename);
> BTW: The message filenames should have the ".json" extension, so that they can
> be associated with editors and so forth.
> 
> http://codereview.chromium.org/173487/diff/2001/3007
> File chrome/browser/extensions/extension_message_handler.cc (right):
> 
> http://codereview.chromium.org/173487/diff/2001/3007#newcode24
> Line 24: // Returns false and sets |error| to actual error message.
> How about just returning the string since the return value is not used?
> 
> http://codereview.chromium.org/173487/diff/2001/3007#newcode31
> Line 31: ExtensionMessageHandler::ExtensionMessageHandler() {
> Nit: Since the constructor and destructors are not used, you don't need to
> declare or define them -- the default ones are fine.
> 
> http://codereview.chromium.org/173487/diff/2001/3007#newcode37
> Line 37: bool ExtensionMessageHandler::Init(const DictionaryValue*
> default_catalog,
> The naming of these parameters is a bit confusing to me as a newcomer to
> localization.
> 
> What about: default_locale_catalog, current_locale_catalog?
> 
> You could use the same terminology in the comments for this method in the
> header: "Merges the catalogs for the default locale of the extension and the
> current locale of the browser into one dictionary..."
> 
> http://codereview.chromium.org/173487/diff/2001/3007#newcode49
> Line 49: if (!GetMessageValue(*key_it, key, app_catalog, &value, error)) {
> Nit: braces unnecessary here.
> 
> http://codereview.chromium.org/173487/diff/2001/3007#newcode52
> Line 52: dictionary_.insert(make_pair(key, value));
> Nit: I think the syntax dictionary[key] = value is more readable.
> 
> http://codereview.chromium.org/173487/diff/2001/3007#newcode77
> Line 77: const DictionaryValue* main_catalog,
> Nit: the "main_catalog" parameter seems misnamed. Just "catalog"?
> 
> http://codereview.chromium.org/173487/diff/2001/3007#newcode79
> Line 79: std::string* error) const {
> Add a DCHECK that the two keys are the same?
> 
> http://codereview.chromium.org/173487/diff/2001/3007#newcode169
> Line 169: if (beg_index == message->npos) {
> All these one line early exits can be braceless.
> 
> http://codereview.chromium.org/173487/diff/2001/3007#newcode189
> Line 189: message->assign(var_name);
> Did you mean to replace the entire input string with the variable name in this
> case? That seems weird.
> 
> http://codereview.chromium.org/173487/diff/2001/3005
> File chrome/browser/extensions/extension_message_handler.h (right):
> 
> http://codereview.chromium.org/173487/diff/2001/3005#newcode18
> Line 18: class ExtensionMessageHandler {
> This class seems mis-named. It isn't really "handling" anything. How about
> ExtensionMessageBundle?
> 
> http://codereview.chromium.org/173487/diff/2001/3005#newcode20
> Line 20: typedef base::hash_map<std::string, std::string> SSMap;
> Avoid abbreviations. Can you come up with a descriptive, but still relatively
> short name for this type? MessageMap? SubstitutionMap?
> 
> http://codereview.chromium.org/173487/diff/2001/3005#newcode42
> Line 42: bool Init(const DictionaryValue* default_dictionary,
> In cases where initialization can fail, we tend to prefer a private
constructor
> with a static Create() method that can return NULL. That makes it so that you
> can never end up with an invalid instance.
> 
> For an example of this pattern, see RSAPrivateKey.
> 
> http://codereview.chromium.org/173487/diff/2001/3005#newcode48
> Line 48: bool GetMessage(const std::string& name, std::string* message) const;
> Nit: Can we just return std::string? It could be empty if the key doesn't
exist.
> If we ever find that we need to distinguish between empty string and
> non-existent key, we could add a HasMessage() method.
> 
> http://codereview.chromium.org/173487/diff/2001/3005#newcode51
> Line 51: size_t GetDictionarySize() const {
> Is this just the number of messages? If so, maybe just size()? Or
> num_messages()? The word "dictionary" seems out of place in the interface
since
> it isn't mentioned elsewhere.
> 
> http://codereview.chromium.org/173487/diff/2001/3005#newcode57
> Line 57: bool ReplaceMessagesInString(std::string* text) const;
> Nit: "InString" is unnecessary as that information is conveyed by the
signature.
> 
> http://codereview.chromium.org/173487/diff/2001/3005#newcode67
> Line 67: bool ReplaceVariablesInString(const SSMap& variables,
> Looks like this should be static as it refers to no state in this class.
> 
> http://codereview.chromium.org/173487/diff/2001/3005#newcode68
> Line 68: const std::string& var_begin,
> Nit: maybe var_begin -> var_begin_delimiter? I thought this was a position
from
> its name alone.
> 
> http://codereview.chromium.org/173487/diff/2001/3005#newcode75
> Line 75: bool IsValidName(const std::string& name) const;
> Looks like this should be static.
> 
> http://codereview.chromium.org/173487/diff/2001/3005#newcode81
> Line 81: bool GetMessageValue(const std::wstring& wkey,
> It is really odd to have a method take two keys in different encodings.
Looking
> at the implementation I can see why you did that, but you should probably
> mention in the comments that the two keys should be the same.
> 
> http://codereview.chromium.org/173487/diff/2001/3005#newcode83
> Line 83: const DictionaryValue* main_catalog,
> Chromium mainly uses const references for input params and non-const pointers
> for output params.
> 
> http://codereview.chromium.org/173487/diff/2001/3008
> File chrome/common/extensions/extension.h (right):
> 
> http://codereview.chromium.org/173487/diff/2001/3008#newcode269
> Line 269: application_locale_ = application_locale;
> Why do we need to store a duplicate copy of the application locale here? It
> seems like we can get this from wherever it is stored when needed.