chrome.i18n API (accept languages part 1)

Added chrome.i18n extension API.
The API proposal doc is 
http://sites.google.com/a/chromium.org/dev/developers/design-documents/extensions/i18n-api

This CL only adds the getter of accept languages: 
chrome.i18n.getAcceptLanguages().

The setter of accept languages will be added in a separate CL.

BUG=http://crbug.com/14574
TEST=TEST=There is an extension in chrome\test\data\extensions\samples\i18n.
Load this extension. It creates a toolstrip button.  Click this button, you
should get chrome's accept languages.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=25810

[xji, 2009-08-26 19:15:04]

Just a reminder ....

[Erik does not do reviews, 2009-08-28 16:23:51]

Sorry for the delay.  I'll get to this now.

Erik


On Wed, Aug 26, 2009 at 12:15 PM, <xji@chromium.org> wrote:
> Just a reminder ....
>
> http://codereview.chromium.org/174116
>

[Erik does not do reviews, 2009-08-28 16:33:58]

I realize that this is giving you conflicting direction to what Nick suggested
on your API proposal thread, but I still stand by my original recommendation to
not have a "chrome.preferences" at all, and to instead use domain-specific
packages like chrome.i18n.  Don't go off and change that just yet though.  I'll
sync up with Nick today and we'll get on the same page.

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

http://codereview.chromium.org/174116/diff/1/7#newcode13
Line 13: #include "chrome/browser/extensions/extension_preferences_module.h"
we're switching these to be "api" instead of "module"

http://codereview.chromium.org/174116/diff/1/5
File chrome/browser/extensions/extension_preferences_module.cc (right):

http://codereview.chromium.org/174116/diff/1/5#newcode7
Line 7: #include "chrome/browser/extensions/extension_function_dispatcher.h"
are all of these #includes necessary?  It looks like only about half of them are
used.

http://codereview.chromium.org/174116/diff/1/4
File chrome/browser/extensions/extension_preferences_module_constants.h (right):

http://codereview.chromium.org/174116/diff/1/4#newcode13
Line 13: extern const char kGetAcceptLanguagesFunction[];
There's no need to make this a separate header / cc file.  just merge it with
the main one.  They're both very small.  See extension_toolstrip_api.h for an
example.

http://codereview.chromium.org/174116/diff/1/2
File chrome/test/data/extensions/samples/preferences/toolstrip.html (right):

http://codereview.chromium.org/174116/diff/1/2#newcode12
Line 12: document.getElementById("languageDiv").innerHTML = language;
don't you want to replace the contents of the <span> instead?

[xji, 2009-09-01 19:41:34]

Hi Erik,

Thanks for the review!
I've updated the CL per your comments and changed the package to be chrome.i18n.

Please review it again.

Thanks,
Xiaomei

On 2009/08/28 16:33:58, Erik Kay wrote:
> I realize that this is giving you conflicting direction to what Nick suggested
> on your API proposal thread, but I still stand by my original recommendation
to
> not have a "chrome.preferences" at all, and to instead use domain-specific
> packages like chrome.i18n.  Don't go off and change that just yet though. 
I'll
> sync up with Nick today and we'll get on the same page.
> 
> http://codereview.chromium.org/174116/diff/1/7
> File chrome/browser/extensions/extension_function_dispatcher.cc (right):
> 
> http://codereview.chromium.org/174116/diff/1/7#newcode13
> Line 13: #include "chrome/browser/extensions/extension_preferences_module.h"
> we're switching these to be "api" instead of "module"
> 
> http://codereview.chromium.org/174116/diff/1/5
> File chrome/browser/extensions/extension_preferences_module.cc (right):
> 
> http://codereview.chromium.org/174116/diff/1/5#newcode7
> Line 7: #include "chrome/browser/extensions/extension_function_dispatcher.h"
> are all of these #includes necessary?  It looks like only about half of them
are
> used.
> 
> http://codereview.chromium.org/174116/diff/1/4
> File chrome/browser/extensions/extension_preferences_module_constants.h
(right):
> 
> http://codereview.chromium.org/174116/diff/1/4#newcode13
> Line 13: extern const char kGetAcceptLanguagesFunction[];
> There's no need to make this a separate header / cc file.  just merge it with
> the main one.  They're both very small.  See extension_toolstrip_api.h for an
> example.
> 
> http://codereview.chromium.org/174116/diff/1/2
> File chrome/test/data/extensions/samples/preferences/toolstrip.html (right):
> 
> http://codereview.chromium.org/174116/diff/1/2#newcode12
> Line 12: document.getElementById("languageDiv").innerHTML = language;
> don't you want to replace the contents of the <span> instead?

[Erik does not do reviews, 2009-09-03 16:42:11]

Please also add a browser_tests API test.  See extension_bookmarks_apitest.cc
and test/data/extensions/api_test/bookmarks for a sample.

http://codereview.chromium.org/174116/diff/5001/5004
File chrome/common/extensions/api/extension_api.json (right):

http://codereview.chromium.org/174116/diff/5001/5004#newcode876
Line 876: {"type": "string", "name": "language", "description": "the accept
languages string of the browser, such as en-US,en,af"}
We should just change the callback to provide an array.  It seems that's what
the caller would need to do anyway.

[xji, 2009-09-03 19:10:36]

On 2009/09/03 16:42:11, Erik Kay wrote:
> Please also add a browser_tests API test.  See extension_bookmarks_apitest.cc
> and test/data/extensions/api_test/bookmarks for a sample.
> 
> http://codereview.chromium.org/174116/diff/5001/5004
> File chrome/common/extensions/api/extension_api.json (right):
> 
> http://codereview.chromium.org/174116/diff/5001/5004#newcode876
> Line 876: {"type": "string", "name": "language", "description": "the accept
> languages string of the browser, such as en-US,en,af"}
> We should just change the callback to provide an array.  It seems that's what
> the caller would need to do anyway.

The caller might just need the string. Given the dictionary/translation
extensions as examples, the accept-language will be used as the target
languages, which will be passed as a string to  AJAX api of
translation/dictionary service.

If needed, the caller can pass the string itself.

Or it is more structural to return an array.

[Erik does not do reviews, 2009-09-03 19:40:09]

On Thu, Sep 3, 2009 at 12:10 PM, <xji@chromium.org> wrote:
> On 2009/09/03 16:42:11, Erik Kay wrote:
>>
>> Please also add a browser_tests API test. =A0See
>
> extension_bookmarks_apitest.cc
>>
>> and test/data/extensions/api_test/bookmarks for a sample.
>
>> http://codereview.chromium.org/174116/diff/5001/5004
>> File chrome/common/extensions/api/extension_api.json (right):
>
>> http://codereview.chromium.org/174116/diff/5001/5004#newcode876
>> Line 876: {"type": "string", "name": "language", "description": "the
>
> accept
>>
>> languages string of the browser, such as en-US,en,af"}
>> We should just change the callback to provide an array. =A0It seems
>
> that's what
>>
>> the caller would need to do anyway.
>
> The caller might just need the string. Given the dictionary/translation
> extensions as examples, the accept-language will be used as the target
> languages, which will be passed as a string to =A0AJAX api of
> translation/dictionary service.
>
> If needed, the caller can pass the string itself.
>
> Or it is more structural to return an array.

I see your point.  You could need either value depending on the
circumstances.  However, the fact that the API uses something in the
same format as the string might not be guaranteed, so in that case,
you'd have to transform it from one string format to another.  Also,
in general, it's safer to construct the string from an array than
vice-versa, so I'd go with the array.

Erik

[xji, 2009-09-04 21:20:13]

Updated the patch with 
1. change the return type of chrome.i18n.getAcceptLanguages() from string to
array of string.
2. add browser_test API test.

Please review again.

Thanks,
Xiaomei


On 2009/09/03 19:40:09, Erik Kay wrote:
> On Thu, Sep 3, 2009 at 12:10 PM, <mailto:xji@chromium.org> wrote:
> > On 2009/09/03 16:42:11, Erik Kay wrote:
> >>
> >> Please also add a browser_tests API test. =A0See
> >
> > extension_bookmarks_apitest.cc
> >>
> >> and test/data/extensions/api_test/bookmarks for a sample.
> >
> >> http://codereview.chromium.org/174116/diff/5001/5004
> >> File chrome/common/extensions/api/extension_api.json (right):
> >
> >> http://codereview.chromium.org/174116/diff/5001/5004#newcode876
> >> Line 876: {"type": "string", "name": "language", "description": "the
> >
> > accept
> >>
> >> languages string of the browser, such as en-US,en,af"}
> >> We should just change the callback to provide an array. =A0It seems
> >
> > that's what
> >>
> >> the caller would need to do anyway.
> >
> > The caller might just need the string. Given the dictionary/translation
> > extensions as examples, the accept-language will be used as the target
> > languages, which will be passed as a string to =A0AJAX api of
> > translation/dictionary service.
> >
> > If needed, the caller can pass the string itself.
> >
> > Or it is more structural to return an array.
> 
> I see your point.  You could need either value depending on the
> circumstances.  However, the fact that the API uses something in the
> same format as the string might not be guaranteed, so in that case,
> you'd have to transform it from one string format to another.  Also,
> in general, it's safer to construct the string from an array than
> vice-versa, so I'd go with the array.
> 
> Erik

[Erik does not do reviews, 2009-09-04 21:50:50]

I'm not sure how defensive we need to be about the formatting of this string. 
Do we need to worry about trimming whitespace around the commas?

http://codereview.chromium.org/174116/diff/10001/10010
File chrome/browser/extensions/extension_i18n_api.cc (right):

http://codereview.chromium.org/174116/diff/10001/10010#newcode16
Line 16: const char kEmptyAcceptLanguagesError[] = "The accept-languages is
empty.";
language nit: either remove "The" at the front or add "preference" or "header"
after "accept-languages".

http://codereview.chromium.org/174116/diff/10001/10010#newcode34
Line 34: acceptLang = acceptLanguages.substr(begin, end - begin);
add a check to verify that end > begin (a malformed value with multiple, in a
row)

http://codereview.chromium.org/174116/diff/10001/10010#newcode39
Line 39: acceptLang = acceptLanguages.substr(begin, end);
need to verify that begin < length (a malformed value with a trailing ,)

http://codereview.chromium.org/174116/diff/10001/10010#newcode42
Line 42: return true;
add another check here for the list being empty

http://codereview.chromium.org/174116/diff/9002/10035
File chrome/test/data/extensions/api_test/i18n/test.js (right):

http://codereview.chromium.org/174116/diff/9002/10035#newcode14
Line 14: 
remove extra newline

[xji, 2009-09-08 17:47:04]

Updated per review feedback.

On 2009/09/04 21:50:50, Erik Kay wrote:
> I'm not sure how defensive we need to be about the formatting of this string. 
> Do we need to worry about trimming whitespace around the commas?
> 
> http://codereview.chromium.org/174116/diff/10001/10010
> File chrome/browser/extensions/extension_i18n_api.cc (right):
> 
> http://codereview.chromium.org/174116/diff/10001/10010#newcode16
> Line 16: const char kEmptyAcceptLanguagesError[] = "The accept-languages is
> empty.";
> language nit: either remove "The" at the front or add "preference" or "header"
> after "accept-languages".
> 
> http://codereview.chromium.org/174116/diff/10001/10010#newcode34
> Line 34: acceptLang = acceptLanguages.substr(begin, end - begin);
> add a check to verify that end > begin (a malformed value with multiple, in a
> row)
> 
> http://codereview.chromium.org/174116/diff/10001/10010#newcode39
> Line 39: acceptLang = acceptLanguages.substr(begin, end);
> need to verify that begin < length (a malformed value with a trailing ,)
> 
> http://codereview.chromium.org/174116/diff/10001/10010#newcode42
> Line 42: return true;
> add another check here for the list being empty
> 
> http://codereview.chromium.org/174116/diff/9002/10035
> File chrome/test/data/extensions/api_test/i18n/test.js (right):
> 
> http://codereview.chromium.org/174116/diff/9002/10035#newcode14
> Line 14: 
> remove extra newline

[Erik does not do reviews, 2009-09-08 21:56:57]

http://codereview.chromium.org/174116/diff/12004/13011
File chrome/browser/extensions/extension_i18n_api.cc (right):

http://codereview.chromium.org/174116/diff/12004/13011#newcode28
Line 28: // spaces. But we did not do any validation (either the format or the
I don't think we need to support users editing their preferences file by hand
(incorrectly), we just need to not crash if they do this.  I'd change the
comment to say that we're adding extra checks to be resistant to crashes caused
by data corruption, etc.

http://codereview.chromium.org/174116/diff/12004/13011#newcode45
Line 45: if (end == std::wstring::npos)
move this to after the if so that we can reuse that code rather than duplicating
it outside of the loop (some of the conditional will need to change as well)

http://codereview.chromium.org/174116/diff/12004/13011#newcode48
Line 48: // Guard against a malformed value with multiple "," in a row.
I don't think you need to trim whitespace.  We don't need to support users
editing the files by themselves.  We just need to not crash with bad data. 
Ideally, we'd avoid passing bad strings on through the API.

http://codereview.chromium.org/174116/diff/12004/13011#newcode61
Line 61: if (begin < acceptLanguages.length()) {
hopefully you can delete this duplication altogether

[xji, 2009-09-09 18:10:14]

Updated per feedback.
Thanks for review again!


On 2009/09/08 21:56:57, Erik Kay wrote:
> http://codereview.chromium.org/174116/diff/12004/13011
> File chrome/browser/extensions/extension_i18n_api.cc (right):
> 
> http://codereview.chromium.org/174116/diff/12004/13011#newcode28
> Line 28: // spaces. But we did not do any validation (either the format or the
> I don't think we need to support users editing their preferences file by hand
> (incorrectly), we just need to not crash if they do this.  I'd change the
> comment to say that we're adding extra checks to be resistant to crashes
caused
> by data corruption, etc.
> 
> http://codereview.chromium.org/174116/diff/12004/13011#newcode45
> Line 45: if (end == std::wstring::npos)
> move this to after the if so that we can reuse that code rather than
duplicating
> it outside of the loop (some of the conditional will need to change as well)
> 
> http://codereview.chromium.org/174116/diff/12004/13011#newcode48
> Line 48: // Guard against a malformed value with multiple "," in a row.
> I don't think you need to trim whitespace.  We don't need to support users
> editing the files by themselves.  We just need to not crash with bad data. 
> Ideally, we'd avoid passing bad strings on through the API.
> 
> http://codereview.chromium.org/174116/diff/12004/13011#newcode61
> Line 61: if (begin < acceptLanguages.length()) {
> hopefully you can delete this duplication altogether

[Erik does not do reviews, 2009-09-09 18:24:45]

LGTM

http://codereview.chromium.org/174116/diff/7006/7015
File chrome/browser/extensions/extension_i18n_api.cc (right):

http://codereview.chromium.org/174116/diff/7006/7015#newcode50
Line 50: // ending with ','.
I don't think this is true.  It guards against a single char value of any kind
right?  I assume that single char language values aren't legal, so this still
seems like a reasonable check, just not an accurate comment.

http://codereview.chromium.org/174116/diff/7006/7018
File chrome/test/data/extensions/api_test/i18n/test.js (right):

http://codereview.chromium.org/174116/diff/7006/7018#newcode8
Line 8: chrome.i18n.getAcceptLanguages(testCallback(true, function(results) {
change this to use chrome.test.callbackPass(function...
(things change quickly ;-)

[xji, 2009-09-09 18:43:57]

http://codereview.chromium.org/174116/diff/7006/7015
File chrome/browser/extensions/extension_i18n_api.cc (right):

http://codereview.chromium.org/174116/diff/7006/7015#newcode50
Line 50: // ending with ','.
On 2009/09/09 18:24:45, Erik Kay wrote:
> I don't think this is true.  It guards against a single char value of any kind
> right?  I assume that single char language values aren't legal, so this still
> seems like a reasonable check, just not an accurate comment.

It guards against ',' at the end of the string.

Given a string with a single char value "a", its begin=0, end=npos, substr="a",
and it exits the loop due to "end=npos". No possibility to crash.

Given a string ending with ',' such as "en,",
its begin=0, end=2, substr="en", then, begin=3,
we should exit the loop by "begin >= 3".

[Erik does not do reviews, 2009-09-09 18:46:39]

http://codereview.chromium.org/174116/diff/7006/7015
File chrome/browser/extensions/extension_i18n_api.cc (right):

http://codereview.chromium.org/174116/diff/7006/7015#newcode50
Line 50: // ending with ','.
On 2009/09/09 18:43:57, xji wrote:
> On 2009/09/09 18:24:45, Erik Kay wrote:
> > I don't think this is true.  It guards against a single char value of any
kind
> > right?  I assume that single char language values aren't legal, so this
still
> > seems like a reasonable check, just not an accurate comment.
> 
> It guards against ',' at the end of the string.
> 
> Given a string with a single char value "a", its begin=0, end=npos,
substr="a",
> and it exits the loop due to "end=npos". No possibility to crash.
> 
> Given a string ending with ',' such as "en,",
> its begin=0, end=2, substr="en", then, begin=3,
> we should exit the loop by "begin >= 3".

yep.  you're right.  my math was off.