[Web Payments] Add inline items to the Order Summary section.

[Web Payments] Add inline items to the Order Summary section.

BUG=698322
Review-Url: https://codereview.chromium.org/2730123002
Cr-Commit-Position: refs/heads/master@{#454943}
Committed: https://chromium.googlesource.com/chromium/src/+/b0233f26f385e8bc8f0a77907499184cfc256c38

[anthonyvd, 2017-03-03 19:33:47]

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

[commit-bot: I haz the power, 2017-03-03 19:34: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-03 19:36:52]

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

[commit-bot: I haz the power, 2017-03-03 19:36:53]

Dry run: Try jobs failed on following builders:
  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-03-03 22:33:44]

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

[anthonyvd, 2017-03-03 22:34:06]

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

[anthonyvd, 2017-03-03 22:34:06]

Hi mathp@,

Can you PTAL? Thanks!

[commit-bot: I haz the power, 2017-03-03 22:34:10]

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-03 23:16:00]

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

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

Dry run: This issue passed the CQ dry run.

[Mathieu, 2017-03-06 15:30:51]

Thanks looking good!

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

https://codereview.chromium.org/2730123002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_request_views_util.cc:244:
label->SetFontList(font_list.DeriveWithWeight(gfx::Font::Weight::BOLD));
optional nit: consider inlining
std::unique_ptr<views::Label> label = base::MakeUnique<views::Label>(text);
label->SetFontList(label->font_list().DeriveWithWeight(gfx::Font::Weight::BOLD));
return label;

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

https://codereview.chromium.org/2730123002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_sheet_view_controller.cc:168:
std::unique_ptr<views::GridLayout> CreateOrderSummarySectionContainerLayout(
Let's document this a bit, including what |host| and |trailing| are.

https://codereview.chromium.org/2730123002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_sheet_view_controller.cc:330:
std::unique_ptr<views::View> item_summary = base::MakeUnique<views::View>();
nit: item_summaries?

https://codereview.chromium.org/2730123002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_sheet_view_controller.cc:333: false /*
trailing */);
nit: would prefer /*trailing=*/false otherwise it could be interpreted both ways

https://codereview.chromium.org/2730123002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_sheet_view_controller.cc:346: 
nit: remove extra line

https://codereview.chromium.org/2730123002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_sheet_view_controller.cc:347: for
(size_t i = 0; i < items.size() && i < 2; ++i) {
constexpr int kMaxNumberOfItemsShown = 2; and reuse below.

OR

Consider reusing |i| after the loop has finished.

https://codereview.chromium.org/2730123002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_sheet_view_controller.cc:354:
request()->GetFormattedCurrencyAmount(items[i]->amount->value)));
this is nice! Could we update it in order_summary_view_controller as well?

https://codereview.chromium.org/2730123002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_sheet_view_controller.cc:364: constexpr
SkColor kMoreItemsLightGray = 0xFF757575;
In editor view controller I was doing this:

  label->SetDisabledColor(label->GetNativeTheme()->GetSystemColor(
      ui::NativeTheme::kColorId_LabelDisabledColor));
  label->SetEnabled(false);

I guess 1 comment and one question
(1) Let's use label->GetNativeTheme()->GetSystemColor(
      ui::NativeTheme::kColorId_LabelDisabledColor)
(2) seems like there are multiple ways to skin a cat! Does
SetAutoColorReadabilityEnabled do something interesting vs
SetDisabledColor+SetDisabled? Perhaps the clickability of the label?

https://codereview.chromium.org/2730123002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_sheet_view_controller.cc:374:
CreateBoldLabel(base::ASCIIToUTF16(request()->details()->total->label))
Where is the total amount being added?

[anthonyvd, 2017-03-06 16:00:17]

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

[anthonyvd, 2017-03-06 16:00:21]

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

https://codereview.chromium.org/2730123002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_request_views_util.cc:244:
label->SetFontList(font_list.DeriveWithWeight(gfx::Font::Weight::BOLD));
On 2017/03/06 at 15:30:45, Mathieu Perreault wrote:
> optional nit: consider inlining
> std::unique_ptr<views::Label> label = base::MakeUnique<views::Label>(text);
>
label->SetFontList(label->font_list().DeriveWithWeight(gfx::Font::Weight::BOLD));
> return label;

Done.

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

https://codereview.chromium.org/2730123002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_sheet_view_controller.cc:168:
std::unique_ptr<views::GridLayout> CreateOrderSummarySectionContainerLayout(
On 2017/03/06 at 15:30:45, Mathieu Perreault wrote:
> Let's document this a bit, including what |host| and |trailing| are.

Oops, good call. Done.

https://codereview.chromium.org/2730123002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_sheet_view_controller.cc:330:
std::unique_ptr<views::View> item_summary = base::MakeUnique<views::View>();
On 2017/03/06 at 15:30:48, Mathieu Perreault wrote:
> nit: item_summaries?

Done.

https://codereview.chromium.org/2730123002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_sheet_view_controller.cc:333: false /*
trailing */);
On 2017/03/06 at 15:30:49, Mathieu Perreault wrote:
> nit: would prefer /*trailing=*/false otherwise it could be interpreted both
ways

Hah, didn't think about that! Done. I'll prefer that style in the future :)

https://codereview.chromium.org/2730123002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_sheet_view_controller.cc:346: 
On 2017/03/06 at 15:30:49, Mathieu Perreault wrote:
> nit: remove extra line

Done.

https://codereview.chromium.org/2730123002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_sheet_view_controller.cc:347: for
(size_t i = 0; i < items.size() && i < 2; ++i) {
On 2017/03/06 at 15:30:45, Mathieu Perreault wrote:
> constexpr int kMaxNumberOfItemsShown = 2; and reuse below.
> 
> OR
> 
> Consider reusing |i| after the loop has finished.

Done.

https://codereview.chromium.org/2730123002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_sheet_view_controller.cc:354:
request()->GetFormattedCurrencyAmount(items[i]->amount->value)));
On 2017/03/06 at 15:30:47, Mathieu Perreault wrote:
> this is nice! Could we update it in order_summary_view_controller as well?

Sure, done.

https://codereview.chromium.org/2730123002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_sheet_view_controller.cc:364: constexpr
SkColor kMoreItemsLightGray = 0xFF757575;
On 2017/03/06 at 15:30:49, Mathieu Perreault wrote:
> In editor view controller I was doing this:
> 
>   label->SetDisabledColor(label->GetNativeTheme()->GetSystemColor(
>       ui::NativeTheme::kColorId_LabelDisabledColor));
>   label->SetEnabled(false);
> 
> I guess 1 comment and one question
> (1) Let's use label->GetNativeTheme()->GetSystemColor(
>       ui::NativeTheme::kColorId_LabelDisabledColor)
> (2) seems like there are multiple ways to skin a cat! Does
SetAutoColorReadabilityEnabled do something interesting vs
SetDisabledColor+SetDisabled? Perhaps the clickability of the label?

(1) Done.
(2) It looks like SetAutoColorReadabilityEnabled(false) prevents the view from
fudging the color to contrast on the background properly. There's no advantage
to that in our case IMO, so your way is strictly better.

https://codereview.chromium.org/2730123002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/payments/payment_sheet_view_controller.cc:374:
CreateBoldLabel(base::ASCIIToUTF16(request()->details()->total->label))
On 2017/03/06 at 15:30:49, Mathieu Perreault wrote:
> Where is the total amount being added?

The CreateOrderSummarySectionContent() call below does. It's a remnant of
earlier times. I inlined the code instead.

[commit-bot: I haz the power, 2017-03-06 16:00: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, 2017-03-06 16:39:39]

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

[commit-bot: I haz the power, 2017-03-06 16:39:40]

Dry run: This issue passed the CQ dry run.

[Mathieu, 2017-03-06 20:22:47]

lgtm

[anthonyvd, 2017-03-06 20:23:25]

The CQ bit was checked by anthonyvd@chromium.org

[commit-bot: I haz the power, 2017-03-06 20:24:34]

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-06 20:34:54]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1488831805935010,
"parent_rev": "84d67b491f426b51a8d19411d313738d0aa9e240", "commit_rev":
"b0233f26f385e8bc8f0a77907499184cfc256c38"}

[commit-bot: I haz the power, 2017-03-06 20:35:38]

Description was changed from

==========
[Web Payments] Add inline items to the Order Summary section.

BUG=698322
==========

to

==========
[Web Payments] Add inline items to the Order Summary section.

BUG=698322

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

[commit-bot: I haz the power, 2017-03-06 20:35:40]

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