From f1f91828ba051c1548b7d67ac55fd3fac3a33231 Mon Sep 17 00:00:00 2001 From: Cheng Zhao Date: Tue, 2 Feb 2016 18:49:43 +0800 Subject: [PATCH 1/5] Rely on OnDownloadDestroyed to destroy downloadItem --- atom/browser/api/atom_api_download_item.cc | 16 +++------------- atom/browser/api/atom_api_download_item.h | 2 -- atom/browser/api/atom_api_session.cc | 4 +--- 3 files changed, 4 insertions(+), 18 deletions(-) diff --git a/atom/browser/api/atom_api_download_item.cc b/atom/browser/api/atom_api_download_item.cc index c3beee5bb094..bb1c58f49b8a 100644 --- a/atom/browser/api/atom_api_download_item.cc +++ b/atom/browser/api/atom_api_download_item.cc @@ -12,7 +12,6 @@ #include "atom/common/native_mate_converters/gurl_converter.h" #include "atom/common/node_includes.h" #include "base/memory/linked_ptr.h" -#include "base/message_loop/message_loop.h" #include "base/strings/utf_string_conversions.h" #include "native_mate/dictionary.h" #include "net/base/filename_util.h" @@ -71,17 +70,6 @@ DownloadItem::DownloadItem(content::DownloadItem* download_item) : } DownloadItem::~DownloadItem() { - Destroy(download_item_); -} - -void DownloadItem::Destroy(content::DownloadItem* item) { - if (item) { - OnDownloadDestroyed(item); - MarkDestroyed(); - - // Destroy the native class in next tick. - base::MessageLoop::current()->PostTask(FROM_HERE, GetDestroyClosure()); - } } void DownloadItem::OnDownloadUpdated(content::DownloadItem* item) { @@ -93,7 +81,9 @@ void DownloadItem::OnDownloadDestroyed(content::DownloadItem* download_item) { 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; + + // Destroy the wrapper once the download_item is destroyed. + delete this; } int64 DownloadItem::GetReceivedBytes() { diff --git a/atom/browser/api/atom_api_download_item.h b/atom/browser/api/atom_api_download_item.h index af388a567513..471913c2266a 100644 --- a/atom/browser/api/atom_api_download_item.h +++ b/atom/browser/api/atom_api_download_item.h @@ -36,8 +36,6 @@ class DownloadItem : public mate::TrackableObject, static void BuildPrototype(v8::Isolate* isolate, v8::Local prototype); - void Destroy(content::DownloadItem* download_item); - protected: explicit DownloadItem(content::DownloadItem* download_item); ~DownloadItem(); diff --git a/atom/browser/api/atom_api_session.cc b/atom/browser/api/atom_api_session.cc index 04ab7a9f510e..e5c5198f0341 100644 --- a/atom/browser/api/atom_api_session.cc +++ b/atom/browser/api/atom_api_session.cc @@ -306,14 +306,12 @@ void Session::OnDownloadCreated(content::DownloadManager* manager, auto web_contents = item->GetWebContents(); if (SavePageHandler::IsSavePageTypes(item->GetMimeType())) return; - auto download_item_handle = DownloadItem::Create(isolate(), item); bool prevent_default = Emit( "will-download", - download_item_handle, + DownloadItem::Create(isolate(), item), api::WebContents::CreateFrom(isolate(), web_contents)); if (prevent_default) { item->Cancel(true); - download_item_handle->Destroy(item); item->Remove(); } } From 65cf85808f071aaa3a91285100dc6f1f87a72770 Mon Sep 17 00:00:00 2001 From: Cheng Zhao Date: Tue, 2 Feb 2016 19:05:58 +0800 Subject: [PATCH 2/5] Calling cancel() should destroy the downloadItem --- atom/browser/api/atom_api_download_item.cc | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/atom/browser/api/atom_api_download_item.cc b/atom/browser/api/atom_api_download_item.cc index bb1c58f49b8a..b0bcd5ef1a1f 100644 --- a/atom/browser/api/atom_api_download_item.cc +++ b/atom/browser/api/atom_api_download_item.cc @@ -12,6 +12,7 @@ #include "atom/common/native_mate_converters/gurl_converter.h" #include "atom/common/node_includes.h" #include "base/memory/linked_ptr.h" +#include "base/message_loop/message_loop.h" #include "base/strings/utf_string_conversions.h" #include "native_mate/dictionary.h" #include "net/base/filename_util.h" @@ -64,12 +65,14 @@ const base::FilePath& DownloadItem::SavePathData::path() { return path_; } -DownloadItem::DownloadItem(content::DownloadItem* download_item) : - download_item_(download_item) { +DownloadItem::DownloadItem(content::DownloadItem* download_item) + : download_item_(download_item) { download_item_->AddObserver(this); + AttachAsUserData(download_item); } DownloadItem::~DownloadItem() { + LOG(ERROR) << "~DownloadItem"; } void DownloadItem::OnDownloadUpdated(content::DownloadItem* item) { @@ -82,8 +85,8 @@ void DownloadItem::OnDownloadDestroyed(content::DownloadItem* download_item) { if (iter != g_download_item_objects.end()) g_download_item_objects.erase(iter); - // Destroy the wrapper once the download_item is destroyed. - delete this; + // Destroy the native class in next tick. + base::MessageLoop::current()->PostTask(FROM_HERE, GetDestroyClosure()); } int64 DownloadItem::GetReceivedBytes() { @@ -132,7 +135,9 @@ void DownloadItem::Resume() { } void DownloadItem::Cancel() { + MarkDestroyed(); download_item_->Cancel(true); + download_item_->Remove(); } // static @@ -156,6 +161,10 @@ void DownloadItem::BuildPrototype(v8::Isolate* isolate, // static mate::Handle DownloadItem::Create( v8::Isolate* isolate, content::DownloadItem* item) { + auto existing = TrackableObject::FromWrappedClass(isolate, item); + if (existing) + return mate::CreateHandle(isolate, static_cast(existing)); + auto handle = mate::CreateHandle(isolate, new DownloadItem(item)); g_wrap_download_item.Run(handle.ToV8()); g_download_item_objects[item->GetId()] = make_linked_ptr( From 12d311fd29d3551b754fcbd2c023637baf5488f7 Mon Sep 17 00:00:00 2001 From: Cheng Zhao Date: Tue, 2 Feb 2016 19:51:15 +0800 Subject: [PATCH 3/5] Gracefully destroy downloadItem --- atom/browser/api/atom_api_download_item.cc | 27 ++++++++++++++-------- vendor/native_mate | 2 +- 2 files changed, 19 insertions(+), 10 deletions(-) diff --git a/atom/browser/api/atom_api_download_item.cc b/atom/browser/api/atom_api_download_item.cc index b0bcd5ef1a1f..d8e87f2cfeba 100644 --- a/atom/browser/api/atom_api_download_item.cc +++ b/atom/browser/api/atom_api_download_item.cc @@ -48,6 +48,7 @@ namespace atom { namespace api { namespace { + // The wrapDownloadItem funtion which is implemented in JavaScript using WrapDownloadItemCallback = base::Callback)>; WrapDownloadItemCallback g_wrap_download_item; @@ -55,6 +56,7 @@ WrapDownloadItemCallback g_wrap_download_item; char kDownloadItemSavePathKey[] = "DownloadItemSavePathKey"; std::map>> g_download_item_objects; + } // namespace DownloadItem::SavePathData::SavePathData(const base::FilePath& path) : @@ -73,6 +75,16 @@ DownloadItem::DownloadItem(content::DownloadItem* download_item) DownloadItem::~DownloadItem() { LOG(ERROR) << "~DownloadItem"; + if (download_item_) { + // Destroyed by either garbage collection or destroy(). + download_item_->RemoveObserver(this); + download_item_->Remove(); + } + + // Remove from the global map. + auto iter = g_download_item_objects.find(weak_map_id()); + if (iter != g_download_item_objects.end()) + g_download_item_objects.erase(iter); } void DownloadItem::OnDownloadUpdated(content::DownloadItem* item) { @@ -80,13 +92,9 @@ void DownloadItem::OnDownloadUpdated(content::DownloadItem* item) { } void DownloadItem::OnDownloadDestroyed(content::DownloadItem* 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); - - // Destroy the native class in next tick. - base::MessageLoop::current()->PostTask(FROM_HERE, GetDestroyClosure()); + download_item_ = nullptr; + // Destroy the native class immediately when downloadItem is destroyed. + delete this; } int64 DownloadItem::GetReceivedBytes() { @@ -135,7 +143,6 @@ void DownloadItem::Resume() { } void DownloadItem::Cancel() { - MarkDestroyed(); download_item_->Cancel(true); download_item_->Remove(); } @@ -167,7 +174,9 @@ mate::Handle DownloadItem::Create( auto handle = mate::CreateHandle(isolate, new DownloadItem(item)); g_wrap_download_item.Run(handle.ToV8()); - g_download_item_objects[item->GetId()] = make_linked_ptr( + + // Reference this object in case it got garbage collected. + g_download_item_objects[handle->weak_map_id()] = make_linked_ptr( new v8::Global(isolate, handle.ToV8())); return handle; } diff --git a/vendor/native_mate b/vendor/native_mate index 26520c5cf4b6..e719eab878c2 160000 --- a/vendor/native_mate +++ b/vendor/native_mate @@ -1 +1 @@ -Subproject commit 26520c5cf4b6a60da2c5cba971393f94b82f5939 +Subproject commit e719eab878c264bb03188d0cd6eb9ad6882bc13a From eaa60e9c171c536e68a20e892dc5d6cee6364609 Mon Sep 17 00:00:00 2001 From: Cheng Zhao Date: Tue, 2 Feb 2016 19:54:41 +0800 Subject: [PATCH 4/5] Destroy the item once item is downloaded --- atom/browser/api/atom_api_download_item.cc | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/atom/browser/api/atom_api_download_item.cc b/atom/browser/api/atom_api_download_item.cc index d8e87f2cfeba..da95b67af1fd 100644 --- a/atom/browser/api/atom_api_download_item.cc +++ b/atom/browser/api/atom_api_download_item.cc @@ -88,7 +88,14 @@ DownloadItem::~DownloadItem() { } void DownloadItem::OnDownloadUpdated(content::DownloadItem* item) { - download_item_->IsDone() ? Emit("done", item->GetState()) : Emit("updated"); + if (download_item_->IsDone()) { + Emit("done", item->GetState()); + + // Destroy the item once item is downloaded. + base::MessageLoop::current()->PostTask(FROM_HERE, GetDestroyClosure()); + } else { + Emit("updated"); + } } void DownloadItem::OnDownloadDestroyed(content::DownloadItem* download_item) { From 50d69fd9bbacf33e69bb489d9d16ee936c06f9f9 Mon Sep 17 00:00:00 2001 From: Cheng Zhao Date: Tue, 2 Feb 2016 20:11:39 +0800 Subject: [PATCH 5/5] Get rid of SavePathData --- atom/browser/api/atom_api_download_item.cc | 99 +++++++++---------- atom/browser/api/atom_api_download_item.h | 35 +++---- .../browser/atom_download_manager_delegate.cc | 28 ++++-- 3 files changed, 76 insertions(+), 86 deletions(-) diff --git a/atom/browser/api/atom_api_download_item.cc b/atom/browser/api/atom_api_download_item.cc index da95b67af1fd..5a8befc9d1db 100644 --- a/atom/browser/api/atom_api_download_item.cc +++ b/atom/browser/api/atom_api_download_item.cc @@ -53,20 +53,10 @@ namespace { 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); @@ -74,7 +64,6 @@ DownloadItem::DownloadItem(content::DownloadItem* download_item) } DownloadItem::~DownloadItem() { - LOG(ERROR) << "~DownloadItem"; if (download_item_) { // Destroyed by either garbage collection or destroy(). download_item_->RemoveObserver(this); @@ -104,43 +93,6 @@ void DownloadItem::OnDownloadDestroyed(content::DownloadItem* download_item) { delete this; } -int64 DownloadItem::GetReceivedBytes() { - return download_item_->GetReceivedBytes(); -} - -int64 DownloadItem::GetTotalBytes() { - return download_item_->GetTotalBytes(); -} - -const GURL& DownloadItem::GetURL() { - return download_item_->GetURL(); -} - -std::string DownloadItem::GetMimeType() { - return download_item_->GetMimeType(); -} - -bool DownloadItem::HasUserGesture() { - return download_item_->HasUserGesture(); -} - -std::string DownloadItem::GetFilename() { - return base::UTF16ToUTF8(net::GenerateFileName(GetURL(), - GetContentDisposition(), - std::string(), - download_item_->GetSuggestedFilename(), - GetMimeType(), - std::string()).LossyDisplayName()); -} - -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(); } @@ -154,6 +106,47 @@ void DownloadItem::Cancel() { download_item_->Remove(); } +int64 DownloadItem::GetReceivedBytes() const { + return download_item_->GetReceivedBytes(); +} + +int64 DownloadItem::GetTotalBytes() const { + return download_item_->GetTotalBytes(); +} + +std::string DownloadItem::GetMimeType() const { + return download_item_->GetMimeType(); +} + +bool DownloadItem::HasUserGesture() const { + return download_item_->HasUserGesture(); +} + +std::string DownloadItem::GetFilename() const { + return base::UTF16ToUTF8(net::GenerateFileName(GetURL(), + GetContentDisposition(), + std::string(), + download_item_->GetSuggestedFilename(), + GetMimeType(), + std::string()).LossyDisplayName()); +} + +std::string DownloadItem::GetContentDisposition() const { + return download_item_->GetContentDisposition(); +} + +const GURL& DownloadItem::GetURL() const { + return download_item_->GetURL(); +} + +void DownloadItem::SetSavePath(const base::FilePath& path) { + save_path_ = path; +} + +base::FilePath DownloadItem::GetSavePath() const { + return save_path_; +} + // static void DownloadItem::BuildPrototype(v8::Isolate* isolate, v8::Local prototype) { @@ -164,12 +157,13 @@ void DownloadItem::BuildPrototype(v8::Isolate* isolate, .SetMethod("cancel", &DownloadItem::Cancel) .SetMethod("getReceivedBytes", &DownloadItem::GetReceivedBytes) .SetMethod("getTotalBytes", &DownloadItem::GetTotalBytes) - .SetMethod("getURL", &DownloadItem::GetURL) .SetMethod("getMimeType", &DownloadItem::GetMimeType) .SetMethod("hasUserGesture", &DownloadItem::HasUserGesture) .SetMethod("getFilename", &DownloadItem::GetFilename) .SetMethod("getContentDisposition", &DownloadItem::GetContentDisposition) - .SetMethod("setSavePath", &DownloadItem::SetSavePath); + .SetMethod("getURL", &DownloadItem::GetURL) + .SetMethod("setSavePath", &DownloadItem::SetSavePath) + .SetMethod("getSavePath", &DownloadItem::GetSavePath); } // static @@ -188,11 +182,6 @@ mate::Handle DownloadItem::Create( return handle; } -// static -void* DownloadItem::UserDataKey() { - return &kDownloadItemSavePathKey; -} - void ClearWrapDownloadItem() { g_wrap_download_item.Reset(); } diff --git a/atom/browser/api/atom_api_download_item.h b/atom/browser/api/atom_api_download_item.h index 471913c2266a..5806c0181768 100644 --- a/atom/browser/api/atom_api_download_item.h +++ b/atom/browser/api/atom_api_download_item.h @@ -20,22 +20,26 @@ namespace api { class DownloadItem : public mate::TrackableObject, 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_; - }; - static mate::Handle Create(v8::Isolate* isolate, content::DownloadItem* item); - static void* UserDataKey(); // mate::TrackableObject: static void BuildPrototype(v8::Isolate* isolate, v8::Local prototype); + void Pause(); + void Resume(); + void Cancel(); + int64 GetReceivedBytes() const; + int64 GetTotalBytes() const; + std::string GetMimeType() const; + bool HasUserGesture() const; + std::string GetFilename() const; + std::string GetContentDisposition() const; + const GURL& GetURL() const; + void SetSavePath(const base::FilePath& path); + base::FilePath GetSavePath() const; + protected: explicit DownloadItem(content::DownloadItem* download_item); ~DownloadItem(); @@ -44,19 +48,8 @@ class DownloadItem : public mate::TrackableObject, void OnDownloadUpdated(content::DownloadItem* download) override; void OnDownloadDestroyed(content::DownloadItem* download) override; - void Pause(); - void Resume(); - void Cancel(); - int64 GetReceivedBytes(); - int64 GetTotalBytes(); - std::string GetMimeType(); - bool HasUserGesture(); - std::string GetFilename(); - std::string GetContentDisposition(); - const GURL& GetURL(); - void SetSavePath(const base::FilePath& path); - private: + base::FilePath save_path_; content::DownloadItem* download_item_; DISALLOW_COPY_AND_ASSIGN(DownloadItem); diff --git a/atom/browser/atom_download_manager_delegate.cc b/atom/browser/atom_download_manager_delegate.cc index a5f5cc6d8e4d..f5bdbbd8598d 100644 --- a/atom/browser/atom_download_manager_delegate.cc +++ b/atom/browser/atom_download_manager_delegate.cc @@ -109,16 +109,24 @@ bool AtomDownloadManagerDelegate::DetermineDownloadTarget( 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; + + // 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; + } + } } AtomBrowserContext* browser_context = static_cast(