fix: update the FileSelectHelper to support the new promise API (#18288)

* fix: update the FileSelectHelper to support the new promise API

Fixes #18254

So it turns out we've successfully introduced a way to write
non-typesafe C++.

This fixes two things:
* Uses the object the promise resolves
* Ensures we attach the Then handler before moving the promise

* fix: also fix misuse of Promise::Then in the download manager

atom/browser/atom_download_manager_delegate.cc

@@ -127,8 +127,8 @@ void AtomDownloadManagerDelegate::OnDownloadPathGenerated(
         base::BindOnce(&AtomDownloadManagerDelegate::OnDownloadSaveDialogDone,
                        base::Unretained(this), download_id, callback);
 
-    file_dialog::ShowSaveDialog(settings, std::move(dialog_promise));
     ignore_result(dialog_promise.Then(std::move(dialog_callback)));
+    file_dialog::ShowSaveDialog(settings, std::move(dialog_promise));
   } else {
     callback.Run(path, download::DownloadItem::TARGET_DISPOSITION_PROMPT,
                  download::DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS, path,
@@ -136,28 +136,23 @@ void AtomDownloadManagerDelegate::OnDownloadPathGenerated(
   }
 }
 
-#if defined(MAS_BUILD)
 void AtomDownloadManagerDelegate::OnDownloadSaveDialogDone(
     uint32_t download_id,
     const content::DownloadTargetCallback& download_callback,
-    bool result,
+    mate::Dictionary result) {
-    const base::FilePath& path,
-    const std::string& bookmark)
-#else
-void AtomDownloadManagerDelegate::OnDownloadSaveDialogDone(
-    uint32_t download_id,
-    const content::DownloadTargetCallback& download_callback,
-    bool result,
-    const base::FilePath& path)
-#endif
-{
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
 
   auto* item = download_manager_->GetDownload(download_id);
   if (!item)
     return;
 
-  if (result) {
+  bool canceled = true;
+  result.Get("canceled", &canceled);
+
+  base::FilePath path;
+
+  if (!canceled) {
+    if (result.Get("filePath", &path)) {
       // Remember the last selected download directory.
       AtomBrowserContext* browser_context = static_cast<AtomBrowserContext*>(
           download_manager_->GetBrowserContext());
@@ -172,18 +167,17 @@ void AtomDownloadManagerDelegate::OnDownloadSaveDialogDone(
       if (download_item)
         download_item->SetSavePath(path);
     }
+  }
 
   // Running the DownloadTargetCallback with an empty FilePath signals that the
   // download should be cancelled. If user cancels the file save dialog, run
   // the callback with empty FilePath.
-  const base::FilePath download_path = result ? path : base::FilePath();
   const auto interrupt_reason =
-      download_path.empty() ? download::DOWNLOAD_INTERRUPT_REASON_USER_CANCELED
+      path.empty() ? download::DOWNLOAD_INTERRUPT_REASON_USER_CANCELED
                    : download::DOWNLOAD_INTERRUPT_REASON_NONE;
-  download_callback.Run(download_path,
+  download_callback.Run(path, download::DownloadItem::TARGET_DISPOSITION_PROMPT,
-                        download::DownloadItem::TARGET_DISPOSITION_PROMPT,
+                        download::DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS, path,
-                        download::DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS,
+                        interrupt_reason);
-                        download_path, interrupt_reason);
 }
 
 void AtomDownloadManagerDelegate::Shutdown() {

atom/browser/atom_download_manager_delegate.h

@@ -45,20 +45,10 @@ class AtomDownloadManagerDelegate : public content::DownloadManagerDelegate {
                                const content::DownloadTargetCallback& callback,
                                const base::FilePath& default_path);
 
-#if defined(MAS_BUILD)
   void OnDownloadSaveDialogDone(
       uint32_t download_id,
       const content::DownloadTargetCallback& download_callback,
-      bool result,
+      mate::Dictionary result);
-      const base::FilePath& path,
-      const std::string& bookmark);
-#else
-  void OnDownloadSaveDialogDone(
-      uint32_t download_id,
-      const content::DownloadTargetCallback& download_callback,
-      bool result,
-      const base::FilePath& path);
-#endif
 
   content::DownloadManager* download_manager_;
   base::WeakPtrFactory<AtomDownloadManagerDelegate> weak_ptr_factory_;

atom/browser/web_dialog_helper.cc

@@ -24,6 +24,7 @@
 #include "content/public/browser/render_view_host.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/browser/web_contents_observer.h"
+#include "native_mate/dictionary.h"
 #include "net/base/mime_util.h"
 #include "ui/shell_dialogs/selected_file_info.h"
 
@@ -51,9 +52,10 @@ class FileSelectHelper : public base::RefCounted<FileSelectHelper>,
     v8::Isolate* isolate = v8::Isolate::GetCurrent();
     atom::util::Promise promise(isolate);
 
-    file_dialog::ShowOpenDialog(settings, std::move(promise));
     auto callback = base::BindOnce(&FileSelectHelper::OnOpenDialogDone, this);
     ignore_result(promise.Then(std::move(callback)));
+
+    file_dialog::ShowOpenDialog(settings, std::move(promise));
   }
 
   void ShowSaveDialog(const file_dialog::DialogSettings& settings) {
@@ -74,16 +76,14 @@ class FileSelectHelper : public base::RefCounted<FileSelectHelper>,
 
   ~FileSelectHelper() override {}
 
-#if defined(MAS_BUILD)
+  void OnOpenDialogDone(mate::Dictionary result) {
-  void OnOpenDialogDone(bool result,
-                        const std::vector<base::FilePath>& paths,
-                        const std::vector<std::string>& bookmarks)
-#else
-  void OnOpenDialogDone(bool result, const std::vector<base::FilePath>& paths)
-#endif
-  {
     std::vector<FileChooserFileInfoPtr> file_info;
-    if (result) {
+    bool canceled = true;
+    result.Get("canceled", &canceled);
+
+    if (!canceled) {
+      std::vector<base::FilePath> paths;
+      if (result.Get("filePaths", &paths)) {
         for (auto& path : paths) {
           file_info.push_back(FileChooserFileInfo::NewNativeFile(
               blink::mojom::NativeFileInfo::New(
@@ -97,23 +97,23 @@ class FileSelectHelper : public base::RefCounted<FileSelectHelper>,
                                                 paths[0].DirName());
         }
       }
+    }
     OnFilesSelected(std::move(file_info));
   }
 
-#if defined(MAS_BUILD)
+  void OnSaveDialogDone(mate::Dictionary result) {
-  void OnSaveDialogDone(bool result,
-                        const base::FilePath& path,
-                        const std::string& bookmark)
-#else
-  void OnSaveDialogDone(bool result, const base::FilePath& path)
-#endif
-  {
     std::vector<FileChooserFileInfoPtr> file_info;
-    if (result) {
+    bool canceled = true;
-      file_info.push_back(
+    result.Get("canceled", &canceled);
-          FileChooserFileInfo::NewNativeFile(blink::mojom::NativeFileInfo::New(
+
+    if (!canceled) {
+      base::FilePath path;
+      if (result.Get("filePath", &path)) {
+        file_info.push_back(FileChooserFileInfo::NewNativeFile(
+            blink::mojom::NativeFileInfo::New(
                 path, path.BaseName().AsUTF16Unsafe())));
       }
+    }
     OnFilesSelected(std::move(file_info));
   }
 
@@ -123,7 +123,6 @@ class FileSelectHelper : public base::RefCounted<FileSelectHelper>,
       listener_.reset();
     }
     render_frame_host_ = nullptr;
-    Release();
   }
 
   // content::WebContentsObserver:

atom/common/promise_util.h

@@ -104,9 +104,25 @@ class Promise {
     return GetInner()->Reject(GetContext(), v8::Undefined(isolate()));
   }
 
-  template <typename ReturnType, typename... ArgTypes>
+  // Please note that using Then is effectively the same as calling .then
+  // in javascript.  This means (a) it is not type safe and (b) please note
+  // it is NOT type safe.
+  // If the base::Callback you provide here is of type void(boolean) and you
+  // resolve the promise with a string, Electron will compile successfully and
+  // then that promise will be rejected as soon as you try to use it as the
+  // mate converters doing work behind the scenes will throw an error for you.
+  // This can be really hard to trace so until either
+  //   * This helper becomes typesafe (by templating the class instead of each
+  //   method)
+  //   * or the world goes mad
+  // Please try your hardest not to use this method
+  // The world thanks you
+  template <typename... ResolveType>
   v8::MaybeLocal<v8::Promise> Then(
-      base::OnceCallback<ReturnType(ArgTypes...)> cb) {
+      base::OnceCallback<void(ResolveType...)> cb) {
+    static_assert(sizeof...(ResolveType) <= 1,
+                  "A promise's 'Then' callback should only receive at most one "
+                  "parameter");
     v8::HandleScope handle_scope(isolate());
     v8::Context::Scope context_scope(
         v8::Local<v8::Context>::New(isolate(), GetContext()));