[Payments] Remove country from shipping label in bottom and fullsheet.

[Payments] Remove country from shipping label in bottom and fullsheet.

BUG=662574
Committed: https://crrev.com/b9f344d725edb0c6514d6df660ab0236a36f83b4
Cr-Commit-Position: refs/heads/master@{#434543}

[sebsg, 2016-11-23 21:16:05]

Patchset #1 (id:1) has been deleted

[sebsg, 2016-11-23 21:17:21]

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

[sebsg, 2016-11-23 21:17:21]

Hi Rouslan PTAL at the java files?

Hi Math PTAL at the C++ files?

[please use gerrit instead, 2016-11-23 21:40:50]

Awesome work! LGTM after comments. I'm out of office Thursday and Friday, so use
your best judgement if you disagree on any of my suggestions and then go ahead
and land the patch.

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

https://codereview.chromium.org/2526943003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillAddress.java:62:
@Nullable
Nit: put on same line to match the other member variables.

https://codereview.chromium.org/2526943003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillAddress.java:108:
/*
Should be /** because this is a javadoc for a public method.

https://codereview.chromium.org/2526943003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillAddress.java:125:
/*
/**

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

https://codereview.chromium.org/2526943003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:1229:

You might want to "assert mShippingAddressesSection.getSelectedItem() instanceof
AutofillAddress;" Otherwise FindBugs might complain about unchecked cast.

https://codereview.chromium.org/2526943003/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentOption.java
(right):

https://codereview.chromium.org/2526943003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentOption.java:19:
public interface OptionSectionFocusChangedDelegate {
Observers are usually named *Observer. The differences are many and at times
subtle. One way distinguish: observers are optional, delegates are required.

Also, observer interfaces should be defined in the class that's firing the
event. OptionSection would be the best. Then you could name this interface
FocusChangedObserver.

https://codereview.chromium.org/2526943003/diff/20001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestShippingAddressTest.java
(right):

https://codereview.chromium.org/2526943003/diff/20001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestShippingAddressTest.java:19:
* A payment integration test for shipping addresses.
...for shipping address labels.

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

https://codereview.chromium.org/2526943003/diff/20001/chrome/browser/autofill...
chrome/browser/autofill/android/personal_data_manager_android.cc:451:
PersonalDataManagerAndroid::GetShippingAddressLabelForPaymentRequest(
Order of methods in .h should match the order of methods in .cc. This method
should be the last one in .cc.

[sebsg, 2016-11-23 22:18:34]

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

[commit-bot: I haz the power, 2016-11-23 22:19:37]

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-24 00:21:16]

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

[commit-bot: I haz the power, 2016-11-24 00:21:17]

Dry run: Try jobs failed on following builders:
  blimp_linux_dbg on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)

[sebsg, 2016-11-25 15:47:35]

Patchset #3 (id:60001) has been deleted

[sebsg, 2016-11-25 15:54:53]

Patchset #3 (id:80001) has been deleted

[sebsg, 2016-11-25 15:55:21]

Thanks Rouslan! Math, Friendly ping? :)

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

https://codereview.chromium.org/2526943003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillAddress.java:62:
@Nullable
On 2016/11/23 21:40:50, rouslan (ooo thanksgiving) wrote:
> Nit: put on same line to match the other member variables.

Yeah sorry it's "git cl format" again :P I should really not use it for java
cls.

https://codereview.chromium.org/2526943003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillAddress.java:108:
/*
On 2016/11/23 21:40:50, rouslan (ooo thanksgiving) wrote:
> Should be /** because this is a javadoc for a public method.

Done.

https://codereview.chromium.org/2526943003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillAddress.java:125:
/*
On 2016/11/23 21:40:50, rouslan (ooo thanksgiving) wrote:
> /**

Done.

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

https://codereview.chromium.org/2526943003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:1229:

On 2016/11/23 21:40:50, rouslan (ooo thanksgiving) wrote:
> You might want to "assert mShippingAddressesSection.getSelectedItem()
instanceof
> AutofillAddress;" Otherwise FindBugs might complain about unchecked cast.

Done.

https://codereview.chromium.org/2526943003/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentOption.java
(right):

https://codereview.chromium.org/2526943003/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentOption.java:19:
public interface OptionSectionFocusChangedDelegate {
On 2016/11/23 21:40:50, rouslan (ooo thanksgiving) wrote:
> Observers are usually named *Observer. The differences are many and at times
> subtle. One way distinguish: observers are optional, delegates are required.
> 
> Also, observer interfaces should be defined in the class that's firing the
> event. OptionSection would be the best. Then you could name this interface
> FocusChangedObserver.

Done.

https://codereview.chromium.org/2526943003/diff/20001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestShippingAddressTest.java
(right):

https://codereview.chromium.org/2526943003/diff/20001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestShippingAddressTest.java:19:
* A payment integration test for shipping addresses.
On 2016/11/23 21:40:50, rouslan (ooo thanksgiving) wrote:
> ...for shipping address labels.

Done.

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

https://codereview.chromium.org/2526943003/diff/20001/chrome/browser/autofill...
chrome/browser/autofill/android/personal_data_manager_android.cc:451:
PersonalDataManagerAndroid::GetShippingAddressLabelForPaymentRequest(
On 2016/11/23 21:40:50, rouslan (ooo thanksgiving) wrote:
> Order of methods in .h should match the order of methods in .cc. This method
> should be the last one in .cc.

Done.

[Mathieu, 2016-11-25 16:05:11]

lgtm with comment

https://codereview.chromium.org/2526943003/diff/100001/chrome/browser/autofil...
File chrome/browser/autofill/android/personal_data_manager_android.cc (right):

https://codereview.chromium.org/2526943003/diff/100001/chrome/browser/autofil...
chrome/browser/autofill/android/personal_data_manager_android.cc:859:
PersonalDataManagerAndroid::GetShippingAddressLabelForPaymentRequest(
So this is called from java, and ends up populating a C++ AutofillProfile by
going back to Java (PopulateNativeProfileFromJava), then returns the jstring to
java. Could we think of a better design in the future? Perhaps one that is
reusable for Desktop/iOS?

Feel free to disagree if you think I'm wrong :)

[sebsg, 2016-11-25 16:21:48]

Thanks!

https://codereview.chromium.org/2526943003/diff/100001/chrome/browser/autofil...
File chrome/browser/autofill/android/personal_data_manager_android.cc (right):

https://codereview.chromium.org/2526943003/diff/100001/chrome/browser/autofil...
chrome/browser/autofill/android/personal_data_manager_android.cc:859:
PersonalDataManagerAndroid::GetShippingAddressLabelForPaymentRequest(
On 2016/11/25 16:05:11, Mathieu Perreault wrote:
> So this is called from java, and ends up populating a C++ AutofillProfile by
> going back to Java (PopulateNativeProfileFromJava), then returns the jstring
to
> java. Could we think of a better design in the future? Perhaps one that is
> reusable for Desktop/iOS?
> 
> Feel free to disagree if you think I'm wrong :)

You are totally right, and the way we get profiles from C++ to Java now is less
than ideal. It's on the list ;)

[sebsg, 2016-11-25 16:21:53]

The CQ bit was checked by sebsg@chromium.org

[sebsg, 2016-11-25 16:21:53]

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/2526943003/#ps100001
(title: "Addressed comments")

[commit-bot: I haz the power, 2016-11-25 16:22: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-25 18:00:20]

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

[commit-bot: I haz the power, 2016-11-25 18:00:23]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[sebsg, 2016-11-25 18:26:09]

The CQ bit was checked by sebsg@chromium.org

[sebsg, 2016-11-25 18:26:10]

The patchset sent to the CQ was uploaded after l-g-t-m from mathp@chromium.org,
rouslan@chromium.org
Link to the patchset: https://codereview.chromium.org/2526943003/#ps120001
(title: "Moved assertion")

[commit-bot: I haz the power, 2016-11-25 18:26:26]

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-25 19:45:43]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1480098369555090,
"parent_rev": "6e861aaff19edfec5221cab02eedb49654c00624", "commit_rev":
"5f792a5d07dd68134b73f4b43b155bdb6656ec15"}

[commit-bot: I haz the power, 2016-11-25 19:46:09]

Committed patchset #4 (id:120001)

[commit-bot: I haz the power, 2016-11-25 19:48:40]

Description was changed from

==========
[Payments] Remove country from shipping label in bottom and fullsheet.

BUG=662574
==========

to

==========
[Payments] Remove country from shipping label in bottom and fullsheet.

BUG=662574

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

[commit-bot: I haz the power, 2016-11-25 19:48:41]

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