[Web Payments] Add line items in the Order Summary Sheet.

Add line items in the Order Summary Sheet.

BUG=681688
Review-Url: https://codereview.chromium.org/2632243003
Cr-Commit-Position: refs/heads/master@{#444201}
Committed: https://chromium.googlesource.com/chromium/src/+/d4a7f3193227e64ff48b7a378f31f9a2fcb23bef

[anthonyvd, 2017-01-16 20:31:50]

anthonyvd@chromium.org changed reviewers:
+ sebsg@chromium.org, sky@chromium.org

[anthonyvd, 2017-01-16 20:31:51]

Hi, this CL populates the Order Summary sheet with the individual line items
passed from the website to PaymentRequest. Can you PTAL at:

sebsg@: autofill_strings.grdp
sky@: order_summary_view_controller.cc

Thanks a ton!

[sebsg, 2017-01-16 20:46:13]

autofill_strings.grdp lgtm

[sky, 2017-01-17 17:51:33]

https://codereview.chromium.org/2632243003/diff/1/chrome/browser/ui/views/pay...
File chrome/browser/ui/views/payments/order_summary_view_controller.cc (right):

https://codereview.chromium.org/2632243003/diff/1/chrome/browser/ui/views/pay...
chrome/browser/ui/views/payments/order_summary_view_controller.cc:47: constexpr
int kRowVerticalInset = 12;
Where does the 12 come from? Is there an existing constant we can use here?

[anthonyvd, 2017-01-17 19:43:22]

https://codereview.chromium.org/2632243003/diff/1/chrome/browser/ui/views/pay...
File chrome/browser/ui/views/payments/order_summary_view_controller.cc (right):

https://codereview.chromium.org/2632243003/diff/1/chrome/browser/ui/views/pay...
chrome/browser/ui/views/payments/order_summary_view_controller.cc:47: constexpr
int kRowVerticalInset = 12;
On 2017/01/17 at 17:51:33, sky wrote:
> Where does the 12 come from? Is there an existing constant we can use here?

It comes from the PaymentRequest mocks and this screen appears to be the only
place where this particular spacing is used. I'll probably move the constants
that get used in multiple places to payment_request_views_util.h as they come
up. WDYT?

[sky, 2017-01-17 21:05:42]

On 2017/01/17 19:43:22, anthonyvd wrote:
>
https://codereview.chromium.org/2632243003/diff/1/chrome/browser/ui/views/pay...
> File chrome/browser/ui/views/payments/order_summary_view_controller.cc
(right):
> 
>
https://codereview.chromium.org/2632243003/diff/1/chrome/browser/ui/views/pay...
> chrome/browser/ui/views/payments/order_summary_view_controller.cc:47:
constexpr
> int kRowVerticalInset = 12;
> On 2017/01/17 at 17:51:33, sky wrote:
> > Where does the 12 come from? Is there an existing constant we can use here?
> 
> It comes from the PaymentRequest mocks and this screen appears to be the only
> place where this particular spacing is used. I'll probably move the constants
> that get used in multiple places to payment_request_views_util.h as they come
> up. WDYT?

There are other places in the code that create multi-item like lists. I'm
wondering if they should all use the same inset? One similar one that comes to
mind is the intent picker:
https://chromium.googlesource.com/chromium/src/+/master/chrome/browser/ui/vie...
. It uses an empty border. I'll LGTM this and leave it to the harmony effort to
unify these values.

[anthonyvd, 2017-01-17 21:11:55]

On 2017/01/17 at 21:05:42, sky wrote:
> On 2017/01/17 19:43:22, anthonyvd wrote:
> >
https://codereview.chromium.org/2632243003/diff/1/chrome/browser/ui/views/pay...
> > File chrome/browser/ui/views/payments/order_summary_view_controller.cc
(right):
> > 
> >
https://codereview.chromium.org/2632243003/diff/1/chrome/browser/ui/views/pay...
> > chrome/browser/ui/views/payments/order_summary_view_controller.cc:47:
constexpr
> > int kRowVerticalInset = 12;
> > On 2017/01/17 at 17:51:33, sky wrote:
> > > Where does the 12 come from? Is there an existing constant we can use
here?
> > 
> > It comes from the PaymentRequest mocks and this screen appears to be the
only
> > place where this particular spacing is used. I'll probably move the
constants
> > that get used in multiple places to payment_request_views_util.h as they
come
> > up. WDYT?
> 
> There are other places in the code that create multi-item like lists. I'm
wondering if they should all use the same inset? One similar one that comes to
mind is the intent picker:
https://chromium.googlesource.com/chromium/src/+/master/chrome/browser/ui/vie...
. It uses an empty border. I'll LGTM this and leave it to the harmony effort to
unify these values.

Ah that's a good point. Anyone we should loop in specifically about this?

Thanks for the review! :)

[anthonyvd, 2017-01-17 22:42:22]

The CQ bit was checked by anthonyvd@chromium.org

[anthonyvd, 2017-01-17 22:42:22]

The patchset sent to the CQ was uploaded after l-g-t-m from sebsg@chromium.org,
sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2632243003/#ps10001
(title: "Rebase")

[commit-bot: I haz the power, 2017-01-17 22:42:48]

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-01-17 22:45:07]

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

[commit-bot: I haz the power, 2017-01-17 22:45:08]

Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[anthonyvd, 2017-01-17 22:52:24]

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

[commit-bot: I haz the power, 2017-01-17 22:53:12]

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

[sky, 2017-01-17 23:24:53]

Maybe kylixrd@.

  -Scott

On Tue, Jan 17, 2017 at 1:11 PM,  <anthonyvd@chromium.org> wrote:
> On 2017/01/17 at 21:05:42, sky wrote:
>> On 2017/01/17 19:43:22, anthonyvd wrote:
>> >
>
https://codereview.chromium.org/2632243003/diff/1/chrome/browser/ui/views/pay...
>> > File chrome/browser/ui/views/payments/order_summary_view_controller.cc
> (right):
>> >
>> >
>
https://codereview.chromium.org/2632243003/diff/1/chrome/browser/ui/views/pay...
>> > chrome/browser/ui/views/payments/order_summary_view_controller.cc:47:
> constexpr
>> > int kRowVerticalInset = 12;
>> > On 2017/01/17 at 17:51:33, sky wrote:
>> > > Where does the 12 come from? Is there an existing constant we can use
> here?
>> >
>> > It comes from the PaymentRequest mocks and this screen appears to be the
> only
>> > place where this particular spacing is used. I'll probably move the
> constants
>> > that get used in multiple places to payment_request_views_util.h as they
> come
>> > up. WDYT?
>>
>> There are other places in the code that create multi-item like lists. I'm
> wondering if they should all use the same inset? One similar one that comes
> to
> mind is the intent picker:
>
https://chromium.googlesource.com/chromium/src/+/master/chrome/browser/ui/vie...
> . It uses an empty border. I'll LGTM this and leave it to the harmony effort
> to
> unify these values.
>
> Ah that's a good point. Anyone we should loop in specifically about this?
>
> Thanks for the review! :)
>
> https://codereview.chromium.org/2632243003/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[commit-bot: I haz the power, 2017-01-17 23:47:29]

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

[commit-bot: I haz the power, 2017-01-17 23:47:30]

Dry run: This issue passed the CQ dry run.

[anthonyvd, 2017-01-18 00:11:55]

The CQ bit was checked by anthonyvd@chromium.org

[anthonyvd, 2017-01-18 00:11:55]

The patchset sent to the CQ was uploaded after l-g-t-m from sebsg@chromium.org,
sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2632243003/#ps30001
(title: "Rebase")

[commit-bot: I haz the power, 2017-01-18 00:12:40]

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-01-18 00:20:57]

CQ is committing da patch.

Bot data: {"patchset_id": 30001, "attempt_start_ts": 1484698315376320,
"parent_rev": "69022eccc884e5541518c448e965f79f8aa17f95", "commit_rev":
"d4a7f3193227e64ff48b7a378f31f9a2fcb23bef"}

[commit-bot: I haz the power, 2017-01-18 00:21:33]

Description was changed from

==========
Add line items in the Order Summary Sheet.

BUG=681688
==========

to

==========
Add line items in the Order Summary Sheet.

BUG=681688

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

[commit-bot: I haz the power, 2017-01-18 00:21:34]

Committed patchset #3 (id:30001) as
https://chromium.googlesource.com/chromium/src/+/d4a7f3193227e64ff48b7a378f31...