Use PaymentRequestItemList for Shipping screen

Use PaymentRequestItemList for Shipping screen

BUG=679449
Review-Url: https://codereview.chromium.org/2720223004
Cr-Commit-Position: refs/heads/master@{#454313}
Committed: https://chromium.googlesource.com/chromium/src/+/946f03b0cb945a0cdd9efe2fdfdd0ff9bab713b7

[tmartino, 2017-02-28 19:56:50]

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

[commit-bot: I haz the power, 2017-02-28 19:57:32]

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

[tmartino, 2017-02-28 20:08:02]

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

[commit-bot: I haz the power, 2017-02-28 20:08:45]

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

[commit-bot: I haz the power, 2017-02-28 20:08:46]

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

[tmartino, 2017-02-28 20:10:26]

On 2017/02/28 at 20:08:02, tmartino wrote:
> 

I'm going back and fixing tests. Wanted to get the non-test part out so you
could see it due to the overlap with your ongoing CL.

[tmartino, 2017-02-28 20:14:52]

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

[commit-bot: I haz the power, 2017-02-28 20:15:33]

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, 2017-02-28 21:06:24]

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

[commit-bot: I haz the power, 2017-02-28 21:06:26]

Dry run: This issue passed the CQ dry run.

[anthonyvd, 2017-03-01 15:37:01]

Looking good! Few comments.

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

https://codereview.chromium.org/2720223004/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_request_views_util.cc:226:
std::unique_ptr<views::ImageView> CreateCheckmark(bool selected) {
While you're moving this around: can you move it to the base Item class right
away (since we'll do it when refactoring anyways)?

https://codereview.chromium.org/2720223004/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_request_views_util.cc:231:
checkmark->SetImage(gfx::CreateVectorIcon(views::kMenuCheckIcon, 0xFF609265));
Can we move the 0xFF609265 color to a constant? (My bad on that one!)

https://codereview.chromium.org/2720223004/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/payments/shipping_list_view_controller.cc (right):

https://codereview.chromium.org/2720223004/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/shipping_list_view_controller.cc:38: //
TODO(tmartino): Pass an actual locale in place of empty string.
As of https://codereview.chromium.org/2715213005/, we have 
PaymentRequestDelegate::GetApplicationLocale(), you can use it here (and we
should use that moving forward instead of passing locales around, at least for
things that have access to the PaymentRequest object).

https://codereview.chromium.org/2720223004/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/shipping_list_view_controller.cc:82:
ShippingListViewController::ShippingListViewController(
Normally I'd say we should add tests to this similar to the ones for the payment
method sheet but I suspect we'll only have one set of such tests post-refactor
so let's wait until then. 

They'd basically be a copy-paste of the payment method ones anyways.

https://codereview.chromium.org/2720223004/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/shipping_list_view_controller.cc:92: for (auto*
profile : request()->shipping_profiles()) {
This loop can be in the constructor (for consistency with the other view
controllers that do the same thing).

[tmartino, 2017-03-01 20:00:30]

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

https://codereview.chromium.org/2720223004/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_request_views_util.cc:226:
std::unique_ptr<views::ImageView> CreateCheckmark(bool selected) {
On 2017/03/01 at 15:37:00, anthonyvd wrote:
> While you're moving this around: can you move it to the base Item class right
away (since we'll do it when refactoring anyways)?

Done

https://codereview.chromium.org/2720223004/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_request_views_util.cc:231:
checkmark->SetImage(gfx::CreateVectorIcon(views::kMenuCheckIcon, 0xFF609265));
On 2017/03/01 at 15:37:00, anthonyvd wrote:
> Can we move the 0xFF609265 color to a constant? (My bad on that one!)

Done

https://codereview.chromium.org/2720223004/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/payments/shipping_list_view_controller.cc (right):

https://codereview.chromium.org/2720223004/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/shipping_list_view_controller.cc:38: //
TODO(tmartino): Pass an actual locale in place of empty string.
On 2017/03/01 at 15:37:00, anthonyvd wrote:
> As of https://codereview.chromium.org/2715213005/, we have 
PaymentRequestDelegate::GetApplicationLocale(), you can use it here (and we
should use that moving forward instead of passing locales around, at least for
things that have access to the PaymentRequest object).

We don't have public access to the delegate from the PaymentRequest. We should
probably add a passthrough method to PaymentRequest to fetch the locale from the
delegate, but I would prefer not to touch PaymentRequest in this CL.

https://codereview.chromium.org/2720223004/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/shipping_list_view_controller.cc:82:
ShippingListViewController::ShippingListViewController(
On 2017/03/01 at 15:37:00, anthonyvd wrote:
> Normally I'd say we should add tests to this similar to the ones for the
payment method sheet but I suspect we'll only have one set of such tests
post-refactor so let's wait until then. 
> 
> They'd basically be a copy-paste of the payment method ones anyways.

Ack

https://codereview.chromium.org/2720223004/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/shipping_list_view_controller.cc:92: for (auto*
profile : request()->shipping_profiles()) {
On 2017/03/01 at 15:37:01, anthonyvd wrote:
> This loop can be in the constructor (for consistency with the other view
controllers that do the same thing).

Done

[tmartino, 2017-03-01 20:02:22]

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

[commit-bot: I haz the power, 2017-03-01 20:03:42]

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

[anthonyvd, 2017-03-01 20:15:35]

lgtm % nits

https://codereview.chromium.org/2720223004/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/payments/shipping_list_view_controller.cc (right):

https://codereview.chromium.org/2720223004/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/shipping_list_view_controller.cc:38: //
TODO(tmartino): Pass an actual locale in place of empty string.
On 2017/03/01 at 20:00:30, tmartino wrote:
> On 2017/03/01 at 15:37:00, anthonyvd wrote:
> > As of https://codereview.chromium.org/2715213005/, we have 
PaymentRequestDelegate::GetApplicationLocale(), you can use it here (and we
should use that moving forward instead of passing locales around, at least for
things that have access to the PaymentRequest object).
> 
> We don't have public access to the delegate from the PaymentRequest. We should
probably add a passthrough method to PaymentRequest to fetch the locale from the
delegate, but I would prefer not to touch PaymentRequest in this CL.

Ahh, agreed with the passthrough and ok with deferring it to another CL.

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

https://codereview.chromium.org/2720223004/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_request_item_list.cc:15: namespace {
nit: anonymous namespace is inside of payments:: in the rest of PR code. It
should be the same here for consistency.

https://codereview.chromium.org/2720223004/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/payments/payment_request_views_util.cc (left):

https://codereview.chromium.org/2720223004/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_request_views_util.cc:218: // Creates a
views::Border object that can paint the gray horizontal ruler used
This comment was on both this and the declaration in the .h, right?

[commit-bot: I haz the power, 2017-03-01 20:49:03]

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

[commit-bot: I haz the power, 2017-03-01 20:49:04]

Dry run: This issue passed the CQ dry run.

[tmartino, 2017-03-01 22:48:07]

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

[tmartino, 2017-03-01 22:48:15]

https://codereview.chromium.org/2720223004/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/payments/shipping_list_view_controller.cc (right):

https://codereview.chromium.org/2720223004/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/shipping_list_view_controller.cc:38: //
TODO(tmartino): Pass an actual locale in place of empty string.
On 2017/03/01 at 20:15:34, anthonyvd wrote:
> On 2017/03/01 at 20:00:30, tmartino wrote:
> > On 2017/03/01 at 15:37:00, anthonyvd wrote:
> > > As of https://codereview.chromium.org/2715213005/, we have 
PaymentRequestDelegate::GetApplicationLocale(), you can use it here (and we
should use that moving forward instead of passing locales around, at least for
things that have access to the PaymentRequest object).
> > 
> > We don't have public access to the delegate from the PaymentRequest. We
should probably add a passthrough method to PaymentRequest to fetch the locale
from the delegate, but I would prefer not to touch PaymentRequest in this CL.
> 
> Ahh, agreed with the passthrough and ok with deferring it to another CL.

sgtm

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

https://codereview.chromium.org/2720223004/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_request_item_list.cc:15: namespace {
On 2017/03/01 at 20:15:34, anthonyvd wrote:
> nit: anonymous namespace is inside of payments:: in the rest of PR code. It
should be the same here for consistency.

Done.

https://codereview.chromium.org/2720223004/diff/60001/chrome/browser/ui/views...
File chrome/browser/ui/views/payments/payment_request_views_util.cc (left):

https://codereview.chromium.org/2720223004/diff/60001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_request_views_util.cc:218: // Creates a
views::Border object that can paint the gray horizontal ruler used
On 2017/03/01 at 20:15:34, anthonyvd wrote:
> This comment was on both this and the declaration in the .h, right?

Yes. They were identical, I think it was an artifact of copy-pasting the
declaration into the cc file.

[commit-bot: I haz the power, 2017-03-01 22:49:05]

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, 2017-03-01 23:34:29]

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

[commit-bot: I haz the power, 2017-03-01 23:34:30]

Dry run: This issue passed the CQ dry run.

[tmartino, 2017-03-01 23:49:17]

The CQ bit was checked by tmartino@chromium.org

[tmartino, 2017-03-01 23:49:18]

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/2720223004/#ps80001
(title: "namespace")

[commit-bot: I haz the power, 2017-03-01 23:50:22]

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-03-01 23:58:04]

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

[commit-bot: I haz the power, 2017-03-01 23:58:10]

Failed to apply patch for
chrome/browser/ui/views/payments/payment_request_dialog_view_ids.h:
While running git apply --index -p1;
  error: patch failed:
chrome/browser/ui/views/payments/payment_request_dialog_view_ids.h:31
  error: chrome/browser/ui/views/payments/payment_request_dialog_view_ids.h:
patch does not apply

Patch:       chrome/browser/ui/views/payments/payment_request_dialog_view_ids.h
Index: chrome/browser/ui/views/payments/payment_request_dialog_view_ids.h
diff --git a/chrome/browser/ui/views/payments/payment_request_dialog_view_ids.h
b/chrome/browser/ui/views/payments/payment_request_dialog_view_ids.h
index
df302b0f1c5b41aa47bb614f3ca36582ea87943a..d9ed9371b6e39d3c856566b692db2bf0485192da
100644
--- a/chrome/browser/ui/views/payments/payment_request_dialog_view_ids.h
+++ b/chrome/browser/ui/views/payments/payment_request_dialog_view_ids.h
@@ -31,7 +31,10 @@ enum class DialogViewID : int {
 
   // The following are views contained within the Payment Method Sheet.
   PAYMENT_METHOD_SHEET_LIST_VIEW,
-  PAYMENT_METHOD_ITEM_CHECKMARK_VIEW,
+
+  // Used in selectable rows. Each row in a view reuses this ID, but the ID is
+  // unique at the scope of the parent row.
+  CHECKMARK_VIEW,
 
   // Used to label the error labels with an offset, which gets added to
   // the Autofill type value they represent (for tests).

[tmartino, 2017-03-02 17:34:00]

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

[commit-bot: I haz the power, 2017-03-02 17:34:25]

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, 2017-03-02 18:19:02]

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

[commit-bot: I haz the power, 2017-03-02 18:19:03]

Dry run: This issue passed the CQ dry run.

[tmartino, 2017-03-02 18:19:57]

The CQ bit was checked by tmartino@chromium.org

[tmartino, 2017-03-02 18:19:58]

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/2720223004/#ps100001
(title: "rebase")

[commit-bot: I haz the power, 2017-03-02 18:20:31]

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-03-02 18:27:53]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1488478797537210,
"parent_rev": "58d5486ea67d2b1065f616c1d604bd75a3026748", "commit_rev":
"946f03b0cb945a0cdd9efe2fdfdd0ff9bab713b7"}

[commit-bot: I haz the power, 2017-03-02 18:28:35]

Description was changed from

==========
Use PaymentRequestItemList for Shipping screen

BUG=679449
==========

to

==========
Use PaymentRequestItemList for Shipping screen

BUG=679449

Review-Url: https://codereview.chromium.org/2720223004
Cr-Commit-Position: refs/heads/master@{#454313}
Committed:
https://chromium.googlesource.com/chromium/src/+/946f03b0cb945a0cdd9efe2fdfdd...
==========

[commit-bot: I haz the power, 2017-03-02 18:28:36]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/946f03b0cb945a0cdd9efe2fdfdd...