Chrome Cleaner UI: Add logs upload permission checkbox to the dialog

chrome/browser/ui/views/chrome_cleaner_dialog_win.cc

Index: chrome/browser/ui/views/chrome_cleaner_dialog_win.cc
diff --git a/chrome/browser/ui/views/chrome_cleaner_dialog_win.cc b/chrome/browser/ui/views/chrome_cleaner_dialog_win.cc
index 6f82660b6b0ce6a3caddab32702b6b2f1c665bff..c876a5b7ffa74b5a3373f3351d07cda5db36310a 100644
--- a/chrome/browser/ui/views/chrome_cleaner_dialog_win.cc
+++ b/chrome/browser/ui/views/chrome_cleaner_dialog_win.cc
@@ -19,9 +19,11 @@
 #include "ui/gfx/geometry/insets.h"
 #include "ui/gfx/native_widget_types.h"
 #include "ui/gfx/text_constants.h"
+#include "ui/views/controls/button/checkbox.h"
 #include "ui/views/controls/button/md_text_button.h"
 #include "ui/views/controls/label.h"
 #include "ui/views/layout/box_layout.h"
+#include "ui/views/layout/layout_provider.h"
 #include "ui/views/widget/widget.h"
 
 namespace chrome {
@@ -45,7 +47,10 @@ constexpr int kDialogWidth = 448;
 
 ChromeCleanerDialog::ChromeCleanerDialog(
     safe_browsing::ChromeCleanerDialogController* controller)
-    : browser_(nullptr), controller_(controller) {
+    : browser_(nullptr),
+      controller_(controller),
+      details_button_(nullptr),
+      logs_permission_checkbox_(nullptr) {
   DCHECK(controller_);
 
   SetLayoutManager(
@@ -92,6 +97,22 @@ base::string16 ChromeCleanerDialog::GetWindowTitle() const {
 
 // DialogDelegate overrides.
 
+views::View* ChromeCleanerDialog::CreateFootnoteView() {
+  DCHECK(!logs_permission_checkbox_);
+  DCHECK(controller_);
+
+  views::View* footnote_view = new views::View();
+  footnote_view->SetLayoutManager(new views::BoxLayout(
+      views::BoxLayout::kVertical, ChromeLayoutProvider::Get()->GetInsetsMetric(
+                                       views::INSETS_DIALOG_CONTENTS)));

[msw, 2017/06/29 21:52:48]

nit: can you just give the checkbox view an empty border with this inset,
instead of nesting the checkbox within a view and using a layout manager to add
some border padding?

[alito, 2017/06/30 00:00:09]

I tried several ways, but I cannot get it to work.

I tried, among other things,

1)
  logs_permission_checkbox_->set_border(CreateEmptyBorder(...));

2)
  std::unique_ptr<views::LabelButtonBorder> border =
      logs_permission_checkbox_->CreateDefaultBorder();
  border->set_insets(logs_permission_checkbox_->border()->GetInsets() +
                     ChromeLayoutProvider::Get()->GetInsetsMetric(
                         views::INSETS_DIALOG_CONTENTS));
  logs_permission_checkbox_->SetBorder(std::move(border));

3)
  Adding the checkbox to a View to which I add an empty border

None of them work (for example, the vertical size of the the footnote area
remains unchanged irrespective of the added border's top and bottom insets). Any
suggestions?

[msw, 2017/06/30 00:26:36]

Hmm, that's strange. What you have is fine.

+  logs_permission_checkbox_ = new views::Checkbox(
+      l10n_util::GetStringUTF16(IDS_CHROME_CLEANUP_LOGS_PERMISSION));
+  logs_permission_checkbox_->SetChecked(controller_->LogsEnabled());
+  logs_permission_checkbox_->set_listener(this);
+  footnote_view->AddChildView(logs_permission_checkbox_);
+  return footnote_view;
+}
+
 base::string16 ChromeCleanerDialog::GetDialogButtonLabel(
     ui::DialogButton button) const {
   DCHECK(button == ui::DIALOG_BUTTON_OK || button == ui::DIALOG_BUTTON_CANCEL);
@@ -104,14 +125,17 @@ base::string16 ChromeCleanerDialog::GetDialogButtonLabel(
 }
 
 views::View* ChromeCleanerDialog::CreateExtraView() {
-  return views::MdTextButton::CreateSecondaryUiButton(
+  DCHECK(!details_button_);
+
+  details_button_ = views::MdTextButton::CreateSecondaryUiButton(
       this, l10n_util::GetStringUTF16(
                 IDS_CHROME_CLEANUP_PROMPT_DETAILS_BUTTON_LABEL));
+  return details_button_;
 }
 
 bool ChromeCleanerDialog::Accept() {
   if (controller_) {
-    controller_->Accept();
+    controller_->Accept(/*logs_enabled=*/logs_permission_checkbox_->checked());

[msw, 2017/06/29 21:52:48]

Consider just calling controller_->SetLogsEnabled before calling Accept (and
DetailsButtonClicked) to just use one clear setter function rather than passing
a commented bool argument to these other two functions.

[alito, 2017/06/30 00:00:09]

I thought about that. If this dialog was the only place from which the user
could accept the cleanup operation that is being offered, I would be all for it.
But we are going to have a webui card in the settings page, and later, we are
going to introduce a stand-alone page where users can manually request a
cleanup. Given the possibly privacy implications of forgetting to call
SetLogsEnabled(), I think it makes sense to keep it this way and force all UI
surfaces to send the permission bit when the user accepts.

[msw, 2017/06/30 00:26:36]

Hmm, it seems like you really only need to call SetLogsEnabled when the checkbox
is pressed, not on each Accept/DetailsButtonClicked call. It seems a better
protection of privacy to only set that bit when the user explicitly toggles the
checkbox, and not via bool arguments to separate functions. Tangentially: what
should happen if the user toggles the checkbox and then clicks cancel/close?

[alito, 2017/06/30 00:54:02]

Some more details: a logs upload permission checkbox will also be present in
Chrome Cleaner's webui page. It is possible (though it should be rare) for a
user to have the webui page and the modal dialog open at the same time in
different browser windows.

Since the dialog's checkbox is not checked/unchecked if a user interacts with
the checkbox on the webui page, it is not enough to rely only on calling
SetLogsEnabled when the user interacts with the dialog's checkbox. We need to
make sure that when the user clicks accept/details in the dialog, it is the
state of the dialog's checkbox at that moment that determines the logs upload
permission bit.

Re your other question, if the user clicks cancel/close, the cleanup operation
is canceled and no logs are uploaded (irrespective of the permission bit).

[msw, 2017/06/30 01:15:58]

Thanks for the additional details and thinking through edges cases.

[msw, 2017/06/30 01:18:40]

Actually, I wonder if then there's no need for SetLogsEnabled to be public or
called when the checkbox value changes, if it's like a form, the value only need
be submitted by Accept and DetailsButtonClicked, and this view doesn't need to
be a listener for the checkbox, it need only read the state when calling those
functions. Either way, still lgtm

     controller_ = nullptr;
   }
   return true;
@@ -145,11 +169,18 @@ void ChromeCleanerDialog::ButtonPressed(views::Button* sender,
                                         const ui::Event& event) {
   DCHECK(browser_);
 
-  // TODO(alito): Navigate to the webui version of the Chrome Cleaner UI when
-  // that is implemented.
-  if (controller_) {
-    controller_->DetailsButtonClicked();
-    controller_ = nullptr;
+  if (sender == details_button_) {
+    if (controller_) {
+      controller_->DetailsButtonClicked(
+          /*logs_enabled=*/logs_permission_checkbox_->checked());
+      controller_ = nullptr;

[robertshield, 2017/06/29 18:43:14]

double checking: there's no way that Show() would get called on the same
instance after this right?

[alito, 2017/06/29 19:06:34]

No. Show() is only called right after the instance is created in
ShowChromeCleanerPrompt().

+    }
+    GetWidget()->Close();
+    return;
   }
-  GetWidget()->Close();
+
+  DCHECK_EQ(logs_permission_checkbox_, sender);
+
+  if (controller_)
+    controller_->SetLogsEnabled(logs_permission_checkbox_->checked());
 }