[Payments] Fix new billing address label.

[Payments] Fix new billing address label.

BUG=666048
Committed: https://crrev.com/cc022347d5d434c2caddfa24568b772d66081dd1
Cr-Commit-Position: refs/heads/master@{#433933}

[sebsg, 2016-11-18 21:28:45]

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

[sebsg, 2016-11-18 21:28:46]

Hi Rouslan, PTAL?

[please use gerrit instead, 2016-11-18 21:56:22]

LGTM after comments are addressed.

https://codereview.chromium.org/2515853002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java
(right):

https://codereview.chromium.org/2515853002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java:220:
// Calculate the label for this profile. The label's format depends on the
country and
Please remove the comment about the label.

https://codereview.chromium.org/2515853002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java:225:
profile.setGUID(pdm.setProfile(profile));
Since "pdm" is now used only once, you can inline it for ease of readability.

profile.setGUID(PersonalDataManager.getInstance().setProfile(profile));

https://codereview.chromium.org/2515853002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/CardEditor.java
(right):

https://codereview.chromium.org/2515853002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/payments/CardEditor.java:338:
}
This is where we need to add a new address if no match was found. :-)

(Separate CL probably would result in a quicker fix.)

https://codereview.chromium.org/2515853002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/payments/CardEditor.java:559:
billingAddress.completeAddress(billingAddress.getProfile());
There seems to be no need to call "completeAddress" here, is there?

https://codereview.chromium.org/2515853002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java
(right):

https://codereview.chromium.org/2515853002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:800:
completeAddress.completeAddress(completeAddress.getProfile());
I don't see the need for calling "completeAddress" here. If there's need, then
please explain.

https://codereview.chromium.org/2515853002/diff/1/chrome/browser/autofill/and...
File chrome/browser/autofill/android/personal_data_manager_android.cc (right):

https://codereview.chromium.org/2515853002/diff/1/chrome/browser/autofill/and...
chrome/browser/autofill/android/personal_data_manager_android.cc:425:
label_fields.push_back(ADDRESS_HOME_COUNTRY);
Add a comment about missing NAME_FULL. The reader of the .cpp file should be
aware the it's removed intentionally. Otherwise they might add it back by
mistake.

https://codereview.chromium.org/2515853002/diff/1/chrome/browser/autofill/and...
chrome/browser/autofill/android/personal_data_manager_android.cc:449:
label_fields.push_back(ADDRESS_HOME_SORTING_CODE);
Add a comment about missing company_name and addres_home_country. The reader of
the .cpp should be aware that these are removed intentionally. Otherwise they
might add it back by mistake.

[sebsg, 2016-11-21 16:07:08]

sebsg@chromium.org changed reviewers:
+ mathp@chromium.org

[sebsg, 2016-11-21 16:07:09]

Hi Math, could you please review the two personal_data_manager_android files?
Thanks!

https://codereview.chromium.org/2515853002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java
(right):

https://codereview.chromium.org/2515853002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java:220:
// Calculate the label for this profile. The label's format depends on the
country and
On 2016/11/18 21:56:22, rouslan wrote:
> Please remove the comment about the label.

Done.

https://codereview.chromium.org/2515853002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/payments/AddressEditor.java:225:
profile.setGUID(pdm.setProfile(profile));
On 2016/11/18 21:56:22, rouslan wrote:
> Since "pdm" is now used only once, you can inline it for ease of readability.
> 
> profile.setGUID(PersonalDataManager.getInstance().setProfile(profile));

Done.

https://codereview.chromium.org/2515853002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/CardEditor.java
(right):

https://codereview.chromium.org/2515853002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/payments/CardEditor.java:338:
}
On 2016/11/18 21:56:22, rouslan wrote:
> This is where we need to add a new address if no match was found. :-)
> 
> (Separate CL probably would result in a quicker fix.)

Acknowledged.

https://codereview.chromium.org/2515853002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/payments/CardEditor.java:559:
billingAddress.completeAddress(billingAddress.getProfile());
On 2016/11/18 21:56:22, rouslan wrote:
> There seems to be no need to call "completeAddress" here, is there?

We have to re-assign the label for the AutofillAddress. But I was doing
unnecessary stuff in completeAddress. Created a new function instead.

https://codereview.chromium.org/2515853002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java
(right):

https://codereview.chromium.org/2515853002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:800:
completeAddress.completeAddress(completeAddress.getProfile());
On 2016/11/18 21:56:22, rouslan wrote:
> I don't see the need for calling "completeAddress" here. If there's need, then
> please explain.

Same as other explanation :)

https://codereview.chromium.org/2515853002/diff/1/chrome/browser/autofill/and...
File chrome/browser/autofill/android/personal_data_manager_android.cc (right):

https://codereview.chromium.org/2515853002/diff/1/chrome/browser/autofill/and...
chrome/browser/autofill/android/personal_data_manager_android.cc:425:
label_fields.push_back(ADDRESS_HOME_COUNTRY);
On 2016/11/18 21:56:22, rouslan wrote:
> Add a comment about missing NAME_FULL. The reader of the .cpp file should be
> aware the it's removed intentionally. Otherwise they might add it back by
> mistake.

Done.

https://codereview.chromium.org/2515853002/diff/1/chrome/browser/autofill/and...
chrome/browser/autofill/android/personal_data_manager_android.cc:449:
label_fields.push_back(ADDRESS_HOME_SORTING_CODE);
On 2016/11/18 21:56:22, rouslan wrote:
> Add a comment about missing company_name and addres_home_country. The reader
of
> the .cpp should be aware that these are removed intentionally. Otherwise they
> might add it back by mistake.

Done.

[please use gerrit instead, 2016-11-21 16:34:53]

https://codereview.chromium.org/2515853002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java
(right):

https://codereview.chromium.org/2515853002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:800:
completeAddress.updateProfileLabel();
Nit: Can you put the PDM.getShippingAddressLabel() call inside of
AutofillAdderss? For example:

public void setShippingAddressLabel() {
    mProfile.setLabel(PersonalDataManager.getInstance()
            .getShippingAddressLabelForPaymentRequest(mProfile);
    updateSubLabel(mProfile.getLabel());
}

public void setBillingAdderssLabel() {
    mProfile.setLabel(PersonalDataManager.getInstance()
            .getBillingAddressLabelForPaymentRequest(mProfile);
    updateSubLabel(mProfile.getLabel());
}

https://codereview.chromium.org/2515853002/diff/20001/chrome/browser/autofill...
File chrome/browser/autofill/android/personal_data_manager_android.cc (right):

https://codereview.chromium.org/2515853002/diff/20001/chrome/browser/autofill...
chrome/browser/autofill/android/personal_data_manager_android.cc:443: // The
company name is not included in the billing address label.
s/company name/company name and country/

[sebsg, 2016-11-21 17:15:00]

The CQ bit was checked by sebsg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-21 17:15:38]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-21 17:18:03]

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

[commit-bot: I haz the power, 2016-11-21 17:18:04]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[sebsg, 2016-11-21 18:30:35]

The CQ bit was checked by sebsg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-21 18:31:11]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-21 19:03:53]

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

[commit-bot: I haz the power, 2016-11-21 19:03:54]

Dry run: This issue passed the CQ dry run.

[sebsg, 2016-11-21 22:29:28]

Patchset #4 (id:60001) has been deleted

[sebsg, 2016-11-21 22:32:14]

The CQ bit was checked by sebsg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-21 22:32:58]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sebsg, 2016-11-21 22:33:13]

The CQ bit was checked by sebsg@chromium.org to run a CQ dry run

[sebsg, 2016-11-21 22:33:28]

Patchset #5 (id:50010) has been deleted

[commit-bot: I haz the power, 2016-11-21 22:33:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-21 23:29:53]

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

[commit-bot: I haz the power, 2016-11-21 23:29:54]

Dry run: This issue passed the CQ dry run.

[Mathieu, 2016-11-22 14:22:31]

pdm lgtm

[sebsg, 2016-11-22 15:06:20]

sebsg@chromium.org changed reviewers:
+ dfalcantara@chromium.org

[sebsg, 2016-11-22 15:06:21]

Hi Dan, could you please review PersonalDataManager.java? Thanks!

https://codereview.chromium.org/2515853002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java
(right):

https://codereview.chromium.org/2515853002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:800:
completeAddress.updateProfileLabel();
On 2016/11/21 16:34:53, rouslan wrote:
> Nit: Can you put the PDM.getShippingAddressLabel() call inside of
> AutofillAdderss? For example:
> 
> public void setShippingAddressLabel() {
>     mProfile.setLabel(PersonalDataManager.getInstance()
>             .getShippingAddressLabelForPaymentRequest(mProfile);
>     updateSubLabel(mProfile.getLabel());
> }
> 
> public void setBillingAdderssLabel() {
>     mProfile.setLabel(PersonalDataManager.getInstance()
>             .getBillingAddressLabelForPaymentRequest(mProfile);
>     updateSubLabel(mProfile.getLabel());
> }

Wish I thought of that. thanks!

[gone, 2016-11-22 18:08:45]

lgtm

[sebsg, 2016-11-22 18:51:49]

The CQ bit was checked by sebsg@chromium.org

[sebsg, 2016-11-22 18:51:49]

The patchset sent to the CQ was uploaded after l-g-t-m from rouslan@chromium.org
Link to the patchset: https://codereview.chromium.org/2515853002/#ps110001
(title: "Updated test and code added in the Rebase")

[commit-bot: I haz the power, 2016-11-22 18:52:09]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-22 18:59:39]

CQ is committing da patch.

Bot data: {"patchset_id": 110001, "attempt_start_ts": 1479840709275780,
"parent_rev": "b2e7cfafd45d8b858c47c6b72b9788c8f6238478", "commit_rev":
"aaadcfbc17ff659d3f8a2a8311b4c87724c1b5cd"}

[commit-bot: I haz the power, 2016-11-22 19:00:18]

Committed patchset #5 (id:110001)

[commit-bot: I haz the power, 2016-11-22 19:03:43]

Description was changed from

==========
[Payments] Fix new billing address label.

BUG=666048
==========

to

==========
[Payments] Fix new billing address label.

BUG=666048

Committed: https://crrev.com/cc022347d5d434c2caddfa24568b772d66081dd1
Cr-Commit-Position: refs/heads/master@{#433933}
==========

[commit-bot: I haz the power, 2016-11-22 19:03:45]

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