Add utility function to determine if a locale is valid syntax; this will...

Add utility function to determine if a locale is valid syntax; this will
be used by the TTS extension API. Moved some locale utility functions from
extension_l10n_util to l10n_util.

BUG=none
TEST=Adds new unit test


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

[Nebojša Ćirić, 2010-12-10 17:19:13]

Looping Jungshik in, to see if it makes sense to do this.

http://codereview.chromium.org/5643002/diff/1/chrome/common/extensions/extens...
File chrome/common/extensions/extension_l10n_util.cc (right):

http://codereview.chromium.org/5643002/diff/1/chrome/common/extensions/extens...
chrome/common/extensions/extension_l10n_util.cc:252: // Check that all
characters are alphanumeric, hyphen, or underscore.
What about @ and = signs in de-DE@collation=phonebook? Locale validation is
tough, given so many standards in use (BCP 47, TR#35...).

http://codereview.chromium.org/5643002/diff/1/chrome/common/extensions/extens...
chrome/common/extensions/extension_l10n_util.cc:286: }
I think app/l10n_util would be a better place for this function (and
GetParentLocales) - since they are not related to extension implementation
details.

[jungshik at Google, 2010-12-10 18:32:16]

http://codereview.chromium.org/5643002/diff/1/chrome/common/extensions/extens...
File chrome/common/extensions/extension_l10n_util.cc (right):

http://codereview.chromium.org/5643002/diff/1/chrome/common/extensions/extens...
chrome/common/extensions/extension_l10n_util.cc:245: // This implements a simple
approximation of RFC 5646; it will accept all
nit: BCP 47 (Best Current Practice 47 :
http://www.rfc-editor.org/rfc/bcp/bcp47.txt ) is a more widely used reference
(and easier to remember :-)). A bit more importantly, more 'stable' name than
RFC 5646, I guess.

http://codereview.chromium.org/5643002/diff/1/chrome/common/extensions/extens...
chrome/common/extensions/extension_l10n_util.cc:255: if (!isalnum(ch) && ch !=
'-' && ch != '_')
isalnum is locale-dependent (C-library locale) and shouldn't be used.
string_util may have something already. If not, you have to add one to check for
a-z, A-Z and 0-9.

As for "@key=value" syntax,  it's obsolete according to UTR 35
(http://www.unicode.org/reports/tr35/#Unicode_Language_and_Locale_Identifiers :
section 3.4). For the immediate need of ours, I guess we don't need to be
backward compatible with old syntax.

http://codereview.chromium.org/5643002/diff/1/chrome/common/extensions/extens...
chrome/common/extensions/extension_l10n_util.cc:268: if (token_index == 0 &&
(token_len < 1 || token_len > 3)) {
can you tighten up a bit here? although BCP 47 reserves a single digit tag for
extension (section 3.7), I don't think any has been registered/used. So, the
first tag can be assumed to be in [a-zA-Z]

http://codereview.chromium.org/5643002/diff/1/chrome/common/extensions/extens...
chrome/common/extensions/extension_l10n_util.cc:286: }
On 2010/12/10 17:19:13, Nebojša Ćirić wrote:
> I think app/l10n_util would be a better place for this function (and
> GetParentLocales) - since they are not related to extension implementation
> details.

I agree

[Nebojša Ćirić, 2010-12-10 18:38:50]

http://codereview.chromium.org/5643002/diff/1/chrome/common/extensions/extens...
File chrome/common/extensions/extension_l10n_util.cc (right):

http://codereview.chromium.org/5643002/diff/1/chrome/common/extensions/extens...
chrome/common/extensions/extension_l10n_util.cc:255: if (!isalnum(ch) && ch !=
'-' && ch != '_')
I think IsAsciiAlpha && IsAsciiDigit would work here. Look at
trunk/src/chrome/common/extensions/extension_message_bundle.cc, IsValidName for
example use.

On 2010/12/10 18:32:17, Jungshik Shin wrote:
> isalnum is locale-dependent (C-library locale) and shouldn't be used.
> string_util may have something already. If not, you have to add one to check
for
> a-z, A-Z and 0-9.
> 
> As for "@key=value" syntax,  it's obsolete according to UTR 35
> (http://www.unicode.org/reports/tr35/#Unicode_Language_and_Locale_Identifiers
:
> section 3.4). For the immediate need of ours, I guess we don't need to be
> backward compatible with old syntax.

[dmazzoni, 2010-12-10 19:35:33]

Thanks for the review!

On Fri, Dec 10, 2010 at 9:19 AM, <cira@chromium.org> wrote:

>
>
http://codereview.chromium.org/5643002/diff/1/chrome/common/extensions/extens...
> File chrome/common/extensions/extension_l10n_util.cc (right):
>
>
>
http://codereview.chromium.org/5643002/diff/1/chrome/common/extensions/extens...
> chrome/common/extensions/extension_l10n_util.cc:252: // Check that all
> characters are alphanumeric, hyphen, or underscore.
> What about @ and = signs in de-DE@collation=phonebook? Locale validation
> is tough, given so many standards in use (BCP 47, TR#35...).
>

Thanks for pointing that out. I added basic support for validating a
keywords section.

I agree that locale validation is VERY hard, and there are a lot of locale
strings out there that don't conform to standards. I think the only
potential value is in something that accepts all known locale strings but
rejects things that are obviously not in the right syntax. This is can help
developers catch errors - if for example they pass the wrong thing to an API
that expects a locale - even though many invalid locale strings might be
accepted because we're not going to test exhaustively and handle all corner
cases and exceptions. Make sense?

http://codereview.chromium.org/5643002/diff/1/chrome/common/extensions/extens...
> chrome/common/extensions/extension_l10n_util.cc:286: }
> I think app/l10n_util would be a better place for this function (and
> GetParentLocales) - since they are not related to extension
> implementation details.


Done, I moved this, NormalizeLocale, and GetParentLocales.

On Fri, Dec 10, 2010 at 10:32 AM, <jshin@chromium.org> wrote:

>
>
http://codereview.chromium.org/5643002/diff/1/chrome/common/extensions/extens...
> chrome/common/extensions/extension_l10n_util.cc:245: // This implements
> a simple approximation of RFC 5646; it will accept all
> nit: BCP 47 (Best Current Practice 47 :
> http://www.rfc-editor.org/rfc/bcp/bcp47.txt ) is a more widely used
> reference (and easier to remember :-)). A bit more importantly, more
> 'stable' name than RFC 5646, I guess.
>

Thanks!


>
>
http://codereview.chromium.org/5643002/diff/1/chrome/common/extensions/extens...
> chrome/common/extensions/extension_l10n_util.cc:255: if (!isalnum(ch) &&
> ch != '-' && ch != '_')
> isalnum is locale-dependent (C-library locale) and shouldn't be used.
> string_util may have something already. If not, you have to add one to
> check for a-z, A-Z and 0-9.
>

Done, using IsAsciiAlpha && IsAsciiDigit.

http://codereview.chromium.org/5643002/diff/1/chrome/common/extensions/extens...
> chrome/common/extensions/extension_l10n_util.cc:268: if (token_index ==
> 0 && (token_len < 1 || token_len > 3)) {
> can you tighten up a bit here? although BCP 47 reserves a single digit
> tag for extension (section 3.7), I don't think any has been
> registered/used. So, the first tag can be assumed to be in [a-zA-Z]
>

Good idea, done.

- Dominic

[Nebojša Ćirić, 2010-12-10 21:42:35]

http://codereview.chromium.org/5643002/diff/9001/app/l10n_util.cc
File app/l10n_util.cc (right):

http://codereview.chromium.org/5643002/diff/9001/app/l10n_util.cc#newcode509
app/l10n_util.cc:509: //     #Unicode_Language_and_Locale_Identifiers
You could move some (url to TR35) to .h file and remove the rest.

http://codereview.chromium.org/5643002/diff/9001/app/l10n_util.cc#newcode521
app/l10n_util.cc:521: size_t split_point = locale.find("@");
You could do:
size_t split_point = locale.find("@");
if (split_point != std::string::npos) {
...
}

http://codereview.chromium.org/5643002/diff/9001/app/l10n_util.cc#newcode532
app/l10n_util.cc:532: // or underscore.
You could call NormalizeLocale before processing the rest. It would save you
checks for -.

[Nebojša Ćirić, 2010-12-10 21:48:45]

http://codereview.chromium.org/5643002/diff/9001/chrome/common/extensions/ext...
File chrome/common/extensions/extension_l10n_util_unittest.cc (right):

http://codereview.chromium.org/5643002/diff/9001/chrome/common/extensions/ext...
chrome/common/extensions/extension_l10n_util_unittest.cc:226: }
This test should go to app/l10n_util_unittest.cc

[jungshik at Google, 2010-12-10 22:21:27]

LGTM with nits below (plus what Cira said : about moving unittests)

http://codereview.chromium.org/5643002/diff/9001/app/l10n_util.h
File app/l10n_util.h (right):

http://codereview.chromium.org/5643002/diff/9001/app/l10n_util.h#newcode70
app/l10n_util.h:70: // 'en', 'pt-BR', 'fil', and 'zh-Hans-CN', but we don't want
to limit it
examples like 'es-419' and 'de_DE@collation=phonebook' can be mentioned here.

http://codereview.chromium.org/5643002/diff/9001/app/l10n_util_unittest.cc
File app/l10n_util_unittest.cc (right):

http://codereview.chromium.org/5643002/diff/9001/app/l10n_util_unittest.cc#ne...
app/l10n_util_unittest.cc:346: EXPECT_EQ(true,
l10n_util::IsValidLocaleSyntax("es-021"));
nit: perhaps, 'es-419' (one of languages CHrome is localized to: Latin American
Spanish) may be a better example. It doesn't really matter, though.

http://codereview.chromium.org/5643002/diff/9001/app/l10n_util_unittest.cc#ne...
app/l10n_util_unittest.cc:348: EXPECT_EQ(true,
l10n_util::IsValidLocaleSyntax("en_IE@currency=IEP"));
would you mind adding en_IE_u_cu_IEP (a newer form for the above)?

[dmazzoni, 2010-12-10 23:08:37]

On Fri, Dec 10, 2010 at 2:21 PM, <jshin@chromium.org> wrote:

> LGTM with nits below (plus what Cira said : about moving unittests)
>

Great!

I moved the spec url to the header file and the GetParentLocales unittest to
l10n_util.


> http://codereview.chromium.org/5643002/diff/9001/app/l10n_util.h
> File app/l10n_util.h (right):
>
> http://codereview.chromium.org/5643002/diff/9001/app/l10n_util.h#newcode70
> app/l10n_util.h:70: // 'en', 'pt-BR', 'fil', and 'zh-Hans-CN', but we
> don't want to limit it
> examples like 'es-419' and 'de_DE@collation=phonebook' can be mentioned
> here.
>

Sure, done.


> http://codereview.chromium.org/5643002/diff/9001/app/l10n_util_unittest.cc
> File app/l10n_util_unittest.cc (right):
>
>
>
http://codereview.chromium.org/5643002/diff/9001/app/l10n_util_unittest.cc#ne...
> app/l10n_util_unittest.cc:346: EXPECT_EQ(true,
> l10n_util::IsValidLocaleSyntax("es-021"));
> nit: perhaps, 'es-419' (one of languages CHrome is localized to: Latin
> American Spanish) may be a better example. It doesn't really matter,
> though.
>

Good idea, done.


>
>
http://codereview.chromium.org/5643002/diff/9001/app/l10n_util_unittest.cc#ne...
> app/l10n_util_unittest.cc:348: EXPECT_EQ(true,
> l10n_util::IsValidLocaleSyntax("en_IE@currency=IEP"));
> would you mind adding en_IE_u_cu_IEP (a newer form for the above)?


Sure. Out of curiosity, where did you find this one? A Google search didn't
return any hits, but it's probably not doing a very good job of matching
punctuation if someone used it with a mix of hyphens and underscores...

[Nebojša Ćirić, 2010-12-13 18:39:59]

Dominic,
 in case you missed it:

http://codereview.chromium.org/5643002/diff/9001/app/l10n_util.cc#newcode509
app/l10n_util.cc:509: //     #Unicode_Language_and_Locale_Identifiers
You could move some (url to TR35) to .h file and remove the rest.

http://codereview.chromium.org/5643002/diff/9001/app/l10n_util.cc#newcode521
app/l10n_util.cc:521: size_t split_point = locale.find("@");
You could do:
size_t split_point = locale.find("@");
if (split_point != std::string::npos) {
...
}

On 2010/12/10 23:08:37, Dominic Mazzoni wrote:
> On Fri, Dec 10, 2010 at 2:21 PM, <mailto:jshin@chromium.org> wrote:
> 
> > LGTM with nits below (plus what Cira said : about moving unittests)
> >
> 
> Great!
> 
> I moved the spec url to the header file and the GetParentLocales unittest to
> l10n_util.
> 
> 
> > http://codereview.chromium.org/5643002/diff/9001/app/l10n_util.h
> > File app/l10n_util.h (right):
> >
> > http://codereview.chromium.org/5643002/diff/9001/app/l10n_util.h#newcode70
> > app/l10n_util.h:70: // 'en', 'pt-BR', 'fil', and 'zh-Hans-CN', but we
> > don't want to limit it
> > examples like 'es-419' and 'de_DE@collation=phonebook' can be mentioned
> > here.
> >
> 
> Sure, done.
> 
> 
> > http://codereview.chromium.org/5643002/diff/9001/app/l10n_util_unittest.cc
> > File app/l10n_util_unittest.cc (right):
> >
> >
> >
>
http://codereview.chromium.org/5643002/diff/9001/app/l10n_util_unittest.cc#ne...
> > app/l10n_util_unittest.cc:346: EXPECT_EQ(true,
> > l10n_util::IsValidLocaleSyntax("es-021"));
> > nit: perhaps, 'es-419' (one of languages CHrome is localized to: Latin
> > American Spanish) may be a better example. It doesn't really matter,
> > though.
> >
> 
> Good idea, done.
> 
> 
> >
> >
>
http://codereview.chromium.org/5643002/diff/9001/app/l10n_util_unittest.cc#ne...
> > app/l10n_util_unittest.cc:348: EXPECT_EQ(true,
> > l10n_util::IsValidLocaleSyntax("en_IE@currency=IEP"));
> > would you mind adding en_IE_u_cu_IEP (a newer form for the above)?
> 
> 
> Sure. Out of curiosity, where did you find this one? A Google search didn't
> return any hits, but it's probably not doing a very good job of matching
> punctuation if someone used it with a mix of hyphens and underscores...

[Nebojša Ćirić, 2010-12-13 18:40:29]

and 
http://codereview.chromium.org/5643002/diff/9001/app/l10n_util.cc#newcode532
app/l10n_util.cc:532: // or underscore.
You could call NormalizeLocale before processing the rest. It would save you
checks for -.

On 2010/12/13 18:39:59, Nebojša Ćirić wrote:
> Dominic,
>  in case you missed it:
> 
> http://codereview.chromium.org/5643002/diff/9001/app/l10n_util.cc#newcode509
> app/l10n_util.cc:509: //     #Unicode_Language_and_Locale_Identifiers
> You could move some (url to TR35) to .h file and remove the rest.
> 
> http://codereview.chromium.org/5643002/diff/9001/app/l10n_util.cc#newcode521
> app/l10n_util.cc:521: size_t split_point = locale.find("@");
> You could do:
> size_t split_point = locale.find("@");
> if (split_point != std::string::npos) {
> ...
> }
> 
> On 2010/12/10 23:08:37, Dominic Mazzoni wrote:
> > On Fri, Dec 10, 2010 at 2:21 PM, <mailto:jshin@chromium.org> wrote:
> > 
> > > LGTM with nits below (plus what Cira said : about moving unittests)
> > >
> > 
> > Great!
> > 
> > I moved the spec url to the header file and the GetParentLocales unittest to
> > l10n_util.
> > 
> > 
> > > http://codereview.chromium.org/5643002/diff/9001/app/l10n_util.h
> > > File app/l10n_util.h (right):
> > >
> > > http://codereview.chromium.org/5643002/diff/9001/app/l10n_util.h#newcode70
> > > app/l10n_util.h:70: // 'en', 'pt-BR', 'fil', and 'zh-Hans-CN', but we
> > > don't want to limit it
> > > examples like 'es-419' and 'de_DE@collation=phonebook' can be mentioned
> > > here.
> > >
> > 
> > Sure, done.
> > 
> > 
> > > http://codereview.chromium.org/5643002/diff/9001/app/l10n_util_unittest.cc
> > > File app/l10n_util_unittest.cc (right):
> > >
> > >
> > >
> >
>
http://codereview.chromium.org/5643002/diff/9001/app/l10n_util_unittest.cc#ne...
> > > app/l10n_util_unittest.cc:346: EXPECT_EQ(true,
> > > l10n_util::IsValidLocaleSyntax("es-021"));
> > > nit: perhaps, 'es-419' (one of languages CHrome is localized to: Latin
> > > American Spanish) may be a better example. It doesn't really matter,
> > > though.
> > >
> > 
> > Good idea, done.
> > 
> > 
> > >
> > >
> >
>
http://codereview.chromium.org/5643002/diff/9001/app/l10n_util_unittest.cc#ne...
> > > app/l10n_util_unittest.cc:348: EXPECT_EQ(true,
> > > l10n_util::IsValidLocaleSyntax("en_IE@currency=IEP"));
> > > would you mind adding en_IE_u_cu_IEP (a newer form for the above)?
> > 
> > 
> > Sure. Out of curiosity, where did you find this one? A Google search didn't
> > return any hits, but it's probably not doing a very good job of matching
> > punctuation if someone used it with a mix of hyphens and underscores...

[dmazzoni, 2010-12-13 19:17:46]

Oops! Fix coming your way now.

On Mon, Dec 13, 2010 at 10:39 AM, <cira@chromium.org> wrote:

> Dominic,
>  in case you missed it:
>
>
> http://codereview.chromium.org/5643002/diff/9001/app/l10n_util.cc#newcode509
> app/l10n_util.cc:509: //     #Unicode_Language_and_Locale_Identifiers
> You could move some (url to TR35) to .h file and remove the rest.
>

This was done.


>
> http://codereview.chromium.org/5643002/diff/9001/app/l10n_util.cc#newcode521
> app/l10n_util.cc:521: size_t split_point = locale.find("@");
> You could do:
> size_t split_point = locale.find("@");
> if (split_point != std::string::npos) {
> ...
> }
>

Fixing in http://codereview.chromium.org/5816001/

and
>
>
> http://codereview.chromium.org/5643002/diff/9001/app/l10n_util.cc#newcode532
> app/l10n_util.cc:532: // or underscore.
> You could call NormalizeLocale before processing the rest. It would save
> you
> checks for -.
>

Fixing in http://codereview.chromium.org/5816001/