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

This reverts commit 86c4251e.

Reason for revert: The fix is applied in crrev.com/c/2024497

Original change's description:
> 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/+/2026970
> Reviewed-by: vitaliii <vitaliii@chromium.org>
> Commit-Queue: vitaliii <vitaliii@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#736302}

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

Change-Id: I9a094d5d1d3105dd5b5712b0c77e9b0d58788420
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 1046693
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2026355Reviewed-by: Daniel Rubery <drubery@chromium.org>
Commit-Queue: Dominique Fauteux-Chapleau <domfc@chromium.org>
Cr-Commit-Position: refs/heads/master@{#736427}

chrome/browser/safe_browsing/cloud_content_scanning/deep_scanning_dialog_delegate.cc

@@ -241,11 +241,9 @@ void DeepScanningDialogDelegate::Cancel() {
   if (callback_.is_null())
     return;
 
-  if (access_point_.has_value()) {
-    RecordDeepScanMetrics(access_point_.value(),
-                          base::TimeTicks::Now() - upload_start_time_, 0,
-                          "CancelledByUser", false);
-  }
+  RecordDeepScanMetrics(access_point_,
+                        base::TimeTicks::Now() - upload_start_time_, 0,
+                        "CancelledByUser", false);
 
   // Make sure to reject everything.
   FillAllResultsWith(false);
@@ -337,7 +335,7 @@ void DeepScanningDialogDelegate::ShowForWebContents(
     content::WebContents* web_contents,
     Data data,
     CompletionCallback callback,
-    base::Optional<DeepScanAccessPoint> access_point) {
+    DeepScanAccessPoint access_point) {
   Factory* testing_factory = GetFactoryStorage();
   bool wait_for_verdict = WaitForVerdict();
 
@@ -392,7 +390,7 @@ DeepScanningDialogDelegate::DeepScanningDialogDelegate(
     content::WebContents* web_contents,
     Data data,
     CompletionCallback callback,
-    base::Optional<DeepScanAccessPoint> access_point)
+    DeepScanAccessPoint access_point)
     : web_contents_(web_contents),
       data_(std::move(data)),
       callback_(std::move(callback)),
@@ -409,11 +407,9 @@ void DeepScanningDialogDelegate::StringRequestCallback(
   int64_t content_size = 0;
   for (const base::string16& entry : data_.text)
     content_size += (entry.size() * sizeof(base::char16));
-  if (access_point_.has_value()) {
-    RecordDeepScanMetrics(access_point_.value(),
-                          base::TimeTicks::Now() - upload_start_time_,
-                          content_size, result, response);
-  }
+  RecordDeepScanMetrics(access_point_,
+                        base::TimeTicks::Now() - upload_start_time_,
+                        content_size, result, response);
 
   MaybeReportDeepScanningVerdict(
       Profile::FromBrowserContext(web_contents_->GetBrowserContext()),
@@ -471,11 +467,9 @@ void DeepScanningDialogDelegate::FileRequestCallback(
   DCHECK(it != data_.paths.end());
   size_t index = std::distance(data_.paths.begin(), it);
 
-  if (access_point_.has_value()) {
-    RecordDeepScanMetrics(access_point_.value(),
-                          base::TimeTicks::Now() - upload_start_time_,
-                          file_info_[index].size, result, response);
-  }
+  RecordDeepScanMetrics(access_point_,
+                        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,7 +14,6 @@
 #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"
@@ -162,11 +161,10 @@ 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,
-      base::Optional<DeepScanAccessPoint> access_point = base::nullopt);
+  static void ShowForWebContents(content::WebContents* web_contents,
+                                 Data data,
+                                 CompletionCallback callback,
+                                 DeepScanAccessPoint access_point);
 
   // In tests, sets a factory function for creating fake
   // DeepScanningDialogDelegates.
@@ -177,11 +175,10 @@ class DeepScanningDialogDelegate {
   static bool ResultShouldAllowDataUse(BinaryUploadService::Result result);
 
  protected:
-  DeepScanningDialogDelegate(
-      content::WebContents* web_contents,
-      Data data,
-      CompletionCallback callback,
-      base::Optional<DeepScanAccessPoint> access_point = base::nullopt);
+  DeepScanningDialogDelegate(content::WebContents* web_contents,
+                             Data data,
+                             CompletionCallback callback,
+                             DeepScanAccessPoint access_point);
 
   // Callbacks from uploading data.  Protected so they can be called from
   // testing derived classes.
@@ -280,9 +277,8 @@ class DeepScanningDialogDelegate {
   // Pointer to UI when enabled.
   DeepScanningDialogViews* dialog_ = nullptr;
 
-  // Access point to use to record UMA metrics. base::nullopt implies no metrics
-  // are to be recorded.
-  base::Optional<DeepScanAccessPoint> access_point_;
+  // Access point to use to record UMA metrics.
+  DeepScanAccessPoint access_point_;
 
   base::TimeTicks upload_start_time_;
 

chrome/browser/safe_browsing/cloud_content_scanning/deep_scanning_dialog_views.cc

@@ -141,8 +141,9 @@ ui::ModalType DeepScanningDialogViews::GetModalType() const {
 }
 
 void DeepScanningDialogViews::ShowResult(bool success) {
-  DCHECK(!scan_success_.has_value());
-  scan_success_ = success;
+  DCHECK(is_pending());
+  dialog_status_ = success ? DeepScanningDialogStatus::SUCCESS
+                           : DeepScanningDialogStatus::FAILURE;
 
   // Cleanup if the pending dialog wasn't shown and the verdict is safe.
   if (!shown_ && success) {
@@ -176,7 +177,7 @@ const views::Widget* DeepScanningDialogViews::GetWidgetImpl() const {
 
 void DeepScanningDialogViews::UpdateDialog() {
   DCHECK(shown_);
-  DCHECK(scan_success_.has_value());
+  DCHECK(is_result());
 
   // Update the buttons.
   SetupButtons();
@@ -194,7 +195,7 @@ void DeepScanningDialogViews::UpdateDialog() {
   delete side_icon_spinner_;
 
   // Update the message. Change the text color only if the scan was negative.
-  if (!scan_success_.value())
+  if (is_failure())
     message_->SetEnabledColor(kScanFailureColor);
   message_->SetText(GetDialogMessage());
 
@@ -203,7 +204,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 (scan_success_.value() || (height_to_add > 0))
+  if (is_success() || (height_to_add > 0))
     Resize(height_to_add);
 
   // Update the dialog.
@@ -211,7 +212,7 @@ void DeepScanningDialogViews::UpdateDialog() {
   widget_->ScheduleLayout();
 
   // Schedule the dialog to close itself in the success case.
-  if (scan_success_.value()) {
+  if (is_success()) {
     base::PostDelayedTask(FROM_HERE, {content::BrowserThread::UI},
                           base::BindOnce(&DialogDelegate::CancelDialog,
                                          weak_ptr_factory_.GetWeakPtr()),
@@ -220,13 +221,14 @@ void DeepScanningDialogViews::UpdateDialog() {
 }
 
 void DeepScanningDialogViews::Resize(int height_to_add) {
-  DCHECK(scan_success_.has_value());
+  // Only resize if the dialog is updated to show a result.
+  DCHECK(is_result());
 
   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 (scan_success_.value()) {
+  if (is_success()) {
     DCHECK(contents_view_->parent());
     DCHECK_EQ(contents_view_->parent()->children().size(), 2ul);
     DCHECK_EQ(contents_view_->parent()->children()[0], contents_view_.get());
@@ -259,7 +261,7 @@ void DeepScanningDialogViews::Resize(int height_to_add) {
 
 void DeepScanningDialogViews::SetupButtons() {
   // TODO(domfc): Add "Learn more" button on scan failure.
-  if (!scan_success_.has_value() || !scan_success_.value()) {
+  if (is_pending() || is_failure()) {
     DialogDelegate::set_buttons(ui::DIALOG_BUTTON_CANCEL);
     DialogDelegate::set_button_label(ui::DIALOG_BUTTON_CANCEL,
                                      GetCancelButtonText());
@@ -272,21 +274,21 @@ void DeepScanningDialogViews::SetupButtons() {
 }
 
 base::string16 DeepScanningDialogViews::GetDialogMessage() const {
-  if (!scan_success_.has_value()) {
+  if (is_pending()) {
     return l10n_util::GetStringUTF16(
         IDS_DEEP_SCANNING_DIALOG_UPLOAD_PENDING_MESSAGE);
   }
   return l10n_util::GetStringUTF16(
-      scan_success_.value() ? IDS_DEEP_SCANNING_DIALOG_SUCCESS_MESSAGE
-                            : IDS_DEEP_SCANNING_DIALOG_UPLOAD_FAILURE_MESSAGE);
+      is_success() ? IDS_DEEP_SCANNING_DIALOG_SUCCESS_MESSAGE
+                   : IDS_DEEP_SCANNING_DIALOG_UPLOAD_FAILURE_MESSAGE);
 }
 
 base::string16 DeepScanningDialogViews::GetCancelButtonText() const {
-  if (!scan_success_.has_value()) {
+  if (is_pending()) {
     return l10n_util::GetStringUTF16(
         IDS_DEEP_SCANNING_DIALOG_CANCEL_UPLOAD_BUTTON);
   }
-  DCHECK(!scan_success_.value());
+  DCHECK(!is_success());
   return l10n_util::GetStringUTF16(IDS_CLOSE);
 }
 
@@ -352,7 +354,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(!scan_success_.has_value() || !scan_success_.value());
+  DCHECK(is_pending() || !is_success());
 
   // The icon left of the text has the appearance of a blue "Enterprise" logo
   // with a spinner when the scan is pending.
@@ -362,13 +364,12 @@ std::unique_ptr<views::View> DeepScanningDialogViews::CreateSideIcon() {
   auto side_image = std::make_unique<views::ImageView>();
   side_image->SetImage(gfx::CreateVectorIcon(
       vector_icons::kBusinessIcon, kSideImageSize,
-      scan_success_.has_value() ? kScanDoneSideImageColor
-                                : kScanPendingSideImageColor));
+      is_result() ? 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 (!scan_success_.has_value()) {
+  if (is_pending()) {
     auto spinner = std::make_unique<views::Throbber>();
     spinner->Start();
     side_icon_spinner_ = icon->AddChildView(std::move(spinner));
@@ -381,22 +382,22 @@ std::unique_ptr<views::View> DeepScanningDialogViews::CreateSideIcon() {
 }
 
 SkColor DeepScanningDialogViews::GetSideImageBackgroundColor() const {
-  DCHECK(scan_success_.has_value());
-  return scan_success_.value() ? kScanSuccessColor : kScanFailureColor;
+  DCHECK(is_result());
+  return is_success() ? kScanSuccessColor : kScanFailureColor;
 }
 
 int DeepScanningDialogViews::GetPasteImageId(bool use_dark) const {
-  if (!scan_success_.has_value())
+  if (is_pending())
     return use_dark ? IDR_PASTE_SCANNING_DARK : IDR_PASTE_SCANNING;
-  if (scan_success_.value())
+  if (is_success())
     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 (!scan_success_.has_value())
+  if (is_pending())
     return use_dark ? IDR_UPLOAD_SCANNING_DARK : IDR_UPLOAD_SCANNING;
-  if (scan_success_.value())
+  if (is_success())
     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,7 +8,6 @@
 #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"
@@ -37,6 +36,22 @@ 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,
@@ -54,40 +69,55 @@ class DeepScanningDialogViews : public views::DialogDelegate {
   // nothing should be shown.
   void ShowResult(bool success);
 
-  // Returns the appropriate top image depending on |scan_success_|.
+  // Returns the appropriate top image depending on |dialog_status_|.
   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 |scan_success_|.
+  // Update the UI depending on |dialog_status_|.
   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 |scan_success_|.
+  // Setup the appropriate buttons depending on |dialog_status_|.
   void SetupButtons();
 
   // Returns a newly created side icon.
   std::unique_ptr<views::View> CreateSideIcon();
 
-  // Returns the appropriate dialog message depending on |scan_success_|.
+  // Returns the appropriate dialog message depending on |dialog_status_|.
   base::string16 GetDialogMessage() const;
 
   // Returns the side image's background circle color.
   SkColor GetSideImageBackgroundColor() const;
 
-  // Returns the appropriate dialog message depending on |scan_success_|.
+  // Returns the appropriate dialog message depending on |dialog_status_|.
   base::string16 GetCancelButtonText() const;
 
-  // Returns the appropriate paste top image ID depending on |scan_success_|.
+  // Returns the appropriate paste top image ID depending on |dialog_status_|.
   int GetPasteImageId(bool use_dark) const;
 
-  // Returns the appropriate upload top image ID depending on |scan_success_|.
+  // Returns the appropriate upload top image ID depending on |dialog_status_|.
   int GetUploadImageId(bool use_dark) const;
 
   // Show the dialog. Sets |shown_| to true.
@@ -111,9 +141,7 @@ class DeepScanningDialogViews : public views::DialogDelegate {
   base::TimeTicks first_shown_timestamp_;
 
   // Used to show the appropriate dialog depending on the scan's status.
-  // 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;
+  DeepScanningDialogStatus dialog_status_ = DeepScanningDialogStatus::PENDING;
 
   // 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,7 +23,8 @@ FakeDeepScanningDialogDelegate::FakeDeepScanningDialogDelegate(
     CompletionCallback callback)
     : DeepScanningDialogDelegate(web_contents,
                                  std::move(data),
-                                 std::move(callback)),
+                                 std::move(callback),
+                                 DeepScanAccessPoint::UPLOAD),
       delete_closure_(delete_closure),
       status_callback_(status_callback),
       encryption_callback_(encryption_callback),