Merge pull request #4325 from atom/download-item-cleanup

Cleanup DownloadItem related code

atom/browser/api/atom_api_download_item.cc

@@ -48,89 +48,49 @@ namespace atom {
 namespace api {
 
 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) :
+DownloadItem::DownloadItem(content::DownloadItem* download_item)
-  path_(path) {
+    : download_item_(download_item) {
-}
-
-const base::FilePath& DownloadItem::SavePathData::path() {
-  return path_;
-}
-
-DownloadItem::DownloadItem(content::DownloadItem* download_item) :
-    download_item_(download_item) {
   download_item_->AddObserver(this);
+  AttachAsUserData(download_item);
 }
 
 DownloadItem::~DownloadItem() {
-  Destroy(download_item_);
+  if (download_item_) {
-}
+    // Destroyed by either garbage collection or destroy().
-
+    download_item_->RemoveObserver(this);
-void DownloadItem::Destroy(content::DownloadItem* item) {
+    download_item_->Remove();
-  if (item) {
-    OnDownloadDestroyed(item);
-    MarkDestroyed();
-
-    // Destroy the native class in next tick.
-    base::MessageLoop::current()->PostTask(FROM_HERE, GetDestroyClosure());
   }
+
+  // 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) {
-  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) {
-  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;
-}
+  // Destroy the native class immediately when downloadItem is destroyed.
-
+  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() {
@@ -143,6 +103,48 @@ void DownloadItem::Resume() {
 
 void DownloadItem::Cancel() {
   download_item_->Cancel(true);
+  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
@@ -155,29 +157,31 @@ 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
 mate::Handle<DownloadItem> DownloadItem::Create(
     v8::Isolate* isolate, content::DownloadItem* item) {
+  auto existing = TrackableObject::FromWrappedClass(isolate, item);
+  if (existing)
+    return mate::CreateHandle(isolate, static_cast<DownloadItem*>(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(
+
+  // Reference this object in case it got garbage collected.
+  g_download_item_objects[handle->weak_map_id()] = make_linked_ptr(
       new v8::Global<v8::Value>(isolate, handle.ToV8()));
   return handle;
 }
 
-// static
-void* DownloadItem::UserDataKey() {
-  return &kDownloadItemSavePathKey;
-}
-
 void ClearWrapDownloadItem() {
   g_wrap_download_item.Reset();
 }

atom/browser/api/atom_api_download_item.h

@@ -20,23 +20,25 @@ namespace api {
 class DownloadItem : public mate::TrackableObject<DownloadItem>,
                      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<DownloadItem> Create(v8::Isolate* isolate,
                                            content::DownloadItem* item);
-  static void* UserDataKey();
 
   // mate::TrackableObject:
   static void BuildPrototype(v8::Isolate* isolate,
                              v8::Local<v8::ObjectTemplate> prototype);
 
-  void Destroy(content::DownloadItem* download_item);
+  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);
@@ -46,19 +48,8 @@ class DownloadItem : public mate::TrackableObject<DownloadItem>,
   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);

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();
   }
 }

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());
+  // Try to get the save path from JS wrapper.
-  if (save_path) {
+  {
-    const base::FilePath& default_download_path =
+    v8::Isolate* isolate = v8::Isolate::GetCurrent();
-        static_cast<api::DownloadItem::SavePathData*>(save_path)->path();
+    v8::Locker locker(isolate);
-    callback.Run(default_download_path,
+    v8::HandleScope handle_scope(isolate);
-                 content::DownloadItem::TARGET_DISPOSITION_OVERWRITE,
+    api::DownloadItem* download_item = api::DownloadItem::FromWrappedClass(
-                 content::DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS,
+        isolate, download);
-                 default_download_path);
+    if (download_item) {
-    return true;
+      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<AtomBrowserContext*>(

vendor/native_mate

@@ -1 +1 @@
-Subproject commit 26520c5cf4b6a60da2c5cba971393f94b82f5939
+Subproject commit e719eab878c264bb03188d0cd6eb9ad6882bc13a