Refactor the SelectFileDialog implementation on Windows

This is meant to make the code easier to read by making ownership more
obvious and standardize a little bit the different functions that
implement the various dialog types.

Also will facilitate a possible transition to the IFileDialog interface

This CL should not change the behavior of the dialogs.

Bug: 73098,884075
Change-Id: I0d2f5cc02e1d08ab3f54385fb166bc34ae1f5b53
Reviewed-on: https://chromium-review.googlesource.com/c/1249911
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Robert Liao <robliao@chromium.org>
Commit-Queue: Patrick Monette <pmonette@chromium.org>
Cr-Commit-Position: refs/heads/master@{#599827}

chrome/browser/net/net_error_diagnostics_dialog_win.cc

@@ -47,13 +47,16 @@ class NetErrorDiagnosticsDialog : public ui::BaseShellDialogImpl {
     if (IsRunningDialogForOwner(parent))
       return;
 
-    RunState run_state = BeginRun(parent);
-    run_state.dialog_task_runner->PostTaskAndReply(
+    std::unique_ptr<RunState> run_state = BeginRun(parent);
+
+    scoped_refptr<base::SingleThreadTaskRunner> task_runner =
+        run_state->dialog_task_runner;
+    task_runner->PostTaskAndReply(
         FROM_HERE,
-        base::Bind(&NetErrorDiagnosticsDialog::ShowDialogOnPrivateThread,
-                   base::Unretained(this), parent, failed_url),
-        base::Bind(&NetErrorDiagnosticsDialog::DiagnosticsDone,
-                   base::Unretained(this), run_state, callback));
+        base::BindOnce(&NetErrorDiagnosticsDialog::ShowDialogOnPrivateThread,
+                       base::Unretained(this), parent, failed_url),
+        base::BindOnce(&NetErrorDiagnosticsDialog::DiagnosticsDone,
+                       base::Unretained(this), std::move(run_state), callback));
   }
 
  private:
@@ -68,8 +71,9 @@ class NetErrorDiagnosticsDialog : public ui::BaseShellDialogImpl {
     NdfCloseIncident(incident_handle);
   }
 
-  void DiagnosticsDone(RunState run_state, const base::Closure& callback) {
-    EndRun(run_state);
+  void DiagnosticsDone(std::unique_ptr<RunState> run_state,
+                       const base::Closure& callback) {
+    EndRun(std::move(run_state));
     callback.Run();
   }
 

chrome/browser/ui/views/certificate_viewer_win.cc

@@ -40,18 +40,21 @@ class CertificateViewerDialog : public ui::BaseShellDialogImpl {
       return;
     }
 
-    RunState run_state = BeginRun(parent);
-    run_state.dialog_task_runner->PostTaskAndReply(
+    std::unique_ptr<RunState> run_state = BeginRun(parent);
+
+    scoped_refptr<base::SingleThreadTaskRunner> task_runner =
+        run_state->dialog_task_runner;
+    task_runner->PostTaskAndReply(
         FROM_HERE,
-        base::Bind(&CertificateViewerDialog::ShowOnDialogThread,
-                   base::Unretained(this), run_state,
-                   base::WrapRefCounted(cert)),
-        base::Bind(&CertificateViewerDialog::OnDialogClosed,
-                   base::Unretained(this), run_state, callback));
+        base::BindOnce(&CertificateViewerDialog::ShowOnDialogThread,
+                       base::Unretained(this), parent,
+                       base::WrapRefCounted(cert)),
+        base::BindOnce(&CertificateViewerDialog::OnDialogClosed,
+                       base::Unretained(this), std::move(run_state), callback));
   }
 
  private:
-  void ShowOnDialogThread(const RunState& run_state,
+  void ShowOnDialogThread(HWND owner,
                           const scoped_refptr<net::X509Certificate>& cert) {
     // Create a new cert context and store containing just the certificate
     // and its intermediate certificates.
@@ -65,7 +68,7 @@ class CertificateViewerDialog : public ui::BaseShellDialogImpl {
 
     CRYPTUI_VIEWCERTIFICATE_STRUCT view_info = {0};
     view_info.dwSize = sizeof(view_info);
-    view_info.hwndParent = run_state.owner;
+    view_info.hwndParent = owner;
     view_info.dwFlags =
         CRYPTUI_DISABLE_EDITPROPERTIES | CRYPTUI_DISABLE_ADDTOSTORE;
     view_info.pCertContext = cert_list.get();
@@ -77,9 +80,9 @@ class CertificateViewerDialog : public ui::BaseShellDialogImpl {
     ::CryptUIDlgViewCertificate(&view_info, &properties_changed);
   }
 
-  void OnDialogClosed(const RunState& run_state,
+  void OnDialogClosed(std::unique_ptr<RunState> run_state,
                       const base::Closure& callback) {
-    EndRun(run_state);
+    EndRun(std::move(run_state));
     // May delete |this|.
     callback.Run();
   }

chrome/browser/ui/views/color_chooser_dialog.cc

@@ -6,6 +6,8 @@
 
 #include <commdlg.h>
 
+#include <utility>
+
 #include "base/bind.h"
 #include "base/macros.h"
 #include "base/single_thread_task_runner.h"
@@ -22,26 +24,21 @@ using content::BrowserThread;
 // static
 COLORREF ColorChooserDialog::g_custom_colors[16];
 
-ColorChooserDialog::ExecuteOpenParams::ExecuteOpenParams(SkColor color,
-                                                         RunState run_state,
-                                                         HWND owner)
-    : color(color),
-      run_state(run_state),
-      owner(owner) {
-}
-
 ColorChooserDialog::ColorChooserDialog(views::ColorChooserListener* listener,
                                        SkColor initial_color,
                                        gfx::NativeWindow owning_window)
     : listener_(listener) {
   DCHECK(listener_);
   CopyCustomColors(g_custom_colors, custom_colors_);
   HWND owning_hwnd = views::HWNDForNativeWindow(owning_window);
-  ExecuteOpenParams execute_params(initial_color, BeginRun(owning_hwnd),
-                                   owning_hwnd);
-  execute_params.run_state.dialog_task_runner->PostTask(
-      FROM_HERE,
-      base::Bind(&ColorChooserDialog::ExecuteOpen, this, execute_params));
+
+  std::unique_ptr<RunState> run_state = BeginRun(owning_hwnd);
+
+  scoped_refptr<base::SingleThreadTaskRunner> task_runner =
+      run_state->dialog_task_runner;
+  task_runner->PostTask(FROM_HERE,
+                        base::BindOnce(&ColorChooserDialog::ExecuteOpen, this,
+                                       initial_color, std::move(run_state)));
 }
 
 bool ColorChooserDialog::IsRunning(gfx::NativeWindow owning_window) const {
@@ -58,25 +55,27 @@ void ColorChooserDialog::ListenerDestroyed() {
 ColorChooserDialog::~ColorChooserDialog() {
 }
 
-void ColorChooserDialog::ExecuteOpen(const ExecuteOpenParams& params) {
+void ColorChooserDialog::ExecuteOpen(SkColor color,
+                                     std::unique_ptr<RunState> run_state) {
   CHOOSECOLOR cc;
   cc.lStructSize = sizeof(CHOOSECOLOR);
-  cc.hwndOwner = params.owner;
-  cc.rgbResult = skia::SkColorToCOLORREF(params.color);
+  cc.hwndOwner = run_state->owner;
+  cc.rgbResult = skia::SkColorToCOLORREF(color);
   cc.lpCustColors = custom_colors_;
   cc.Flags = CC_ANYCOLOR | CC_FULLOPEN | CC_RGBINIT;
   bool success = !!ChooseColor(&cc);
   DisableOwner(cc.hwndOwner);
   base::PostTaskWithTraits(
       FROM_HERE, {BrowserThread::UI},
-      base::Bind(&ColorChooserDialog::DidCloseDialog, this, success,
-                 skia::COLORREFToSkColor(cc.rgbResult), params.run_state));
+      base::BindOnce(&ColorChooserDialog::DidCloseDialog, this, success,
+                     skia::COLORREFToSkColor(cc.rgbResult),
+                     std::move(run_state)));
 }
 
 void ColorChooserDialog::DidCloseDialog(bool chose_color,
                                         SkColor color,
-                                        RunState run_state) {
-  EndRun(run_state);
+                                        std::unique_ptr<RunState> run_state) {
+  EndRun(std::move(run_state));
   CopyCustomColors(custom_colors_, g_custom_colors);
   if (listener_) {
     if (chose_color)

chrome/browser/ui/views/color_chooser_dialog.h

@@ -5,6 +5,8 @@
 #ifndef CHROME_BROWSER_UI_VIEWS_COLOR_CHOOSER_DIALOG_H_
 #define CHROME_BROWSER_UI_VIEWS_COLOR_CHOOSER_DIALOG_H_
 
+#include <memory>
+
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
 #include "chrome/browser/ui/views/color_chooser_dialog.h"
@@ -33,24 +35,19 @@ class ColorChooserDialog
   friend class base::RefCountedThreadSafe<ColorChooserDialog>;
   ~ColorChooserDialog() override;
 
-  struct ExecuteOpenParams {
-    ExecuteOpenParams(SkColor color, RunState run_state, HWND owner);
-    SkColor color;
-    RunState run_state;
-    HWND owner;
-  };
-
   // Called on the dialog thread to show the actual color chooser.  This is
-  // shown modal to |params.owner|.  Once it's closed, calls back to
+  // shown modal to |run_state->owner|.  Once it's closed, calls back to
   // DidCloseDialog() on the UI thread.
-  void ExecuteOpen(const ExecuteOpenParams& params);
+  void ExecuteOpen(SkColor color, std::unique_ptr<RunState> run_state);
 
   // Called on the UI thread when a color chooser is closed.  |chose_color| is
   // true if the user actually chose a color, in which case |color| is the
   // chosen color.  Calls back to the |listener_| (if applicable) to notify it
   // of the results, and copies the modified array of |custom_colors_| back to
   // |g_custom_colors| so future dialogs will see the changes.
-  void DidCloseDialog(bool chose_color, SkColor color, RunState run_state);
+  void DidCloseDialog(bool chose_color,
+                      SkColor color,
+                      std::unique_ptr<RunState> run_state);
 
   // Copies the array of colors in |src| to |dst|.
   void CopyCustomColors(COLORREF*, COLORREF*);

chrome/browser/ui/webui/print_preview/pdf_printer_handler_win_unittest.cc

@@ -22,17 +22,24 @@ using content::WebContents;
 
 namespace {
 
-class FakePdfPrinterHandler;
-bool GetOpenFileNameImpl(OPENFILENAME* ofn);
-bool GetSaveFileNameImpl(FakePdfPrinterHandler* handler, OPENFILENAME* ofn);
+std::pair<std::vector<base::FilePath>, int> ExecuteCancelledSelectFileDialog(
+    ui::SelectFileDialog::Type type,
+    const base::string16& title,
+    const base::FilePath& default_path,
+    const base::string16& filter,
+    int file_type_index,
+    const base::string16& default_extension,
+    HWND owner) {
+  // Return an empty result to simulate a cancelled dialog.
+  return {{}, 0};
+}
 
 class FakePdfPrinterHandler : public PdfPrinterHandler {
  public:
   FakePdfPrinterHandler(Profile* profile,
                         content::WebContents* contents,
                         printing::StickySettings* sticky_settings)
       : PdfPrinterHandler(profile, contents, sticky_settings),
-        init_called_(false),
         save_failed_(false) {}
 
   void FileSelected(const base::FilePath& path,
@@ -55,10 +62,6 @@ class FakePdfPrinterHandler : public PdfPrinterHandler {
 
   bool save_failed() const { return save_failed_; }
 
-  bool init_called() const { return init_called_; }
-
-  void set_init_called() { init_called_ = true; }
-
  private:
   // Simplified version of select file to avoid checking preferences and sticky
   // settings in the test
@@ -70,46 +73,18 @@ class FakePdfPrinterHandler : public PdfPrinterHandler {
     file_type_info.extensions[0].push_back(FILE_PATH_LITERAL("pdf"));
     select_file_dialog_ = ui::CreateWinSelectFileDialog(
         this, nullptr /*policy already checked*/,
-        base::Bind(GetOpenFileNameImpl), base::Bind(GetSaveFileNameImpl, this));
+        base::BindRepeating(&ExecuteCancelledSelectFileDialog));
     select_file_dialog_->SelectFile(
         ui::SelectFileDialog::SELECT_SAVEAS_FILE, base::string16(),
         default_filename, &file_type_info, 0, base::FilePath::StringType(),
         platform_util::GetTopLevel(preview_web_contents_->GetNativeView()),
         nullptr);
   }
 
-  bool init_called_;
   bool save_failed_;
   base::RunLoop run_loop_;
 };
 
-// Hook function to cancel the dialog when it is successfully initialized.
-UINT_PTR CALLBACK PdfPrinterHandlerTestHookFunction(HWND hdlg,
-                                                    UINT message,
-                                                    WPARAM wparam,
-                                                    LPARAM lparam) {
-  if (message != WM_INITDIALOG)
-    return 0;
-  OPENFILENAME* ofn = reinterpret_cast<OPENFILENAME*>(lparam);
-  FakePdfPrinterHandler* handler =
-      reinterpret_cast<FakePdfPrinterHandler*>(ofn->lCustData);
-  handler->set_init_called();
-  PostMessage(GetParent(hdlg), WM_COMMAND, MAKEWPARAM(IDCANCEL, 0), 0);
-  return 1;
-}
-
-bool GetOpenFileNameImpl(OPENFILENAME* ofn) {
-  return ::GetOpenFileName(ofn);
-}
-
-bool GetSaveFileNameImpl(FakePdfPrinterHandler* handler, OPENFILENAME* ofn) {
-  // Modify ofn so that the hook function will be called.
-  ofn->Flags |= OFN_ENABLEHOOK;
-  ofn->lpfnHook = PdfPrinterHandlerTestHookFunction;
-  ofn->lCustData = reinterpret_cast<LPARAM>(handler);
-  return ::GetSaveFileName(ofn);
-}
-
 }  // namespace
 
 class PdfPrinterHandlerWinTest : public BrowserWithTestWindowTest {
@@ -138,7 +113,6 @@ class PdfPrinterHandlerWinTest : public BrowserWithTestWindowTest {
 
 TEST_F(PdfPrinterHandlerWinTest, TestSaveAsPdf) {
   pdf_printer_->StartPrintToPdf(L"111111111111111111111.html");
-  EXPECT_TRUE(pdf_printer_->init_called());
   EXPECT_TRUE(pdf_printer_->save_failed());
 }
 
@@ -149,6 +123,5 @@ TEST_F(PdfPrinterHandlerWinTest, TestSaveAsPdfLongFileName) {
       L"11111111111111111111111111111111111111111111111111111111111111111111111"
       L"11111111111111111111111111111111111111111111111111111111111111111111111"
       L"1111111111111111111111111111111111111111111111111.html");
-  EXPECT_TRUE(pdf_printer_->init_called());
   EXPECT_TRUE(pdf_printer_->save_failed());
 }

chrome/browser/ui/webui/settings_utils_win.cc

@@ -41,26 +41,29 @@ class ManageCertificatesDialog : public ui::BaseShellDialogImpl {
       return;
     }
 
-    RunState run_state = BeginRun(parent);
-    run_state.dialog_task_runner->PostTaskAndReply(
+    std::unique_ptr<RunState> run_state = BeginRun(parent);
+
+    base::SingleThreadTaskRunner* task_runner =
+        run_state->dialog_task_runner.get();
+    task_runner->PostTaskAndReply(
         FROM_HERE,
-        base::Bind(&ManageCertificatesDialog::ShowOnDialogThread,
-                   base::Unretained(this), run_state),
-        base::Bind(&ManageCertificatesDialog::OnDialogClosed,
-                   base::Unretained(this), run_state, callback));
+        base::BindOnce(&ManageCertificatesDialog::ShowOnDialogThread,
+                       base::Unretained(this), parent),
+        base::BindOnce(&ManageCertificatesDialog::OnDialogClosed,
+                       base::Unretained(this), std::move(run_state), callback));
   }
 
  private:
-  void ShowOnDialogThread(const RunState& run_state) {
+  void ShowOnDialogThread(HWND owner) {
     CRYPTUI_CERT_MGR_STRUCT cert_mgr = {0};
     cert_mgr.dwSize = sizeof(CRYPTUI_CERT_MGR_STRUCT);
-    cert_mgr.hwndParent = run_state.owner;
+    cert_mgr.hwndParent = owner;
     ::CryptUIDlgCertMgr(&cert_mgr);
   }
 
-  void OnDialogClosed(const RunState& run_state,
+  void OnDialogClosed(std::unique_ptr<RunState> run_state,
                       const base::Closure& callback) {
-    EndRun(run_state);
+    EndRun(std::move(run_state));
     // May delete |this|.
     callback.Run();
   }