Implementing chrome.i18n.getMessage call, that loads message from the extensi...

Implementing chrome.i18n.getMessage call, that loads message from the extension catalog, and if necessary replaces placeholders (up to 9).

I have 3 forms of getMessage call:

getMessage("name") for simple messages without placeholders.
getMessage("name", "one param") for messages with only one placeholder.
getMessage("name", ["one", "two"]) for messages with only one or more placeholders.

getMessage returns string.

BUG=12131
TEST=Load samples/i18n extension (switch Chrome to sr locale) and observe ext. name, description and toolstrip texts should be in Serbian.

[jungshik at Google, 2009-09-23 18:50:47]

It'd be nice if an extension with a non-trivial use of these APIs can be added,
but ...

http://codereview.chromium.org/225009/diff/1/12
File chrome/common/extensions/docs/static/i18n.html (right):

http://codereview.chromium.org/225009/diff/1/12#newcode29
Line 29: arg2.
Shouldn't we give examples of all 3 forms? Also, for this form accepting an
array, it'd be better to comment that it's not just limited to 2
args/placeholders but it can accept up to 9 elements. 

Because the sample you added is too simple to show all the capability of this
API, it may be better to document how to use multiple placeholders (unless
you're planning to add another separate document).

[Nebojša Ćirić, 2009-09-23 19:44:46]

http://codereview.chromium.org/225009/diff/1/12
File chrome/common/extensions/docs/static/i18n.html (right):

http://codereview.chromium.org/225009/diff/1/12#newcode29
Line 29: arg2.
I think that API docs have a section that says 1 - 9 placeholders are allowed,
so that should be clear.

I am working on a continuous build extension that should use all kinds of
localization support (__MSG_name__ replacement, dynamic replacements...).

I can add all three forms in the example section if you feel that API docs are
not clear enough.

On 2009/09/23 18:50:48, Jungshik Shin wrote:
> Shouldn't we give examples of all 3 forms? Also, for this form accepting an
> array, it'd be better to comment that it's not just limited to 2
> args/placeholders but it can accept up to 9 elements. 
> 
> Because the sample you added is too simple to show all the capability of this
> API, it may be better to document how to use multiple placeholders (unless
> you're planning to add another separate document).

[Erik does not do reviews, 2009-09-24 01:08:34]

Apologies in advance, but I think this needs to be rethought a bit.

While almost all of our APIs are async and do a round-trip to the browser, I
think this one shouldn't be.  The data is static and doesn't depend on any
browser state.  We should just ship the messages for the appropriate languages
down into the extension process and then handle these functions synchronously. 
We have a few examples of sync functions in the API.  Much of chrome.extension.*
is like this.

http://codereview.chromium.org/225009/diff/1/13
File chrome/common/extensions/api/extension_api.json (right):

http://codereview.chromium.org/225009/diff/1/13#newcode1125
Line 1125: "description": "Message name from the extension catalog."},
nit: move }, to next line

http://codereview.chromium.org/225009/diff/1/13#newcode1128
Line 1128: { "type": "string"},
nit: space before } here

[Nebojša Ćirić, 2009-09-24 15:43:27]

No need to apologize, since that was how I wanted to do it in the first place,
but couldn't get api.json to comply with my wishes.

I've used "returns {type:string}" section, and it didn't seem to work (JS code
would keep complaining how getMessage returns undefined value, even thou result_
on C++ side got populated). And I kinda thought to be consistent with all of non
chrome.extensions.* API.

I'll ping you again when I have simple return working.

On 2009/09/24 01:08:34, Erik Kay wrote:
> Apologies in advance, but I think this needs to be rethought a bit.
> 
> While almost all of our APIs are async and do a round-trip to the browser, I
> think this one shouldn't be.  The data is static and doesn't depend on any
> browser state.  We should just ship the messages for the appropriate languages
> down into the extension process and then handle these functions synchronously.

> We have a few examples of sync functions in the API.  Much of
chrome.extension.*
> is like this.
> 
> http://codereview.chromium.org/225009/diff/1/13
> File chrome/common/extensions/api/extension_api.json (right):
> 
> http://codereview.chromium.org/225009/diff/1/13#newcode1125
> Line 1125: "description": "Message name from the extension catalog."},
> nit: move }, to next line
> 
> http://codereview.chromium.org/225009/diff/1/13#newcode1128
> Line 1128: { "type": "string"},
> nit: space before } here

[Nebojša Ćirić, 2009-09-24 18:29:35]

Before I do anything let me just make sure I have correct idea:

1. Extension object (and message bundles) lives in browser process
2. I would have to send message bundle data to renderer process (same way as
page actions do) in order to implement "sting getMessage()" vs.
"getMessage(callback())".

MessageBundles could potentially be large, so we cannot afford copying them
across. Jungshik mentioned I could put them in shared memory...

Does this sound reasonable, or too complex?

On 2009/09/24 01:08:34, Erik Kay wrote:
> Apologies in advance, but I think this needs to be rethought a bit.
> 
> While almost all of our APIs are async and do a round-trip to the browser, I
> think this one shouldn't be.  The data is static and doesn't depend on any
> browser state.  We should just ship the messages for the appropriate languages
> down into the extension process and then handle these functions synchronously.

> We have a few examples of sync functions in the API.  Much of
chrome.extension.*
> is like this.
> 
> http://codereview.chromium.org/225009/diff/1/13
> File chrome/common/extensions/api/extension_api.json (right):
> 
> http://codereview.chromium.org/225009/diff/1/13#newcode1125
> Line 1125: "description": "Message name from the extension catalog."},
> nit: move }, to next line
> 
> http://codereview.chromium.org/225009/diff/1/13#newcode1128
> Line 1128: { "type": "string"},
> nit: space before } here

[Erik does not do reviews, 2009-09-24 20:27:33]

On Thu, Sep 24, 2009 at 11:29 AM,  <cira@chromium.org> wrote:
> Before I do anything let me just make sure I have correct idea:
>
> 1. Extension object (and message bundles) lives in browser process
> 2. I would have to send message bundle data to renderer process (same way=
 as
> page actions do) in order to implement "sting getMessage()" vs.
> "getMessage(callback())".

This sounds right.


> MessageBundles could potentially be large, so we cannot afford copying th=
em
> across. Jungshik mentioned I could put them in shared memory...

How big do we have to worry about?  A few hundred kB?  We pass some
pretty large things across IPC already, so I wouldn't say that it's a
given that these will be too big.  I don't think shared memory is the
way to go here since it doesn't actually need to be in used by
multiple processes.  This is just a one-time push.  If the files
really do turn out to be too large, then we could just pass a file
reference across.  The renderer can read files if we give it explicit
permission to do so.  However, I'd start with the assumption that
they'll go across IPC directly just fine.  We can switch to a file
implementation later if need be.

Erik


> Does this sound reasonable, or too complex?
>
> On 2009/09/24 01:08:34, Erik Kay wrote:
>>
>> Apologies in advance, but I think this needs to be rethought a bit.
>
>> While almost all of our APIs are async and do a round-trip to the browse=
r,
>> I
>> think this one shouldn't be. =A0The data is static and doesn't depend on=
 any
>> browser state. =A0We should just ship the messages for the appropriate
>> languages
>> down into the extension process and then handle these functions
>> synchronously.
>
>> We have a few examples of sync functions in the API. =A0Much of
>
> chrome.extension.*
>>
>> is like this.
>
>> http://codereview.chromium.org/225009/diff/1/13
>> File chrome/common/extensions/api/extension_api.json (right):
>
>> http://codereview.chromium.org/225009/diff/1/13#newcode1125
>> Line 1125: "description": "Message name from the extension catalog."},
>> nit: move }, to next line
>
>> http://codereview.chromium.org/225009/diff/1/13#newcode1128
>> Line 1128: { "type": "string"},
>> nit: space before } here
>
>
>
> http://codereview.chromium.org/225009
>

[Nebojša Ćirić, 2009-09-25 21:22:27]

Ready for next pass.

Testing is somewhat harder to get right with JS methods that don't use
callbacks.

I was not aware of deps check between /chrome/browser and /chrome/renderer so I
had to duplicate small piece of code to look up messages from dictionary. What
do you think about moving whole i18n extension logic from chrome/browser to
chrome/common? If you don't see problems with that, I would do it in the next
CL.

http://codereview.chromium.org/225009/diff/9001/9017
File chrome/renderer/extensions/extension_process_bindings.cc (right):

http://codereview.chromium.org/225009/diff/9001/9017#newcode298
Line 298: return "";
indent for the whole function already fixed.

http://codereview.chromium.org/225009/diff/9001/9007
File chrome/test/data/extensions/api_test/i18n/test.js (right):

http://codereview.chromium.org/225009/diff/9001/9007#newcode25
Line 25: // Testing framework expect some kind of notification, and since we
don't
expect->expects, done.

[Erik does not do reviews, 2009-09-25 23:16:06]

http://codereview.chromium.org/225009/diff/9001/9014
File chrome/browser/extensions/extension_process_manager.cc (right):

http://codereview.chromium.org/225009/diff/9001/9014#newcode118
Line 118: // Send l10n messages to the renderer.
nit: newline above comment

http://codereview.chromium.org/225009/diff/9001/9007
File chrome/test/data/extensions/api_test/i18n/test.js (right):

http://codereview.chromium.org/225009/diff/9001/9007#newcode27
Line 27: chrome.test.notifyPass();
this should be chrome.test.succeed(); (sorry, docs are weak for the non-callback
cases)

http://codereview.chromium.org/225009/diff/14002/14018
File chrome/renderer/extensions/extension_process_bindings.cc (right):

http://codereview.chromium.org/225009/diff/14002/14018#newcode288
Line 288: // TODO(cira): move ExtensionMessageBundle and utils to
I'd prefer it if you moved it as part of this CL

http://codereview.chromium.org/225009/diff/14002/14020
File chrome/renderer/render_thread.h (right):

http://codereview.chromium.org/225009/diff/14002/14020#newcode144
Line 144: const std::vector<std::string>& permissions);
not you I know, but could you fix up these two 80 column glitches?

[Nebojša Ćirić, 2009-09-26 00:12:09]

All done.

On 2009/09/25 23:16:06, Erik Kay wrote:
> http://codereview.chromium.org/225009/diff/9001/9014
> File chrome/browser/extensions/extension_process_manager.cc (right):
> 
> http://codereview.chromium.org/225009/diff/9001/9014#newcode118
> Line 118: // Send l10n messages to the renderer.
> nit: newline above comment
> 
> http://codereview.chromium.org/225009/diff/9001/9007
> File chrome/test/data/extensions/api_test/i18n/test.js (right):
> 
> http://codereview.chromium.org/225009/diff/9001/9007#newcode27
> Line 27: chrome.test.notifyPass();
> this should be chrome.test.succeed(); (sorry, docs are weak for the
non-callback
> cases)
> 
> http://codereview.chromium.org/225009/diff/14002/14018
> File chrome/renderer/extensions/extension_process_bindings.cc (right):
> 
> http://codereview.chromium.org/225009/diff/14002/14018#newcode288
> Line 288: // TODO(cira): move ExtensionMessageBundle and utils to
> I'd prefer it if you moved it as part of this CL
> 
> http://codereview.chromium.org/225009/diff/14002/14020
> File chrome/renderer/render_thread.h (right):
> 
> http://codereview.chromium.org/225009/diff/14002/14020#newcode144
> Line 144: const std::vector<std::string>& permissions);
> not you I know, but could you fix up these two 80 column glitches?

[Erik does not do reviews, 2009-09-26 00:35:48]

LGTM with these changes

http://codereview.chromium.org/225009/diff/15009/15025
File chrome/renderer/extensions/extension_process_bindings.cc (right):

http://codereview.chromium.org/225009/diff/15009/15025#newcode289
Line 289: if (args.Length() != 2 || !args[0]->IsString())
NOTREACHED()

http://codereview.chromium.org/225009/diff/15009/15025#newcode290
Line 290: return v8::String::New("");
return V8::Undefined instead (and the other places you do this below)

http://codereview.chromium.org/225009/diff/15009/15025#newcode312
Line 312: v8::Array* placeholders = static_cast<v8::Array*>(*args[1]);
verify that the length of placeholders is > 0 and <= 9 and add a DCHECK

http://codereview.chromium.org/225009/diff/15009/15025#newcode319
Line 319: } else {
NOTREACHED()

[Nebojša Ćirić, 2009-09-26 18:56:09]

>
> http://codereview.chromium.org/225009/diff/15009/15025#newcode312
> Line 312: v8::Array* placeholders = static_cast<v8::Array*>(*args[1]);
> verify that the length of placeholders is > 0 and <= 9 and add a DCHECK
>
> extension_api.json already checks for min 1 and max 9 for this parameter.
 Do you still want me to add this check?

Cira

[Erik does not do reviews, 2009-09-26 19:26:50]

On Sat, Sep 26, 2009 at 11:55 AM, Neboj=C5=A1a =C4=86iri=C4=87 <cira@chromi=
um.org> wrote:
>> http://codereview.chromium.org/225009/diff/15009/15025#newcode312
>> Line 312: v8::Array* placeholders =3D static_cast<v8::Array*>(*args[1]);
>> verify that the length of placeholders is > 0 and <=3D 9 and add a DCHEC=
K
>>
> extension_api.json already checks for min 1 and max 9 for this parameter.
> =C2=A0Do you still want me to add this check?
> Cira

Yes.  Defense in depth is a good thing, particularly when we're
dealing with untrusted code.

Erik