Redefine 'will-download' design.

atom/browser/api/atom_api_download_item.cc

@@ -4,9 +4,13 @@
 
 #include "atom/browser/api/atom_api_download_item.h"
 
+#include <map>
+
 #include "atom/common/native_mate_converters/callback.h"
+#include "atom/common/native_mate_converters/file_path_converter.h"
 #include "atom/common/native_mate_converters/gurl_converter.h"
 #include "atom/common/node_includes.h"
+#include "base/memory/linked_ptr.h"
 #include "native_mate/dictionary.h"
 
 namespace mate {
@@ -43,27 +47,36 @@ namespace {
 // The wrapDownloadItem funtion which is implemented in JavaScript
 using WrapDownloadItemCallback = base::Callback<void(v8::Local<v8::Value>)>;
 WrapDownloadItemCallback g_wrap_download_item;
+
+char kDownloadItemSavePathKey[] = "DownloadItemSavePathKey";
+
+std::map<uint32, linked_ptr<v8::Global<v8::Value>>> g_download_item_objects;
 }  // namespace
 
+DownloadItem::SavePathData::SavePathData(const base::FilePath& path) :
+  path_(path) {
+}
+
+const base::FilePath& DownloadItem::SavePathData::path() {
+  return path_;
+}
+
 DownloadItem::DownloadItem(content::DownloadItem* download_item) :
     download_item_(download_item) {
   download_item_->AddObserver(this);
 }
 
 DownloadItem::~DownloadItem() {
-  download_item_->RemoveObserver(this);
-}
-
-void DownloadItem::OnDownloadUpdated(content::DownloadItem* item) {
-  if (download_item_ == item) {
-    download_item_->IsDone() ?
-        Emit("done", item->GetState()) : Emit("updated");
-  }
+  Destroy();
 }
 
-void DownloadItem::OnDownloadDestroyed(content::DownloadItem* download) {
-  if (download_item_ == download) {
+void DownloadItem::Destroy() {
+  if (download_item_) {
     download_item_->RemoveObserver(this);
+    auto iter = g_download_item_objects.find(download_item_->GetId());
+    if (iter != g_download_item_objects.end())
+      g_download_item_objects.erase(iter);
+    download_item_ = nullptr;
   }
 }
 
@@ -71,8 +84,12 @@ bool DownloadItem::IsDestroyed() const {
   return download_item_ == nullptr;
 }
 
-void DownloadItem::Destroy() {
-  download_item_ = nullptr;
+void DownloadItem::OnDownloadUpdated(content::DownloadItem* item) {
+  download_item_->IsDone() ? Emit("done", item->GetState()) : Emit("updated");
+}
+
+void DownloadItem::OnDownloadDestroyed(content::DownloadItem* download) {
+  Destroy();
 }
 
 int64 DownloadItem::GetReceivedBytes() {
@@ -83,7 +100,7 @@ int64 DownloadItem::GetTotalBytes() {
   return download_item_->GetTotalBytes();
 }
 
-const GURL& DownloadItem::GetURL() {
+const GURL& DownloadItem::GetUrl() {
   return download_item_->GetURL();
 }
 
@@ -103,6 +120,10 @@ std::string DownloadItem::GetContentDisposition() {
   return download_item_->GetContentDisposition();
 }
 
+void DownloadItem::SetSavePath(const base::FilePath& path) {
+  download_item_->SetUserData(UserDataKey(), new SavePathData(path));
+}
+
 void DownloadItem::Pause() {
   download_item_->Pause();
 }
@@ -121,13 +142,14 @@ mate::ObjectTemplateBuilder DownloadItem::GetObjectTemplateBuilder(
       .SetMethod("pause", &DownloadItem::Pause)
       .SetMethod("resume", &DownloadItem::Resume)
       .SetMethod("cancel", &DownloadItem::Cancel)
-      .SetMethod("getReceiveBytes", &DownloadItem::GetReceivedBytes)
+      .SetMethod("getReceivedBytes", &DownloadItem::GetReceivedBytes)
       .SetMethod("getTotalBytes", &DownloadItem::GetTotalBytes)
-      .SetMethod("getURL", &DownloadItem::GetURL)
+      .SetMethod("getUrl", &DownloadItem::GetUrl)
       .SetMethod("getMimeType", &DownloadItem::GetMimeType)
       .SetMethod("hasUserGesture", &DownloadItem::HasUserGesture)
       .SetMethod("getSuggestedFilename", &DownloadItem::GetSuggestedFilename)
-      .SetMethod("getContentDisposition", &DownloadItem::GetContentDisposition);
+      .SetMethod("getContentDisposition", &DownloadItem::GetContentDisposition)
+      .SetMethod("setSavePath", &DownloadItem::SetSavePath);
 }
 
 void SetWrapDownloadItem(const WrapDownloadItemCallback& callback) {
@@ -138,13 +160,21 @@ void ClearWrapDownloadItem() {
   g_wrap_download_item.Reset();
 }
 
+// static
 mate::Handle<DownloadItem> DownloadItem::Create(
     v8::Isolate* isolate, content::DownloadItem* item) {
   auto handle = mate::CreateHandle(isolate, new DownloadItem(item));
   g_wrap_download_item.Run(handle.ToV8());
+  g_download_item_objects[item->GetId()] = make_linked_ptr(
+      new v8::Global<v8::Value>(isolate, handle.ToV8()));
   return handle;
 }
 
+// static
+void* DownloadItem::UserDataKey() {
+  return &kDownloadItemSavePathKey;
+}
+
 }  // namespace api
 
 }  // namespace atom

atom/browser/api/atom_api_download_item.h

@@ -8,6 +8,7 @@
 #include <string>
 
 #include "atom/browser/api/trackable_object.h"
+#include "base/files/file_path.h"
 #include "content/public/browser/download_item.h"
 #include "native_mate/handle.h"
 #include "url/gurl.h"
@@ -16,9 +17,17 @@ namespace atom {
 
 namespace api {
 
-class DownloadItem : public mate::TrackableObject<DownloadItem>,
+class DownloadItem : public mate::EventEmitter,
                      public content::DownloadItem::Observer {
  public:
+  class SavePathData : public base::SupportsUserData::Data {
+   public:
+    explicit SavePathData(const base::FilePath& path);
+    const base::FilePath& path();
+   private:
+    base::FilePath path_;
+  };
+
   explicit DownloadItem(content::DownloadItem* download_item);
   ~DownloadItem();
   static mate::Handle<DownloadItem> Create(v8::Isolate* isolate,
@@ -37,16 +46,17 @@ class DownloadItem : public mate::TrackableObject<DownloadItem>,
   bool HasUserGesture();
   std::string GetSuggestedFilename();
   std::string GetContentDisposition();
-  const GURL& GetURL();
+  const GURL& GetUrl();
+  void SetSavePath(const base::FilePath& path);
 
+  static void* UserDataKey();
  private:
   // mate::Wrappable:
   mate::ObjectTemplateBuilder GetObjectTemplateBuilder(
       v8::Isolate* isolate) override;
   bool IsDestroyed() const override;
 
-  // mate::TrackableObject:
-  void Destroy() override;
+  void Destroy();
 
   content::DownloadItem* download_item_;
 

atom/browser/api/atom_api_session.cc

@@ -10,7 +10,6 @@
 #include "atom/browser/api/atom_api_cookies.h"
 #include "atom/browser/api/atom_api_download_item.h"
 #include "atom/browser/atom_browser_context.h"
-#include "atom/browser/atom_download_manager_delegate.h"
 #include "atom/browser/api/atom_api_web_contents.h"
 #include "atom/common/native_mate_converters/callback.h"
 #include "atom/common/native_mate_converters/gurl_converter.h"
@@ -295,13 +294,6 @@ void Session::SetDownloadPath(const base::FilePath& path) {
       prefs::kDownloadDefaultDirectory, path);
 }
 
-void Session::SetOpenDownloadDialog(bool open_download_dialog) {
-  AtomDownloadManagerDelegate* delegate =
-      static_cast<AtomDownloadManagerDelegate*>(
-          browser_context()->GetDownloadManagerDelegate());
-  delegate->SetOpenDownloadDialog(open_download_dialog);
-}
-
 v8::Local<v8::Value> Session::Cookies(v8::Isolate* isolate) {
   if (cookies_.IsEmpty()) {
     auto handle = atom::api::Cookies::Create(isolate, browser_context());
@@ -318,7 +310,6 @@ mate::ObjectTemplateBuilder Session::GetObjectTemplateBuilder(
       .SetMethod("clearStorageData", &Session::ClearStorageData)
       .SetMethod("setProxy", &Session::SetProxy)
       .SetMethod("setDownloadPath", &Session::SetDownloadPath)
-      .SetMethod("setOpenDownloadDialog", &Session::SetOpenDownloadDialog)
       .SetProperty("cookies", &Session::Cookies);
 }
 

atom/browser/api/atom_api_session.h

@@ -43,8 +43,6 @@ class Session: public mate::TrackableObject<Session>,
 
   AtomBrowserContext* browser_context() const { return browser_context_.get(); }
 
-  void SetOpenDownloadDialog(bool open_download_dialog);
-
  protected:
   explicit Session(AtomBrowserContext* browser_context);
   ~Session();

atom/browser/api/lib/app.coffee

@@ -15,7 +15,7 @@ wrapDownloadItem = (download_item) ->
   # download_item is an Event Emitter.
   download_item.__proto__ = EventEmitter.prototype
   # Be compatible with old APIs.
-  download_item.url = download_item.getURL()
+  download_item.url = download_item.getUrl()
   download_item.filename = download_item.getSuggestedFilename()
   download_item.mimeType = download_item.getMimeType()
   download_item.hasUserGesture = download_item.hasUserGesture()

atom/browser/atom_download_manager_delegate.cc

@@ -6,6 +6,7 @@
 
 #include <string>
 
+#include "atom/browser/api/atom_api_download_item.h"
 #include "atom/browser/atom_browser_context.h"
 #include "atom/browser/native_window.h"
 #include "atom/browser/ui/file_dialog.h"
@@ -23,8 +24,7 @@ namespace atom {
 AtomDownloadManagerDelegate::AtomDownloadManagerDelegate(
     content::DownloadManager* manager)
     : download_manager_(manager),
-      weak_ptr_factory_(this),
-      open_download_dialog_(true) {}
+      weak_ptr_factory_(this) {}
 
 AtomDownloadManagerDelegate::~AtomDownloadManagerDelegate() {
   if (download_manager_) {
@@ -74,20 +74,14 @@ void AtomDownloadManagerDelegate::OnDownloadPathGenerated(
   if (relay)
     window = relay->window.get();
 
-  file_dialog::Filters filters;
   base::FilePath path;
-  if (!open_download_dialog_) {
-    // Use default_path if download dialog is disabled.
-    path = default_path;
-  } else {
-    if (file_dialog::ShowSaveDialog(window, item->GetURL().spec(), default_path,
-                                    filters, &path)) {
-      // Remember the last selected download directory.
-      AtomBrowserContext* browser_context = static_cast<AtomBrowserContext*>(
-          download_manager_->GetBrowserContext());
-      browser_context->prefs()->SetFilePath(prefs::kDownloadDefaultDirectory,
-                                            path.DirName());
-    }
+  if (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());
+    browser_context->prefs()->SetFilePath(prefs::kDownloadDefaultDirectory,
+                                          path.DirName());
   }
 
   // Running the DownloadTargetCallback with an empty FilePath signals that the
@@ -98,11 +92,6 @@ void AtomDownloadManagerDelegate::OnDownloadPathGenerated(
                content::DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS, path);
 }
 
-void AtomDownloadManagerDelegate::SetOpenDownloadDialog(
-    bool open_download_dialog) {
-  open_download_dialog_ = open_download_dialog;
-}
-
 void AtomDownloadManagerDelegate::Shutdown() {
   weak_ptr_factory_.InvalidateWeakPtrs();
   download_manager_ = nullptr;
@@ -113,6 +102,25 @@ bool AtomDownloadManagerDelegate::DetermineDownloadTarget(
     const content::DownloadTargetCallback& callback) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
 
+  if (!download->GetForcedFilePath().empty()) {
+    callback.Run(download->GetForcedFilePath(),
+                 content::DownloadItem::TARGET_DISPOSITION_OVERWRITE,
+                 content::DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS,
+                 download->GetForcedFilePath());
+    return true;
+  }
+  base::SupportsUserData::Data* save_path = download->GetUserData(
+      atom::api::DownloadItem::UserDataKey());
+  if (save_path) {
+    const base::FilePath& default_download_path =
+        static_cast<api::DownloadItem::SavePathData*>(save_path)->path();
+    callback.Run(default_download_path,
+                 content::DownloadItem::TARGET_DISPOSITION_OVERWRITE,
+                 content::DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS,
+                 default_download_path);
+    return true;
+  }
+
   AtomBrowserContext* browser_context = static_cast<AtomBrowserContext*>(
       download_manager_->GetBrowserContext());
   base::FilePath default_download_path = browser_context->prefs()->GetFilePath(
@@ -123,14 +131,6 @@ bool AtomDownloadManagerDelegate::DetermineDownloadTarget(
     default_download_path = path.Append(FILE_PATH_LITERAL("Downloads"));
   }
 
-  if (!download->GetForcedFilePath().empty()) {
-    callback.Run(download->GetForcedFilePath(),
-                 content::DownloadItem::TARGET_DISPOSITION_OVERWRITE,
-                 content::DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS,
-                 download->GetForcedFilePath());
-    return true;
-  }
-
   CreateDownloadPathCallback download_path_callback =
       base::Bind(&AtomDownloadManagerDelegate::OnDownloadPathGenerated,
                  weak_ptr_factory_.GetWeakPtr(),

atom/browser/atom_download_manager_delegate.h

@@ -49,7 +49,6 @@ class AtomDownloadManagerDelegate : public content::DownloadManagerDelegate {
  private:
   content::DownloadManager* download_manager_;
   base::WeakPtrFactory<AtomDownloadManagerDelegate> weak_ptr_factory_;
-  bool open_download_dialog_;
 
   DISALLOW_COPY_AND_ASSIGN(AtomDownloadManagerDelegate);
 };

docs/api/download-item.md

@@ -5,16 +5,15 @@ is used in `will-download` event of `Session` module, and allows users to
 control the download item.
 
 ```javascript
-// Disable showing the download saving dialog.
-win.webContents.session.setOpenDownloadDialog(false);
-
+// In the main process.
 win.webContents.session.on('will-download', function(event, item, webContents) {
-  console.log("Download from " + item.getURL());
+  // Set the save path, making Electron not to prompt a save dialog.
+  item.setSavePath('/tmp/save.pdf');
   console.log(item.getMimeType());
   console.log(item.getSuggestedFilename());
   console.log(item.getTotalBytes());
   item.on('updated', function() {
-    console.log('Recived bytes: ' + item.getReceiveBytes());
+    console.log('Recived bytes: ' + item.getReceivedBytes());
   });
   item.on('done', function(e, state) {
     if (state == "completed") {
@@ -48,6 +47,14 @@ download that can't be resumed.
 
 The `downloadItem` object has the following methods:
 
+### `downloadItem.setSavePath(path)`
+
+* `path` String - Set the save file path of the download item.
+
+The API is only available in session's `will-download` callback function.
+If user doesn't set the save path via the API, Electron will use the original
+routine to determine the save path(Usually prompts a save dialog).
+
 ### `downloadItem.pause()`
 
 Pauses the download.

docs/api/session.md

@@ -191,10 +191,3 @@ proxy-uri = [<proxy-scheme>"://"]<proxy-host>[":"<proxy-port>]
 
 Sets download saving directory. By default, the download directory will be the
 `Downloads` under the respective app folder.
-
-### `session.setOpenDownloadDialog(openDownloadDialog)`
-
-* `openDownloadDialog` Boolean - Whether a download saving dialog will be
-  prompted when the download starts.
-
-By default, the download saving dialog is enable in Electron.

spec/api-session-spec.coffee

@@ -75,15 +75,11 @@ describe 'session module', ->
           w.webContents.send 'getcount'
 
   describe 'DownloadItem', ->
-    # A 5MB mock pdf.
-    mockPDF = new Buffer(1024*1024*5)
+    # A 5 MB mock pdf.
+    mockPDF = new Buffer 1024 * 1024 * 5
     contentDisposition = 'inline; filename="mock.pdf"'
-    # TODO(hokein): Change the download directory to spec/fixtures directory.
-    # We have to use the # default download directory due to the broken
-    # session.setDownloadPath API is broken. Once the API is fixed, we can make
-    # this change.
-    defaultDownloadDir = path.join app.getPath('userData'), 'Downloads'
-    downloadFilePath = path.join defaultDownloadDir, "mock.pdf"
+    ipc = require 'ipc'
+    downloadFilePath = path.join fixtures, 'mock.pdf'
     downloadServer = http.createServer (req, res) ->
       res.writeHead 200, {
         'Content-Length': mockPDF.length,
@@ -96,29 +92,30 @@ describe 'session module', ->
     it 'can download successfully', (done) ->
       downloadServer.listen 0, '127.0.0.1', ->
         {port} = downloadServer.address()
+        ipc.sendSync 'set-download-option', false
         w.loadUrl "#{url}:#{port}"
-        w.webContents.session.setOpenDownloadDialog false
-
-        w.webContents.session.once 'will-download', (e, item, webContents) ->
-          item.on 'done', (e, state) ->
-            assert.equal state, "completed"
-            assert.equal item.getContentDisposition(), contentDisposition
-            assert.equal item.getReceiveBytes(), mockPDF.length
-            assert.equal item.getTotalBytes(), mockPDF.length
-            assert fs.existsSync downloadFilePath
-            fs.unlinkSync downloadFilePath
-            done()
-          assert.equal item.getURL(), "#{url}:#{port}/"
-          assert.equal item.getMimeType(), "application/pdf"
+        ipc.once 'download-done', (state, url, mimeType, receivedBytes,
+            totalBytes, disposition) ->
+          assert.equal state, 'completed'
+          assert.equal url, "http://127.0.0.1:#{port}/"
+          assert.equal mimeType, 'application/pdf'
+          assert.equal receivedBytes, mockPDF.length
+          assert.equal totalBytes, mockPDF.length
+          assert.equal disposition, contentDisposition
+          assert fs.existsSync downloadFilePath
+          fs.unlinkSync downloadFilePath
+          done()
 
     it 'can cancel download', (done) ->
       downloadServer.listen 0, '127.0.0.1', ->
         {port} = downloadServer.address()
+        ipc.sendSync 'set-download-option', true
         w.loadUrl "#{url}:#{port}/"
-        w.webContents.session.setOpenDownloadDialog false
-        w.webContents.session.once 'will-download', (e, item, webContents) ->
-          item.pause()
-          item.on 'done', (e, state) ->
-            assert.equal state, "cancelled"
-            done()
-          item.cancel()
+        ipc.once 'download-done', (state, url, mimeType, receivedBytes,
+            totalBytes, disposition) ->
+          assert.equal state, 'cancelled'
+          assert.equal mimeType, 'application/pdf'
+          assert.equal receivedBytes, 0
+          assert.equal totalBytes, mockPDF.length
+          assert.equal disposition, contentDisposition
+          done()

spec/static/main.js

@@ -1,6 +1,7 @@
 var app = require('app');
 var ipc = require('ipc');
 var dialog = require('dialog');
+var path = require('path');
 var BrowserWindow = require('browser-window');
 
 var window = null;
@@ -73,4 +74,26 @@ app.on('ready', function() {
     });
     if (chosen == 0) window.destroy();
   });
+
+  // For session's download test, listen 'will-download' event in browser, and
+  // reply the result to renderer for verifying
+  var downloadFilePath = path.join(__dirname, '..', 'fixtures', 'mock.pdf');
+  require('ipc').on('set-download-option', function(event, need_cancel) {
+    window.webContents.session.once('will-download',
+        function(e, item, webContents) {
+          item.setSavePath(downloadFilePath);
+          item.on('done', function(e, state) {
+            window.webContents.send('download-done',
+                                    state,
+                                    item.getUrl(),
+                                    item.getMimeType(),
+                                    item.getReceivedBytes(),
+                                    item.getTotalBytes(),
+                                    item.getContentDisposition());
+          });
+          if (need_cancel)
+            item.cancel();
+        });
+    event.returnValue = "done";
+  });
 });