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

chrome/browser/ui/views/payments/payment_sheet_view_controller.cc

Index: chrome/browser/ui/views/payments/payment_sheet_view_controller.cc
diff --git a/chrome/browser/ui/views/payments/payment_sheet_view_controller.cc b/chrome/browser/ui/views/payments/payment_sheet_view_controller.cc
index 8fbc5f0260c697055a589cb3f066b9367c048258..089c51aca4756e3ccaf0fd1190062f9a59af08e6 100644
--- a/chrome/browser/ui/views/payments/payment_sheet_view_controller.cc
+++ b/chrome/browser/ui/views/payments/payment_sheet_view_controller.cc
@@ -12,6 +12,7 @@
 
 #include "base/macros.h"
 #include "base/memory/ptr_util.h"
+#include "base/strings/string_number_conversions.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/browser_process.h"
@@ -164,6 +165,20 @@ std::unique_ptr<views::Button> CreatePaymentSheetRow(
   return std::move(row);
 }
 
+std::unique_ptr<views::GridLayout> CreateOrderSummarySectionContainerLayout(

[Mathieu, 2017/03/06 15:30:45]

Let's document this a bit, including what |host| and |trailing| are.

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

Oops, good call. Done.

+    views::View* host,
+    bool trailing) {
+  std::unique_ptr<views::GridLayout> layout =
+      base::MakeUnique<views::GridLayout>(host);
+
+  views::ColumnSet* columns = layout->AddColumnSet(0);
+  columns->AddColumn(
+      trailing ? views::GridLayout::TRAILING : views::GridLayout::LEADING,
+      views::GridLayout::LEADING, 1, views::GridLayout::USE_PREF, 0, 0);
+
+  return layout;
+}
+
 }  // namespace
 
 PaymentSheetViewController::PaymentSheetViewController(
@@ -295,26 +310,80 @@ PaymentSheetViewController::CreateOrderSummarySectionContent() {
       request()->details()->total->amount->currency_system,
       g_browser_process->GetApplicationLocale());
   base::string16 label_value = l10n_util::GetStringFUTF16(
-      IDS_PAYMENT_REQUEST_ORDER_SUMMARY_SECTION_TOTAL_FORMAT,
-      base::UTF8ToUTF16(request()->details()->total->label),
+      IDS_PAYMENT_REQUEST_ORDER_SUMMARY_SHEET_TOTAL_FORMAT,
       base::UTF8ToUTF16(formatter->formatted_currency_code()),
       formatter->Format(request()->details()->total->amount->value));
 
-  return base::MakeUnique<views::Label>(label_value);
+  return CreateBoldLabel(label_value);
 }
 
 // Creates the Order Summary row, which contains an "Order Summary" label,
-// a Total Amount label, and a Chevron.
+// an inline list of display items, a Total Amount label, and a Chevron.
 // +----------------------------------------------+
-// | Order Summary           Total USD $12.34   > |
+// | Order Summary   Item 1            $ 1.34     |
+// |                 Item 2            $ 2.00   > |
+// |                 2 more items...              |
+// |                 Total         USD $12.34     |
 // +----------------------------------------------+
 std::unique_ptr<views::Button>
 PaymentSheetViewController::CreatePaymentSheetSummaryRow() {
+  std::unique_ptr<views::View> item_summary = base::MakeUnique<views::View>();

[Mathieu, 2017/03/06 15:30:48]

nit: item_summaries?

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

Done.

+  std::unique_ptr<views::GridLayout> item_summary_layout =
+      CreateOrderSummarySectionContainerLayout(item_summary.get(),
+                                               false /* trailing */);

[Mathieu, 2017/03/06 15:30:49]

nit: would prefer /*trailing=*/false otherwise it could be interpreted both ways

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

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

+
+  std::unique_ptr<views::View> item_amounts = base::MakeUnique<views::View>();
+  std::unique_ptr<views::GridLayout> item_amounts_layout =
+      CreateOrderSummarySectionContainerLayout(item_amounts.get(),
+                                               true /* trailing */);
+
+  const std::vector<mojom::PaymentItemPtr>& items =
+      request()->details()->display_items;
+  // The inline items section contains the first 2 display items of the
+  // request's details, followed by a label indicating "N more items..." if
+  // there are more than 2 items in the details. The total label and amount
+  // always follow.
+

[Mathieu, 2017/03/06 15:30:49]

nit: remove extra line

[anthonyvd, 2017/03/06 16:00:20]

Done.

+  for (size_t i = 0; i < items.size() && i < 2; ++i) {

[Mathieu, 2017/03/06 15:30:45]

constexpr int kMaxNumberOfItemsShown = 2; and reuse below.

OR

Consider reusing |i| after the loop has finished.

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

Done. 

+    item_summary_layout->StartRow(0, 0);
+    item_summary_layout->AddView(
+        new views::Label(base::ASCIIToUTF16(items[i]->label)));
+
+    item_amounts_layout->StartRow(0, 0);
+    item_amounts_layout->AddView(new views::Label(
+        request()->GetFormattedCurrencyAmount(items[i]->amount->value)));

[Mathieu, 2017/03/06 15:30:47]

this is nice! Could we update it in order_summary_view_controller as well?

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

Sure, done.

+  }
+
+  int hidden_item_count = items.size() - 2;
+  if (hidden_item_count > 0) {
+    item_summary_layout->StartRow(0, 0);
+    views::Label* label = new views::Label(
+        l10n_util::GetStringFUTF16(IDS_PAYMENT_REQUEST_ORDER_SUMMARY_MORE_ITEMS,
+                                   base::IntToString16(hidden_item_count)));
+    label->SetAutoColorReadabilityEnabled(false);
+    constexpr SkColor kMoreItemsLightGray = 0xFF757575;

[Mathieu, 2017/03/06 15:30:49]

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?

[anthonyvd, 2017/03/06 16:00:20]

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

+    label->SetEnabledColor(kMoreItemsLightGray);
+    item_summary_layout->AddView(label);
+
+    item_amounts_layout->StartRow(0, 0);
+    item_amounts_layout->AddView(new views::Label(base::ASCIIToUTF16("")));
+  }
+
+  item_summary_layout->StartRow(0, 0);
+  item_summary_layout->AddView(
+      CreateBoldLabel(base::ASCIIToUTF16(request()->details()->total->label))

[Mathieu, 2017/03/06 15:30:49]

Where is the total amount being added?

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

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

+          .release());
+
+  item_amounts_layout->StartRow(0, 0);
+  item_amounts_layout->AddView(CreateOrderSummarySectionContent().release());
+
+  item_summary->SetLayoutManager(item_summary_layout.release());
+  item_amounts->SetLayoutManager(item_amounts_layout.release());
+
   std::unique_ptr<views::Button> section = CreatePaymentSheetRow(
       this,
       l10n_util::GetStringUTF16(IDS_PAYMENT_REQUEST_ORDER_SUMMARY_SECTION_NAME),
-      std::unique_ptr<views::View>(nullptr),
-      CreateOrderSummarySectionContent(),
+      std::move(item_summary), std::move(item_amounts),
       widest_name_column_view_width_);
   section->set_tag(static_cast<int>(
       PaymentSheetViewControllerTags::SHOW_ORDER_SUMMARY_BUTTON));