[Web Payments] Refactor the row display code.

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 1fbb4bcfe4cc6e915a383460a0dfa42d56e35dbf..1d55e5cf602caabd4b23cb2202b738c5577a0162 100644
--- a/chrome/browser/ui/views/payments/payment_sheet_view_controller.cc
+++ b/chrome/browser/ui/views/payments/payment_sheet_view_controller.cc
@@ -9,11 +9,13 @@
 #include <utility>
 #include <vector>
 
+#include "base/macros.h"
 #include "base/memory/ptr_util.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/ui/views/payments/payment_request_dialog.h"
+#include "chrome/browser/ui/views/payments/payment_request_row_view.h"
 #include "chrome/browser/ui/views/payments/payment_request_views_util.h"
 #include "chrome/grit/generated_resources.h"
 #include "components/autofill/core/browser/autofill_data_util.h"
@@ -25,16 +27,13 @@
 #include "components/payments/payment_request.h"
 #include "components/strings/grit/components_strings.h"
 #include "content/public/browser/web_contents.h"
-#include "third_party/skia/include/core/SkColor.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/base/resource/resource_bundle.h"
 #include "ui/gfx/color_utils.h"
 #include "ui/gfx/font.h"
-#include "ui/gfx/geometry/insets.h"
 #include "ui/gfx/paint_vector_icon.h"
 #include "ui/gfx/range/range.h"
 #include "ui/views/border.h"
-#include "ui/views/controls/button/custom_button.h"
 #include "ui/views/controls/button/label_button.h"
 #include "ui/views/controls/button/md_text_button.h"
 #include "ui/views/controls/image_view.h"
@@ -59,84 +58,21 @@ enum class PaymentSheetViewControllerTags {
   SHOW_CONTACT_INFO_BUTTON,
 };
 
-// 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
-class PaymentSheetRow : public views::CustomButton {
+// This class's sole purpose is to prevent event processing by subviews of
+// clickable rows in the dialog. This is required to make sure that hover and
+// click events propagate to the row itself.
+class NoEventView : public views::View {
  public:
-  PaymentSheetRow(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)
-    : views::CustomButton(listener) {
-    SetBorder(views::CreateSolidSidedBorder(0, 0, 1, 0, SK_ColorLTGRAY));
-    views::GridLayout* layout = new views::GridLayout(this);
-
-    constexpr int kRowVerticalInset = 8;
-    // The rows have extra inset compared to the header so that their right edge
-    // lines up with the close button's X rather than its invisible right edge.
-    constexpr int kRowExtraRightInset = 8;
-    layout->SetInsets(
-        kRowVerticalInset, 0, kRowVerticalInset, kRowExtraRightInset);
-    SetLayoutManager(layout);
-
-    views::ColumnSet* columns = layout->AddColumnSet(0);
-    // A column for the section name.
-    columns->AddColumn(views::GridLayout::LEADING,
-                       views::GridLayout::LEADING,
-                       0,
-                       views::GridLayout::FIXED,
-                       name_column_width,
-                       0);
-
-    constexpr int kPaddingColumnsWidth = 25;
-    columns->AddPaddingColumn(0, kPaddingColumnsWidth);
-
-    // A column for the content.
-    columns->AddColumn(views::GridLayout::FILL, views::GridLayout::LEADING,
-                       1, views::GridLayout::USE_PREF, 0, 0);
-    // A column for the extra content.
-    columns->AddColumn(views::GridLayout::TRAILING, views::GridLayout::CENTER,
-                       0, views::GridLayout::USE_PREF, 0, 0);
+  NoEventView() {
+    SetLayoutManager(new views::FillLayout);
+  }
 
-    columns->AddPaddingColumn(0, kPaddingColumnsWidth);
-    // A column for the chevron.
-    columns->AddColumn(views::GridLayout::TRAILING, views::GridLayout::CENTER,
-                       0, views::GridLayout::USE_PREF, 0, 0);
+  ~NoEventView() override {}
 
-    layout->StartRow(0, 0);
-    views::Label* name_label = new views::Label(section_name);
-    layout->AddView(name_label);
-
-    if (content_view) {
-      layout->AddView(content_view.release());
-    } else {
-      layout->SkipColumns(1);
-    }
-
-    if (extra_content_view) {
-      layout->AddView(extra_content_view.release());
-    } else {
-      layout->SkipColumns(1);
-    }
-
-    views::ImageView* chevron = new views::ImageView();
-    chevron->SetImage(gfx::CreateVectorIcon(
-        views::kSubmenuArrowIcon,
-        color_utils::DeriveDefaultIconColor(name_label->enabled_color())));
-    layout->AddView(chevron);
-  }
+  bool CanProcessEventsWithinSubtree() const override { return false; }

[sky, 2017/01/25 21:45:20]

You are right that subclassing like is the only way to change the behavior. I'm
actually fine with adding a setter to View.

[anthonyvd, 2017/01/26 18:56:06]

Went ahead and added the setter. This'll be much simpler.

 
  private:
-  DISALLOW_COPY_AND_ASSIGN(PaymentSheetRow);
+  DISALLOW_COPY_AND_ASSIGN(NoEventView);
 };
 
 int ComputeWidestNameColumnViewWidth() {
@@ -164,6 +100,93 @@ 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,
+    int name_column_width) {
+  std::unique_ptr<PaymentRequestRowView> row =
+      base::MakeUnique<PaymentRequestRowView>(listener);
+  views::GridLayout* layout = new views::GridLayout(row.get());
+
+  // The rows have extra inset compared to the header so that their right edge
+  // lines up with the close button's X rather than its invisible right edge.
+  constexpr int kRowExtraRightInset = 8;
+  layout->SetInsets(kPaymentRequestRowVerticalInsets,
+                    kPaymentRequestRowHorizontalInsets,
+                    kPaymentRequestRowVerticalInsets,
+                    kPaymentRequestRowHorizontalInsets + kRowExtraRightInset);
+  row->SetLayoutManager(layout);
+
+  views::ColumnSet* columns = layout->AddColumnSet(0);
+  // A column for the section name.
+  columns->AddColumn(views::GridLayout::LEADING,
+                     views::GridLayout::LEADING,
+                     0,
+                     views::GridLayout::FIXED,
+                     name_column_width,
+                     0);
+
+  constexpr int kPaddingColumnsWidth = 25;
+  columns->AddPaddingColumn(0, kPaddingColumnsWidth);
+
+  // A column for the content.
+  columns->AddColumn(views::GridLayout::FILL, views::GridLayout::LEADING,
+                     1, views::GridLayout::USE_PREF, 0, 0);
+  // A column for the extra content.
+  columns->AddColumn(views::GridLayout::TRAILING, views::GridLayout::CENTER,
+                     0, views::GridLayout::USE_PREF, 0, 0);
+
+  columns->AddPaddingColumn(0, kPaddingColumnsWidth);
+  // A column for the chevron.
+  columns->AddColumn(views::GridLayout::TRAILING, views::GridLayout::CENTER,
+                     0, views::GridLayout::USE_PREF, 0, 0);
+
+  layout->StartRow(0, 0);
+  views::Label* name_label = new views::Label(section_name);
+  layout->AddView(name_label);
+
+  if (content_view) {
+    // Wrap this view in a NoEventView so that the contained subviews don't
+    // process click/hover events themselves.
+    std::unique_ptr<NoEventView> container = base::MakeUnique<NoEventView>();
+    container->AddChildView(content_view.release());
+    layout->AddView(container.release());
+  } else {
+    layout->SkipColumns(1);
+  }
+
+  if (extra_content_view) {
+    // Wrap this view in a NoEventView so that the contained subviews don't
+    // process click/hover events themselves.
+    std::unique_ptr<NoEventView> container = base::MakeUnique<NoEventView>();
+    container->AddChildView(extra_content_view.release());
+    layout->AddView(container.release());
+  } else {
+    layout->SkipColumns(1);
+  }
+
+  views::ImageView* chevron = new views::ImageView();
+  chevron->set_interactive(false);
+  chevron->SetImage(gfx::CreateVectorIcon(
+      views::kSubmenuArrowIcon,
+      color_utils::DeriveDefaultIconColor(name_label->enabled_color())));
+  layout->AddView(chevron);
+
+  return std::move(row);
+}
+
 }  // namespace
 
 PaymentSheetViewController::PaymentSheetViewController(
@@ -253,7 +276,7 @@ PaymentSheetViewController::CreateOrderSummarySectionContent() {
 // +----------------------------------------------+
 std::unique_ptr<views::Button>
 PaymentSheetViewController::CreatePaymentSheetSummaryRow() {
-  std::unique_ptr<views::Button> section = base::MakeUnique<PaymentSheetRow>(
+  std::unique_ptr<views::Button> section = CreatePaymentSheetRow(
       this,
       l10n_util::GetStringUTF16(IDS_PAYMENT_REQUEST_ORDER_SUMMARY_SECTION_NAME),
       std::unique_ptr<views::View>(nullptr),
@@ -283,7 +306,7 @@ PaymentSheetViewController::CreateShippingSectionContent() {
 // |                    1800MYPOTUS               |
 // +----------------------------------------------+
 std::unique_ptr<views::Button> PaymentSheetViewController::CreateShippingRow() {
-  std::unique_ptr<views::Button> section = base::MakeUnique<PaymentSheetRow>(
+  std::unique_ptr<views::Button> section = CreatePaymentSheetRow(
       this,
       l10n_util::GetStringUTF16(IDS_PAYMENT_REQUEST_SHIPPING_SECTION_NAME),
       CreateShippingSectionContent(), std::unique_ptr<views::View>(nullptr),
@@ -325,6 +348,7 @@ PaymentSheetViewController::CreatePaymentMethodRow() {
             g_browser_process->GetApplicationLocale())));
 
     card_icon_view = base::MakeUnique<views::ImageView>();
+    card_icon_view->set_interactive(false);
     card_icon_view->SetImage(
       ResourceBundle::GetSharedInstance()
           .GetImageNamed(autofill::data_util::GetPaymentRequestData(
@@ -337,7 +361,7 @@ PaymentSheetViewController::CreatePaymentMethodRow() {
     card_icon_view->SetImageSize(kCardIconSize);
   }
 
-  std::unique_ptr<views::Button> section = base::MakeUnique<PaymentSheetRow>(
+  std::unique_ptr<views::Button> section = CreatePaymentSheetRow(
       this,
       l10n_util::GetStringUTF16(
           IDS_PAYMENT_REQUEST_PAYMENT_METHOD_SECTION_NAME),
@@ -368,7 +392,7 @@ PaymentSheetViewController::CreateContactInfoSectionContent() {
 // +----------------------------------------------+
 std::unique_ptr<views::Button>
 PaymentSheetViewController::CreateContactInfoRow() {
-  std::unique_ptr<views::Button> section = base::MakeUnique<PaymentSheetRow>(
+  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),