Revert "Cleanup base::Optional use in Deep Scanning dialog code"

This reverts commit 0f668c37.

Reason for revert: Fails Linux MSan Tests crbug.com/1046693.

Original change's description:
> Cleanup base::Optional use in Deep Scanning dialog code
> 
> 2 instances of optional can be removed from DeepScanningDialog* code to
> make it more readable:
> - The access point no longer needs to be optional since all the access
>   points have been added to Chrome. A default value is still included
>   in order to simplify tests calling ShowForWebContents.
> 
> - The state variable in DeepScanningDialogViews is better as an enum
>   than an optional bool indicanting pending/success/failure, especially
>   since a timeout state is very likely in the future.
> 
> Change-Id: I767db78e428a607a7cc2dfe39d1d63c0f85c30d7
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2020891
> Reviewed-by: Daniel Rubery <drubery@chromium.org>
> Commit-Queue: Dominique Fauteux-Chapleau <domfc@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#735857}

TBR=drubery@chromium.org,domfc@chromium.org

Change-Id: Id4814d523935f867ab8fb69942281a6fc8fcf3b0
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 1046693
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2026970Reviewed-by: vitaliii <vitaliii@chromium.org>
Commit-Queue: vitaliii <vitaliii@chromium.org>
Cr-Commit-Position: refs/heads/master@{#736302}

chrome/browser/safe_browsing/cloud_content_scanning/deep_scanning_dialog_delegate.cc

@@ -241,9 +241,11 @@ void DeepScanningDialogDelegate::Cancel() {
   if (callback_.is_null())
     return;
 
-  RecordDeepScanMetrics(access_point_,
-                        base::TimeTicks::Now() - upload_start_time_, 0,
-                        "CancelledByUser", false);
+  if (access_point_.has_value()) {
+    RecordDeepScanMetrics(access_point_.value(),
+                          base::TimeTicks::Now() - upload_start_time_, 0,
+                          "CancelledByUser", false);
+  }
 
   // Make sure to reject everything.
   FillAllResultsWith(false);
@@ -335,7 +337,7 @@ void DeepScanningDialogDelegate::ShowForWebContents(
     content::WebContents* web_contents,
     Data data,
     CompletionCallback callback,
-    DeepScanAccessPoint access_point) {
+    base::Optional<DeepScanAccessPoint> access_point) {
   Factory* testing_factory = GetFactoryStorage();
   bool wait_for_verdict = WaitForVerdict();
 
@@ -390,7 +392,7 @@ DeepScanningDialogDelegate::DeepScanningDialogDelegate(
     content::WebContents* web_contents,
     Data data,
     CompletionCallback callback,
-    DeepScanAccessPoint access_point)
+    base::Optional<DeepScanAccessPoint> access_point)
     : web_contents_(web_contents),
       data_(std::move(data)),
       callback_(std::move(callback)),
@@ -407,9 +409,11 @@ void DeepScanningDialogDelegate::StringRequestCallback(
   int64_t content_size = 0;
   for (const base::string16& entry : data_.text)
     content_size += (entry.size() * sizeof(base::char16));
-  RecordDeepScanMetrics(access_point_,
-                        base::TimeTicks::Now() - upload_start_time_,
-                        content_size, result, response);
+  if (access_point_.has_value()) {
+    RecordDeepScanMetrics(access_point_.value(),
+                          base::TimeTicks::Now() - upload_start_time_,
+                          content_size, result, response);
+  }
 
   MaybeReportDeepScanningVerdict(
       Profile::FromBrowserContext(web_contents_->GetBrowserContext()),
@@ -467,9 +471,11 @@ void DeepScanningDialogDelegate::FileRequestCallback(
   DCHECK(it != data_.paths.end());
   size_t index = std::distance(data_.paths.begin(), it);
 
-  RecordDeepScanMetrics(access_point_,
-                        base::TimeTicks::Now() - upload_start_time_,
-                        file_info_[index].size, result, response);
+  if (access_point_.has_value()) {
+    RecordDeepScanMetrics(access_point_.value(),
+                          base::TimeTicks::Now() - upload_start_time_,
+                          file_info_[index].size, result, response);
+  }
 
   base::PostTaskAndReplyWithResult(
       FROM_HERE,

chrome/browser/safe_browsing/cloud_content_scanning/deep_scanning_dialog_delegate.h

@@ -14,6 +14,7 @@
 #include "base/files/file_path.h"
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
+#include "base/optional.h"
 #include "base/strings/string16.h"
 #include "base/time/time.h"
 #include "chrome/browser/safe_browsing/cloud_content_scanning/binary_upload_service.h"
@@ -161,10 +162,11 @@ class DeepScanningDialogDelegate {
   // in the background.
   //
   // Whether the UI is enabled or not, verdicts of the scan will be reported.
-  static void ShowForWebContents(content::WebContents* web_contents,
-                                 Data data,
-                                 CompletionCallback callback,
-                                 DeepScanAccessPoint access_point);
+  static void ShowForWebContents(
+      content::WebContents* web_contents,
+      Data data,
+      CompletionCallback callback,
+      base::Optional<DeepScanAccessPoint> access_point = base::nullopt);
 
   // In tests, sets a factory function for creating fake
   // DeepScanningDialogDelegates.
@@ -175,10 +177,11 @@ class DeepScanningDialogDelegate {
   static bool ResultShouldAllowDataUse(BinaryUploadService::Result result);
 
  protected:
-  DeepScanningDialogDelegate(content::WebContents* web_contents,
-                             Data data,
-                             CompletionCallback callback,
-                             DeepScanAccessPoint access_point);
+  DeepScanningDialogDelegate(
+      content::WebContents* web_contents,
+      Data data,
+      CompletionCallback callback,
+      base::Optional<DeepScanAccessPoint> access_point = base::nullopt);
 
   // Callbacks from uploading data.  Protected so they can be called from
   // testing derived classes.
@@ -277,8 +280,9 @@ class DeepScanningDialogDelegate {
   // Pointer to UI when enabled.
   DeepScanningDialogViews* dialog_ = nullptr;
 
-  // Access point to use to record UMA metrics.
-  DeepScanAccessPoint access_point_;
+  // Access point to use to record UMA metrics. base::nullopt implies no metrics
+  // are to be recorded.
+  base::Optional<DeepScanAccessPoint> access_point_;
 
   base::TimeTicks upload_start_time_;
 

chrome/browser/safe_browsing/cloud_content_scanning/deep_scanning_dialog_views.cc

@@ -141,9 +141,8 @@ ui::ModalType DeepScanningDialogViews::GetModalType() const {
 }
 
 void DeepScanningDialogViews::ShowResult(bool success) {
-  DCHECK(is_pending());
-  dialog_status_ = success ? DeepScanningDialogStatus::SUCCESS
-                           : DeepScanningDialogStatus::FAILURE;
+  DCHECK(!scan_success_.has_value());
+  scan_success_ = success;
 
   // Cleanup if the pending dialog wasn't shown and the verdict is safe.
   if (!shown_ && success) {
@@ -177,7 +176,7 @@ const views::Widget* DeepScanningDialogViews::GetWidgetImpl() const {
 
 void DeepScanningDialogViews::UpdateDialog() {
   DCHECK(shown_);
-  DCHECK(is_result());
+  DCHECK(scan_success_.has_value());
 
   // Update the buttons.
   SetupButtons();
@@ -195,7 +194,7 @@ void DeepScanningDialogViews::UpdateDialog() {
   delete side_icon_spinner_;
 
   // Update the message. Change the text color only if the scan was negative.
-  if (is_failure())
+  if (!scan_success_.value())
     message_->SetEnabledColor(kScanFailureColor);
   message_->SetText(GetDialogMessage());
 
@@ -204,7 +203,7 @@ void DeepScanningDialogViews::UpdateDialog() {
   int text_height = message_->GetRequiredLines() * message_->GetLineHeight();
   int row_height = message_->parent()->height();
   int height_to_add = std::max(text_height - row_height, 0);
-  if (is_success() || (height_to_add > 0))
+  if (scan_success_.value() || (height_to_add > 0))
     Resize(height_to_add);
 
   // Update the dialog.
@@ -212,7 +211,7 @@ void DeepScanningDialogViews::UpdateDialog() {
   widget_->ScheduleLayout();
 
   // Schedule the dialog to close itself in the success case.
-  if (is_success()) {
+  if (scan_success_.value()) {
     base::PostDelayedTask(FROM_HERE, {content::BrowserThread::UI},
                           base::BindOnce(&DialogDelegate::CancelDialog,
                                          weak_ptr_factory_.GetWeakPtr()),
@@ -221,14 +220,13 @@ void DeepScanningDialogViews::UpdateDialog() {
 }
 
 void DeepScanningDialogViews::Resize(int height_to_add) {
-  // Only resize if the dialog is updated to show a result.
-  DCHECK(is_result());
+  DCHECK(scan_success_.has_value());
 
   gfx::Rect dialog_rect = widget_->GetContentsView()->GetContentsBounds();
   int new_height = dialog_rect.height();
 
   // Remove the button row's height if it's removed in the success case.
-  if (is_success()) {
+  if (scan_success_.value()) {
     DCHECK(contents_view_->parent());
     DCHECK_EQ(contents_view_->parent()->children().size(), 2ul);
     DCHECK_EQ(contents_view_->parent()->children()[0], contents_view_.get());
@@ -261,7 +259,7 @@ void DeepScanningDialogViews::Resize(int height_to_add) {
 
 void DeepScanningDialogViews::SetupButtons() {
   // TODO(domfc): Add "Learn more" button on scan failure.
-  if (is_pending() || is_failure()) {
+  if (!scan_success_.has_value() || !scan_success_.value()) {
     DialogDelegate::set_buttons(ui::DIALOG_BUTTON_CANCEL);
     DialogDelegate::set_button_label(ui::DIALOG_BUTTON_CANCEL,
                                      GetCancelButtonText());
@@ -274,21 +272,21 @@ void DeepScanningDialogViews::SetupButtons() {
 }
 
 base::string16 DeepScanningDialogViews::GetDialogMessage() const {
-  if (is_pending()) {
+  if (!scan_success_.has_value()) {
     return l10n_util::GetStringUTF16(
         IDS_DEEP_SCANNING_DIALOG_UPLOAD_PENDING_MESSAGE);
   }
   return l10n_util::GetStringUTF16(
-      is_success() ? IDS_DEEP_SCANNING_DIALOG_SUCCESS_MESSAGE
-                   : IDS_DEEP_SCANNING_DIALOG_UPLOAD_FAILURE_MESSAGE);
+      scan_success_.value() ? IDS_DEEP_SCANNING_DIALOG_SUCCESS_MESSAGE
+                            : IDS_DEEP_SCANNING_DIALOG_UPLOAD_FAILURE_MESSAGE);
 }
 
 base::string16 DeepScanningDialogViews::GetCancelButtonText() const {
-  if (is_pending()) {
+  if (!scan_success_.has_value()) {
     return l10n_util::GetStringUTF16(
         IDS_DEEP_SCANNING_DIALOG_CANCEL_UPLOAD_BUTTON);
   }
-  DCHECK(!is_success());
+  DCHECK(!scan_success_.value());
   return l10n_util::GetStringUTF16(IDS_CLOSE);
 }
 
@@ -354,7 +352,7 @@ std::unique_ptr<views::View> DeepScanningDialogViews::CreateSideIcon() {
   // The side icon is created either:
   // - When the pending dialog is shown
   // - When the response was fast enough that the failure dialog is shown first
-  DCHECK(is_pending() || !is_success());
+  DCHECK(!scan_success_.has_value() || !scan_success_.value());
 
   // The icon left of the text has the appearance of a blue "Enterprise" logo
   // with a spinner when the scan is pending.
@@ -364,12 +362,13 @@ std::unique_ptr<views::View> DeepScanningDialogViews::CreateSideIcon() {
   auto side_image = std::make_unique<views::ImageView>();
   side_image->SetImage(gfx::CreateVectorIcon(
       vector_icons::kBusinessIcon, kSideImageSize,
-      is_result() ? kScanDoneSideImageColor : kScanPendingSideImageColor));
+      scan_success_.has_value() ? kScanDoneSideImageColor
+                                : kScanPendingSideImageColor));
   side_image->SetBorder(views::CreateEmptyBorder(kSideImageInsets));
   side_icon_image_ = icon->AddChildView(std::move(side_image));
 
   // Add a spinner if the scan result is pending, otherwise add a background.
-  if (is_pending()) {
+  if (!scan_success_.has_value()) {
     auto spinner = std::make_unique<views::Throbber>();
     spinner->Start();
     side_icon_spinner_ = icon->AddChildView(std::move(spinner));
@@ -382,22 +381,22 @@ std::unique_ptr<views::View> DeepScanningDialogViews::CreateSideIcon() {
 }
 
 SkColor DeepScanningDialogViews::GetSideImageBackgroundColor() const {
-  DCHECK(is_result());
-  return is_success() ? kScanSuccessColor : kScanFailureColor;
+  DCHECK(scan_success_.has_value());
+  return scan_success_.value() ? kScanSuccessColor : kScanFailureColor;
 }
 
 int DeepScanningDialogViews::GetPasteImageId(bool use_dark) const {
-  if (is_pending())
+  if (!scan_success_.has_value())
     return use_dark ? IDR_PASTE_SCANNING_DARK : IDR_PASTE_SCANNING;
-  if (is_success())
+  if (scan_success_.value())
     return use_dark ? IDR_PASTE_SUCCESS_DARK : IDR_PASTE_SUCCESS;
   return use_dark ? IDR_PASTE_VIOLATION_DARK : IDR_PASTE_VIOLATION;
 }
 
 int DeepScanningDialogViews::GetUploadImageId(bool use_dark) const {
-  if (is_pending())
+  if (!scan_success_.has_value())
     return use_dark ? IDR_UPLOAD_SCANNING_DARK : IDR_UPLOAD_SCANNING;
-  if (is_success())
+  if (scan_success_.value())
     return use_dark ? IDR_UPLOAD_SUCCESS_DARK : IDR_UPLOAD_SUCCESS;
   return use_dark ? IDR_UPLOAD_VIOLATION_DARK : IDR_UPLOAD_VIOLATION;
 }

chrome/browser/safe_browsing/cloud_content_scanning/deep_scanning_dialog_views.h

@@ -8,6 +8,7 @@
 #include <memory>
 
 #include "base/memory/weak_ptr.h"
+#include "base/optional.h"
 #include "base/time/time.h"
 #include "chrome/browser/safe_browsing/cloud_content_scanning/deep_scanning_dialog_delegate.h"
 #include "chrome/browser/safe_browsing/cloud_content_scanning/deep_scanning_utils.h"
@@ -36,22 +37,6 @@ class DeepScanningTopImageView;
 // upload to the user.
 class DeepScanningDialogViews : public views::DialogDelegate {
  public:
-  // Enum used to represent what the dialog is currently showing.
-  enum class DeepScanningDialogStatus {
-    // The dialog is shown with an explanation that the scan is being performed
-    // and that the result is pending.
-    PENDING,
-
-    // The dialog is shown with a short message indicating that the scan was a
-    // success and that the user may proceed with their upload, drag-and-drop or
-    // paste.
-    SUCCESS,
-
-    // The dialog is shown with a message indicating that the scan was a failure
-    // and that the user may not proceed with their upload, drag-and-drop or
-    // paste.
-    FAILURE,
-  };
   DeepScanningDialogViews(std::unique_ptr<DeepScanningDialogDelegate> delegate,
                           content::WebContents* web_contents,
                           base::Optional<DeepScanAccessPoint> access_point,
@@ -69,55 +54,40 @@ class DeepScanningDialogViews : public views::DialogDelegate {
   // nothing should be shown.
   void ShowResult(bool success);
 
-  // Returns the appropriate top image depending on |dialog_status_|.
+  // Returns the appropriate top image depending on |scan_success_|.
   const gfx::ImageSkia* GetTopImage() const;
 
-  // Accessors to simplify |dialog_status_| checking.
-  inline bool is_success() const {
-    return dialog_status_ == DeepScanningDialogStatus::SUCCESS;
-  }
-
-  inline bool is_failure() const {
-    return dialog_status_ == DeepScanningDialogStatus::FAILURE;
-  }
-
-  inline bool is_result() const { return is_success() || is_failure(); }
-
-  inline bool is_pending() const {
-    return dialog_status_ == DeepScanningDialogStatus::PENDING;
-  }
-
  private:
   ~DeepScanningDialogViews() override;
 
   // views::DialogDelegate:
   const views::Widget* GetWidgetImpl() const override;
 
-  // Update the UI depending on |dialog_status_|.
+  // Update the UI depending on |scan_success_|.
   void UpdateDialog();
 
   // Resizes the already shown dialog to accommodate changes in its content.
   void Resize(int height_to_add);
 
-  // Setup the appropriate buttons depending on |dialog_status_|.
+  // Setup the appropriate buttons depending on |scan_success_|.
   void SetupButtons();
 
   // Returns a newly created side icon.
   std::unique_ptr<views::View> CreateSideIcon();
 
-  // Returns the appropriate dialog message depending on |dialog_status_|.
+  // Returns the appropriate dialog message depending on |scan_success_|.
   base::string16 GetDialogMessage() const;
 
   // Returns the side image's background circle color.
   SkColor GetSideImageBackgroundColor() const;
 
-  // Returns the appropriate dialog message depending on |dialog_status_|.
+  // Returns the appropriate dialog message depending on |scan_success_|.
   base::string16 GetCancelButtonText() const;
 
-  // Returns the appropriate paste top image ID depending on |dialog_status_|.
+  // Returns the appropriate paste top image ID depending on |scan_success_|.
   int GetPasteImageId(bool use_dark) const;
 
-  // Returns the appropriate upload top image ID depending on |dialog_status_|.
+  // Returns the appropriate upload top image ID depending on |scan_success_|.
   int GetUploadImageId(bool use_dark) const;
 
   // Show the dialog. Sets |shown_| to true.
@@ -141,7 +111,9 @@ class DeepScanningDialogViews : public views::DialogDelegate {
   base::TimeTicks first_shown_timestamp_;
 
   // Used to show the appropriate dialog depending on the scan's status.
-  DeepScanningDialogStatus dialog_status_ = DeepScanningDialogStatus::PENDING;
+  // base::nullopt represents a pending scan, true represents a scan with no
+  // malware or DLP violation and false represents a scan with such a violation.
+  base::Optional<bool> scan_success_ = base::nullopt;
 
   // Used to animate dialog height changes.
   std::unique_ptr<views::BoundsAnimator> bounds_animator_;

chrome/browser/safe_browsing/cloud_content_scanning/fake_deep_scanning_dialog_delegate.cc

@@ -23,8 +23,7 @@ FakeDeepScanningDialogDelegate::FakeDeepScanningDialogDelegate(
     CompletionCallback callback)
     : DeepScanningDialogDelegate(web_contents,
                                  std::move(data),
-                                 std::move(callback),
-                                 DeepScanAccessPoint::UPLOAD),
+                                 std::move(callback)),
       delete_closure_(delete_closure),
       status_callback_(status_callback),
       encryption_callback_(encryption_callback),