Detecting form locale

Detecting form locale

Just an intermediate checkpoint to make sure the code doesn't vanish :)

BUG=none
TEST=unit_tests --gtest_filter=AutoFillLocaleModelTest.*

[dhollowa, 2010-08-31 22:18:35]

http://codereview.chromium.org/3226001/diff/2001/3012
File chrome/renderer/form_manager.cc (right):

http://codereview.chromium.org/3226001/diff/2001/3012#newcode335
chrome/renderer/form_manager.cc:335: form->locale =
UTF16ToASCII(element.getLangAttribute());
Let's land the renderer-side changes separately from the rest.  Great to see
end-to-end thinking, but let's split things up to land.

http://codereview.chromium.org/3226001/diff/2001/3013
File chrome/renderer/form_manager_browsertest.cc (right):

http://codereview.chromium.org/3226001/diff/2001/3013#newcode39
chrome/renderer/form_manager_browsertest.cc:39: "      lang=\"fr-CA\">"
Let's add new tests for this stuff.  But add EXPECT_EQ("en-US", form.locale) for
existing tests.

http://codereview.chromium.org/3226001/diff/2001/3015
File webkit/glue/form_data.h (right):

http://codereview.chromium.org/3226001/diff/2001/3015#newcode28
webkit/glue/form_data.h:28: std::string locale;
string16 please.

[Ilya Sherman, 2010-09-02 23:04:30]

WebKit patch landed; the renderer part of the patch is functionally ready to go.

[dhollowa, 2010-09-03 18:44:24]

http://codereview.chromium.org/3226001/diff/22001/23001
File chrome/browser/autofill/autofill_locale_model.cc (right):

http://codereview.chromium.org/3226001/diff/22001/23001#newcode17
chrome/browser/autofill/autofill_locale_model.cc:17: // TODO(isherman): Should
these structs and arrays be declared as static?
For file local stuff anonymous namespaces are best.  See discussion here:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Namesp...

http://codereview.chromium.org/3226001/diff/22001/23001#newcode35
chrome/browser/autofill/autofill_locale_model.cc:35: // TODO(isherman): Expand
this list, make sure that it conforms to the IANA
When this is expanded please sight your source in comments.

http://codereview.chromium.org/3226001/diff/22001/23001#newcode63
chrome/browser/autofill/autofill_locale_model.cc:63: bool IsLowerASCII(const
std::string& str) {
Might be worth a search of the codebase to see if anyone else is doing it.  If
not, leave them here.  But if these are used only for DCHECKs then they should
be moved to unit test file instead (see comment below).

http://codereview.chromium.org/3226001/diff/22001/23001#newcode80
chrome/browser/autofill/autofill_locale_model.cc:80: for (size_t i = 0; i <
arraysize(kAutoFillLanguages); ++i) {
These computational checks are best moved to unit tests.  We don't need them in
production code.  Once moved, used EXPECT_TRUEs not DCHECKs.

http://codereview.chromium.org/3226001/diff/22001/23002
File chrome/browser/autofill/autofill_locale_model.h (right):

http://codereview.chromium.org/3226001/diff/22001/23002#newcode22
chrome/browser/autofill/autofill_locale_model.h:22: std::string region;
Let's make these string16s.  And below.

http://codereview.chromium.org/3226001/diff/22001/23002#newcode87
chrome/browser/autofill/autofill_locale_model.h:87: 
nit:no newline needed here.

http://codereview.chromium.org/3226001/diff/22001/23003
File chrome/browser/autofill/autofill_locale_model_unittest.cc (right):

http://codereview.chromium.org/3226001/diff/22001/23003#newcode21
chrome/browser/autofill/autofill_locale_model_unittest.cc:21: class
MockLanguageState : public LanguageState {
Please add comment that describes the roll of this class.

http://codereview.chromium.org/3226001/diff/22001/23003#newcode34
chrome/browser/autofill/autofill_locale_model_unittest.cc:34: };
Add DISALLOW_COPY_AND_ASSIGN.

http://codereview.chromium.org/3226001/diff/22001/23003#newcode36
chrome/browser/autofill/autofill_locale_model_unittest.cc:36: 
nit: one newline not two.

http://codereview.chromium.org/3226001/diff/22001/23004
File chrome/browser/autofill/autofill_manager.cc (right):

http://codereview.chromium.org/3226001/diff/22001/23004#newcode394
chrome/browser/autofill/autofill_manager.cc:394: DCHECK(type ==
NotificationType::TAB_LANGUAGE_DETERMINED);
Instead of DCHECK on type, how about adding an if clause, with an else
NOTREACHED?  Then if/when we extend we can simply add more else if clauses. 
Also, DCHECKs are compiled out for release so we'd hit the code below in
release.  i.e.

if (type == NotificationType::TAB_LANGUAGE_DETERMINED) {
  locale_model_...
  ...
} else {
  NOTREACHED();
}

http://codereview.chromium.org/3226001/diff/22001/23005
File chrome/browser/autofill/autofill_manager.h (right):

http://codereview.chromium.org/3226001/diff/22001/23005#newcode73
chrome/browser/autofill/autofill_manager.h:73: // Called with source
|tab_contents_| when the tab language is detected.
Convention in this file is to add a comment like:
// NotificationObserver implementation:

Also, please move this down to follow the AutoFillDownloadManager::Observer
methods.

http://codereview.chromium.org/3226001/diff/22001/23006
File chrome/browser/autofill/autofill_xml_parser.cc (right):

http://codereview.chromium.org/3226001/diff/22001/23006#newcode10
chrome/browser/autofill/autofill_xml_parser.cc:10: #include "base/logging.h"
Is this for debugging?  Or really needed?

http://codereview.chromium.org/3226001/diff/22001/23008
File chrome/browser/autofill/form_structure.h (right):

http://codereview.chromium.org/3226001/diff/22001/23008#newcode114
chrome/browser/autofill/form_structure.h:114: const std::string& locale() const
{ return locale_; }
As before, let's keep things as string16.  Also, should return object and not a
reference to an object.  This is dangerous from a lifetime / ownership point of
view.  Copies are safer than references.

http://codereview.chromium.org/3226001/diff/22001/23013
File chrome/renderer/form_manager.cc (right):

http://codereview.chromium.org/3226001/diff/22001/23013#newcode335
chrome/renderer/form_manager.cc:335: form->locale =
element.computeInheritedLanguage();
Maybe just inheritedLanguage, and drop the "compute"?

http://codereview.chromium.org/3226001/diff/22001/23014
File chrome/renderer/form_manager_browsertest.cc (right):

http://codereview.chromium.org/3226001/diff/22001/23014#newcode40
chrome/renderer/form_manager_browsertest.cc:40: // Test that we have extracted
teh default form data (disregarding the form
s/teh/the/

http://codereview.chromium.org/3226001/diff/22001/23014#newcode133
chrome/renderer/form_manager_browsertest.cc:133: TEST_F(FormManagerTest,
FormLocale) {
Please add some negative tests...  For example: 
  (1) invalid whitespace: lang="  x  ",
  (2) case insensitivity: lang="EN-GB"
  (3) what do we want to do with ISO 639.2/3/4/5/6?  Are we supporting only
639.1?

http://codereview.chromium.org/3226001/diff/22001/23014#newcode325
chrome/renderer/form_manager_browsertest.cc:325: EXPECT_EQ(ASCIIToUTF16(""),
form.locale);
s/ASCIIToUTF16("")/string16()/

http://codereview.chromium.org/3226001/diff/22001/23015
File webkit/glue/dom_operations.cc (right):

http://codereview.chromium.org/3226001/diff/22001/23015#newcode10
webkit/glue/dom_operations.cc:10: #include "base/logging.h"
Needed?  Or from debugging?

[Ilya Sherman, 2010-09-03 21:49:04]

A few quick responses -- will post an updated patch addressing the remaining
comments in a bit.

http://codereview.chromium.org/3226001/diff/22001/23002
File chrome/browser/autofill/autofill_locale_model.h (right):

http://codereview.chromium.org/3226001/diff/22001/23002#newcode22
chrome/browser/autofill/autofill_locale_model.h:22: std::string region;
On 2010/09/03 18:44:24, dhollowa wrote:
> Let's make these string16s.  And below.

Why?  They will only ever contain ASCII data -- what's the advantage of
string16?

http://codereview.chromium.org/3226001/diff/22001/23006
File chrome/browser/autofill/autofill_xml_parser.cc (right):

http://codereview.chromium.org/3226001/diff/22001/23006#newcode10
chrome/browser/autofill/autofill_xml_parser.cc:10: #include "base/logging.h"
On 2010/09/03 18:44:24, dhollowa wrote:
> Is this for debugging?  Or really needed?

It's for the DCHECKs in this file.  This used to be indirectly included via
webkit/glue/form_data.h but isn't anymore.

http://codereview.chromium.org/3226001/diff/22001/23013
File chrome/renderer/form_manager.cc (right):

http://codereview.chromium.org/3226001/diff/22001/23013#newcode335
chrome/renderer/form_manager.cc:335: form->locale =
element.computeInheritedLanguage();
On 2010/09/03 18:44:24, dhollowa wrote:
> Maybe just inheritedLanguage, and drop the "compute"?

This function name is part of the WebKit patch.  The reviewers thought that
since the function was doing real work (rather than looking up a cached value),
it would be better to have a suggestive verb in the name.

http://codereview.chromium.org/3226001/diff/22001/23014
File chrome/renderer/form_manager_browsertest.cc (right):

http://codereview.chromium.org/3226001/diff/22001/23014#newcode133
chrome/renderer/form_manager_browsertest.cc:133: TEST_F(FormManagerTest,
FormLocale) {
On 2010/09/03 18:44:24, dhollowa wrote:
> Please add some negative tests...  For example: 
>   (1) invalid whitespace: lang="  x  ",
>   (2) case insensitivity: lang="EN-GB"
>   (3) what do we want to do with ISO 639.2/3/4/5/6?  Are we supporting only
> 639.1?

This is purely double-checking that WebKit is giving us whatever it thinks is
the language tag.  WebKit does absolutely no error checking of language tags;
I'll add a test to make sure we notice if that ever changes.

http://codereview.chromium.org/3226001/diff/22001/23015
File webkit/glue/dom_operations.cc (right):

http://codereview.chromium.org/3226001/diff/22001/23015#newcode10
webkit/glue/dom_operations.cc:10: #include "base/logging.h"
On 2010/09/03 18:44:24, dhollowa wrote:
> Needed?  Or from debugging?

Also for DCHECKs.

[Ilya Sherman, 2010-09-03 23:35:57]

New patch up addressing most of the review comments; follow-up
comments/questions from me for the remainder.

http://codereview.chromium.org/3226001/diff/22001/23001
File chrome/browser/autofill/autofill_locale_model.cc (right):

http://codereview.chromium.org/3226001/diff/22001/23001#newcode17
chrome/browser/autofill/autofill_locale_model.cc:17: // TODO(isherman): Should
these structs and arrays be declared as static?
On 2010/09/03 18:44:24, dhollowa wrote:
> For file local stuff anonymous namespaces are best.  See discussion here:
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Namesp...

Ah, good call :)

http://codereview.chromium.org/3226001/diff/22001/23001#newcode63
chrome/browser/autofill/autofill_locale_model.cc:63: bool IsLowerASCII(const
std::string& str) {
On 2010/09/03 18:44:24, dhollowa wrote:
> Might be worth a search of the codebase to see if anyone else is doing it.  If
> not, leave them here.  But if these are used only for DCHECKs then they should
> be moved to unit test file instead (see comment below).

Searching (as below) suggests these are just used in DCHECKs, in a few places. 
Does that suggest I should add a function to string_util.h, or logging.h, or
somewhere else; or not at all, as there are only a few instances?

$git gs "\(.*\) == StringToLowerASCII(\1)"
chrome/browser/autofill/autofill_locale_model.cc:65:  return str ==
StringToLowerASCII(str);
net/base/mime_util.cc:378:  DCHECK(mime_type_pattern ==
StringToLowerASCII(mime_type_pattern));
net/base/mime_util.cc:379:  DCHECK(mime_type == StringToLowerASCII(mime_type));
webkit/glue/plugins/plugin_list.cc:275:  DCHECK(mime_type ==
StringToLowerASCII(mime_type));
webkit/glue/plugins/plugin_list.cc:293:  DCHECK(mime_type ==
StringToLowerASCII(mime_type));

http://codereview.chromium.org/3226001/diff/22001/23001#newcode80
chrome/browser/autofill/autofill_locale_model.cc:80: for (size_t i = 0; i <
arraysize(kAutoFillLanguages); ++i) {
On 2010/09/03 18:44:24, dhollowa wrote:
> These computational checks are best moved to unit tests.  We don't need them
in
> production code.  Once moved, used EXPECT_TRUEs not DCHECKs.

How do I move these into a unit test if the arrays are declared to be local to
this file?

http://codereview.chromium.org/3226001/diff/22001/23008
File chrome/browser/autofill/form_structure.h (right):

http://codereview.chromium.org/3226001/diff/22001/23008#newcode114
chrome/browser/autofill/form_structure.h:114: const std::string& locale() const
{ return locale_; }
On 2010/09/03 18:44:24, dhollowa wrote:
> As before, let's keep things as string16.  Also, should return object and not
a
> reference to an object.  This is dangerous from a lifetime / ownership point
of
> view.  Copies are safer than references.

Again, not sure what the advantage of string16 is -- meaningful locales will
always be ASCII data.

Also, I'm not convinced that we should return a copy rather than a reference: 
Scanning with 'git gs "url() const"' reveals that we generally prefer to return
const references.  If the caller makes the explicit decision to store the result
as a reference, that's a strong signal in C++ in general to think about object
lifetimes.

Mostly, though, I think we should be consistent with the rest of the codebase,
whether that means returning a copy or a const reference.  I defer to general
consensus for that; preliminarily, @erg suggests that the const reference is the
way to go.

http://codereview.chromium.org/3226001/diff/22001/23014
File chrome/renderer/form_manager_browsertest.cc (right):

http://codereview.chromium.org/3226001/diff/22001/23014#newcode133
chrome/renderer/form_manager_browsertest.cc:133: TEST_F(FormManagerTest,
FormLocale) {
On 2010/09/03 18:44:24, dhollowa wrote:
> Please add some negative tests...  For example: 
>   (1) invalid whitespace: lang="  x  ",
>   (2) case insensitivity: lang="EN-GB"

Added these to autofill_locale_model_unittest though.

>   (3) what do we want to do with ISO 639.2/3/4/5/6?  Are we supporting only
> 639.1?

We are mostly supporting the IANA subtag registry [1].  Really, though, we are
aiming to support any/all tags that give us useful signals for the languages we
are targeting.  This is currently controlled by the constants declared in
autofill_locale_model.cc.

[1] http://www.iana.org/assignments/language-subtag-registry

[dhollowa, 2010-09-04 00:38:16]

http://codereview.chromium.org/3226001/diff/22001/23001
File chrome/browser/autofill/autofill_locale_model.cc (right):

http://codereview.chromium.org/3226001/diff/22001/23001#newcode35
chrome/browser/autofill/autofill_locale_model.cc:35: // TODO(isherman): Expand
this list, make sure that it conforms to the IANA
On 2010/09/03 18:44:24, dhollowa wrote:
> When this is expanded please *cite* your source in comments.

Worth a look at: chrome/common/extensions/extension_l10n_util.h:85.  Possible
this list already exists in some form?

Also, the data files from the GWT address widgets probably have similar list. 
So we should look there too.

http://codereview.chromium.org/3226001/diff/22001/23001#newcode63
chrome/browser/autofill/autofill_locale_model.cc:63: bool IsLowerASCII(const
std::string& str) {
On 2010/09/03 23:35:57, Ilya Sherman wrote:
> On 2010/09/03 18:44:24, dhollowa wrote:
> > Might be worth a search of the codebase to see if anyone else is doing it. 
If
> > not, leave them here.  But if these are used only for DCHECKs then they
should
> > be moved to unit test file instead (see comment below).
> 
> Searching (as below) suggests these are just used in DCHECKs, in a few places.

> Does that suggest I should add a function to string_util.h, or logging.h, or
> somewhere else; or not at all, as there are only a few instances?
> 
> $git gs "\(.*\) == StringToLowerASCII(\1)"
> chrome/browser/autofill/autofill_locale_model.cc:65:  return str ==
> StringToLowerASCII(str);
> net/base/mime_util.cc:378:  DCHECK(mime_type_pattern ==
> StringToLowerASCII(mime_type_pattern));
> net/base/mime_util.cc:379:  DCHECK(mime_type ==
StringToLowerASCII(mime_type));
> webkit/glue/plugins/plugin_list.cc:275:  DCHECK(mime_type ==
> StringToLowerASCII(mime_type));
> webkit/glue/plugins/plugin_list.cc:293:  DCHECK(mime_type ==
> StringToLowerASCII(mime_type));

My vote is for not at all.  Normally I'm a radical opponent of code
duplication... (DRY principle).  But in this case let's see how the code evolves
and if it makes sense later we can consolidate.

http://codereview.chromium.org/3226001/diff/22001/23001#newcode80
chrome/browser/autofill/autofill_locale_model.cc:80: for (size_t i = 0; i <
arraysize(kAutoFillLanguages); ++i) {
On 2010/09/03 23:35:57, Ilya Sherman wrote:
> On 2010/09/03 18:44:24, dhollowa wrote:
> > These computational checks are best moved to unit tests.  We don't need them
> in
> > production code.  Once moved, used EXPECT_TRUEs not DCHECKs.
> 
> How do I move these into a unit test if the arrays are declared to be local to
> this file?
> 

One way would be to pull these checks out into a private static class method,
make the unit test a friend of the class, and then call the method from the unit
test.

Another way would be to pull the struct declarations into the header, keeping
the data here, but providing an accessor to class static data.  Then move the
checking logic into the unit tests.

I'd lean towards the latter because it keeps testing logic out of the
implementation.  But I'm wondering if we really need these checks at all?  Do we
expect the list to evolve over time?  I would guess not...

http://codereview.chromium.org/3226001/diff/22001/23002
File chrome/browser/autofill/autofill_locale_model.h (right):

http://codereview.chromium.org/3226001/diff/22001/23002#newcode22
chrome/browser/autofill/autofill_locale_model.h:22: std::string region;
On 2010/09/03 21:49:04, Ilya Sherman wrote:
> On 2010/09/03 18:44:24, dhollowa wrote:
> > Let's make these string16s.  And below.
> 
> Why?  They will only ever contain ASCII data -- what's the advantage of
> string16?
> 

I recant.  I was going for uniformity here, but after looking at
app/l10n_util.cc it seems that std::string is the norm.  We should probably undo
the string16 change on the renderer side then too.

http://codereview.chromium.org/3226001/diff/22001/23008
File chrome/browser/autofill/form_structure.h (right):

http://codereview.chromium.org/3226001/diff/22001/23008#newcode114
chrome/browser/autofill/form_structure.h:114: const std::string& locale() const
{ return locale_; }
On 2010/09/03 23:35:57, Ilya Sherman wrote:
> On 2010/09/03 18:44:24, dhollowa wrote:
> > As before, let's keep things as string16.  Also, should return object and
not
> a
> > reference to an object.  This is dangerous from a lifetime / ownership point
> of
> > view.  Copies are safer than references.
> 
> Again, not sure what the advantage of string16 is -- meaningful locales will
> always be ASCII data.
> 
> Also, I'm not convinced that we should return a copy rather than a reference: 
> Scanning with 'git gs "url() const"' reveals that we generally prefer to
return
> const references.  If the caller makes the explicit decision to store the
result
> as a reference, that's a strong signal in C++ in general to think about object
> lifetimes.
> 
> Mostly, though, I think we should be consistent with the rest of the codebase,
> whether that means returning a copy or a const reference.  I defer to general
> consensus for that; preliminarily, @erg suggests that the const reference is
the
> way to go.

Ok, const reference it is then.  I've seen this bite folks in the past, thus my
sensitivity.  And compiler's Return Value Optimization techniques tend to
optimize the copying away anyway.  But I agree, consistency is good.

http://codereview.chromium.org/3226001/diff/22001/23014
File chrome/renderer/form_manager_browsertest.cc (right):

http://codereview.chromium.org/3226001/diff/22001/23014#newcode133
chrome/renderer/form_manager_browsertest.cc:133: TEST_F(FormManagerTest,
FormLocale) {
On 2010/09/03 23:35:57, Ilya Sherman wrote:
> On 2010/09/03 18:44:24, dhollowa wrote:
> > Please add some negative tests...  For example: 
> >   (1) invalid whitespace: lang="  x  ",
> >   (2) case insensitivity: lang="EN-GB"
> 
> Added these to autofill_locale_model_unittest though.
> 
> >   (3) what do we want to do with ISO 639.2/3/4/5/6?  Are we supporting only
> > 639.1?
> 
> We are mostly supporting the IANA subtag registry [1].  Really, though, we are
> aiming to support any/all tags that give us useful signals for the languages
we
> are targeting.  This is currently controlled by the constants declared in
> autofill_locale_model.cc.
> 
> [1] http://www.iana.org/assignments/language-subtag-registry

Understood, thanks.  We'll need to spec out the mappings between form, system,
and address codes.  It's not quite clear to me yet how the three will relate.

[James Hawkins, 2010-09-07 20:54:21]

http://codereview.chromium.org/3226001/diff/37001/38001
File chrome/browser/autofill/autofill_locale_model.cc (right):

http://codereview.chromium.org/3226001/diff/37001/38001#newcode7
chrome/browser/autofill/autofill_locale_model.cc:7: #include <string>
These are already included in autofill_locale_model.h, please remove them.

http://codereview.chromium.org/3226001/diff/37001/38001#newcode18
chrome/browser/autofill/autofill_locale_model.cc:18: // TODO(isherman): Should
these structs and arrays be declared as static?
No, they're in an unnamed namespace so they should not have static modifier.

http://codereview.chromium.org/3226001/diff/37001/38001#newcode63
chrome/browser/autofill/autofill_locale_model.cc:63: // TODO(isherman): Does it
make sense to add these to base/string_util.h?
If you can find this code in several places in the code base, then yes;
otherwise, remove the comment.

http://codereview.chromium.org/3226001/diff/37001/38001#newcode80
chrome/browser/autofill/autofill_locale_model.cc:80: // TODO(isherman): Do we
want to use DCHECKs for this, or something else?
Per my comment in the header, don't enforce case at any level.

http://codereview.chromium.org/3226001/diff/37001/38001#newcode121
chrome/browser/autofill/autofill_locale_model.cc:121: bool
AutoFillLocaleModel::IsLanguageSubtag(const std::string& tag) {
This method should be removed from AutoFillLocaleModel and added to the unnamed
namespace.

http://codereview.chromium.org/3226001/diff/37001/38001#newcode131
chrome/browser/autofill/autofill_locale_model.cc:131: bool
AutoFillLocaleModel::IsRegionSubtag(const std::string& tag) {
This method should be removed from AutoFillLocaleModel and added to the unnamed
namespace.

http://codereview.chromium.org/3226001/diff/37001/38001#newcode141
chrome/browser/autofill/autofill_locale_model.cc:141: void
AutoFillLocaleModel::ParseLocale(const std::string& locale,
This method should be removed from AutoFillLocaleModel and added to the unnamed
namespace.

http://codereview.chromium.org/3226001/diff/37001/38001#newcode171
chrome/browser/autofill/autofill_locale_model.cc:171: bool
AutoFillLocaleModel::CompareAndSetLanguageIfEmpty(
This method should be removed from AutoFillLocaleModel and added to the unnamed
namespace.

http://codereview.chromium.org/3226001/diff/37001/38001#newcode198
chrome/browser/autofill/autofill_locale_model.cc:198: return "";
s/""/string16()/

http://codereview.chromium.org/3226001/diff/37001/38002
File chrome/browser/autofill/autofill_locale_model.h (right):

http://codereview.chromium.org/3226001/diff/37001/38002#newcode20
chrome/browser/autofill/autofill_locale_model.h:20: // locale indicators for
autofillable forms.
s/autofillable/auto-fillable/

http://codereview.chromium.org/3226001/diff/37001/38002#newcode31
chrome/browser/autofill/autofill_locale_model.h:31: void
SetTabLanguageDetermined(bool flag) {
|flag| is a bad variable name.  |language_determined| is better.

http://codereview.chromium.org/3226001/diff/37001/38002#newcode47
chrome/browser/autofill/autofill_locale_model.h:47: // Returns whether |tag| is
an IANA language subtag we support.
This is the first use of IANA in this file, please expand the acronym in
parentheses.

http://codereview.chromium.org/3226001/diff/37001/38002#newcode48
chrome/browser/autofill/autofill_locale_model.h:48: // Expects |tag| to be
lowercase ASCII.
Why force the caller (this class) to lowercase |tag| when we can do that in the
method?

http://codereview.chromium.org/3226001/diff/37001/38002#newcode52
chrome/browser/autofill/autofill_locale_model.h:52: // Expects |tag| to be
uppercase ASCII.
This is weird. We expect |tag| to be lowercase in IsLanguageSubtag() yet here we
expect |tag| to be uppercase?  Sounds fragile to me.

http://codereview.chromium.org/3226001/diff/37001/38002#newcode63
chrome/browser/autofill/autofill_locale_model.h:63: static bool
CompareAndSetLanguageIfEmpty(const std::string& target,
Conditionally this method will perform a comparison and return the result, or it
will set an out variable and unconditionally return true? Why do you need this?
More specifically, why can't this be split up?

http://codereview.chromium.org/3226001/diff/37001/38002#newcode68
chrome/browser/autofill/autofill_locale_model.h:68: // Otherwise, returns the
empty string.
s/the/an/

http://codereview.chromium.org/3226001/diff/37001/38004
File chrome/browser/autofill/autofill_manager.cc (right):

http://codereview.chromium.org/3226001/diff/37001/38004#newcode416
chrome/browser/autofill/autofill_manager.cc:416: if (type ==
NotificationType::TAB_LANGUAGE_DETERMINED) {
nit: You can save indentation by changing the logic ordering:

if (type != NotificationType::TAB_LANGUAGE_DETERMINED) {
  NOTREACHED();
  return;
}

DCHECK(...);
...

http://codereview.chromium.org/3226001/diff/37001/38004#newcode521
chrome/browser/autofill/autofill_manager.cc:521: locale_model_(NULL),
You have |tab_contents| at this point, so why don't you send it in to
|local_model_| constructor?

http://codereview.chromium.org/3226001/diff/37001/38013
File chrome/renderer/form_manager.cc (right):

http://codereview.chromium.org/3226001/diff/37001/38013#newcode335
chrome/renderer/form_manager.cc:335: form->locale =
element.computeInheritedLanguage();
I'm concerned about the timing hit we'll get from calling
computeInheritedLanguage().  Have you measured this change?

[Ilya Sherman, 2010-09-08 10:03:15]

New patch up :)

http://codereview.chromium.org/3226001/diff/22001/23001
File chrome/browser/autofill/autofill_locale_model.cc (right):

http://codereview.chromium.org/3226001/diff/22001/23001#newcode35
chrome/browser/autofill/autofill_locale_model.cc:35: // TODO(isherman): Expand
this list, make sure that it conforms to the IANA
On 2010/09/04 00:38:16, dhollowa wrote:
> On 2010/09/03 18:44:24, dhollowa wrote:
> > When this is expanded please *cite* your source in comments.
> 
> Worth a look at: chrome/common/extensions/extension_l10n_util.h:85.  Possible
> this list already exists in some form?
> 
> Also, the data files from the GWT address widgets probably have similar list. 
> So we should look there too.

Actually, it looks like the ICU library is a nice match for this -- switching to
it :)


There are also two noteworthy lists in the chrome source code:

app/l10n_util.cc:46 -- listing supported Chrome UI languages
third_party/GTM/AppKit/GTMGoogleSearch.m:41 -- mapping languages + regions to
domains

Neither is quite what we want, though.

http://codereview.chromium.org/3226001/diff/22001/23001#newcode80
chrome/browser/autofill/autofill_locale_model.cc:80: for (size_t i = 0; i <
arraysize(kAutoFillLanguages); ++i) {
On 2010/09/04 00:38:16, dhollowa wrote:
> On 2010/09/03 23:35:57, Ilya Sherman wrote:
> > On 2010/09/03 18:44:24, dhollowa wrote:
> > > These computational checks are best moved to unit tests.  We don't need
them
> > in
> > > production code.  Once moved, used EXPECT_TRUEs not DCHECKs.
> > 
> > How do I move these into a unit test if the arrays are declared to be local
to
> > this file?
> > 
> 
> One way would be to pull these checks out into a private static class method,
> make the unit test a friend of the class, and then call the method from the
unit
> test.
> 
> Another way would be to pull the struct declarations into the header, keeping
> the data here, but providing an accessor to class static data.  Then move the
> checking logic into the unit tests.
> 
> I'd lean towards the latter because it keeps testing logic out of the
> implementation.  But I'm wondering if we really need these checks at all?  Do
we
> expect the list to evolve over time?  I would guess not...
> 

We do expect these lists to evolve over time, as we add support for more
languages/regions.  I'm not sure whether the lists will end up living here or
somewhere else, though -- holding off on moving the checks to a unit test until
we decide where exactly the lists are going to live.

http://codereview.chromium.org/3226001/diff/37001/38001#newcode7
chrome/browser/autofill/autofill_locale_model.cc:7: #include <string>
On 2010/09/07 20:54:22, James Hawkins wrote:
> These are already included in autofill_locale_model.h, please remove them.

Good call, thanks :)

http://codereview.chromium.org/3226001/diff/37001/38001#newcode80
chrome/browser/autofill/autofill_locale_model.cc:80: for (size_t i = 0; i <
arraysize(kAutoFillLanguages); ++i) {
On 2010/09/07 20:54:22, James Hawkins wrote:
> Per my comment in the header, don't enforce case at any level.

We might as well enforce case here -- these are constants local to the
implementation file; and they should match the IANA registry, which specifies
case.

http://codereview.chromium.org/3226001/diff/37001/38001#newcode121
chrome/browser/autofill/autofill_locale_model.cc:121: for (size_t i = 0; i <
arraysize(kAutoFillLanguages); ++i) {
On 2010/09/07 20:54:22, James Hawkins wrote:
> This method should be removed from AutoFillLocaleModel and added to the
unnamed
> namespace.

Done.  What's the general philosophy on when it is more appropriate to declare a
method in the unnamed namespace?  Is it to declare private methods in the class
only when they need access to class data?

http://codereview.chromium.org/3226001/diff/37001/38002
File chrome/browser/autofill/autofill_locale_model.h (right):

http://codereview.chromium.org/3226001/diff/37001/38002#newcode48
chrome/browser/autofill/autofill_locale_model.h:48: // Expects |tag| to be
lowercase ASCII.
On 2010/09/07 20:54:22, James Hawkins wrote:
> Why force the caller (this class) to lowercase |tag| when we can do that in
the
> method?

Eventually the subtags are used to build up a locale string, like "en-US", so we
should standardize the case somewhere.  I've changed this to take a pointer to
the string and lowercase it as the first step -- not sure if that's much better
though...

http://codereview.chromium.org/3226001/diff/37001/38002#newcode52
chrome/browser/autofill/autofill_locale_model.h:52: // Expects |tag| to be
uppercase ASCII.
On 2010/09/07 20:54:22, James Hawkins wrote:
> This is weird. We expect |tag| to be lowercase in IsLanguageSubtag() yet here
we
> expect |tag| to be uppercase?  Sounds fragile to me.

In the IANA subtag registry, "ru" is a language (Russian) and "RU" is a region
(Russia) -- thus the different case expectations.

http://codereview.chromium.org/3226001/diff/37001/38002#newcode63
chrome/browser/autofill/autofill_locale_model.h:63: static bool
CompareAndSetLanguageIfEmpty(const std::string& target,
On 2010/09/07 20:54:22, James Hawkins wrote:
> Conditionally this method will perform a comparison and return the result, or
it
> will set an out variable and unconditionally return true? Why do you need
this?
> More specifically, why can't this be split up?

Yeah, this was a "useful" method before I cleaned up the code, but is just
really ridiculous now.  Removing it :)

http://codereview.chromium.org/3226001/diff/37001/38004
File chrome/browser/autofill/autofill_manager.cc (right):

http://codereview.chromium.org/3226001/diff/37001/38004#newcode521
chrome/browser/autofill/autofill_manager.cc:521: locale_model_(NULL),
On 2010/09/07 20:54:22, James Hawkins wrote:
> You have |tab_contents| at this point, so why don't you send it in to
> |local_model_| constructor?

Good point.

http://codereview.chromium.org/3226001/diff/37001/38013
File chrome/renderer/form_manager.cc (right):

http://codereview.chromium.org/3226001/diff/37001/38013#newcode335
chrome/renderer/form_manager.cc:335: form->locale =
element.computeInheritedLanguage();
On 2010/09/07 20:54:22, James Hawkins wrote:
> I'm concerned about the timing hit we'll get from calling
> computeInheritedLanguage().  Have you measured this change?

Just measured: seems to be on the order of 0.015 ms per form.  It's just walking
up the DOM tree, so in general it should not result in much of a performance hit
at all.

[Ilya Sherman, 2010-09-08 10:06:37]

Oops, a bunch of those comments are from before I switched over to the ICU
library -- please disregard 'em :)

[Ilya Sherman, 2010-09-09 01:33:12]

New patch up.  I'd especially appreciate feedback on the API changes in this
patch -- they feel a little off, but I'm not sure how to make them better.

[James Hawkins, 2010-09-10 19:18:46]

Please respond to fixed review comments by pressing the 'Done' button. Otherwise
it's much harder for the reviewer to see what has been fixed from previous patch
sets.

[James Hawkins, 2010-09-10 19:21:49]

http://codereview.chromium.org/3226001/diff/37001/38001
File chrome/browser/autofill/autofill_locale_model.cc (right):

http://codereview.chromium.org/3226001/diff/37001/38001#newcode121
chrome/browser/autofill/autofill_locale_model.cc:121: bool
AutoFillLocaleModel::IsLanguageSubtag(const std::string& tag) {
On 2010/09/08 10:03:16, Ilya Sherman wrote:
> On 2010/09/07 20:54:22, James Hawkins wrote:
> > This method should be removed from AutoFillLocaleModel and added to the
> unnamed
> > namespace.
> 
> Done.  What's the general philosophy on when it is more appropriate to declare
a
> method in the unnamed namespace?  Is it to declare private methods in the
class
> only when they need access to class data?

Correct, or when the class has utility methods used by code outside of the
class.

[Ilya Sherman, 2010-09-10 22:56:16]

"Done." =)

http://codereview.chromium.org/3226001/diff/37001/38001
File chrome/browser/autofill/autofill_locale_model.cc (right):

http://codereview.chromium.org/3226001/diff/37001/38001#newcode7
chrome/browser/autofill/autofill_locale_model.cc:7: #include <string>
On 2010/09/07 20:54:22, James Hawkins wrote:
> These are already included in autofill_locale_model.h, please remove them.

Done.

http://codereview.chromium.org/3226001/diff/37001/38001#newcode18
chrome/browser/autofill/autofill_locale_model.cc:18: // TODO(isherman): Should
these structs and arrays be declared as static?
On 2010/09/07 20:54:22, James Hawkins wrote:
> No, they're in an unnamed namespace so they should not have static modifier.

Done.

http://codereview.chromium.org/3226001/diff/37001/38001#newcode63
chrome/browser/autofill/autofill_locale_model.cc:63: // TODO(isherman): Does it
make sense to add these to base/string_util.h?
On 2010/09/07 20:54:22, James Hawkins wrote:
> If you can find this code in several places in the code base, then yes;
> otherwise, remove the comment.

Done.

http://codereview.chromium.org/3226001/diff/37001/38001#newcode80
chrome/browser/autofill/autofill_locale_model.cc:80: // TODO(isherman): Do we
want to use DCHECKs for this, or something else?
On 2010/09/07 20:54:22, James Hawkins wrote:
> Per my comment in the header, don't enforce case at any level.

Done.

http://codereview.chromium.org/3226001/diff/37001/38001#newcode131
chrome/browser/autofill/autofill_locale_model.cc:131: bool
AutoFillLocaleModel::IsRegionSubtag(const std::string& tag) {
On 2010/09/07 20:54:22, James Hawkins wrote:
> This method should be removed from AutoFillLocaleModel and added to the
unnamed
> namespace.

Done.

http://codereview.chromium.org/3226001/diff/37001/38001#newcode141
chrome/browser/autofill/autofill_locale_model.cc:141: void
AutoFillLocaleModel::ParseLocale(const std::string& locale,
On 2010/09/07 20:54:22, James Hawkins wrote:
> This method should be removed from AutoFillLocaleModel and added to the
unnamed
> namespace.

Done.

http://codereview.chromium.org/3226001/diff/37001/38001#newcode171
chrome/browser/autofill/autofill_locale_model.cc:171: bool
AutoFillLocaleModel::CompareAndSetLanguageIfEmpty(
On 2010/09/07 20:54:22, James Hawkins wrote:
> This method should be removed from AutoFillLocaleModel and added to the
unnamed
> namespace.

Done.

http://codereview.chromium.org/3226001/diff/37001/38001#newcode198
chrome/browser/autofill/autofill_locale_model.cc:198: return "";
On 2010/09/07 20:54:22, James Hawkins wrote:
> s/""/string16()/

Done.  (std::string())

http://codereview.chromium.org/3226001/diff/37001/38002
File chrome/browser/autofill/autofill_locale_model.h (right):

http://codereview.chromium.org/3226001/diff/37001/38002#newcode20
chrome/browser/autofill/autofill_locale_model.h:20: // locale indicators for
autofillable forms.
On 2010/09/07 20:54:22, James Hawkins wrote:
> s/autofillable/auto-fillable/

Done.

http://codereview.chromium.org/3226001/diff/37001/38002#newcode31
chrome/browser/autofill/autofill_locale_model.h:31: void
SetTabLanguageDetermined(bool flag) {
On 2010/09/07 20:54:22, James Hawkins wrote:
> |flag| is a bad variable name.  |language_determined| is better.

Done.

http://codereview.chromium.org/3226001/diff/37001/38002#newcode47
chrome/browser/autofill/autofill_locale_model.h:47: // Returns whether |tag| is
an IANA language subtag we support.
On 2010/09/07 20:54:22, James Hawkins wrote:
> This is the first use of IANA in this file, please expand the acronym in
> parentheses.

Done.

http://codereview.chromium.org/3226001/diff/37001/38002#newcode48
chrome/browser/autofill/autofill_locale_model.h:48: // Expects |tag| to be
lowercase ASCII.
On 2010/09/07 20:54:22, James Hawkins wrote:
> Why force the caller (this class) to lowercase |tag| when we can do that in
the
> method?

Done.

http://codereview.chromium.org/3226001/diff/37001/38002#newcode52
chrome/browser/autofill/autofill_locale_model.h:52: // Expects |tag| to be
uppercase ASCII.
On 2010/09/07 20:54:22, James Hawkins wrote:
> This is weird. We expect |tag| to be lowercase in IsLanguageSubtag() yet here
we
> expect |tag| to be uppercase?  Sounds fragile to me.

Done.

http://codereview.chromium.org/3226001/diff/37001/38002#newcode63
chrome/browser/autofill/autofill_locale_model.h:63: static bool
CompareAndSetLanguageIfEmpty(const std::string& target,
On 2010/09/07 20:54:22, James Hawkins wrote:
> Conditionally this method will perform a comparison and return the result, or
it
> will set an out variable and unconditionally return true? Why do you need
this?
> More specifically, why can't this be split up?

Done.

http://codereview.chromium.org/3226001/diff/37001/38002#newcode68
chrome/browser/autofill/autofill_locale_model.h:68: // Otherwise, returns the
empty string.
On 2010/09/07 20:54:22, James Hawkins wrote:
> s/the/an/

Done.

http://codereview.chromium.org/3226001/diff/37001/38004
File chrome/browser/autofill/autofill_manager.cc (right):

http://codereview.chromium.org/3226001/diff/37001/38004#newcode416
chrome/browser/autofill/autofill_manager.cc:416: if (type ==
NotificationType::TAB_LANGUAGE_DETERMINED) {
On 2010/09/07 20:54:22, James Hawkins wrote:
> nit: You can save indentation by changing the logic ordering:
> 
> if (type != NotificationType::TAB_LANGUAGE_DETERMINED) {
>   NOTREACHED();
>   return;
> }
> 
> DCHECK(...);
> ...

Done.

http://codereview.chromium.org/3226001/diff/37001/38004#newcode521
chrome/browser/autofill/autofill_manager.cc:521: locale_model_(NULL),
On 2010/09/07 20:54:22, James Hawkins wrote:
> You have |tab_contents| at this point, so why don't you send it in to
> |local_model_| constructor?

Done.

[James Hawkins, 2010-09-13 17:16:56]

Can I see a design doc that details how this is going to be used?

http://codereview.chromium.org/3226001/diff/17002/32002
File chrome/browser/autofill/autofill_locale_model.cc (right):

http://codereview.chromium.org/3226001/diff/17002/32002#newcode16
chrome/browser/autofill/autofill_locale_model.cc:16: namespace {
Blank lines after 'namespace {' and before the final '}'.

http://codereview.chromium.org/3226001/diff/17002/32002#newcode35
chrome/browser/autofill/autofill_locale_model.cc:35:
net::RegistryControlledDomainService::GetDomainAndRegistry(url);
Move |host| declaration and initialization after the if (registry_length ==
0...) block.

http://codereview.chromium.org/3226001/diff/17002/32002#newcode47
chrome/browser/autofill/autofill_locale_model.cc:47: // * An empty |locale_id|
returns to an empty result, rather than ICU's default
...returns an empty result,

http://codereview.chromium.org/3226001/diff/17002/32002#newcode55
chrome/browser/autofill/autofill_locale_model.cc:55: scoped_ptr<char>
sanitized_locale(new char[locale_id.size() + 1]);
You're mixing new[] with delete. Use scoped_array here.

http://codereview.chromium.org/3226001/diff/17002/32002#newcode82
chrome/browser/autofill/autofill_locale_model.cc:82: // Returns "en-US" as a
default if both |language| and |region| are empty.
This comment doesn't really match the code. The method returns "en-US" if
|language| is empty and returns |language| if |region| is empty.

http://codereview.chromium.org/3226001/diff/17002/32002#newcode93
chrome/browser/autofill/autofill_locale_model.cc:93: }
}  // namespace

http://codereview.chromium.org/3226001/diff/17002/32002#newcode180
chrome/browser/autofill/autofill_locale_model.cc:180: // trust the locales "en"
and "en-US", and so ignore them for now.
Can you provide reasoning in the comment for why we don't trust those locales?

http://codereview.chromium.org/3226001/diff/17002/32003
File chrome/browser/autofill/autofill_locale_model.h (right):

http://codereview.chromium.org/3226001/diff/17002/32003#newcode26
chrome/browser/autofill/autofill_locale_model.h:26: void
UpdateLocale(FormStructure* form) const;
Document this method.

http://codereview.chromium.org/3226001/diff/17002/32003#newcode35
chrome/browser/autofill/autofill_locale_model.h:35: return
is_tab_language_determined_;
Remove is_ from this variable and method name.

http://codereview.chromium.org/3226001/diff/17002/32003#newcode37
chrome/browser/autofill/autofill_locale_model.h:37: void
SetTabLanguageDetermined(bool language_determined) {
set_language_tag_determined(...)

http://codereview.chromium.org/3226001/diff/17002/32004
File chrome/browser/autofill/autofill_locale_model_unittest.cc (right):

http://codereview.chromium.org/3226001/diff/17002/32004#newcode9
chrome/browser/autofill/autofill_locale_model_unittest.cc:9: #include
"base/utf_string_conversions.h"
I might have missed it, but I don't see this include being used anywhere.

http://codereview.chromium.org/3226001/diff/17002/32004#newcode52
chrome/browser/autofill/autofill_locale_model_unittest.cc:52: const
std::vector<std::string>& languages = it->second.languages;
#include <vector>

http://codereview.chromium.org/3226001/diff/17002/32004#newcode98
chrome/browser/autofill/autofill_locale_model_unittest.cc:98:
SCOPED_TRACE("\n\tDOM: \"" + dom_locale + "\"\n\tCLD: \"" + detected_locale +
80 cols.

http://codereview.chromium.org/3226001/diff/17002/32004#newcode197
chrome/browser/autofill/autofill_locale_model_unittest.cc:197: } // namespace
Two spaces before comments.

[dhollowa, 2010-09-13 17:23:41]

On 2010/09/13 17:16:56, James Hawkins wrote:
> Can I see a design doc that details how this is going to be used?
> 

I'll work on a doc for this.  We have a rough functional specification, but we
need the  engineering design documented as well.

[Ilya Sherman, 2010-09-13 19:00:02]

Updated patch up

http://codereview.chromium.org/3226001/diff/17002/32002
File chrome/browser/autofill/autofill_locale_model.cc (right):

http://codereview.chromium.org/3226001/diff/17002/32002#newcode16
chrome/browser/autofill/autofill_locale_model.cc:16: namespace {
On 2010/09/13 17:16:56, James Hawkins wrote:
> Blank lines after 'namespace {' and before the final '}'.

Done.

http://codereview.chromium.org/3226001/diff/17002/32002#newcode35
chrome/browser/autofill/autofill_locale_model.cc:35:
net::RegistryControlledDomainService::GetDomainAndRegistry(url);
On 2010/09/13 17:16:56, James Hawkins wrote:
> Move |host| declaration and initialization after the if (registry_length ==
> 0...) block.

Done.

http://codereview.chromium.org/3226001/diff/17002/32002#newcode47
chrome/browser/autofill/autofill_locale_model.cc:47: // * An empty |locale_id|
returns to an empty result, rather than ICU's default
On 2010/09/13 17:16:56, James Hawkins wrote:
> ...returns an empty result,

Done.

http://codereview.chromium.org/3226001/diff/17002/32002#newcode55
chrome/browser/autofill/autofill_locale_model.cc:55: scoped_ptr<char>
sanitized_locale(new char[locale_id.size() + 1]);
On 2010/09/13 17:16:56, James Hawkins wrote:
> You're mixing new[] with delete. Use scoped_array here.

Done.

http://codereview.chromium.org/3226001/diff/17002/32002#newcode82
chrome/browser/autofill/autofill_locale_model.cc:82: // Returns "en-US" as a
default if both |language| and |region| are empty.
On 2010/09/13 17:16:56, James Hawkins wrote:
> This comment doesn't really match the code. The method returns "en-US" if
> |language| is empty and returns |language| if |region| is empty.

Done.

http://codereview.chromium.org/3226001/diff/17002/32002#newcode93
chrome/browser/autofill/autofill_locale_model.cc:93: }
On 2010/09/13 17:16:56, James Hawkins wrote:
> }  // namespace

Done.

http://codereview.chromium.org/3226001/diff/17002/32002#newcode180
chrome/browser/autofill/autofill_locale_model.cc:180: // trust the locales "en"
and "en-US", and so ignore them for now.
On 2010/09/13 17:16:56, James Hawkins wrote:
> Can you provide reasoning in the comment for why we don't trust those locales?

Done.

http://codereview.chromium.org/3226001/diff/17002/32003
File chrome/browser/autofill/autofill_locale_model.h (right):

http://codereview.chromium.org/3226001/diff/17002/32003#newcode26
chrome/browser/autofill/autofill_locale_model.h:26: void
UpdateLocale(FormStructure* form) const;
On 2010/09/13 17:16:56, James Hawkins wrote:
> Document this method.

Done.

http://codereview.chromium.org/3226001/diff/17002/32003#newcode35
chrome/browser/autofill/autofill_locale_model.h:35: return
is_tab_language_determined_;
On 2010/09/13 17:16:56, James Hawkins wrote:
> Remove is_ from this variable and method name.

Done.

http://codereview.chromium.org/3226001/diff/17002/32003#newcode37
chrome/browser/autofill/autofill_locale_model.h:37: void
SetTabLanguageDetermined(bool language_determined) {
On 2010/09/13 17:16:56, James Hawkins wrote:
> set_language_tag_determined(...)

Done.

http://codereview.chromium.org/3226001/diff/17002/32004
File chrome/browser/autofill/autofill_locale_model_unittest.cc (right):

http://codereview.chromium.org/3226001/diff/17002/32004#newcode9
chrome/browser/autofill/autofill_locale_model_unittest.cc:9: #include
"base/utf_string_conversions.h"
On 2010/09/13 17:16:56, James Hawkins wrote:
> I might have missed it, but I don't see this include being used anywhere.

Done.  Good catch! =)

http://codereview.chromium.org/3226001/diff/17002/32004#newcode52
chrome/browser/autofill/autofill_locale_model_unittest.cc:52: const
std::vector<std::string>& languages = it->second.languages;
On 2010/09/13 17:16:56, James Hawkins wrote:
> #include <vector>

Done.

http://codereview.chromium.org/3226001/diff/17002/32004#newcode98
chrome/browser/autofill/autofill_locale_model_unittest.cc:98:
SCOPED_TRACE("\n\tDOM: \"" + dom_locale + "\"\n\tCLD: \"" + detected_locale +
On 2010/09/13 17:16:56, James Hawkins wrote:
> 80 cols.

Done.

http://codereview.chromium.org/3226001/diff/17002/32004#newcode197
chrome/browser/autofill/autofill_locale_model_unittest.cc:197: } // namespace
On 2010/09/13 17:16:56, James Hawkins wrote:
> Two spaces before comments.

Done.

[dhollowa, 2010-09-16 20:06:20]

On 2010/09/13 17:23:41, dhollowa wrote:
> On 2010/09/13 17:16:56, James Hawkins wrote:
> > Can I see a design doc that details how this is going to be used?
> > 
> 
> I'll work on a doc for this.  We have a rough functional specification, but we
> need the  engineering design documented as well.

James, I've posted an initial draft of a design doc here:

http://sites.google.com/a/chromium.org/dev/developers/design-documents/form-a...

We have some investigation to do with respect to the efficacy of the CLD method
of determining form locale.  My thinking at the moment is that we can dispense
with the autofill_local_model stuff that looks at URLs.  With top i18l sites as
dataset if CLD does the job effectively, then we can stop with that.

[Ilya Sherman, 2010-09-16 23:26:48]

> We have some investigation to do with respect to the efficacy of the CLD
method
> of determining form locale.  My thinking at the moment is that we can dispense
> with the autofill_local_model stuff that looks at URLs.  With top i18l sites
as
> dataset if CLD does the job effectively, then we can stop with that.

David and I talked offline about this.  A quick summary: the CLD (compact
language detection) library is fantastic at determining the language (e.g.
English), but not the region (e.g. Great Britain).  From conversation with Dick
Sites, the i18n go-to guy when it comes to automatic language detection, there
is not going to be a library like CLD that can provide us with region-detection
anytime soon -- the data is too sparse.  He recommended using heuristics based
on signals like looking at the hostname, looking at the server's OS version from
the HTTP headers, looking for region-specific labels in the form data.

All that said, David and I think it's best to table this cl until we have more
of the basic autofill i18n infrastructure built up.  At that point it will be
clearer how much marginal benefit detecting the form locale would give us.

[James Hawkins, 2010-09-18 22:36:40]

On 2010/09/16 23:26:48, Ilya Sherman wrote:
> > We have some investigation to do with respect to the efficacy of the CLD
> method
> > of determining form locale.  My thinking at the moment is that we can
dispense
> > with the autofill_local_model stuff that looks at URLs.  With top i18l sites
> as
> > dataset if CLD does the job effectively, then we can stop with that.
> 
> David and I talked offline about this.  A quick summary: the CLD (compact
> language detection) library is fantastic at determining the language (e.g.
> English), but not the region (e.g. Great Britain).  From conversation with
Dick
> Sites, the i18n go-to guy when it comes to automatic language detection, there
> is not going to be a library like CLD that can provide us with
region-detection
> anytime soon -- the data is too sparse.  He recommended using heuristics based
> on signals like looking at the hostname, looking at the server's OS version
from
> the HTTP headers, looking for region-specific labels in the form data.
> 
> All that said, David and I think it's best to table this cl until we have more
> of the basic autofill i18n infrastructure built up.  At that point it will be
> clearer how much marginal benefit detecting the form locale would give us.

Will you be closing this CL and re-opening a copy at a later date or are you
going to keep this one open?

[Ilya Sherman, 2010-09-20 19:31:58]

On 2010/09/18 22:36:40, James Hawkins wrote:
> Will you be closing this CL and re-opening a copy at a later date or are you
> going to keep this one open?

I guess I can close this and reopen a copy later.