Avoiding Vectors usage while setting info in AutofillProfile.

Avoiding Vectors usage while setting info in AutofillProfile.

As we are setting the first value that are getting fetched.
So no need of the vectors, we can directly set the info from the query coloums and set "LIMIT 1" to get only one row of data from sql query.

BUG=503055
Committed: https://crrev.com/38ab25d28eb5cda9b5e9398ff67b2b4c0e74087a
Cr-Commit-Position: refs/heads/master@{#336373}

[Deepak, 2015-06-22 09:23:39]

deepak.m1@samsung.com changed reviewers:
+ ajith.v@chromium.org

[Deepak, 2015-06-22 09:23:40]

PTAL for peer review.

[AKV, 2015-06-22 10:00:38]

https://codereview.chromium.org/1198013002/diff/1/components/autofill/core/br...
File components/autofill/core/browser/webdata/autofill_table.cc (right):

https://codereview.chromium.org/1198013002/diff/1/components/autofill/core/br...
components/autofill/core/browser/webdata/autofill_table.cc:197: "WHERE
guid=?"));
Can we restrict the retrieval limit by 1 instead of getting all rows. So only
one entry will be there in result set.

https://codereview.chromium.org/1198013002/diff/1/components/autofill/core/br...
components/autofill/core/browser/webdata/autofill_table.cc:208:
profile->SetRawInfo(NAME_FULL, s.ColumnString16(4));
s.Succeded() needs to be checked whether Query forced SQL table in a valid
state.

[Deepak, 2015-06-22 11:31:51]

Changes done as suggested.
PTAL

https://codereview.chromium.org/1198013002/diff/1/components/autofill/core/br...
File components/autofill/core/browser/webdata/autofill_table.cc (right):

https://codereview.chromium.org/1198013002/diff/1/components/autofill/core/br...
components/autofill/core/browser/webdata/autofill_table.cc:197: "WHERE
guid=?"));
On 2015/06/22 10:00:38, AKV wrote:
> Can we restrict the retrieval limit by 1 instead of getting all rows. So only
> one entry will be there in result set.

Done.

https://codereview.chromium.org/1198013002/diff/1/components/autofill/core/br...
components/autofill/core/browser/webdata/autofill_table.cc:208:
profile->SetRawInfo(NAME_FULL, s.ColumnString16(4));
On 2015/06/22 10:00:38, AKV wrote:
> s.Succeded() needs to be checked whether Query forced SQL table in a valid
> state.

Done.

[AKV, 2015-06-22 11:44:35]

peer review looks good to me!

[Deepak, 2015-06-22 11:47:55]

deepak.m1@samsung.com changed reviewers:
+ estade@chromium.org, vabr@chromium.org

[Deepak, 2015-06-22 11:47:55]

PTAL

[Deepak, 2015-06-23 05:27:35]

deepak.m1@samsung.com changed reviewers:
+ gcasto@chromium.org

[Deepak, 2015-06-23 05:27:36]

@Garrett
Please review.
Thanks

[Garrett Casto, 2015-06-23 16:49:14]

gcasto@chromium.org changed reviewers:
+ isherman@chromium.org
- estade@chromium.org, vabr@chromium.org

[Garrett Casto, 2015-06-23 16:49:15]

If you look at the OWNERS file, I only own password manager code in
components/autofill. Re-routing to Ilya.

[Ilya Sherman, 2015-06-23 21:23:59]

isherman@chromium.org changed reviewers:
+ estade@chromium.org

[Ilya Sherman, 2015-06-23 21:23:59]

Thanks, but this is not the intention of the TODO.  These fields used to be
lists, but are now singletons; so what's desired is a database schema update.

[Deepak, 2015-06-24 07:36:18]

On 2015/06/23 21:23:59, Ilya Sherman wrote:
> Thanks, but this is not the intention of the TODO.  These fields used to be
> lists, but are now singletons; so what's desired is a database schema update.

ok.
I think now their will be only one name_first,name_middle,name_last,name_full
will be there.
and similar to that will be one email_address and phone_home_whole_number.

so I tried by changing 'repeated string' to 'optional string' for email_address
for
understanding in autofill_specifies.proto file.

But then I am  getting some build errors like add_email_address() no present for
specifies in 
autofill_profile_syncable_service.cc 

I think tables schema is fine, but the way data is inserted and retrieved need
changes.
please correct me if I misunderstand something.

[Deepak, 2015-06-24 10:23:00]

ok.
I think now their will be only one name_first,name_middle,name_last,name_full
will be there, as you mentioned that these are singleton now.
and similar to that will be one email_address and phone_home_whole_number.

1) I tried by changing 'repeated string' to 'optional string' for email_address
for understanding in autofill_specifics.proto file.
But then I am  getting some build errors like add_email_address() no present for
specifies in autofill_profile_syncable_service.cc 

I saw table schema for email in InitProfileEmailsTable() and appears fine to me.
but the way data is inserted and retrieved need changes.

2) How generation of add_email_address() and other depends upon the datatype ?

please correct me if I misunderstand something.

[Ilya Sherman, 2015-06-24 23:55:49]

We currently have four tables: autofill_profiles, autofill_profile_names,
autofill_profile_emails, and autofill_profile_phones.  These should be combined
into a single table; there is no longer a need to associate multiple names or
emails or phone numbers per unique profile GUID.

[Ilya Sherman, 2015-06-24 23:57:27]

On 2015/06/24 23:55:49, Ilya Sherman wrote:
> We currently have four tables: autofill_profiles, autofill_profile_names,
> autofill_profile_emails, and autofill_profile_phones.  These should be
combined
> into a single table; there is no longer a need to associate multiple names or
> emails or phone numbers per unique profile GUID.

FWIW, I suspect that merging the tables back into a single table will involve a
fairly large number of changes to the code, and very careful attention to
testing.  If you wish to pick up this work, I would not recommend thinking of it
as a quick fix.

[Deepak, 2015-06-25 02:47:56]

On 2015/06/24 23:57:27, Ilya Sherman wrote:
> On 2015/06/24 23:55:49, Ilya Sherman wrote:
> > We currently have four tables: autofill_profiles, autofill_profile_names,
> > autofill_profile_emails, and autofill_profile_phones.  These should be
> combined
> > into a single table; there is no longer a need to associate multiple names
or
> > emails or phone numbers per unique profile GUID.
> 
> FWIW, I suspect that merging the tables back into a single table will involve
a
> fairly large number of changes to the code, and very careful attention to
> testing.  If you wish to pick up this work, I would not recommend thinking of
it
> as a quick fix.

@llya
Thanks for reply.
How about improving the current code by doing clean up like changes in this CL
and similar to this in autofill_profile_syncable_service.cc ?

[Ilya Sherman, 2015-06-25 03:09:52]

On 2015/06/25 02:47:56, Deepak wrote:
> On 2015/06/24 23:57:27, Ilya Sherman wrote:
> > On 2015/06/24 23:55:49, Ilya Sherman wrote:
> > > We currently have four tables: autofill_profiles, autofill_profile_names,
> > > autofill_profile_emails, and autofill_profile_phones.  These should be
> > combined
> > > into a single table; there is no longer a need to associate multiple names
> or
> > > emails or phone numbers per unique profile GUID.
> > 
> > FWIW, I suspect that merging the tables back into a single table will
involve
> a
> > fairly large number of changes to the code, and very careful attention to
> > testing.  If you wish to pick up this work, I would not recommend thinking
of
> it
> > as a quick fix.
> 
> @llya
> Thanks for reply.
> How about improving the current code by doing clean up like changes in this CL
> and similar to this in autofill_profile_syncable_service.cc ?

I'll defer to Evan's preference on that -- he should be back tomorrow, and might
need a day or two to catch up on email backlog.  I do think that at least the
TODO should be preserved, since the schema change is still desired.

[Deepak, 2015-06-25 04:03:44]

On 2015/06/25 03:09:52, Ilya Sherman wrote:
> On 2015/06/25 02:47:56, Deepak wrote:
> > On 2015/06/24 23:57:27, Ilya Sherman wrote:
> > > On 2015/06/24 23:55:49, Ilya Sherman wrote:
> > > > We currently have four tables: autofill_profiles,
autofill_profile_names,
> > > > autofill_profile_emails, and autofill_profile_phones.  These should be
> > > combined
> > > > into a single table; there is no longer a need to associate multiple
names
> > or
> > > > emails or phone numbers per unique profile GUID.
> > > 
> > > FWIW, I suspect that merging the tables back into a single table will
> involve
> > a
> > > fairly large number of changes to the code, and very careful attention to
> > > testing.  If you wish to pick up this work, I would not recommend thinking
> of
> > it
> > > as a quick fix.
> > 
> > @llya
> > Thanks for reply.
> > How about improving the current code by doing clean up like changes in this
CL
> > and similar to this in autofill_profile_syncable_service.cc ?
> 
> I'll defer to Evan's preference on that -- he should be back tomorrow, and
might
> need a day or two to catch up on email backlog.  I do think that at least the
> TODO should be preserved, since the schema change is still desired.

I agree with you regarding preserving TODO.
and would like to know Evan's opinion.

[Evan Stade, 2015-06-25 18:02:16]

https://codereview.chromium.org/1198013002/diff/40001/components/autofill/cor...
File components/autofill/core/browser/webdata/autofill_table.cc (left):

https://codereview.chromium.org/1198013002/diff/40001/components/autofill/cor...
components/autofill/core/browser/webdata/autofill_table.cc:217: // TODO(estade):
update schema so these aren't vectors.
this change overall is ok, but you haven't really fixed this TODO --- the schema
haven't been updated. Please restore some version of this comment and add a link
to crbug.com/497934

[Deepak, 2015-06-26 09:13:22]

@Evan
Thanks for review.
Changes done as suggested.
PTAL.

https://codereview.chromium.org/1198013002/diff/40001/components/autofill/cor...
File components/autofill/core/browser/webdata/autofill_table.cc (left):

https://codereview.chromium.org/1198013002/diff/40001/components/autofill/cor...
components/autofill/core/browser/webdata/autofill_table.cc:217: // TODO(estade):
update schema so these aren't vectors.
On 2015/06/25 18:02:16, Evan Stade wrote:
> this change overall is ok, but you haven't really fixed this TODO --- the
schema
> haven't been updated. Please restore some version of this comment and add a
link
> to crbug.com/497934

Done.

[Evan Stade, 2015-06-26 14:31:15]

lgtm with nit fixed

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

https://codereview.chromium.org/1198013002/diff/60001/components/autofill/cor...
components/autofill/core/browser/webdata/autofill_table.cc:205: // unique
profile guid. Please refer https://crbug.com/497934.
nit, move comment to above statement that has LIMIT 1, or to where the schema is
defined.

[Deepak, 2015-06-26 14:40:43]

Thanks for review.
nit addressed.

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

https://codereview.chromium.org/1198013002/diff/60001/components/autofill/cor...
components/autofill/core/browser/webdata/autofill_table.cc:205: // unique
profile guid. Please refer https://crbug.com/497934.
On 2015/06/26 14:31:14, Evan Stade wrote:
> nit, move comment to above statement that has LIMIT 1, or to where the schema
is
> defined.

Done.

[Deepak, 2015-06-26 14:40:52]

The CQ bit was checked by deepak.m1@samsung.com

[Deepak, 2015-06-26 14:40:53]

The patchset sent to the CQ was uploaded after l-g-t-m from estade@chromium.org
Link to the patchset: https://codereview.chromium.org/1198013002/#ps80001
(title: "Addressing nit.")

[commit-bot: I haz the power, 2015-06-26 14:41:30]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1198013002/80001

[commit-bot: I haz the power, 2015-06-26 15:31:22]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-06-26 15:32:19]

Patchset 5 (id:??) landed as
https://crrev.com/38ab25d28eb5cda9b5e9398ff67b2b4c0e74087a
Cr-Commit-Position: refs/heads/master@{#336373}

[Scott Hess - ex-Googler, 2015-08-12 21:31:29]

shess@chromium.org changed reviewers:
+ shess@chromium.org

[Scott Hess - ex-Googler, 2015-08-12 21:31:30]

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

https://codereview.chromium.org/1198013002/diff/80001/components/autofill/cor...
components/autofill/core/browser/webdata/autofill_table.cc:200: "LIMIT 1"));
Note that if you have multiple items with the same GUID, without an ORDER BY
clause there is no guarantee that the first result will be the same each time. 
In practice a given release of SQLite is likely to return results in the same
implementation-defined order from run to run.  This can lead to problems like
bug 412968, where a new SQLite implementation did not return things in the same
order as the old one (in fact, in some cases the GUID meant that the order was
not stable between runs of the tests).

Since the set of items matching GUID is probably tiny, I'd probably go with
ORDER BY 2, 3, 4, 5 or something of the sort, just to make it the same each
time.  If the guid is indexed, the performance difference will probably not be
measurable.

Of course, this isn't really a problem with this CL, it was present in the
earlier code.  It's more of a hygiene consideration.