Add billing address to credit cards.

Add billing address to credit cards.

Local credit cards now can have local profiles as their billing
addresses. This patch changes the autofill table version from 65 to 66.
The user can change the billing address in the Android credit card
settings UI. The billing address is necessary for PaymentRequest.

BUG=602663
Committed: https://crrev.com/7c7c0eb2a4166ab67530c1aa3ae3a465b9f1125b
Cr-Commit-Position: refs/heads/master@{#401619}

[please use gerrit instead, 2016-06-22 18:08:04]

rouslan@chromium.org changed reviewers:
+ dfalcantara@chromium.org, mathp@chromium.org

[please use gerrit instead, 2016-06-22 18:08:04]

dfalcantara@, ptal patch 1.

Video in https://drive.google.com/open?id=0BwnAxYoFTeYPUkJuOThBa1IwR1k

Do you think we should polish the UI in this patch or a follow up?

----------------------

mathp@, ptal patch 1.

Did you have a personal_data_manager unit test in mind for this feature?

[please use gerrit instead, 2016-06-22 18:41:15]

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

[please use gerrit instead, 2016-06-22 18:41:16]

shess@, ptal components/webdata/ in patch 2.

[Mathieu, 2016-06-22 18:58:36]

mathp@chromium.org changed reviewers:
- shess@chromium.org

[Mathieu, 2016-06-22 18:58:37]

chrome/browser/autofill and components/autofill stuff all looks good to me, lgtm

[please use gerrit instead, 2016-06-22 19:15:09]

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

[please use gerrit instead, 2016-06-22 19:15:09]

shess@, ptal patch 2 components/webdata/

[Scott Hess - ex-Googler, 2016-06-22 21:52:16]

lgtm.

WRT nits, please remove the transaction indicated, change to Execute() where
indicated.

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

https://codereview.chromium.org/2085133003/diff/40001/components/autofill/cor...
components/autofill/core/browser/webdata/autofill_table.cc:1452:
s.BindString(10, credit_card.guid());
You may wish to experiment with writing "WHERE guid=?1" to tell SQLite that the
last parameter should match the first parameter.  Then you could drop this bind.

https://codereview.chromium.org/2085133003/diff/40001/components/autofill/cor...
components/autofill/core/browser/webdata/autofill_table.cc:1690:
"billing_address_id VARCHAR) ")) {
The default for this column will be NULL.  Connection::ColumnString() will
return empty string for that, so I think you're fine, just wanted to make
certain it's intentionally right not accidentally right :-).

https://codereview.chromium.org/2085133003/diff/40001/components/autofill/cor...
components/autofill/core/browser/webdata/autofill_table.cc:2281: return false;
No need for a transaction for a single-statement sequence.  Note that if the
transaction is removed, then the result of Execute can be returned directly.

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

https://codereview.chromium.org/2085133003/diff/40001/components/webdata/comm...
components/webdata/common/web_database_migration_unittest.cc:1007: .Run();
Maybe connection.Execute() instead?

[please use gerrit instead, 2016-06-23 01:26:58]

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

https://codereview.chromium.org/2085133003/diff/40001/components/autofill/cor...
components/autofill/core/browser/webdata/autofill_table.cc:1452:
s.BindString(10, credit_card.guid());
On 2016/06/22 21:52:15, Scott Hess wrote:
> You may wish to experiment with writing "WHERE guid=?1" to tell SQLite that
the
> last parameter should match the first parameter.  Then you could drop this
bind.

Done.

https://codereview.chromium.org/2085133003/diff/40001/components/autofill/cor...
components/autofill/core/browser/webdata/autofill_table.cc:1690:
"billing_address_id VARCHAR) ")) {
On 2016/06/22 21:52:16, Scott Hess wrote:
> The default for this column will be NULL.  Connection::ColumnString() will
> return empty string for that, so I think you're fine, just wanted to make
> certain it's intentionally right not accidentally right :-).

We're fine, but I've added several of comments to make this clear.

https://codereview.chromium.org/2085133003/diff/40001/components/autofill/cor...
components/autofill/core/browser/webdata/autofill_table.cc:2281: return false;
On 2016/06/22 21:52:16, Scott Hess wrote:
> No need for a transaction for a single-statement sequence.  Note that if the
> transaction is removed, then the result of Execute can be returned directly.

Done.

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

https://codereview.chromium.org/2085133003/diff/40001/components/webdata/comm...
components/webdata/common/web_database_migration_unittest.cc:1007: .Run();
On 2016/06/22 21:52:16, Scott Hess wrote:
> Maybe connection.Execute() instead?

Done.

[gone, 2016-06-23 15:56:27]

Would be nice if the spinny were a little bit more evenly padded, but I can take
it on my end.

android stuff lgtm

https://chromiumcodereview.appspot.com/2085133003/diff/60001/chrome/android/j...
File chrome/android/java/res/layout/autofill_credit_card_editor.xml (right):

https://chromiumcodereview.appspot.com/2085133003/diff/60001/chrome/android/j...
chrome/android/java/res/layout/autofill_credit_card_editor.xml:105:
android:paddingTop="@dimen/pref_autofill_field_top_padding"
Any reason why there isn't a bottom padding here?

[please use gerrit instead, 2016-06-23 16:00:44]

Sending to cq.

https://chromiumcodereview.appspot.com/2085133003/diff/60001/chrome/android/j...
File chrome/android/java/res/layout/autofill_credit_card_editor.xml (right):

https://chromiumcodereview.appspot.com/2085133003/diff/60001/chrome/android/j...
chrome/android/java/res/layout/autofill_credit_card_editor.xml:105:
android:paddingTop="@dimen/pref_autofill_field_top_padding"
On 2016/06/23 15:56:27, dfalcantara wrote:
> Any reason why there isn't a bottom padding here?

Copy-pasta from above, that's all.

[please use gerrit instead, 2016-06-23 16:01:29]

The CQ bit was checked by rouslan@chromium.org

[please use gerrit instead, 2016-06-23 16:01:29]

The patchset sent to the CQ was uploaded after l-g-t-m from mathp@chromium.org,
shess@chromium.org
Link to the patchset: https://chromiumcodereview.appspot.com/2085133003/#ps60001
(title: "Addressed comments")

[commit-bot: I haz the power, 2016-06-23 16:01:40]

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

[commit-bot: I haz the power, 2016-06-23 16:06:34]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-06-23 16:09:19]

Description was changed from

==========
Add billing address to credit cards.

Local credit cards now can have local profiles as their billing
addresses. This patch changes the autofill table version from 65 to 66.
The user can change the billing address in the Android credit card
settings UI. The billing address is necessary for PaymentRequest.

BUG=602663
==========

to

==========
Add billing address to credit cards.

Local credit cards now can have local profiles as their billing
addresses. This patch changes the autofill table version from 65 to 66.
The user can change the billing address in the Android credit card
settings UI. The billing address is necessary for PaymentRequest.

BUG=602663

Committed: https://crrev.com/7c7c0eb2a4166ab67530c1aa3ae3a465b9f1125b
Cr-Commit-Position: refs/heads/master@{#401619}
==========

[commit-bot: I haz the power, 2016-06-23 16:09:20]

Patchset 4 (id:??) landed as
https://crrev.com/7c7c0eb2a4166ab67530c1aa3ae3a465b9f1125b
Cr-Commit-Position: refs/heads/master@{#401619}