[Web Payments] Prettify the payment sheet rows in some states.

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 c7706ff744650a9127975665bf5f521cf2748418..66e21c711f44be28475fa8f042d0ae95f3a29d17 100644
--- a/chrome/browser/ui/views/payments/payment_sheet_view_controller.cc
+++ b/chrome/browser/ui/views/payments/payment_sheet_view_controller.cc
@@ -87,24 +87,16 @@ int ComputeWidestNameColumnViewWidth() {
   return widest_column_width;
 }
 
-// Creates a clickable row to be displayed in the Payment Sheet. It contains
-// a section name and some content, followed by a chevron as a clickability
-// affordance. Both, either, or none of |content_view| and |extra_content_view|
-// may be present, the difference between the two being that content is pinned
-// to the left and extra_content is pinned to the right.
-// The row also displays a light gray horizontal ruler on its lower boundary.
-// The name column has a fixed width equal to |name_column_width|.
-// +----------------------------+
-// | Name | Content | Extra | > |
-// +~~~~~~~~~~~~~~~~~~~~~~~~~~~~+ <-- ruler
 std::unique_ptr<views::Button> CreatePaymentSheetRow(
     views::ButtonListener* listener,
     const base::string16& section_name,
     std::unique_ptr<views::View> content_view,
     std::unique_ptr<views::View> extra_content_view,
+    std::unique_ptr<views::View> trailing_button,
+    bool clickable,
     int name_column_width) {
   std::unique_ptr<PaymentRequestRowView> row =
-      base::MakeUnique<PaymentRequestRowView>(listener);
+      base::MakeUnique<PaymentRequestRowView>(listener, clickable);
   views::GridLayout* layout = new views::GridLayout(row.get());
 
   // The rows have extra inset compared to the header so that their right edge
@@ -135,7 +127,7 @@ std::unique_ptr<views::Button> CreatePaymentSheetRow(
                      0, views::GridLayout::USE_PREF, 0, 0);
 
   columns->AddPaddingColumn(0, kPaddingColumnsWidth);
-  // A column for the chevron.
+  // A column for the trailing_button.
   columns->AddColumn(views::GridLayout::TRAILING, views::GridLayout::CENTER,
                      0, views::GridLayout::USE_PREF, 0, 0);
 
@@ -157,14 +149,59 @@ std::unique_ptr<views::Button> CreatePaymentSheetRow(
     layout->SkipColumns(1);
   }
 
-  views::ImageView* chevron = new views::ImageView();
+  layout->AddView(trailing_button.release());
+
+  return std::move(row);
+}
+
+std::unique_ptr<views::Button> CreatePaymentSheetRowWithButton(

[Mathieu, 2017/04/13 00:12:45]

would be great to have some ASCII here too

[anthonyvd, 2017/04/13 18:33:33]

Done.

+    views::ButtonListener* listener,
+    const base::string16& section_name,
+    const base::string16& truncated_content,
+    const base::string16& button_string,
+    int button_tag,
+    int button_id,
+    int name_column_width) {
+  std::unique_ptr<views::Button> button(
+      views::MdTextButton::CreateSecondaryUiBlueButton(listener,
+                                                       button_string));
+  button->set_tag(button_tag);
+  button->set_id(button_id);
+  std::unique_ptr<views::Label> content_view =
+      base::MakeUnique<views::Label>(truncated_content);
+  return CreatePaymentSheetRow(listener, section_name, std::move(content_view),
+                               nullptr, std::move(button),
+                               /* clickable= */ false, name_column_width);

[Mathieu, 2017/04/13 00:12:45]

very nit: I prefer /*clickable=*/false because then it stays one item once
formatting is done (although it's possible git cl format does the right thing
too)

[anthonyvd, 2017/04/13 18:33:33]

Done.

+}
+
+// Creates a clickable row to be displayed in the Payment Sheet. It contains
+// a section name and some content, followed by a chevron as a clickability
+// affordance. Both, either, or none of |content_view| and |extra_content_view|
+// may be present, the difference between the two being that content is pinned
+// to the left and extra_content is pinned to the right.
+// The row also displays a light gray horizontal ruler on its lower boundary.
+// The name column has a fixed width equal to |name_column_width|.
+// +----------------------------+
+// | Name | Content | Extra | > |
+// +~~~~~~~~~~~~~~~~~~~~~~~~~~~~+ <-- ruler
+std::unique_ptr<views::Button> CreatePaymentSheetRowWithChevron(
+    views::ButtonListener* listener,
+    const base::string16& section_name,
+    std::unique_ptr<views::View> content_view,
+    std::unique_ptr<views::View> extra_content_view,
+    int name_column_width) {

[Mathieu, 2017/04/13 00:12:45]

could we also accept the section_tag and section_id here?

[anthonyvd, 2017/04/13 18:33:33]

Done.

+  std::unique_ptr<views::ImageView> chevron =
+      base::MakeUnique<views::ImageView>();
   chevron->set_can_process_events_within_subtree(false);
+  std::unique_ptr<views::Label> label =
+      base::MakeUnique<views::Label>(section_name);
   chevron->SetImage(gfx::CreateVectorIcon(
       views::kSubmenuArrowIcon,
-      color_utils::DeriveDefaultIconColor(name_label->enabled_color())));
-  layout->AddView(chevron);
-
-  return std::move(row);
+      color_utils::DeriveDefaultIconColor(label->enabled_color())));
+  return CreatePaymentSheetRow(listener, section_name, std::move(content_view),
+                               std::move(extra_content_view),
+                               std::move(chevron), /* clickable= */ true,
+                               name_column_width);
 }
 
 // Creates a GridLayout object to be used in the Order Summary section's list of
@@ -380,7 +417,7 @@ PaymentSheetViewController::CreatePaymentSheetSummaryRow() {
   item_summaries->SetLayoutManager(item_summaries_layout.release());
   item_amounts->SetLayoutManager(item_amounts_layout.release());
 
-  std::unique_ptr<views::Button> section = CreatePaymentSheetRow(
+  std::unique_ptr<views::Button> section = CreatePaymentSheetRowWithChevron(
       this,
       l10n_util::GetStringUTF16(IDS_PAYMENT_REQUEST_ORDER_SUMMARY_SECTION_NAME),
       std::move(item_summaries), std::move(item_amounts),
@@ -410,14 +447,31 @@ PaymentSheetViewController::CreateShippingSectionContent() {
 // |                    1800MYPOTUS               |
 // +----------------------------------------------+
 std::unique_ptr<views::Button> PaymentSheetViewController::CreateShippingRow() {
-  std::unique_ptr<views::Button> section = CreatePaymentSheetRow(
-      this, GetShippingAddressSectionString(spec()->shipping_type()),
-      CreateShippingSectionContent(), std::unique_ptr<views::View>(nullptr),
-      widest_name_column_view_width_);
-  section->set_tag(
-      static_cast<int>(PaymentSheetViewControllerTags::SHOW_SHIPPING_BUTTON));
-  section->set_id(
-      static_cast<int>(DialogViewID::PAYMENT_SHEET_SHIPPING_ADDRESS_SECTION));
+  std::unique_ptr<views::Button> section;
+  if (state()->selected_shipping_profile()) {
+    section = CreatePaymentSheetRowWithChevron(
+        this, GetShippingAddressSectionString(spec()->shipping_type()),
+        CreateShippingSectionContent(), std::unique_ptr<views::View>(nullptr),

[Mathieu, 2017/04/13 00:12:45]

I'm curious, we're the only ones that do std::unique_ptr<views::View>(nullptr)
in the codebase. I see a lot more of base::MakeUnique<...>(nullptr), can we do
that?

[anthonyvd, 2017/04/13 18:33:33]

I think just nullptr is fine and we have that in other places. Let's go with
that everywhere since it's the shorter form (and nullptr doesn't change
ownership so the semantics of unique_ptr are irrelevant).

+        widest_name_column_view_width_);
+    section->set_tag(
+        static_cast<int>(PaymentSheetViewControllerTags::SHOW_SHIPPING_BUTTON));
+    section->set_id(
+        static_cast<int>(DialogViewID::PAYMENT_SHEET_SHIPPING_ADDRESS_SECTION));
+  } else {
+    base::string16 button_string =
+        state()->shipping_profiles().size()
+            ? l10n_util::GetStringUTF16(
+                  IDS_PAYMENT_REQUEST_CHOOSE_SECTION_BUTTON)
+            : l10n_util::GetStringUTF16(IDS_PAYMENT_REQUEST_ADD_SECTION_BUTTON);
+
+    section = CreatePaymentSheetRowWithButton(
+        this, GetShippingAddressSectionString(spec()->shipping_type()),
+        base::ASCIIToUTF16(""), button_string,
+        static_cast<int>(PaymentSheetViewControllerTags::SHOW_SHIPPING_BUTTON),
+        static_cast<int>(DialogViewID::PAYMENT_SHEET_SHIPPING_ADDRESS_SECTION),

[Mathieu, 2017/04/13 00:12:45]

I think we should create a different DialogViewID for buttons, so that we can
test the proper interactions

[anthonyvd, 2017/04/13 18:33:33]

Done.

+        widest_name_column_view_width_);
+  }
+
   return section;
 }
 
@@ -434,6 +488,7 @@ PaymentSheetViewController::CreatePaymentMethodRow() {
 
   std::unique_ptr<views::View> content_view;
   std::unique_ptr<views::ImageView> card_icon_view;
+  std::unique_ptr<views::Button> section;
   if (selected_instrument) {
     content_view = base::MakeUnique<views::View>();
 
@@ -451,19 +506,35 @@ PaymentSheetViewController::CreatePaymentMethodRow() {
     card_icon_view = CreateInstrumentIconView(
         selected_instrument->icon_resource_id(), selected_instrument->label());
     card_icon_view->SetImageSize(gfx::Size(32, 20));
+
+    section = CreatePaymentSheetRowWithChevron(
+        this,
+        l10n_util::GetStringUTF16(
+            IDS_PAYMENT_REQUEST_PAYMENT_METHOD_SECTION_NAME),
+        std::move(content_view), std::move(card_icon_view),
+        widest_name_column_view_width_);
+    section->set_tag(static_cast<int>(
+        PaymentSheetViewControllerTags::SHOW_PAYMENT_METHOD_BUTTON));
+    section->set_id(
+        static_cast<int>(DialogViewID::PAYMENT_SHEET_PAYMENT_METHOD_SECTION));
+  } else {
+    base::string16 button_string =
+        state()->available_instruments().size()
+            ? l10n_util::GetStringUTF16(
+                  IDS_PAYMENT_REQUEST_CHOOSE_SECTION_BUTTON)
+            : l10n_util::GetStringUTF16(IDS_PAYMENT_REQUEST_ADD_SECTION_BUTTON);
+
+    section = CreatePaymentSheetRowWithButton(
+        this,
+        l10n_util::GetStringUTF16(
+            IDS_PAYMENT_REQUEST_PAYMENT_METHOD_SECTION_NAME),
+        base::ASCIIToUTF16(""), button_string,
+        static_cast<int>(
+            PaymentSheetViewControllerTags::SHOW_PAYMENT_METHOD_BUTTON),
+        static_cast<int>(DialogViewID::PAYMENT_SHEET_PAYMENT_METHOD_SECTION),
+        widest_name_column_view_width_);
   }
 
-  std::unique_ptr<views::Button> section = CreatePaymentSheetRow(
-      this,
-      l10n_util::GetStringUTF16(
-          IDS_PAYMENT_REQUEST_PAYMENT_METHOD_SECTION_NAME),
-      std::move(content_view),
-      std::move(card_icon_view),
-      widest_name_column_view_width_);
-  section->set_tag(static_cast<int>(
-      PaymentSheetViewControllerTags::SHOW_PAYMENT_METHOD_BUTTON));
-  section->set_id(
-      static_cast<int>(DialogViewID::PAYMENT_SHEET_PAYMENT_METHOD_SECTION));
   return section;
 }
 
@@ -485,15 +556,36 @@ PaymentSheetViewController::CreateContactInfoSectionContent() {
 // +----------------------------------------------+
 std::unique_ptr<views::Button>
 PaymentSheetViewController::CreateContactInfoRow() {
-  std::unique_ptr<views::Button> section = CreatePaymentSheetRow(
-      this,
-      l10n_util::GetStringUTF16(IDS_PAYMENT_REQUEST_CONTACT_INFO_SECTION_NAME),
-      CreateContactInfoSectionContent(), std::unique_ptr<views::View>(nullptr),
-      widest_name_column_view_width_);
-  section->set_tag(static_cast<int>(
-      PaymentSheetViewControllerTags::SHOW_CONTACT_INFO_BUTTON));
-  section->set_id(
-      static_cast<int>(DialogViewID::PAYMENT_SHEET_CONTACT_INFO_SECTION));
+  std::unique_ptr<views::Button> section;
+  if (state()->selected_contact_profile()) {
+    section = CreatePaymentSheetRowWithChevron(
+        this,
+        l10n_util::GetStringUTF16(
+            IDS_PAYMENT_REQUEST_CONTACT_INFO_SECTION_NAME),
+        CreateContactInfoSectionContent(),
+        std::unique_ptr<views::View>(nullptr), widest_name_column_view_width_);
+    section->set_tag(static_cast<int>(
+        PaymentSheetViewControllerTags::SHOW_CONTACT_INFO_BUTTON));
+    section->set_id(
+        static_cast<int>(DialogViewID::PAYMENT_SHEET_CONTACT_INFO_SECTION));
+  } else {
+    base::string16 button_string =
+        state()->contact_profiles().size()
+            ? l10n_util::GetStringUTF16(
+                  IDS_PAYMENT_REQUEST_CHOOSE_SECTION_BUTTON)
+            : l10n_util::GetStringUTF16(IDS_PAYMENT_REQUEST_ADD_SECTION_BUTTON);
+
+    section = CreatePaymentSheetRowWithButton(
+        this,
+        l10n_util::GetStringUTF16(
+            IDS_PAYMENT_REQUEST_CONTACT_INFO_SECTION_NAME),
+        base::ASCIIToUTF16(""), button_string,
+        static_cast<int>(
+            PaymentSheetViewControllerTags::SHOW_CONTACT_INFO_BUTTON),
+        static_cast<int>(DialogViewID::PAYMENT_SHEET_CONTACT_INFO_SECTION),
+        widest_name_column_view_width_);
+  }
+
   return section;
 }
 
@@ -501,21 +593,43 @@ std::unique_ptr<views::Button>
 PaymentSheetViewController::CreateShippingOptionRow() {
   mojom::PaymentShippingOption* selected_option =
       spec()->selected_shipping_option();
-  if (!selected_option)
-    return nullptr;
-
-  std::unique_ptr<views::View> option_label = CreateShippingOptionLabel(
-      selected_option, selected_option ? spec()->GetFormattedCurrencyAmount(
-                                             selected_option->amount->value)
-                                       : base::ASCIIToUTF16(""));
-  std::unique_ptr<views::Button> section = CreatePaymentSheetRow(
-      this, GetShippingOptionSectionString(spec()->shipping_type()),
-      std::move(option_label), std::unique_ptr<views::View>(nullptr),
-      widest_name_column_view_width_);
-  section->set_tag(static_cast<int>(
-      PaymentSheetViewControllerTags::SHOW_SHIPPING_OPTION_BUTTON));
-  section->set_id(
-      static_cast<int>(DialogViewID::PAYMENT_SHEET_SHIPPING_OPTION_SECTION));
+
+  // The shipping option section displays the currently selected option if there
+  // is one. It's not possible to select an option without selecting an address
+  // first.
+  std::unique_ptr<views::Button> section;
+  if (state()->selected_shipping_profile()) {
+    if (spec()->details().shipping_options.size()) {

[Mathieu, 2017/04/13 00:12:45]

if (!... or .empty()

[anthonyvd, 2017/04/13 18:33:33]

Done.

+      // TODO(anthonyvd): Display placeholder if there's no available shipping
+      // option.
+      return nullptr;
+    }
+
+    std::unique_ptr<views::View> option_label = CreateShippingOptionLabel(
+        selected_option, selected_option ? spec()->GetFormattedCurrencyAmount(
+                                               selected_option->amount->value)
+                                         : base::ASCIIToUTF16(""));
+    section = CreatePaymentSheetRowWithChevron(
+        this, GetShippingOptionSectionString(spec()->shipping_type()),
+        std::move(option_label), std::unique_ptr<views::View>(nullptr),
+        widest_name_column_view_width_);
+    section->set_tag(static_cast<int>(
+        PaymentSheetViewControllerTags::SHOW_SHIPPING_OPTION_BUTTON));
+    section->set_id(
+        static_cast<int>(DialogViewID::PAYMENT_SHEET_SHIPPING_OPTION_SECTION));
+  } else {
+    // TODO(anthonyvd): This should be a disabled row with a disabled button and
+    // some text to indicate that an address must be selected first.
+    section = CreatePaymentSheetRowWithButton(
+        this, GetShippingOptionSectionString(spec()->shipping_type()),
+        base::ASCIIToUTF16(""),
+        l10n_util::GetStringUTF16(IDS_PAYMENT_REQUEST_CHOOSE_SECTION_BUTTON),
+        static_cast<int>(
+            PaymentSheetViewControllerTags::SHOW_SHIPPING_OPTION_BUTTON),
+        static_cast<int>(DialogViewID::PAYMENT_SHEET_SHIPPING_OPTION_SECTION),
+        widest_name_column_view_width_);
+  }
+
   return section;
 }