refactor: have ShowSaveDialogSync() return a std::optional<base::FilePath> (#47452)

* refactor: have ShowSaveDialogSync() return a std::optional<base::FilePath>

Co-authored-by: Charles Kerr <charles@charleskerr.com>

* fixup! refactor: have ShowSaveDialogSync() return a std::optional<base::FilePath>

Co-authored-by: Charles Kerr <charles@charleskerr.com>

---------

Co-authored-by: trop[bot] <37223003+trop[bot]@users.noreply.github.com>
Co-authored-by: Charles Kerr <charles@charleskerr.com>

shell/browser/api/electron_api_dialog.cc

@@ -71,9 +71,8 @@ v8::Local<v8::Promise> ShowOpenDialog(
 
 void ShowSaveDialogSync(const file_dialog::DialogSettings& settings,
                         gin::Arguments* args) {
-  base::FilePath path;
+  if (const auto path = file_dialog::ShowSaveDialogSync(settings))
-  if (file_dialog::ShowSaveDialogSync(settings, &path))
+    args->Return(*path);
-    args->Return(path);
 }
 
 v8::Local<v8::Promise> ShowSaveDialog(

shell/browser/api/electron_api_web_contents.cc

@@ -18,6 +18,7 @@
 #include "base/containers/fixed_flat_map.h"
 #include "base/containers/flat_set.h"
 #include "base/containers/id_map.h"
+#include "base/containers/map_util.h"
 #include "base/files/file_util.h"
 #include "base/json/json_reader.h"
 #include "base/no_destructor.h"
@@ -4024,30 +4025,35 @@ void WebContents::DevToolsSaveToFile(const std::string& url,
                                      const std::string& content,
                                      bool save_as,
                                      bool is_base64) {
-  base::FilePath path;
+  const base::FilePath* path = nullptr;
-  auto it = saved_files_.find(url);
+
-  if (it != saved_files_.end() && !save_as) {
+  if (!save_as)
-    path = it->second;
+    base::FindOrNull(saved_files_, url);
-  } else {
+
+  if (path == nullptr) {
     file_dialog::DialogSettings settings;
     settings.parent_window = owner_window();
     settings.force_detached = offscreen_;
     settings.title = url;
     settings.default_path = base::FilePath::FromUTF8Unsafe(url);
-    if (!file_dialog::ShowSaveDialogSync(settings, &path)) {
+    if (auto new_path = file_dialog::ShowSaveDialogSync(settings)) {
-      inspectable_web_contents_->CallClientFunction(
+      auto [iter, _] = saved_files_.try_emplace(url, std::move(*new_path));
-          "DevToolsAPI", "canceledSaveURL", base::Value(url));
+      path = &iter->second;
-      return;
     }
   }
 
-  saved_files_[url] = path;
+  if (path == nullptr) {
+    inspectable_web_contents_->CallClientFunction(
+        "DevToolsAPI", "canceledSaveURL", base::Value{url});
+    return;
+  }
+
   // Notify DevTools.
   inspectable_web_contents_->CallClientFunction(
-      "DevToolsAPI", "savedURL", base::Value(url),
+      "DevToolsAPI", "savedURL", base::Value{url},
-      base::Value(path.AsUTF8Unsafe()));
+      base::Value{path->AsUTF8Unsafe()});
   file_task_runner_->PostTask(
-      FROM_HERE, base::BindOnce(&WriteToFile, path, content, is_base64));
+      FROM_HERE, base::BindOnce(&WriteToFile, *path, content, is_base64));
 }
 
 void WebContents::DevToolsAppendToFile(const std::string& url,

shell/browser/ui/file_dialog.h

@@ -5,6 +5,7 @@
 #ifndef ELECTRON_SHELL_BROWSER_UI_FILE_DIALOG_H_
 #define ELECTRON_SHELL_BROWSER_UI_FILE_DIALOG_H_
 
+#include <optional>
 #include <string>
 #include <utility>
 #include <vector>
@@ -72,7 +73,8 @@ bool ShowOpenDialogSync(const DialogSettings& settings,
 void ShowOpenDialog(const DialogSettings& settings,
                     gin_helper::Promise<gin_helper::Dictionary> promise);
 
-bool ShowSaveDialogSync(const DialogSettings& settings, base::FilePath* path);
+std::optional<base::FilePath> ShowSaveDialogSync(
+    const DialogSettings& settings);
 
 void ShowSaveDialog(const DialogSettings& settings,
                     gin_helper::Promise<gin_helper::Dictionary> promise);

shell/browser/ui/file_dialog_linux.cc

@@ -233,20 +233,25 @@ void ShowOpenDialog(const DialogSettings& settings,
   dialog->RunOpenDialog(std::move(promise), settings);
 }
 
-bool ShowSaveDialogSync(const DialogSettings& settings, base::FilePath* path) {
+std::optional<base::FilePath> ShowSaveDialogSync(
-  base::RunLoop run_loop(base::RunLoop::Type::kNestableTasksAllowed);
+    const DialogSettings& settings) {
-  auto cb = base::BindOnce(
+  std::optional<base::FilePath> path;
-      [](base::RepeatingClosure cb, base::FilePath* file_path,
-         gin_helper::Dictionary result) {
-        result.Get("filePath", file_path);
-        std::move(cb).Run();
-      },
-      run_loop.QuitClosure(), path);
 
-  FileChooserDialog* dialog = new FileChooserDialog();
+  base::RunLoop run_loop{base::RunLoop::Type::kNestableTasksAllowed};
-  dialog->RunSaveDialog(std::move(cb), settings);
+  auto on_chooser_dialog_done = base::BindOnce(
+      [](base::RepeatingClosure run_loop_closure,
+         std::optional<base::FilePath>* path, gin_helper::Dictionary result) {
+        if (base::FilePath val; result.Get("filePath", &val))
+          *path = std::move(val);
+        std::move(run_loop_closure).Run();
+      },
+      run_loop.QuitClosure(), &path);
+
+  auto* const dialog = new FileChooserDialog{};
+  dialog->RunSaveDialog(std::move(on_chooser_dialog_done), settings);
   run_loop.Run();
-  return !path->empty();
+
+  return path;
 }
 
 void ShowSaveDialog(const DialogSettings& settings,

shell/browser/ui/file_dialog_mac.mm

@@ -436,19 +436,18 @@ void ShowOpenDialog(const DialogSettings& settings,
   }
 }
 
-bool ShowSaveDialogSync(const DialogSettings& settings, base::FilePath* path) {
+std::optional<base::FilePath> ShowSaveDialogSync(
-  DCHECK(path);
+    const DialogSettings& settings) {
   NSSavePanel* dialog = [NSSavePanel savePanel];
 
   SetupDialog(dialog, settings);
   SetupSaveDialogForProperties(dialog, settings.properties);
 
-  int chosen = RunModalDialog(dialog, settings);
+  const int chosen = RunModalDialog(dialog, settings);
   if (chosen == NSModalResponseCancel || ![[dialog URL] isFileURL])
-    return false;
+    return {};
 
-  *path = base::FilePath(base::SysNSStringToUTF8([[dialog URL] path]));
+  return base::FilePath{base::SysNSStringToUTF8([[dialog URL] path])};
-  return true;
 }
 
 void SaveDialogCompletion(int chosen,

shell/browser/ui/file_dialog_win.cc

@@ -233,11 +233,12 @@ void ShowOpenDialog(const DialogSettings& settings,
                      base::BindOnce(done, std::move(promise)));
 }
 
-bool ShowSaveDialogSync(const DialogSettings& settings, base::FilePath* path) {
+std::optional<base::FilePath> ShowSaveDialogSync(
+    const DialogSettings& settings) {
   ATL::CComPtr<IFileSaveDialog> file_save_dialog;
   HRESULT hr = file_save_dialog.CoCreateInstance(CLSID_FileSaveDialog);
   if (FAILED(hr))
-    return false;
+    return {};
 
   DWORD options = FOS_FORCEFILESYSTEM | FOS_PATHMUSTEXIST | FOS_OVERWRITEPROMPT;
   if (settings.properties & SAVE_DIALOG_SHOW_HIDDEN_FILES)
@@ -250,32 +251,31 @@ bool ShowSaveDialogSync(const DialogSettings& settings, base::FilePath* path) {
   hr = ShowFileDialog(file_save_dialog, settings);
 
   if (FAILED(hr))
-    return false;
+    return {};
 
   CComPtr<IShellItem> pItem;
   hr = file_save_dialog->GetResult(&pItem);
   if (FAILED(hr))
-    return false;
+    return {};
 
   PWSTR result_path = nullptr;
   hr = pItem->GetDisplayName(SIGDN_FILESYSPATH, &result_path);
   if (!SUCCEEDED(hr))
-    return false;
+    return {};
 
-  *path = base::FilePath(result_path);
+  auto path = base::FilePath{result_path};
   CoTaskMemFree(result_path);
-
+  return path;
-  return true;
 }
 
 void ShowSaveDialog(const DialogSettings& settings,
                     gin_helper::Promise<gin_helper::Dictionary> promise) {
   auto done = [](gin_helper::Promise<gin_helper::Dictionary> promise,
-                 bool success, base::FilePath result) {
+                 std::optional<base::FilePath> result) {
     v8::HandleScope handle_scope(promise.isolate());
     auto dict = gin::Dictionary::CreateEmpty(promise.isolate());
-    dict.Set("canceled", !success);
+    dict.Set("canceled", !result.has_value());
-    dict.Set("filePath", result);
+    dict.Set("filePath", result.value_or(base::FilePath{}));
     promise.Resolve(dict);
   };
   dialog_thread::Run(base::BindOnce(ShowSaveDialogSync, settings),