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

* 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::Bind(&AtomDownloadManagerDelegate::OnDownloadSaveDialogDone,
                    base::Unretained(this), download_id, callback);
 
-    file_dialog::ShowSaveDialog(settings, std::move(dialog_promise));
     ignore_result(dialog_promise.Then(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,54 +136,48 @@ void AtomDownloadManagerDelegate::OnDownloadPathGenerated(
   }
 }
 
-#if defined(MAS_BUILD)
-void AtomDownloadManagerDelegate::OnDownloadSaveDialogDone(
-    uint32_t download_id,
-    const content::DownloadTargetCallback& download_callback,
-    bool 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
-{
+    mate::Dictionary result) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
 
   auto* item = download_manager_->GetDownload(download_id);
   if (!item)
     return;
 
-  if (result) {
-    // Remember the last selected download directory.
-    AtomBrowserContext* browser_context = static_cast<AtomBrowserContext*>(
-        download_manager_->GetBrowserContext());
-    browser_context->prefs()->SetFilePath(prefs::kDownloadDefaultDirectory,
-                                          path.DirName());
+  bool canceled = true;
+  result.Get("canceled", &canceled);
 
-    v8::Isolate* isolate = v8::Isolate::GetCurrent();
-    v8::Locker locker(isolate);
-    v8::HandleScope handle_scope(isolate);
-    api::DownloadItem* download_item =
-        api::DownloadItem::FromWrappedClass(isolate, item);
-    if (download_item)
-      download_item->SetSavePath(path);
+  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());
+      browser_context->prefs()->SetFilePath(prefs::kDownloadDefaultDirectory,
+                                            path.DirName());
+
+      v8::Isolate* isolate = v8::Isolate::GetCurrent();
+      v8::Locker locker(isolate);
+      v8::HandleScope handle_scope(isolate);
+      api::DownloadItem* download_item =
+          api::DownloadItem::FromWrappedClass(isolate, item);
+      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
-                            : download::DOWNLOAD_INTERRUPT_REASON_NONE;
-  download_callback.Run(download_path,
-                        download::DownloadItem::TARGET_DISPOSITION_PROMPT,
-                        download::DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS,
-                        download_path, interrupt_reason);
+      path.empty() ? download::DOWNLOAD_INTERRUPT_REASON_USER_CANCELED
+                   : download::DOWNLOAD_INTERRUPT_REASON_NONE;
+  download_callback.Run(path, download::DownloadItem::TARGET_DISPOSITION_PROMPT,
+                        download::DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS, 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,
-      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
+      mate::Dictionary result);
 
   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::Bind(&FileSelectHelper::OnOpenDialogDone, this);
     ignore_result(promise.Then(callback));
+
+    file_dialog::ShowOpenDialog(settings, std::move(promise));
   }
 
   void ShowSaveDialog(const file_dialog::DialogSettings& settings) {
@@ -74,45 +76,43 @@ class FileSelectHelper : public base::RefCounted<FileSelectHelper>,
 
   ~FileSelectHelper() override {}
 
-#if defined(MAS_BUILD)
-  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
-  {
+  void OnOpenDialogDone(mate::Dictionary result) {
     std::vector<FileChooserFileInfoPtr> file_info;
-    if (result) {
-      for (auto& path : paths) {
-        file_info.push_back(FileChooserFileInfo::NewNativeFile(
-            blink::mojom::NativeFileInfo::New(
-                path, path.BaseName().AsUTF16Unsafe())));
-      }
-
-      if (render_frame_host_ && !paths.empty()) {
-        auto* browser_context = static_cast<atom::AtomBrowserContext*>(
-            render_frame_host_->GetProcess()->GetBrowserContext());
-        browser_context->prefs()->SetFilePath(prefs::kSelectFileLastDirectory,
-                                              paths[0].DirName());
+    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(
+                  path, path.BaseName().AsUTF16Unsafe())));
+        }
+
+        if (render_frame_host_ && !paths.empty()) {
+          auto* browser_context = static_cast<atom::AtomBrowserContext*>(
+              render_frame_host_->GetProcess()->GetBrowserContext());
+          browser_context->prefs()->SetFilePath(prefs::kSelectFileLastDirectory,
+                                                paths[0].DirName());
+        }
       }
     }
     OnFilesSelected(std::move(file_info));
   }
 
-#if defined(MAS_BUILD)
-  void OnSaveDialogDone(bool result,
-                        const base::FilePath& path,
-                        const std::string& bookmark)
-#else
-  void OnSaveDialogDone(bool result, const base::FilePath& path)
-#endif
-  {
+  void OnSaveDialogDone(mate::Dictionary result) {
     std::vector<FileChooserFileInfoPtr> file_info;
-    if (result) {
-      file_info.push_back(
-          FileChooserFileInfo::NewNativeFile(blink::mojom::NativeFileInfo::New(
-              path, path.BaseName().AsUTF16Unsafe())));
+    bool canceled = true;
+    result.Get("canceled", &canceled);
+
+    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

@@ -103,8 +103,24 @@ class Promise {
     return GetInner()->Reject(GetContext(), v8::Undefined(isolate()));
   }
 
-  template <typename ReturnType, typename... ArgTypes>
-  v8::MaybeLocal<v8::Promise> Then(base::Callback<ReturnType(ArgTypes...)> cb) {
+  // 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::Callback<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()));