Merge pull request #6983 from electron/download-item-prompt

Check DownloadItem save path before prompting

atom/browser/atom_download_manager_delegate.cc

@@ -35,6 +35,17 @@ AtomDownloadManagerDelegate::~AtomDownloadManagerDelegate() {
   }
 }
 
+void AtomDownloadManagerDelegate::GetItemSavePath(content::DownloadItem* item,
+                                                  base::FilePath* path) {
+  v8::Isolate* isolate = v8::Isolate::GetCurrent();
+  v8::Locker locker(isolate);
+  v8::HandleScope handle_scope(isolate);
+  api::DownloadItem* download = api::DownloadItem::FromWrappedClass(isolate,
+                                                                    item);
+  if (download)
+    *path = download->GetSavePath();
+}
+
 void AtomDownloadManagerDelegate::CreateDownloadPath(
     const GURL& url,
     const std::string& content_disposition,
@@ -77,9 +88,12 @@ void AtomDownloadManagerDelegate::OnDownloadPathGenerated(
     window = relay->window.get();
 
   base::FilePath path;
-  if (file_dialog::ShowSaveDialog(window, item->GetURL().spec(),
-                                  "", default_path,
-                                  file_dialog::Filters(), &path)) {
+  GetItemSavePath(item, &path);
+  // Show save dialog if save path was not set already on item
+  if (path.empty() && file_dialog::ShowSaveDialog(window, item->GetURL().spec(),
+                                                  "", default_path,
+                                                  file_dialog::Filters(),
+                                                  &path)) {
     // Remember the last selected download directory.
     AtomBrowserContext* browser_context = static_cast<AtomBrowserContext*>(
         download_manager_->GetBrowserContext());
@@ -122,22 +136,14 @@ bool AtomDownloadManagerDelegate::DetermineDownloadTarget(
   }
 
   // Try to get the save path from JS wrapper.
-  {
-    v8::Isolate* isolate = v8::Isolate::GetCurrent();
-    v8::Locker locker(isolate);
-    v8::HandleScope handle_scope(isolate);
-    api::DownloadItem* download_item = api::DownloadItem::FromWrappedClass(
-        isolate, download);
-    if (download_item) {
-      base::FilePath save_path = download_item->GetSavePath();
-      if (!save_path.empty()) {
-        callback.Run(save_path,
-                     content::DownloadItem::TARGET_DISPOSITION_OVERWRITE,
-                     content::DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS,
-                     save_path);
-        return true;
-      }
-    }
+  base::FilePath save_path;
+  GetItemSavePath(download, &save_path);
+  if (!save_path.empty()) {
+    callback.Run(save_path,
+                 content::DownloadItem::TARGET_DISPOSITION_OVERWRITE,
+                 content::DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS,
+                 save_path);
+    return true;
   }
 
   AtomBrowserContext* browser_context = static_cast<AtomBrowserContext*>(

atom/browser/atom_download_manager_delegate.h

@@ -46,6 +46,9 @@ class AtomDownloadManagerDelegate : public content::DownloadManagerDelegate {
   void GetNextId(const content::DownloadIdCallback& callback) override;
 
  private:
+  // Get the save path set on the associated api::DownloadItem object
+  void GetItemSavePath(content::DownloadItem* item, base::FilePath* path);
+
   content::DownloadManager* download_manager_;
   base::WeakPtrFactory<AtomDownloadManagerDelegate> weak_ptr_factory_;
 

spec/api-session-spec.js

@@ -319,6 +319,17 @@ describe('session module', function () {
         })
       })
     })
+
+    describe('when a save path is specified and the URL is unavailable', function () {
+      it('does not display a save dialog and reports the done state as interrupted', function (done) {
+        ipcRenderer.sendSync('set-download-option', false, false)
+        ipcRenderer.once('download-done', (event, state) => {
+          assert.equal(state, 'interrupted')
+          done()
+        })
+        w.webContents.downloadURL('file://' + path.join(__dirname, 'does-not-exist.txt'))
+      })
+    })
   })
 
   describe('ses.protocol', function () {