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

chrome/browser/ui/views/chrome_cleaner_dialog_win.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/chrome_cleaner_dialog_win.h"
 
 #include "base/strings/string16.h"
 #include "chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_dialog_controller_win.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_dialogs.h"
 #include "chrome/browser/ui/browser_window.h"
 #include "chrome/browser/ui/views/harmony/chrome_layout_provider.h"
 #include "chrome/grit/chromium_strings.h"
 #include "chrome/grit/generated_resources.h"
 #include "components/constrained_window/constrained_window_views.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/base/ui_base_types.h"
 #include "ui/events/event.h"
 #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 {
 
 void ShowChromeCleanerPrompt(
     Browser* browser,
     safe_browsing::ChromeCleanerDialogController* controller) {
   ChromeCleanerDialog* dialog = new ChromeCleanerDialog(controller);
   dialog->Show(browser);
 }
 
 }  // namespace chrome
 
 namespace {
 constexpr int kDialogWidth = 448;
 
 }  // namespace
 
 ////////////////////////////////////////////////////////////////////////////////
 // ChromeCleanerDialog
 
 ChromeCleanerDialog::ChromeCleanerDialog(
     safe_browsing::ChromeCleanerDialogController* controller)
-    : browser_(nullptr), controller_(controller) {
+    : browser_(nullptr),
+      controller_(controller),
+      details_button_(nullptr),
+      logs_permission_checkbox_(nullptr) {
   DCHECK(controller_);
 
   SetLayoutManager(
       new views::BoxLayout(views::BoxLayout::kVertical,
                            ChromeLayoutProvider::Get()->GetInsetsMetric(
                                views::INSETS_DIALOG_CONTENTS),
                            0));
   views::Label* label = new views::Label(
       l10n_util::GetStringUTF16(IDS_CHROME_CLEANUP_PROMPT_EXPLANATION));
   label->SetMultiLine(true);
@@ skipping 26 matching lines @@
   return ui::MODAL_TYPE_WINDOW;
 }
 
 base::string16 ChromeCleanerDialog::GetWindowTitle() const {
   DCHECK(controller_);
   return l10n_util::GetStringUTF16(IDS_CHROME_CLEANUP_PROMPT_TITLE);
 }
 
 // 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);
   DCHECK(controller_);
 
   return button == ui::DIALOG_BUTTON_OK
              ? l10n_util::GetStringUTF16(
                    IDS_CHROME_CLEANUP_PROMPT_REMOVE_BUTTON_LABEL)
              : DialogDelegate::GetDialogButtonLabel(button);
 }
 
 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;
 }
 
 bool ChromeCleanerDialog::Cancel() {
   if (controller_) {
     controller_->Cancel();
     controller_ = nullptr;
   }
@@ skipping 13 matching lines @@
 gfx::Size ChromeCleanerDialog::CalculatePreferredSize() const {
   return gfx::Size(kDialogWidth, GetHeightForWidth(kDialogWidth));
 }
 
 // views::ButtonListener overrides.
 
 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());
 }