Chrome Cleaner UI: Add a dialog class for the new UI

chrome/browser/ui/views/srt_prompt_dialog.cc

+// Copyright 2017 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/browser/ui/views/srt_prompt_dialog.h"
+
+#include <vector>
+
+#include "base/strings/string16.h"
+#include "chrome/app/vector_icons/vector_icons.h"
+#include "chrome/browser/safe_browsing/srt_prompt_controller.h"
+#include "chrome/browser/ui/browser.h"
+#include "chrome/browser/ui/chrome_web_modal_dialog_manager_delegate.h"
+#include "chrome/browser/ui/views/frame/browser_view.h"
+#include "components/constrained_window/constrained_window_views.h"
+#include "components/web_modal/web_contents_modal_dialog_host.h"
+#include "ui/base/ui_base_types.h"
+#include "ui/events/event.h"
+#include "ui/gfx/geometry/size.h"
+#include "ui/gfx/native_widget_types.h"
+#include "ui/gfx/paint_vector_icon.h"
+#include "ui/gfx/text_constants.h"
+#include "ui/native_theme/native_theme.h"
+#include "ui/views/controls/label.h"
+#include "ui/views/controls/scroll_view.h"
+#include "ui/views/controls/separator.h"
+#include "ui/views/layout/box_layout.h"
+#include "ui/views/layout/grid_layout.h"
+#include "ui/views/layout/layout_constants.h"
+#include "ui/views/widget/widget.h"
+
+namespace safe_browsing {
+
+// static
+void SRTPromptController::ShowSRTPrompt(Browser* browser,
+                                        SRTPromptController* controller) {
+  SRTPromptDialog* dialog = new SRTPromptDialog(controller);
+  dialog->Show(browser);
+}
+
+}  // namespace safe_browsing
+
+namespace {
+
+using LabelInfo = safe_browsing::SRTPromptController::LabelInfo;
+
+constexpr int kDialogWidth = 448;
+constexpr int kDetailsSectionMaxHeight = 150;
+constexpr int kBulletColumnWidth = 10;
+// Constants used for the layout of the label views.
+static constexpr int kMainColumSetId = 0;

[sky, 2017/04/05 21:17:42]

Be consistent. No need for the static here.

[alito, 2017/04/06 00:20:22]

Done.

+static constexpr int kBulletColumnSetId = 1;
+
+// Returns a view containing |item| with insets defined by |top|, |left|,
+// |bottom|, and |right|.
+views::View* CreateViewWithInsets(views::View* item,
+                                  int top,
+                                  int left,
+                                  int bottom,
+                                  int right) {
+  views::View* view = new views::View();
+  views::GridLayout* layout = new views::GridLayout(view);
+  view->SetLayoutManager(layout);
+  layout->SetInsets(top, left, bottom, right);
+  layout->AddColumnSet(0)->AddColumn(

[sky, 2017/04/05 21:17:42]

GridLayout seems like overkill here. Isn't this the same as a FillLayout and
EmptyBorder?

[alito, 2017/04/06 00:20:22]

Indeed. I had tried adding an empty border to the view before, but hadn't
realized that you also need to provide a layout manager.

+      views::GridLayout::LEADING, views::GridLayout::LEADING,
+      /*resize_percent=*/1, views::GridLayout::USE_PREF,
+      /*fixed_width=*/0,
+      /*min_width=*/0);
+  layout->StartRow(0, 0);
+  layout->AddView(item);
+
+  return view;
+}
+
+// Helper function used by |CreateLabelView()| below that adds |labels| to
+// |label_view|.
+void AddLabelsToLabelView(views::View* label_view,
+                          views::GridLayout* layout,
+                          const std::vector<LabelInfo>& labels) {
+  static constexpr base::char16 kBulletPoint[] = {0x2022, 0};
+
+  bool first_label = true;
+  bool last_label_was_bullet = false;
+  for (const LabelInfo& label_info : labels) {
+    const bool is_bullet = label_info.type == LabelInfo::BULLET_ITEM;
+    views::Label* label = new views::Label(label_info.text);
+    label->SetMultiLine(true);
+    label->SetHorizontalAlignment(gfx::ALIGN_LEFT);
+
+    // Do not add a padding row if
+    // - this is the first label being added, or
+    // - a bullet item is being added and the last label was also a bullet item.
+    bool skip_padding = first_label || (is_bullet && last_label_was_bullet);
+    if (!skip_padding)
+      layout->AddPaddingRow(0, views::kRelatedControlVerticalSpacing);
+
+    layout->StartRow(0, is_bullet ? kBulletColumnSetId : kMainColumSetId);
+    if (is_bullet) {
+      views::Label* bullet = new views::Label(base::string16(kBulletPoint));
+      layout->AddView(bullet);
+    }
+    layout->AddView(label);
+
+    last_label_was_bullet = is_bullet;
+    first_label = false;
+  }
+}
+
+// Creates a view that displays two types of labels: multiline labels
+// representing whole paragraphs or indented labels that are start with a bullet
+// point.
+//
+// A vertical padding of size |top_vertical_space| is added before the labels.
+views::View* CreateLabelView(int top_vertical_space,
+                             const std::vector<LabelInfo>& labels) {
+  views::View* label_view = new views::View();
+  views::GridLayout* layout = new views::GridLayout(label_view);
+  layout->SetInsets(0, views::kButtonHEdgeMarginNew, 0,
+                    views::kButtonHEdgeMarginNew);
+
+  label_view->SetLayoutManager(layout);
+
+  views::ColumnSet* main_column_set = layout->AddColumnSet(kMainColumSetId);
+  main_column_set->AddColumn(views::GridLayout::FILL,
+                             views::GridLayout::LEADING,
+                             /*resize_percent=*/1, views::GridLayout::USE_PREF,
+                             /*fixed_width=*/0,
+                             /*min_width=*/0);
+
+  views::ColumnSet* bullet_column_set_ =
+      layout->AddColumnSet(kBulletColumnSetId);
+  bullet_column_set_->AddPaddingColumn(
+      /*resize_percent=*/0, views::kUnrelatedControlLargeHorizontalSpacing);
+  bullet_column_set_->AddColumn(
+      views::GridLayout::FILL, views::GridLayout::LEADING,
+      /*resize_percent=*/0, views::GridLayout::USE_PREF,
+      /*fixed_width=*/0,
+      /*min_width=*/0);
+  bullet_column_set_->AddPaddingColumn(/*resize_percent=*/0,
+                                       kBulletColumnWidth);
+  bullet_column_set_->AddColumn(
+      views::GridLayout::FILL, views::GridLayout::LEADING,
+      /*resize_percent=*/1, views::GridLayout::USE_PREF,
+      /*fixed_width=*/0,
+      /*min_width=*/0);
+
+  if (top_vertical_space > 0)
+    layout->AddPaddingRow(/*vertical_resize=*/0, top_vertical_space);
+
+  AddLabelsToLabelView(label_view, layout, labels);
+
+  layout->AddPaddingRow(/*vertical_resize=*/0,
+                        views::kUnrelatedControlLargeHorizontalSpacing);
+  return label_view;
+}
+
+}  // namespace
+
+////////////////////////////////////////////////////////////////////////////////
+// SRTPromptDialog::ExpandableMessageView
+//
+// A view, whose visibilty can be toggled, and will be used for the details
+// section the main dialog.
+class SRTPromptDialog::ExpandableMessageView : public views::View {

[sky, 2017/04/05 21:17:42]

It seems like you're subclassing here for convenience and not specialization.
The only thing you really need is having visibility effect preferred size.
Consider remove all child views and then adding back as necessary. That way you
don't need a subclass.

[alito, 2017/04/06 00:20:22]

I plan to add animation to the expansion and folding of the details section.
Given that, I think it makes sense to keep this as a class?

[sky, 2017/04/06 02:10:02]

It's easy to add it back when you get there, vs once code is landed it's easy to
ignore.

[alito, 2017/04/06 03:37:46]

Fair enough. I've changed this to populate the details section when needed.

+ public:
+  explicit ExpandableMessageView(const std::vector<LabelInfo>& labels);
+  ~ExpandableMessageView() override;
+
+  void ToggleShowView();
+
+  // views::View overrides.
+  gfx::Size GetPreferredSize() const override;
+  int GetHeightForWidth(int width) const override;
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(ExpandableMessageView);
+};
+
+SRTPromptDialog::ExpandableMessageView::ExpandableMessageView(
+    const std::vector<LabelInfo>& labels) {
+  views::GridLayout* layout = new views::GridLayout(this);
+  SetLayoutManager(layout);
+
+  SetVisible(false);
+
+  constexpr int kColumnId = 0;
+  views::ColumnSet* column_set = layout->AddColumnSet(kColumnId);
+  column_set->AddColumn(views::GridLayout::FILL, views::GridLayout::LEADING,
+                        /*resize_percent=*/1, views::GridLayout::USE_PREF,
+                        /*fixed_width=*/0,
+                        /*min_width=*/0);
+
+  // Add the main message view inside a scroll view.
+  views::View* label_view =
+      CreateLabelView(views::kUnrelatedControlLargeHorizontalSpacing, labels);
+  views::ScrollView* scroll_view = new views::ScrollView();
+  scroll_view->ClipHeightTo(/*min_height=*/0, kDetailsSectionMaxHeight);
+  scroll_view->SetContents(label_view);
+  layout->StartRow(0, kColumnId);
+  layout->AddView(scroll_view);
+}
+
+SRTPromptDialog::ExpandableMessageView::~ExpandableMessageView() {}
+
+void SRTPromptDialog::ExpandableMessageView::ToggleShowView() {
+  SetVisible(!visible());
+  PreferredSizeChanged();
+}
+
+gfx::Size SRTPromptDialog::ExpandableMessageView::GetPreferredSize() const {
+  gfx::Size size = views::View::GetPreferredSize();
+  return gfx::Size(size.width(), visible() ? size.height() : 0);
+}
+
+int SRTPromptDialog::ExpandableMessageView::GetHeightForWidth(int width) const {
+  return GetPreferredSize().height();
+}
+
+////////////////////////////////////////////////////////////////////////////////
+// SRTPromptDialog
+
+SRTPromptDialog::SRTPromptDialog(safe_browsing::SRTPromptController* controller)
+    : browser_(nullptr),
+      controller_(controller),
+      interaction_done_(false),
+      details_view_(nullptr),
+      expand_details_button_(nullptr),
+      expand_details_button_color_(GetNativeTheme()->GetSystemColor(

[sky, 2017/04/05 21:17:42]

This variables is only used in this function. No need for it to be a member.

[alito, 2017/04/06 00:20:22]

A private member function now returns the color (after removing the icons as
members, I need to get the color in two functions).

+          ui::NativeTheme::kColorId_LinkEnabled)),
+      expand_icon_(

[sky, 2017/04/05 21:17:42]

Why do you need to cache these icons? Can you look them up as needed?

[alito, 2017/04/06 00:20:22]

Changed to look them up as needed.

+          gfx::CreateVectorIcon(kCaretDownIcon, expand_details_button_color_)),
+      fold_icon_(
+          gfx::CreateVectorIcon(kCaretUpIcon, expand_details_button_color_)) {
+  DCHECK(controller_);
+
+  SetLayoutManager(new views::BoxLayout(
+      /*orientation=*/views::BoxLayout::kVertical,
+      /*inside_border_horizontal_spacing=*/0,
+      /*inside_border_vertical_spacing=*/views::kPanelVertMargin,
+      /*between_child_spacing=*/0));
+
+  AddChildView(CreateLabelView(0, controller_->GetMainText()));
+  AddChildView(new views::Separator());
+
+  details_view_ = new ExpandableMessageView(controller_->GetDetailsText());
+  AddChildView(details_view_);
+
+  expand_details_button_ =
+      new views::LabelButton(this, controller_->GetShowDetailsLabel());
+  expand_details_button_->SetEnabledTextColors(expand_details_button_color_);
+  expand_details_button_->SetImage(
+      views::Button::STATE_NORMAL,
+      details_view_->visible() ? fold_icon_ : expand_icon_);
+  AddChildView(CreateViewWithInsets(
+      expand_details_button_, views::kPanelVertMargin,
+      views::kButtonHEdgeMarginNew, views::kPanelVertMargin,
+      views::kButtonHEdgeMarginNew));
+  AddChildView(new views::Separator());
+}
+
+SRTPromptDialog::~SRTPromptDialog() {
+  // Make sure the controller is correctly notified in case the dialog widget is
+  // closed by some other means than the dialog buttons.
+  Close();

[sky, 2017/04/05 21:17:42]

This class is owned by the widget and deleted when the widget is destroyed. You
should not need this call here.

[alito, 2017/04/06 00:20:22]

This call ensures that the controller is notified about the dialog being closed
even when the dialog is somehow closed without user interaction.

[sky, 2017/04/06 02:10:02]

It's better to be explicit rather than relying on the implementation of Close()
and the current implementation of Cancel(). For example, it's easy to miss (as I
did) that Close() calls Cancel() and may lead to some confusion.

[alito, 2017/04/06 03:37:46]

Changed to explicitly notify the controller if needed.

+}
+
+void SRTPromptDialog::Show(Browser* browser) {
+  DCHECK(browser);

[sky, 2017/04/05 21:17:42]

DCHECK(!browser_)?

[alito, 2017/04/06 00:20:22]

Done.

+
+  browser_ = browser;
+  BrowserView* browser_view = BrowserView::GetBrowserViewForBrowser(browser_);

[sky, 2017/04/05 21:17:42]

Why do you need to get the BrowserView for browser_? You can get the get the
NativeWindow by way of browser_->window()->GetNativeWindow().

[alito, 2017/04/06 00:20:22]

Ah! That's a shortcut I hadn't noticed.

+  constrained_window::CreateBrowserModalDialogViews(
+      this, browser_view->GetNativeWindow())
+      ->Show();
+  controller_->DialogShown();
+}
+
+// DialogModel overrides.
+
+bool SRTPromptDialog::ShouldDefaultButtonBeBlue() const {
+  return true;
+}
+
+// WidgetDelegate overrides.
+
+ui::ModalType SRTPromptDialog::GetModalType() const {
+  return ui::MODAL_TYPE_WINDOW;
+}
+
+base::string16 SRTPromptDialog::GetWindowTitle() const {
+  return controller_->GetWindowTitle();
+}
+
+// DialogDelegate overrides.
+
+base::string16 SRTPromptDialog::GetDialogButtonLabel(
+    ui::DialogButton button) const {
+  DCHECK(button == ui::DIALOG_BUTTON_OK || button == ui::DIALOG_BUTTON_CANCEL);
+
+  if (button == ui::DIALOG_BUTTON_OK)
+    return controller_->GetAcceptButtonLabel();
+  return DialogDelegate::GetDialogButtonLabel(button);
+}
+
+bool SRTPromptDialog::Accept() {
+  if (!interaction_done_) {
+    interaction_done_ = true;
+    controller_->Accept();
+  }
+  return true;
+}
+
+bool SRTPromptDialog::Cancel() {
+  if (!interaction_done_) {

[sky, 2017/04/05 21:17:42]

If the underlying widget is destroyed outside of cancel/accept then neither
accept or cancel is called. If you care, you should deal with that in the
destructor of this class.

[alito, 2017/04/06 00:20:22]

This is already handled in the destructor, which calls the Close() function.

[sky, 2017/04/06 02:10:02]

Acknowledged.

+    interaction_done_ = true;
+    controller_->Cancel();
+  }
+  return true;
+}
+
+// View overrides.
+
+gfx::Size SRTPromptDialog::GetPreferredSize() const {
+  return gfx::Size(kDialogWidth, GetHeightForWidth(kDialogWidth));
+}
+
+// views::ButtonListener overrides.
+
+void SRTPromptDialog::ButtonPressed(views::Button* sender,
+                                    const ui::Event& event) {
+  DCHECK_EQ(sender, expand_details_button_);
+  DCHECK(browser_);
+
+  details_view_->ToggleShowView();
+  expand_details_button_->SetText(details_view_->visible()
+                                      ? controller_->GetHideDetailsLabel()
+                                      : controller_->GetShowDetailsLabel());
+  expand_details_button_->SetImage(
+      views::Button::STATE_NORMAL,
+      details_view_->visible() ? fold_icon_ : expand_icon_);
+
+  ChromeWebModalDialogManagerDelegate* manager = browser_;
+  constrained_window::UpdateWidgetModalDialogPosition(
+      GetWidget(), manager->GetWebContentsModalDialogHost());
+}