[Payments] Display the shipping option error alongside the address

[Payments] Display the shipping option error alongside the address

In the Payment Sheet and the Address selector, display the shipping
option error.

BUG=710004
TEST=browser_tests

Review-Url: https://codereview.chromium.org/2855203002
Cr-Commit-Position: refs/heads/master@{#469397}
Committed: https://chromium.googlesource.com/chromium/src/+/eb8892ff597ac955e4eebe688a57ca0b81b02bd4

[Mathieu, 2017-05-03 20:34:29]

Description was changed from

==========
[Payments] Display the shipping option error alongside the address

In the Payment Sheet and the Address selector, display the shipping
option error.

BUG=710004
TEST=browser_tests
==========

to

==========
[Payments] Display the shipping option error alongside the address

In the Payment Sheet and the Address selector, display the shipping
option error.

BUG=710004
TEST=browser_tests
==========

[Mathieu, 2017-05-03 20:34:30]

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

[Mathieu, 2017-05-03 20:34:44]

Anthony, PTAL. I've added screenshots to the bug.

[Mathieu, 2017-05-03 20:34:55]

Patchset #2 (id:20001) has been deleted

[anthonyvd, 2017-05-04 13:07:37]

Looks awesome! Few comments.

https://codereview.chromium.org/2855203002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/payments/payment_method_view_controller.cc (right):

https://codereview.chromium.org/2855203002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_method_view_controller.cc:113: bool
IsDisabled() override {
Can this be IsEnabled instead? Affirmative is easier to reason about than
Negative.

https://codereview.chromium.org/2855203002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/payments/payment_request_item_list.h (right):

https://codereview.chromium.org/2855203002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_request_item_list.h:86: virtual bool
CanBeSelected() = 0;
Why remove const?

https://codereview.chromium.org/2855203002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/payments/payment_request_views_util.cc (right):

https://codereview.chromium.org/2855203002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_request_views_util.cc:338: if
(!missing.empty()) {
This entire block is the same as the one on line 296, can we make it a function?

[Mathieu, 2017-05-04 13:51:55]

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

[commit-bot: I haz the power, 2017-05-04 13:52:34]

Dry run: CQ is trying da patch.

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

[Mathieu, 2017-05-04 13:53:25]

Thanks! I made the changes and added a few more:

* Made selected_shipping_option_error a base::string16
* The error can now be a generic string if not specified by the merchant.

https://codereview.chromium.org/2855203002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/payments/payment_method_view_controller.cc (right):

https://codereview.chromium.org/2855203002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_method_view_controller.cc:113: bool
IsDisabled() override {
On 2017/05/04 13:07:37, anthonyvd wrote:
> Can this be IsEnabled instead? Affirmative is easier to reason about than
> Negative.

Sure thing!

https://codereview.chromium.org/2855203002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/payments/payment_request_item_list.h (right):

https://codereview.chromium.org/2855203002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_request_item_list.h:86: virtual bool
CanBeSelected() = 0;
On 2017/05/04 13:07:37, anthonyvd wrote:
> Why remove const?

It calls IsEnabled, which calls into state(), which can't be const unless we do
some refactoring in PaymentRequestSheetController/PaymentRequestState

https://codereview.chromium.org/2855203002/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/payments/payment_request_views_util.cc (right):

https://codereview.chromium.org/2855203002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_request_views_util.cc:338: if
(!missing.empty()) {
On 2017/05/04 13:07:37, anthonyvd wrote:
> This entire block is the same as the one on line 296, can we make it a
function?

Done.

[commit-bot: I haz the power, 2017-05-04 13:59:39]

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

[commit-bot: I haz the power, 2017-05-04 13:59:40]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Mathieu, 2017-05-04 14:06:47]

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

[commit-bot: I haz the power, 2017-05-04 14:06:59]

Dry run: CQ is trying da patch.

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

[anthonyvd, 2017-05-04 14:11:34]

lgtm % couple comments. Thanks!

https://codereview.chromium.org/2855203002/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/payments/profile_list_view_controller.cc (right):

https://codereview.chromium.org/2855203002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/profile_list_view_controller.cc:92: // If not
disabled, the editor is opened to complete the invalid profile.
nit: If enabled

https://codereview.chromium.org/2855203002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/profile_list_view_controller.cc:318: return
!(profile == state()->selected_shipping_profile() &&
nit: distribute the ! and expand the comment as per our discussion.

[Mathieu, 2017-05-04 14:24:24]

Patchset #5 (id:100001) has been deleted

[Mathieu, 2017-05-04 14:37:53]

The CQ bit was checked by mathp@chromium.org

[Mathieu, 2017-05-04 14:37:54]

The patchset sent to the CQ was uploaded after l-g-t-m from
anthonyvd@chromium.org
Link to the patchset: https://codereview.chromium.org/2855203002/#ps120001
(title: "address last comments")

[commit-bot: I haz the power, 2017-05-04 14:38:09]

CQ is trying da patch.

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

[Mathieu, 2017-05-04 14:38:56]

Submitting!

https://codereview.chromium.org/2855203002/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/payments/profile_list_view_controller.cc (right):

https://codereview.chromium.org/2855203002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/profile_list_view_controller.cc:92: // If not
disabled, the editor is opened to complete the invalid profile.
On 2017/05/04 14:11:34, anthonyvd wrote:
> nit: If enabled

Done.

https://codereview.chromium.org/2855203002/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/profile_list_view_controller.cc:318: return
!(profile == state()->selected_shipping_profile() &&
On 2017/05/04 14:11:34, anthonyvd wrote:
> nit: distribute the ! and expand the comment as per our discussion.

Done.

[commit-bot: I haz the power, 2017-05-04 16:28:16]

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

[commit-bot: I haz the power, 2017-05-04 16:28:17]

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...)

[Mathieu, 2017-05-04 17:02:16]

The CQ bit was checked by mathp@chromium.org

[Mathieu, 2017-05-04 17:02:16]

The patchset sent to the CQ was uploaded after l-g-t-m from
anthonyvd@chromium.org
Link to the patchset: https://codereview.chromium.org/2855203002/#ps140001
(title: "android test fix")

[commit-bot: I haz the power, 2017-05-04 17:03:01]

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, 2017-05-04 18:43:05]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1493917336069660,
"parent_rev": "63c287aa94594af1018f6c3b966aa72e6f315801", "commit_rev":
"eb8892ff597ac955e4eebe688a57ca0b81b02bd4"}

[commit-bot: I haz the power, 2017-05-04 18:43:17]

Description was changed from

==========
[Payments] Display the shipping option error alongside the address

In the Payment Sheet and the Address selector, display the shipping
option error.

BUG=710004
TEST=browser_tests
==========

to

==========
[Payments] Display the shipping option error alongside the address

In the Payment Sheet and the Address selector, display the shipping
option error.

BUG=710004
TEST=browser_tests

Review-Url: https://codereview.chromium.org/2855203002
Cr-Commit-Position: refs/heads/master@{#469397}
Committed:
https://chromium.googlesource.com/chromium/src/+/eb8892ff597ac955e4eebe688a57...
==========

[commit-bot: I haz the power, 2017-05-04 18:43:20]

Committed patchset #6 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/eb8892ff597ac955e4eebe688a57...