Store the language code for the address in autofill profile.

Store the language code for the address in autofill profile.

After BuildComponents() returns the language code that should be used to
format the address, Chrome should store and sync this language code
along with the autofill profile. The language code should not be
communicated to the autofill server, because it's not the type of data
that will be sent to an input field on a webpage.

BUG=354955
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=263311

[please use gerrit instead, 2014-03-27 23:12:08]

Evan: PTAL autofill.

Scott: PTAL web database.

Nicolas: PTAL sync.

[please use gerrit instead, 2014-03-28 18:18:31]

Ilya: As Evan is OOO for a week, do you have any comments on this CL?

[Nicolas Zea, 2014-03-28 18:43:56]

sync LGTM

https://codereview.chromium.org/212873003/diff/180001/components/autofill/cor...
File
components/autofill/core/browser/webdata/autofill_profile_syncable_service.cc
(right):

https://codereview.chromium.org/212873003/diff/180001/components/autofill/cor...
components/autofill/core/browser/webdata/autofill_profile_syncable_service.cc:380:
if (specifics.address_home_language_code() != profile->language_code()) {
You should probably check has_address_home_language_code() first, to handle data
from previous versions.

[Scott Hess - ex-Googler, 2014-03-28 19:53:57]

https://codereview.chromium.org/212873003/diff/180001/components/autofill/cor...
File components/autofill/core/browser/webdata/autofill_table.cc (right):

https://codereview.chromium.org/212873003/diff/180001/components/autofill/cor...
components/autofill/core/browser/webdata/autofill_table.cc:1280: "language_code
VARCHAR, "
Since ALTER TABLE can only add new columns, I generally prefer to add new
columns when editing CREATE TABLE statements, so that the ordering is the same
regardless of which path the user took to a particular schema version.  I do not
know if webdata has practices in this area, though.

https://codereview.chromium.org/212873003/diff/180001/components/autofill/cor...
components/autofill/core/browser/webdata/autofill_table.cc:2271: bool
AutofillTable::MigrateToVersion56AddProfileLanguageCodeForFormatting() {
AFAICT, this is just manually implementing ALTER TABLE, except that ALTER TABLE
won't trampoline the table data.  Is there something which requires
language_code to not have a default value specified?

https://codereview.chromium.org/212873003/diff/180001/components/test/data/we...
File components/test/data/web_database/version_54.sql (right):

https://codereview.chromium.org/212873003/diff/180001/components/test/data/we...
components/test/data/web_database/version_54.sql:29: CREATE TABLE
autofill_profiles ( guid VARCHAR PRIMARY KEY, company_name VARCHAR,
street_address VARCHAR, dependent_locality VARCHAR, city VARCHAR, state VARCHAR,
zipcode VARCHAR, sorting_code VARCHAR, country_code VARCHAR, date_modified
INTEGER NOT NULL DEFAULT 0, origin VARCHAR DEFAULT '');
Golden files should never be modified, the v54->v55 update code should handle
the case.

[Scott Hess - ex-Googler, 2014-03-28 19:55:12]

Also, just FYI, I'm mostly a SQLite reviewer.  I suspect this code has local
conventions I may not be fully aware of which might go against my general
suggestions.

[please use gerrit instead, 2014-03-28 22:00:38]

Scott: PTAL Patch Set 2.

https://codereview.chromium.org/212873003/diff/180001/components/autofill/cor...
File
components/autofill/core/browser/webdata/autofill_profile_syncable_service.cc
(right):

https://codereview.chromium.org/212873003/diff/180001/components/autofill/cor...
components/autofill/core/browser/webdata/autofill_profile_syncable_service.cc:380:
if (specifics.address_home_language_code() != profile->language_code()) {
On 2014/03/28 18:43:57, Nicolas Zea wrote:
> You should probably check has_address_home_language_code() first, to handle
data
> from previous versions.

Done.

https://codereview.chromium.org/212873003/diff/180001/components/autofill/cor...
File components/autofill/core/browser/webdata/autofill_table.cc (right):

https://codereview.chromium.org/212873003/diff/180001/components/autofill/cor...
components/autofill/core/browser/webdata/autofill_table.cc:1280: "language_code
VARCHAR, "
On 2014/03/28 19:53:57, shess wrote:
> Since ALTER TABLE can only add new columns, I generally prefer to add new
> columns when editing CREATE TABLE statements, so that the ordering is the same
> regardless of which path the user took to a particular schema version.  I do
not
> know if webdata has practices in this area, though.

Changed the statement to append the column instead. That sounds like the right
approach. because it mimics what ALTER TABLE ADD COLUMN does.

FYI, the fields street_address, dependent_locality, and sorting_code were added
in logical ordering:

http://src.chromium.org/viewvc/chrome/trunk/src/components/autofill/core/brow...

https://codereview.chromium.org/212873003/diff/180001/components/autofill/cor...
components/autofill/core/browser/webdata/autofill_table.cc:2271: bool
AutofillTable::MigrateToVersion56AddProfileLanguageCodeForFormatting() {
On 2014/03/28 19:53:57, shess wrote:
> AFAICT, this is just manually implementing ALTER TABLE, except that ALTER
TABLE
> won't trampoline the table data.  Is there something which requires
> language_code to not have a default value specified?

Replaced the manual operations with a single alter table. This approach is much
shorter and more resilient to failure. (It makes fewer assumptions about
existing data.)

https://codereview.chromium.org/212873003/diff/180001/components/test/data/we...
File components/test/data/web_database/version_54.sql (right):

https://codereview.chromium.org/212873003/diff/180001/components/test/data/we...
components/test/data/web_database/version_54.sql:29: CREATE TABLE
autofill_profiles ( guid VARCHAR PRIMARY KEY, company_name VARCHAR,
street_address VARCHAR, dependent_locality VARCHAR, city VARCHAR, state VARCHAR,
zipcode VARCHAR, sorting_code VARCHAR, country_code VARCHAR, date_modified
INTEGER NOT NULL DEFAULT 0, origin VARCHAR DEFAULT '');
On 2014/03/28 19:53:57, shess wrote:
> Golden files should never be modified, the v54->v55 update code should handle
> the case.

This change was originally necessary because version_54.sql had v53
autofill_profiles schema. The change is no longer necessary after I switched to
the "ALTER TABLE ADD COLUMN" statement as you advised. However, I think that
WebDatabaseMigrationTest.MigrateVersion54ToCurrent unit test might fail the next
time someone alters autofill_profiles schema again.

TL;DR: I removed this change.

[please use gerrit instead, 2014-03-28 22:01:42]

Ruslan: PTAL autofill_dialog_controller_android.cc in Patch Set 2.

[aruslan, 2014-03-28 22:07:16]

android bits lgtm assuming the tests pass.

[Ilya Sherman, 2014-03-29 01:24:42]

Thanks, Rouslan!  Looks pretty good.  A few comments inline:

https://codereview.chromium.org/212873003/diff/220001/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right):

https://codereview.chromium.org/212873003/diff/220001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:3312: default:
please explicitly list all SECTION_'s rather than using a default case, so that
the compiler will catch it if someone forgets to update this switch stmt.

https://codereview.chromium.org/212873003/diff/220001/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/autofill_dialog_i18n_input.h (right):

https://codereview.chromium.org/212873003/diff/220001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_dialog_i18n_input.h:37: // or shipping).
nit: I'd move this above the description for the |language_code|, since this
param comes first.

https://codereview.chromium.org/212873003/diff/220001/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/autofill_dialog_i18n_input_unittest.cc (right):

https://codereview.chromium.org/212873003/diff/220001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_dialog_i18n_input_unittest.cc:40:
&language_code);
nit: Might as well either pass NULL or verify the resulting language.

https://codereview.chromium.org/212873003/diff/220001/components/autofill/cor...
File components/autofill/core/browser/autofill_profile.h (right):

https://codereview.chromium.org/212873003/diff/220001/components/autofill/cor...
components/autofill/core/browser/autofill_profile.h:193: std::string
language_code_;
nit: Docs, please.

https://codereview.chromium.org/212873003/diff/220001/components/autofill/cor...
File
components/autofill/core/browser/webdata/autofill_profile_syncable_service_unittest.cc
(right):

https://codereview.chromium.org/212873003/diff/220001/components/autofill/cor...
components/autofill/core/browser/webdata/autofill_profile_syncable_service_unittest.cc:830:
// updates.
Please add a similar test for the language code field.

https://codereview.chromium.org/212873003/diff/220001/components/autofill/cor...
File components/autofill/core/browser/webdata/autofill_table.cc (right):

https://codereview.chromium.org/212873003/diff/220001/components/autofill/cor...
components/autofill/core/browser/webdata/autofill_table.cc:1282: "language_code
VARCHAR)")) {
nit: Please sort this between country_code and date_modified as you have
elsewhere.

https://codereview.chromium.org/212873003/diff/220001/components/autofill/cor...
components/autofill/core/browser/webdata/autofill_table.cc:2278: // the table
was newly created when migrating from a pre-version-23 database.
This sort of thing should no longer be needed.  It should be safe to reduce this
entire method to

return db_->Execute("ALTER TABLE ...");

https://codereview.chromium.org/212873003/diff/220001/components/webdata/comm...
File components/webdata/common/web_database_migration_unittest.cc (right):

https://codereview.chromium.org/212873003/diff/220001/components/webdata/comm...
components/webdata/common/web_database_migration_unittest.cc:2638:
EXPECT_EQ(base::string16(), s_profiles.ColumnString16(9));
Is the datatype string16 or std::string?

[Scott Hess - ex-Googler, 2014-03-31 17:46:37]

WRT the v54 golden file, if it's broken, probably more appropriate to fix it in
a separate CL.  If it is broken, I'd fix it sooner rather than later, fixing
6-month-old schema mistakes is annoying due to the amount of code archaeology
you have to do.

https://codereview.chromium.org/212873003/diff/180001/components/test/data/we...
File components/test/data/web_database/version_54.sql (right):

https://codereview.chromium.org/212873003/diff/180001/components/test/data/we...
components/test/data/web_database/version_54.sql:29: CREATE TABLE
autofill_profiles ( guid VARCHAR PRIMARY KEY, company_name VARCHAR,
street_address VARCHAR, dependent_locality VARCHAR, city VARCHAR, state VARCHAR,
zipcode VARCHAR, sorting_code VARCHAR, country_code VARCHAR, date_modified
INTEGER NOT NULL DEFAULT 0, origin VARCHAR DEFAULT '');
On 2014/03/28 22:00:38, Rouslan Solomakhin wrote:
> On 2014/03/28 19:53:57, shess wrote:
> > Golden files should never be modified, the v54->v55 update code should
handle
> > the case.
> 
> This change was originally necessary because version_54.sql had v53
> autofill_profiles schema. The change is no longer necessary after I switched
to
> the "ALTER TABLE ADD COLUMN" statement as you advised. However, I think that
> WebDatabaseMigrationTest.MigrateVersion54ToCurrent unit test might fail the
next
> time someone alters autofill_profiles schema again.
> 
> TL;DR: I removed this change.

Hmm, if I'm reading this right, you're saying that the v54 golden file actually
doesn't match the schema generated by creating the database with v54 code?  In
that case it needs to be fixed.  The golden files should match what was in
production as of that version, which is why they should never be modified :-).

Maybe there should be a generic test to verify that the current golden schema
actually matches the current code output.

https://codereview.chromium.org/212873003/diff/220001/components/autofill/cor...
File components/autofill/core/browser/webdata/autofill_table.cc (right):

https://codereview.chromium.org/212873003/diff/220001/components/autofill/cor...
components/autofill/core/browser/webdata/autofill_table.cc:1282: "language_code
VARCHAR)")) {
On 2014/03/29 01:24:42, Ilya Sherman wrote:
> nit: Please sort this between country_code and date_modified as you have
> elsewhere.

It should be at the end to match the ALTER TABLE.  It is not always possible to
exactly match organic series-of-upgrades schema with freshly-created schema, but
reducing the differences generally reduces mistakes.

https://codereview.chromium.org/212873003/diff/220001/components/autofill/cor...
components/autofill/core/browser/webdata/autofill_table.cc:2278: // the table
was newly created when migrating from a pre-version-23 database.
On 2014/03/29 01:24:42, Ilya Sherman wrote:
> This sort of thing should no longer be needed.  It should be safe to reduce
this
> entire method to
> 
> return db_->Execute("ALTER TABLE ...");

I agree, no reason for a commit if there's only a single thread of control with
a single atomic change.  You do need a commit if someone can race between the
existence check and the alter.

https://codereview.chromium.org/212873003/diff/220001/components/autofill/cor...
File components/autofill/core/browser/webdata/autofill_table.h (right):

https://codereview.chromium.org/212873003/diff/220001/components/autofill/cor...
components/autofill/core/browser/webdata/autofill_table.h:72: //                
     Added in version 56.
Put the comment at the end if the code is at the end.  As a general thing, the
ordering of these items in storage is really irrelevant, it's just an
implementation detail.  But this comment appears to be ordered by physical order
currently, so maintain that.

[please use gerrit instead, 2014-04-02 21:54:51]

Ilya & Scott: PTAL Patch Set 3.

https://codereview.chromium.org/212873003/diff/220001/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right):

https://codereview.chromium.org/212873003/diff/220001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:3312: default:
On 2014/03/29 01:24:42, Ilya Sherman wrote:
> please explicitly list all SECTION_'s rather than using a default case, so
that
> the compiler will catch it if someone forgets to update this switch stmt.

Done.

https://codereview.chromium.org/212873003/diff/220001/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/autofill_dialog_i18n_input.h (right):

https://codereview.chromium.org/212873003/diff/220001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_dialog_i18n_input.h:37: // or shipping).
On 2014/03/29 01:24:42, Ilya Sherman wrote:
> nit: I'd move this above the description for the |language_code|, since this
> param comes first.

Done.

https://codereview.chromium.org/212873003/diff/220001/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/autofill_dialog_i18n_input_unittest.cc (right):

https://codereview.chromium.org/212873003/diff/220001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_dialog_i18n_input_unittest.cc:40:
&language_code);
On 2014/03/29 01:24:42, Ilya Sherman wrote:
> nit: Might as well either pass NULL or verify the resulting language.

Done.

https://codereview.chromium.org/212873003/diff/220001/components/autofill/cor...
File components/autofill/core/browser/autofill_profile.h (right):

https://codereview.chromium.org/212873003/diff/220001/components/autofill/cor...
components/autofill/core/browser/autofill_profile.h:193: std::string
language_code_;
On 2014/03/29 01:24:42, Ilya Sherman wrote:
> nit: Docs, please.

Done.

https://codereview.chromium.org/212873003/diff/220001/components/autofill/cor...
File
components/autofill/core/browser/webdata/autofill_profile_syncable_service_unittest.cc
(right):

https://codereview.chromium.org/212873003/diff/220001/components/autofill/cor...
components/autofill/core/browser/webdata/autofill_profile_syncable_service_unittest.cc:830:
// updates.
On 2014/03/29 01:24:42, Ilya Sherman wrote:
> Please add a similar test for the language code field.

Done.

https://codereview.chromium.org/212873003/diff/220001/components/autofill/cor...
File components/autofill/core/browser/webdata/autofill_table.cc (right):

https://codereview.chromium.org/212873003/diff/220001/components/autofill/cor...
components/autofill/core/browser/webdata/autofill_table.cc:1282: "language_code
VARCHAR)")) {
On 2014/03/31 17:46:38, shess wrote:
> On 2014/03/29 01:24:42, Ilya Sherman wrote:
> > nit: Please sort this between country_code and date_modified as you have
> > elsewhere.
> 
> It should be at the end to match the ALTER TABLE.  It is not always possible
to
> exactly match organic series-of-upgrades schema with freshly-created schema,
but
> reducing the differences generally reduces mistakes.

Keeping the field at the end to enable using ALTER TABLE in migration. ALTER
TABLE is more succinct, but can can only append columns at the end.

https://codereview.chromium.org/212873003/diff/220001/components/autofill/cor...
components/autofill/core/browser/webdata/autofill_table.cc:2278: // the table
was newly created when migrating from a pre-version-23 database.
On 2014/03/31 17:46:38, shess wrote:
> On 2014/03/29 01:24:42, Ilya Sherman wrote:
> > This sort of thing should no longer be needed.  It should be safe to reduce
> this
> > entire method to
> > 
> > return db_->Execute("ALTER TABLE ...");
> 
> I agree, no reason for a commit if there's only a single thread of control
with
> a single atomic change.  You do need a commit if someone can race between the
> existence check and the alter.

Since the existence check is no longer necessary (why was it necessary in other
methods, btw?), I've simplified the method to:

return db->Execute("ALTER TABLE ...");

There's no need for a transaction, because it's a single operation.

https://codereview.chromium.org/212873003/diff/220001/components/autofill/cor...
File components/autofill/core/browser/webdata/autofill_table.h (right):

https://codereview.chromium.org/212873003/diff/220001/components/autofill/cor...
components/autofill/core/browser/webdata/autofill_table.h:72: //                
     Added in version 56.
On 2014/03/31 17:46:38, shess wrote:
> Put the comment at the end if the code is at the end.  As a general thing, the
> ordering of these items in storage is really irrelevant, it's just an
> implementation detail.  But this comment appears to be ordered by physical
order
> currently, so maintain that.

Done.

https://codereview.chromium.org/212873003/diff/220001/components/webdata/comm...
File components/webdata/common/web_database_migration_unittest.cc (right):

https://codereview.chromium.org/212873003/diff/220001/components/webdata/comm...
components/webdata/common/web_database_migration_unittest.cc:2638:
EXPECT_EQ(base::string16(), s_profiles.ColumnString16(9));
On 2014/03/29 01:24:42, Ilya Sherman wrote:
> Is the datatype string16 or std::string?

std::string. Fixed.

[Scott Hess - ex-Googler, 2014-04-02 22:20:21]

https://codereview.chromium.org/212873003/diff/220001/components/autofill/cor...
File components/autofill/core/browser/webdata/autofill_table.cc (right):

https://codereview.chromium.org/212873003/diff/220001/components/autofill/cor...
components/autofill/core/browser/webdata/autofill_table.cc:2278: // the table
was newly created when migrating from a pre-version-23 database.
On 2014/04/02 21:54:52, Rouslan Solomakhin wrote:
> On 2014/03/31 17:46:38, shess wrote:
> > On 2014/03/29 01:24:42, Ilya Sherman wrote:
> > > This sort of thing should no longer be needed.  It should be safe to
reduce
> > this
> > > entire method to
> > > 
> > > return db_->Execute("ALTER TABLE ...");
> > 
> > I agree, no reason for a commit if there's only a single thread of control
> with
> > a single atomic change.  You do need a commit if someone can race between
the
> > existence check and the alter.
> 
> Since the existence check is no longer necessary (why was it necessary in
other
> methods, btw?), I've simplified the method to:
> 
> return db->Execute("ALTER TABLE ...");
> 
> There's no need for a transaction, because it's a single operation.

Unfortunately, Chromium's overall schema version management is kind of broken,
and the existence checks tend to be a wart stemming from that.  The specific
place where you can see failures is where version 1 doesn't have a table,
version 2 adds a table, and version 3 adds a column to that table.  In our
world, if you are currently at version 1, the code will first create a version-3
table, and then attempt to run the v1->v2 and v2->v3 upgrades.  The v2->v3
upgrade will fail because the table already has the column it's attempting to
add.

I have no idea if that is a possibility for this specific case.  But if the
column somehow does exist, it will fail the ALTER and the user will never
progress.  Since checking the existence is the style in this case, you probably
should keep the check.

[Ilya Sherman, 2014-04-02 23:25:44]

On 2014/04/02 22:20:21, shess wrote:
>
https://codereview.chromium.org/212873003/diff/220001/components/autofill/cor...
> File components/autofill/core/browser/webdata/autofill_table.cc (right):
> 
>
https://codereview.chromium.org/212873003/diff/220001/components/autofill/cor...
> components/autofill/core/browser/webdata/autofill_table.cc:2278: // the table
> was newly created when migrating from a pre-version-23 database.
> On 2014/04/02 21:54:52, Rouslan Solomakhin wrote:
> > On 2014/03/31 17:46:38, shess wrote:
> > > On 2014/03/29 01:24:42, Ilya Sherman wrote:
> > > > This sort of thing should no longer be needed.  It should be safe to
> reduce
> > > this
> > > > entire method to
> > > > 
> > > > return db_->Execute("ALTER TABLE ...");
> > > 
> > > I agree, no reason for a commit if there's only a single thread of control
> > with
> > > a single atomic change.  You do need a commit if someone can race between
> the
> > > existence check and the alter.
> > 
> > Since the existence check is no longer necessary (why was it necessary in
> other
> > methods, btw?), I've simplified the method to:
> > 
> > return db->Execute("ALTER TABLE ...");
> > 
> > There's no need for a transaction, because it's a single operation.
> 
> Unfortunately, Chromium's overall schema version management is kind of broken,
> and the existence checks tend to be a wart stemming from that.  The specific
> place where you can see failures is where version 1 doesn't have a table,
> version 2 adds a table, and version 3 adds a column to that table.  In our
> world, if you are currently at version 1, the code will first create a
version-3
> table, and then attempt to run the v1->v2 and v2->v3 upgrades.  The v2->v3
> upgrade will fail because the table already has the column it's attempting to
> add.
> 
> I have no idea if that is a possibility for this specific case.  But if the
> column somehow does exist, it will fail the ALTER and the user will never
> progress.  Since checking the existence is the style in this case, you
probably
> should keep the check.

This is no longer the case -- I fixed this in [
https://codereview.chromium.org/207643002 ].  The code no longer creates tables
ahead of migration steps, precisely so that these sorts of existence checks
aren't needed anymore.

[Ilya Sherman, 2014-04-02 23:48:11]

https://codereview.chromium.org/212873003/diff/250001/components/autofill/cor...
File components/autofill/core/browser/autofill_profile.cc (right):

https://codereview.chromium.org/212873003/diff/250001/components/autofill/cor...
components/autofill/core/browser/autofill_profile.cc:448: return
language_code().compare(profile.language_code());
Hmm, it's a little strange to include the language_code here but not the origin.
 Does it make sense to potentially have a single address stored with multiple
different language codes?

https://codereview.chromium.org/212873003/diff/250001/components/autofill/cor...
File
components/autofill/core/browser/webdata/autofill_profile_syncable_service.cc
(right):

https://codereview.chromium.org/212873003/diff/250001/components/autofill/cor...
components/autofill/core/browser/webdata/autofill_profile_syncable_service.cc:380:
if (specifics.has_address_home_language_code() &&
Hmm, I wonder whether we should similarly test specifics.has_origin() on line
319.  Could you please write a testcase for this, and possibly fix up that code,
if need be?

https://codereview.chromium.org/212873003/diff/250001/components/autofill/cor...
File
components/autofill/core/browser/webdata/autofill_profile_syncable_service_unittest.cc
(right):

https://codereview.chromium.org/212873003/diff/250001/components/autofill/cor...
components/autofill/core/browser/webdata/autofill_profile_syncable_service_unittest.cc:883:
// explicitly set.
Hmm, this seems wrong.  If a local profile has a language set, that *should*
generate a Sync event.  The language should propagate to all Sync clients.

What *shouldn't* generate a Sync change is if the local profile has an empty
language, whereas the Sync profile has *no* language.  I guess the
MergeDataEmptyOrigins test above would have been a more useful test to compare
to than the one I commented on.

https://codereview.chromium.org/212873003/diff/270001/components/autofill/cor...
File components/autofill/core/browser/webdata/autofill_table.h (right):

https://codereview.chromium.org/212873003/diff/270001/components/autofill/cor...
components/autofill/core/browser/webdata/autofill_table.h:76: //                
     Added in version 56.
Please add test coverage to autofill_table_unittest.cc

[Scott Hess - ex-Googler, 2014-04-03 18:00:47]

On 2014/04/02 23:25:44, Ilya Sherman wrote:
> This is no longer the case -- I fixed this in [
> https://codereview.chromium.org/207643002 ].  The code no longer creates
tables
> ahead of migration steps, precisely so that these sorts of existence checks
> aren't needed anymore.

Oh, this is good.  Having two code paths still makes me a bit nervous, but this
is better than before.

["Two code paths"?  AFAICT, there's no reason to have the migrate code path and
the fresh-create code path be distinct.  The fresh-create code path is in theory
faster, but in practice running the migrate code path on an empty database isn't
going to be notably slower for something that only happens once per profile.]

[Scott Hess - ex-Googler, 2014-04-03 18:03:08]

I think LGTM from the SQLite side.

[please use gerrit instead, 2014-04-08 21:33:31]

Ilya & Evan: PTAL Patch Set 5.

https://codereview.chromium.org/212873003/diff/250001/components/autofill/cor...
File components/autofill/core/browser/autofill_profile.cc (right):

https://codereview.chromium.org/212873003/diff/250001/components/autofill/cor...
components/autofill/core/browser/autofill_profile.cc:448: return
language_code().compare(profile.language_code());
On 2014/04/02 23:48:11, Ilya Sherman wrote:
> Hmm, it's a little strange to include the language_code here but not the
origin.
>  Does it make sense to potentially have a single address stored with multiple
> different language codes?

Removed.

https://codereview.chromium.org/212873003/diff/250001/components/autofill/cor...
File
components/autofill/core/browser/webdata/autofill_profile_syncable_service.cc
(right):

https://codereview.chromium.org/212873003/diff/250001/components/autofill/cor...
components/autofill/core/browser/webdata/autofill_profile_syncable_service.cc:380:
if (specifics.has_address_home_language_code() &&
On 2014/04/02 23:48:11, Ilya Sherman wrote:
> Hmm, I wonder whether we should similarly test specifics.has_origin() on line
> 319.  Could you please write a testcase for this, and possibly fix up that
code,
> if need be?

Done.

https://codereview.chromium.org/212873003/diff/250001/components/autofill/cor...
File
components/autofill/core/browser/webdata/autofill_profile_syncable_service_unittest.cc
(right):

https://codereview.chromium.org/212873003/diff/250001/components/autofill/cor...
components/autofill/core/browser/webdata/autofill_profile_syncable_service_unittest.cc:883:
// explicitly set.
On 2014/04/02 23:48:11, Ilya Sherman wrote:
> Hmm, this seems wrong.  If a local profile has a language set, that *should*
> generate a Sync event.  The language should propagate to all Sync clients.

From my understanding of how
AutofillProfileSyncableService::MergeDataAndStartSyncing() is implemented, the
method only sends new profiles to sync server. It does not send send updates of
existing profiles to the sync server. That functionality is in
AutofillProfileChanged(). I've added a test that AutofillProfileChanged()
updates the language code.

> What *shouldn't* generate a Sync change is if the local profile has an empty
> language, whereas the Sync profile has *no* language.  I guess the
> MergeDataEmptyOrigins test above would have been a more useful test to compare
> to than the one I commented on.

Done.

https://codereview.chromium.org/212873003/diff/270001/components/autofill/cor...
File components/autofill/core/browser/webdata/autofill_table.h (right):

https://codereview.chromium.org/212873003/diff/270001/components/autofill/cor...
components/autofill/core/browser/webdata/autofill_table.h:76: //                
     Added in version 56.
On 2014/04/02 23:48:11, Ilya Sherman wrote:
> Please add test coverage to autofill_table_unittest.cc

Done.

[please use gerrit instead, 2014-04-08 21:34:16]

On 2014/04/08 21:33:31, Rouslan Solomakhin wrote:
> Ilya & Evan: PTAL Patch Set 5.

The components/autofill/ and chrome/browser/ui/autofill parts.

[Ilya Sherman, 2014-04-08 22:55:14]

https://codereview.chromium.org/212873003/diff/250001/components/autofill/cor...
File
components/autofill/core/browser/webdata/autofill_profile_syncable_service_unittest.cc
(right):

https://codereview.chromium.org/212873003/diff/250001/components/autofill/cor...
components/autofill/core/browser/webdata/autofill_profile_syncable_service_unittest.cc:883:
// explicitly set.
On 2014/04/08 21:33:31, Rouslan Solomakhin wrote:
> On 2014/04/02 23:48:11, Ilya Sherman wrote:
> > Hmm, this seems wrong.  If a local profile has a language set, that *should*
> > generate a Sync event.  The language should propagate to all Sync clients.
> 
> From my understanding of how
> AutofillProfileSyncableService::MergeDataAndStartSyncing() is implemented, the
> method only sends new profiles to sync server. It does not send send updates
of
> existing profiles to the sync server. That functionality is in
> AutofillProfileChanged(). I've added a test that AutofillProfileChanged()
> updates the language code.

Uhm, this sounds really bad if true.  What happens if the client starts up and
has updates relative to what Sync has available?  Will these profiles just never
get updated?

[please use gerrit instead, 2014-04-08 23:00:59]

On 2014/04/08 22:55:14, Ilya Sherman wrote:
>
https://codereview.chromium.org/212873003/diff/250001/components/autofill/cor...
> File
>
components/autofill/core/browser/webdata/autofill_profile_syncable_service_unittest.cc
> (right):
> 
>
https://codereview.chromium.org/212873003/diff/250001/components/autofill/cor...
>
components/autofill/core/browser/webdata/autofill_profile_syncable_service_unittest.cc:883:
> // explicitly set.
> On 2014/04/08 21:33:31, Rouslan Solomakhin wrote:
> > On 2014/04/02 23:48:11, Ilya Sherman wrote:
> > > Hmm, this seems wrong.  If a local profile has a language set, that
*should*
> > > generate a Sync event.  The language should propagate to all Sync clients.
> > 
> > From my understanding of how
> > AutofillProfileSyncableService::MergeDataAndStartSyncing() is implemented,
the
> > method only sends new profiles to sync server. It does not send send updates
> of
> > existing profiles to the sync server. That functionality is in
> > AutofillProfileChanged(). I've added a test that AutofillProfileChanged()
> > updates the language code.
> 
> Uhm, this sounds really bad if true.  What happens if the client starts up and
> has updates relative to what Sync has available?  Will these profiles just
never
> get updated?

The offline changes to profiles (if preserved after MergeDataAndStartSyncing)
will be sent to the sync server via AutofillProfileChanged when the user
modifies the local profile.

[Ilya Sherman, 2014-04-08 23:08:19]

https://codereview.chromium.org/212873003/diff/310001/components/autofill/cor...
File
components/autofill/core/browser/webdata/autofill_profile_syncable_service.cc
(right):

https://codereview.chromium.org/212873003/diff/310001/components/autofill/cor...
components/autofill/core/browser/webdata/autofill_profile_syncable_service.cc:640:
merge_into->origin() != merge_from.origin());
Should this check the language code as well?  Perhaps operator==() is more
appropriate than Compare() here?  Or would that try to compare too much?

You'll also want to double-check the calls to Compare() in
AutofillTable::UpdateAutofillProfile() and PersonalDataManager::UpdateProfile().
 There is also an indirect reference via PersonalDataManager::SetProfiles()'s
call to FindByContents().  If anything is currently broken in any of these call
sites, but not currently being caught by tests, please add test coverage as
well.

(It's definitely awkward that we have both of these methods available for
comparison.  It would be great to design away this wart in the AutofillProfile
API.)

[Ilya Sherman, 2014-04-08 23:09:51]

On 2014/04/08 23:00:59, Rouslan Solomakhin wrote:
> On 2014/04/08 22:55:14, Ilya Sherman wrote:
> >
>
https://codereview.chromium.org/212873003/diff/250001/components/autofill/cor...
> > File
> >
>
components/autofill/core/browser/webdata/autofill_profile_syncable_service_unittest.cc
> > (right):
> > 
> >
>
https://codereview.chromium.org/212873003/diff/250001/components/autofill/cor...
> >
>
components/autofill/core/browser/webdata/autofill_profile_syncable_service_unittest.cc:883:
> > // explicitly set.
> > On 2014/04/08 21:33:31, Rouslan Solomakhin wrote:
> > > On 2014/04/02 23:48:11, Ilya Sherman wrote:
> > > > Hmm, this seems wrong.  If a local profile has a language set, that
> *should*
> > > > generate a Sync event.  The language should propagate to all Sync
clients.
> > > 
> > > From my understanding of how
> > > AutofillProfileSyncableService::MergeDataAndStartSyncing() is implemented,
> the
> > > method only sends new profiles to sync server. It does not send send
updates
> > of
> > > existing profiles to the sync server. That functionality is in
> > > AutofillProfileChanged(). I've added a test that AutofillProfileChanged()
> > > updates the language code.
> > 
> > Uhm, this sounds really bad if true.  What happens if the client starts up
and
> > has updates relative to what Sync has available?  Will these profiles just
> never
> > get updated?
> 
> The offline changes to profiles (if preserved after MergeDataAndStartSyncing)
> will be sent to the sync server via AutofillProfileChanged when the user
> modifies the local profile.

So, if the user never further modifies the local profile, the local state and
the Sync server state will just be out of sync?  Could you please file a bug to
investigate this?

[please use gerrit instead, 2014-04-09 18:18:11]

On 2014/04/08 23:09:51, Ilya Sherman wrote:
> On 2014/04/08 23:00:59, Rouslan Solomakhin wrote:
> > On 2014/04/08 22:55:14, Ilya Sherman wrote:
> > >
> >
>
https://codereview.chromium.org/212873003/diff/250001/components/autofill/cor...
> > > File
> > >
> >
>
components/autofill/core/browser/webdata/autofill_profile_syncable_service_unittest.cc
> > > (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/212873003/diff/250001/components/autofill/cor...
> > >
> >
>
components/autofill/core/browser/webdata/autofill_profile_syncable_service_unittest.cc:883:
> > > // explicitly set.
> > > On 2014/04/08 21:33:31, Rouslan Solomakhin wrote:
> > > > On 2014/04/02 23:48:11, Ilya Sherman wrote:
> > > > > Hmm, this seems wrong.  If a local profile has a language set, that
> > *should*
> > > > > generate a Sync event.  The language should propagate to all Sync
> clients.
> > > > 
> > > > From my understanding of how
> > > > AutofillProfileSyncableService::MergeDataAndStartSyncing() is
implemented,
> > the
> > > > method only sends new profiles to sync server. It does not send send
> updates
> > > of
> > > > existing profiles to the sync server. That functionality is in
> > > > AutofillProfileChanged(). I've added a test that
AutofillProfileChanged()
> > > > updates the language code.
> > > 
> > > Uhm, this sounds really bad if true.  What happens if the client starts up
> and
> > > has updates relative to what Sync has available?  Will these profiles just
> > never
> > > get updated?
> > 
> > The offline changes to profiles (if preserved after
MergeDataAndStartSyncing)
> > will be sent to the sync server via AutofillProfileChanged when the user
> > modifies the local profile.
> 
> So, if the user never further modifies the local profile, the local state and
> the Sync server state will just be out of sync?  Could you please file a bug
to
> investigate this?

Filed https://code.google.com/p/chromium/issues/detail?id=361688.

[Ilya Sherman, 2014-04-09 22:41:12]

Thanks for filing the bug.  Just wanted to make sure you saw my other message as
well -- it might have gotten buried among the others.

[please use gerrit instead, 2014-04-09 22:48:11]

On 2014/04/09 22:41:12, Ilya Sherman wrote:
> Thanks for filing the bug.  Just wanted to make sure you saw my other message
as
> well -- it might have gotten buried among the others.

Saw it, but it requires more work for a reply :-)

[please use gerrit instead, 2014-04-09 22:57:55]

Ilya: PTAL Patch Set 6.

https://codereview.chromium.org/212873003/diff/310001/components/autofill/cor...
File
components/autofill/core/browser/webdata/autofill_profile_syncable_service.cc
(right):

https://codereview.chromium.org/212873003/diff/310001/components/autofill/cor...
components/autofill/core/browser/webdata/autofill_profile_syncable_service.cc:640:
merge_into->origin() != merge_from.origin());
On 2014/04/08 23:08:20, Ilya Sherman wrote:
> Should this check the language code as well?  Perhaps operator==() is more
> appropriate than Compare() here?  Or would that try to compare too much?

AutofillProfile::OverwriteWithOrAddTo() unconditionally overwrites the origin of
|merge_into| with the origin of |merge_from|. It's not necessary to compare the
origin.

operator==() would also compare GUID, which OverwriteWithOrAddTo() does not
touch. Therefore, operator==() would not be correct here.

> You'll also want to double-check the calls to Compare() in
> AutofillTable::UpdateAutofillProfile() and
PersonalDataManager::UpdateProfile().
>  There is also an indirect reference via PersonalDataManager::SetProfiles()'s
> call to FindByContents().

FindByContents() is correct. It uses Compare() method, which compares only
contents. In general, FindByContents() is always preceded by a comment "Don't
add a duplicate." We don't want to have multiple copies of an address with
different formatting language codes. Therefore, we should not change
FindByContents().

> If anything is currently broken in any of these call
> sites, but not currently being caught by tests, please add test coverage as
> well.

Done.

> (It's definitely awkward that we have both of these methods available for
> comparison.  It would be great to design away this wart in the AutofillProfile
> API.)

I've added EqualsSansGuid(), EqualsSansOrigin(), and EqualsSansGuidAndOrigin().
I think that these method names should clarify exact intention of the code.

[Ilya Sherman, 2014-04-09 23:17:48]

https://codereview.chromium.org/212873003/diff/310001/components/autofill/cor...
File
components/autofill/core/browser/webdata/autofill_profile_syncable_service.cc
(right):

https://codereview.chromium.org/212873003/diff/310001/components/autofill/cor...
components/autofill/core/browser/webdata/autofill_profile_syncable_service.cc:640:
merge_into->origin() != merge_from.origin());
On 2014/04/09 22:57:56, Rouslan Solomakhin wrote:
> On 2014/04/08 23:08:20, Ilya Sherman wrote:
> > Should this check the language code as well?  Perhaps operator==() is more
> > appropriate than Compare() here?  Or would that try to compare too much?
> 
> AutofillProfile::OverwriteWithOrAddTo() unconditionally overwrites the origin
of
> |merge_into| with the origin of |merge_from|. It's not necessary to compare
the
> origin.

Good call.

> operator==() would also compare GUID, which OverwriteWithOrAddTo() does not
> touch. Therefore, operator==() would not be correct here.
> 
> > You'll also want to double-check the calls to Compare() in
> > AutofillTable::UpdateAutofillProfile() and
> PersonalDataManager::UpdateProfile().
> >  There is also an indirect reference via
PersonalDataManager::SetProfiles()'s
> > call to FindByContents().
> 
> FindByContents() is correct. It uses Compare() method, which compares only
> contents. In general, FindByContents() is always preceded by a comment "Don't
> add a duplicate." We don't want to have multiple copies of an address with
> different formatting language codes. Therefore, we should not change
> FindByContents().
> 
> > If anything is currently broken in any of these call
> > sites, but not currently being caught by tests, please add test coverage as
> > well.
> 
> Done.
> 
> > (It's definitely awkward that we have both of these methods available for
> > comparison.  It would be great to design away this wart in the
AutofillProfile
> > API.)
> 
> I've added EqualsSansGuid(), EqualsSansOrigin(), and
EqualsSansGuidAndOrigin().
> I think that these method names should clarify exact intention of the code. 

Thanks.

https://codereview.chromium.org/212873003/diff/350001/components/autofill/cor...
File components/autofill/core/browser/autofill_profile.cc (right):

https://codereview.chromium.org/212873003/diff/350001/components/autofill/cor...
components/autofill/core/browser/autofill_profile.cc:459: Compare(profile) == 0;
nit: Perhaps call EqualsSansGuidAndOrigin() from both of these methods?

https://codereview.chromium.org/212873003/diff/350001/components/autofill/cor...
components/autofill/core/browser/autofill_profile.cc:472: Compare(profile) == 0;
nit: Perhaps call one of EqualsSans...() methods here?

https://codereview.chromium.org/212873003/diff/350001/components/autofill/cor...
File
components/autofill/core/browser/webdata/autofill_profile_syncable_service.cc
(right):

https://codereview.chromium.org/212873003/diff/350001/components/autofill/cor...
components/autofill/core/browser/webdata/autofill_profile_syncable_service.cc:504:
if (local_profile->EqualsSansGuidAndOrigin(*new_profile)) {
Hmm, I think this probably should still be Compare -- otherwise, can't we get
duplicates if the profiles differ only in their language code?

[please use gerrit instead, 2014-04-09 23:43:57]

Ilya: PTAL Patch Set 7.

https://codereview.chromium.org/212873003/diff/350001/components/autofill/cor...
File components/autofill/core/browser/autofill_profile.cc (right):

https://codereview.chromium.org/212873003/diff/350001/components/autofill/cor...
components/autofill/core/browser/autofill_profile.cc:459: Compare(profile) == 0;
On 2014/04/09 23:17:49, Ilya Sherman wrote:
> nit: Perhaps call EqualsSansGuidAndOrigin() from both of these methods?

EqualsSansGuidAndOrigin() is removed, as it's no longer used anywhere.

https://codereview.chromium.org/212873003/diff/350001/components/autofill/cor...
components/autofill/core/browser/autofill_profile.cc:472: Compare(profile) == 0;
On 2014/04/09 23:17:49, Ilya Sherman wrote:
> nit: Perhaps call one of EqualsSans...() methods here?

Calling EqualsSansGuid().

https://codereview.chromium.org/212873003/diff/350001/components/autofill/cor...
File
components/autofill/core/browser/webdata/autofill_profile_syncable_service.cc
(right):

https://codereview.chromium.org/212873003/diff/350001/components/autofill/cor...
components/autofill/core/browser/webdata/autofill_profile_syncable_service.cc:504:
if (local_profile->EqualsSansGuidAndOrigin(*new_profile)) {
On 2014/04/09 23:17:49, Ilya Sherman wrote:
> Hmm, I think this probably should still be Compare -- otherwise, can't we get
> duplicates if the profiles differ only in their language code?

True. We wouldn't want multiple profiles for the same address, but with
different language code. Changed back to Compare() for content-only comparison.

[Ilya Sherman, 2014-04-09 23:58:30]

LGTM.  Thanks, Rouslan!

https://codereview.chromium.org/212873003/diff/370001/components/autofill/cor...
File components/autofill/core/browser/autofill_profile.cc (right):

https://codereview.chromium.org/212873003/diff/370001/components/autofill/cor...
components/autofill/core/browser/autofill_profile.cc:464:
EqualsSansGuid(profile);
nit: Looks like this fits on a single line now?

[please use gerrit instead, 2014-04-10 00:14:22]

Evan: I have enough ell-gees to send to CQ now. Would you like to skim over the
patch?

https://codereview.chromium.org/212873003/diff/370001/components/autofill/cor...
File components/autofill/core/browser/autofill_profile.cc (right):

https://codereview.chromium.org/212873003/diff/370001/components/autofill/cor...
components/autofill/core/browser/autofill_profile.cc:464:
EqualsSansGuid(profile);
On 2014/04/09 23:58:31, Ilya Sherman wrote:
> nit: Looks like this fits on a single line now?

Done.

[Evan Stade, 2014-04-10 00:58:48]

https://codereview.chromium.org/212873003/diff/390001/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right):

https://codereview.chromium.org/212873003/diff/390001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:3631:
AddressLanguageCodeForSection(SECTION_SHIPPING));
use shipping_langauge_code_ directly

https://codereview.chromium.org/212873003/diff/390001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:3953:
AddressLanguageCodeForSection(SECTION_BILLING));
ditto

https://codereview.chromium.org/212873003/diff/390001/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/autofill_dialog_i18n_input.cc (right):

https://codereview.chromium.org/212873003/diff/390001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_dialog_i18n_input.cc:42: std::string*
languge_code) {
nit: spelling

https://codereview.chromium.org/212873003/diff/390001/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/autofill_dialog_i18n_input.h (right):

https://codereview.chromium.org/212873003/diff/390001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_dialog_i18n_input.h:41: std::string*
language_code);
it's unclear to me from your above comment whether this is supposed to be an
outparam or inparam

[please use gerrit instead, 2014-04-10 20:39:44]

Evan: PTAL Patch Set 9.

https://codereview.chromium.org/212873003/diff/390001/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right):

https://codereview.chromium.org/212873003/diff/390001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:3631:
AddressLanguageCodeForSection(SECTION_SHIPPING));
On 2014/04/10 00:58:49, Evan Stade wrote:
> use shipping_langauge_code_ directly

Good point.

https://codereview.chromium.org/212873003/diff/390001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:3953:
AddressLanguageCodeForSection(SECTION_BILLING));
On 2014/04/10 00:58:49, Evan Stade wrote:
> ditto

Done.

https://codereview.chromium.org/212873003/diff/390001/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/autofill_dialog_i18n_input.cc (right):

https://codereview.chromium.org/212873003/diff/390001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_dialog_i18n_input.cc:42: std::string*
languge_code) {
On 2014/04/10 00:58:49, Evan Stade wrote:
> nit: spelling

thenk you.

https://codereview.chromium.org/212873003/diff/390001/chrome/browser/ui/autof...
File chrome/browser/ui/autofill/autofill_dialog_i18n_input.h (right):

https://codereview.chromium.org/212873003/diff/390001/chrome/browser/ui/autof...
chrome/browser/ui/autofill/autofill_dialog_i18n_input.h:41: std::string*
language_code);
On 2014/04/10 00:58:49, Evan Stade wrote:
> it's unclear to me from your above comment whether this is supposed to be an
> outparam or inparam

It's an out-param. I've changed the comment to clarify this point.

[Evan Stade, 2014-04-10 21:21:18]

lgtm

[please use gerrit instead, 2014-04-10 21:22:09]

The CQ bit was checked by rouslan@chromium.org

[commit-bot: I haz the power, 2014-04-10 21:23:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rouslan@chromium.org/212873003/410001

[commit-bot: I haz the power, 2014-04-11 01:10:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-11 01:10:21]

Retried try job too often on win_rel for step(s) nacl_integration
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[please use gerrit instead, 2014-04-11 16:58:14]

The CQ bit was checked by rouslan@chromium.org

[commit-bot: I haz the power, 2014-04-11 16:59:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rouslan@chromium.org/212873003/410001

[commit-bot: I haz the power, 2014-04-11 19:35:02]

Change committed as 263311