From 0861d5d44bb0e2692535aa63cb46eed303524ac4 Mon Sep 17 00:00:00 2001 From: Haojian Wu Date: Thu, 24 Sep 2015 15:55:45 +0800 Subject: [PATCH] Redefine 'will-download' design. --- atom/browser/api/atom_api_download_item.cc | 62 +++++++++++++++++++------- atom/browser/api/atom_api_download_item.h | 18 ++++++-- atom/browser/api/atom_api_session.cc | 9 ---- atom/browser/api/atom_api_session.h | 2 - atom/browser/api/lib/app.coffee | 2 +- atom/browser/atom_download_manager_delegate.cc | 56 +++++++++++------------ atom/browser/atom_download_manager_delegate.h | 1 - docs/api/download-item.md | 17 ++++--- docs/api/session.md | 7 --- spec/api-session-spec.coffee | 53 +++++++++++----------- spec/static/main.js | 23 ++++++++++ 11 files changed, 149 insertions(+), 101 deletions(-) diff --git a/atom/browser/api/atom_api_download_item.cc b/atom/browser/api/atom_api_download_item.cc index 6c6bb4d..3d15046 100644 --- a/atom/browser/api/atom_api_download_item.cc +++ b/atom/browser/api/atom_api_download_item.cc @@ -4,9 +4,13 @@ #include "atom/browser/api/atom_api_download_item.h" +#include + #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)>; WrapDownloadItemCallback g_wrap_download_item; + +char kDownloadItemSavePathKey[] = "DownloadItemSavePathKey"; + +std::map>> 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::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(isolate, handle.ToV8())); return handle; } +// static +void* DownloadItem::UserDataKey() { + return &kDownloadItemSavePathKey; +} + } // namespace api } // namespace atom diff --git a/atom/browser/api/atom_api_download_item.h b/atom/browser/api/atom_api_download_item.h index e12064c..3e90ced 100644 --- a/atom/browser/api/atom_api_download_item.h +++ b/atom/browser/api/atom_api_download_item.h @@ -8,6 +8,7 @@ #include #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, +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 Create(v8::Isolate* isolate, @@ -37,16 +46,17 @@ class DownloadItem : public mate::TrackableObject, 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_; diff --git a/atom/browser/api/atom_api_session.cc b/atom/browser/api/atom_api_session.cc index 27e52e1..7d5f75c 100644 --- a/atom/browser/api/atom_api_session.cc +++ b/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( - browser_context()->GetDownloadManagerDelegate()); - delegate->SetOpenDownloadDialog(open_download_dialog); -} - v8::Local 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); } diff --git a/atom/browser/api/atom_api_session.h b/atom/browser/api/atom_api_session.h index 2c53cfd..14406e5 100644 --- a/atom/browser/api/atom_api_session.h +++ b/atom/browser/api/atom_api_session.h @@ -43,8 +43,6 @@ class Session: public mate::TrackableObject, AtomBrowserContext* browser_context() const { return browser_context_.get(); } - void SetOpenDownloadDialog(bool open_download_dialog); - protected: explicit Session(AtomBrowserContext* browser_context); ~Session(); diff --git a/atom/browser/api/lib/app.coffee b/atom/browser/api/lib/app.coffee index 3bf85c5..48ddd17 100644 --- a/atom/browser/api/lib/app.coffee +++ b/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() diff --git a/atom/browser/atom_download_manager_delegate.cc b/atom/browser/atom_download_manager_delegate.cc index 88daec2..b6b6566 100644 --- a/atom/browser/atom_download_manager_delegate.cc +++ b/atom/browser/atom_download_manager_delegate.cc @@ -6,6 +6,7 @@ #include +#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( - 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( + 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(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( 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(), diff --git a/atom/browser/atom_download_manager_delegate.h b/atom/browser/atom_download_manager_delegate.h index d518e16..d602cb9 100644 --- a/atom/browser/atom_download_manager_delegate.h +++ b/atom/browser/atom_download_manager_delegate.h @@ -49,7 +49,6 @@ class AtomDownloadManagerDelegate : public content::DownloadManagerDelegate { private: content::DownloadManager* download_manager_; base::WeakPtrFactory weak_ptr_factory_; - bool open_download_dialog_; DISALLOW_COPY_AND_ASSIGN(AtomDownloadManagerDelegate); }; diff --git a/docs/api/download-item.md b/docs/api/download-item.md index 7b496db..d18433c 100644 --- a/docs/api/download-item.md +++ b/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. diff --git a/docs/api/session.md b/docs/api/session.md index 2219cf3..ae0aacf 100644 --- a/docs/api/session.md +++ b/docs/api/session.md @@ -191,10 +191,3 @@ proxy-uri = ["://"][":"] 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. diff --git a/spec/api-session-spec.coffee b/spec/api-session-spec.coffee index fe5765b..fb036bc 100644 --- a/spec/api-session-spec.coffee +++ b/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() diff --git a/spec/static/main.js b/spec/static/main.js index 38ba7cc..6f84c3f 100644 --- a/spec/static/main.js +++ b/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"; + }); }); -- 2.7.4