Since DownloadJob class now handles the download, we should move most of the download logic there and keep DownloadItem simple. This will move the download file ownership to DownloadJob, and let download job handle file related error cases.
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/5555a916120874ae028efea267e9ef7077f1e0df commit 5555a916120874ae028efea267e9ef7077f1e0df Author: qinmin <qinmin@chromium.org> Date: Fri May 26 23:15:45 2017 Don't create parallel request until download file is initialized Chrome should wait until download file is initialized before creating parallel requests. With this CL, it also makes more sense for DownloadJob to own download file. I will do that in follow up CLs. BUG=726487 Review-Url: https://codereview.chromium.org/2901383007 Cr-Commit-Position: refs/heads/master@{#475176} [modify] https://crrev.com/5555a916120874ae028efea267e9ef7077f1e0df/content/browser/download/download_item_impl.cc [modify] https://crrev.com/5555a916120874ae028efea267e9ef7077f1e0df/content/browser/download/download_item_impl.h [modify] https://crrev.com/5555a916120874ae028efea267e9ef7077f1e0df/content/browser/download/download_job.cc [modify] https://crrev.com/5555a916120874ae028efea267e9ef7077f1e0df/content/browser/download/download_job.h [modify] https://crrev.com/5555a916120874ae028efea267e9ef7077f1e0df/content/browser/download/download_job_impl.cc [modify] https://crrev.com/5555a916120874ae028efea267e9ef7077f1e0df/content/browser/download/download_job_impl.h [modify] https://crrev.com/5555a916120874ae028efea267e9ef7077f1e0df/content/browser/download/mock_download_job.h [modify] https://crrev.com/5555a916120874ae028efea267e9ef7077f1e0df/content/browser/download/parallel_download_job.cc [modify] https://crrev.com/5555a916120874ae028efea267e9ef7077f1e0df/content/browser/download/parallel_download_job.h [modify] https://crrev.com/5555a916120874ae028efea267e9ef7077f1e0df/content/browser/download/parallel_download_job_unittest.cc
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/1f10526ada0aa15edb8dbbc0668a3be0b82b4948 commit 1f10526ada0aa15edb8dbbc0668a3be0b82b4948 Author: Min Qin <qinmin@chromium.org> Date: Mon Jun 05 20:06:07 2017 Introduce RequestInfo struct for DownloadItemImpl Currently DownloadItemImpl handles both SavePackage and regular downloads. These 2 types of downloads have different behaviors and should be handled by their own DownloadJob subclasses. And DownloadItemImpl should serve as a simple class to represent all types of downloads. This CL introduces a RequestInfo, a struct that contains fields used by both types of downloads. Both SavePackage and url download can create an DownloadItemImpl object by supplying a RequestInfo object. And they can have their own DownloadJob implementation. Most DownloadItemImpl code will be splitted into UrlDownloadJob and SavePackageDownloadJob. This will happen in follow up CLs. BUG=726487 Change-Id: I29a863903c536ccd9ad756afa78f93f8e8a2cc98 Reviewed-on: https://chromium-review.googlesource.com/520685 Reviewed-by: David Trainor <dtrainor@chromium.org> Commit-Queue: Min Qin <qinmin@chromium.org> Cr-Commit-Position: refs/heads/master@{#477061} [modify] https://crrev.com/1f10526ada0aa15edb8dbbc0668a3be0b82b4948/content/browser/download/download_item_impl.cc [modify] https://crrev.com/1f10526ada0aa15edb8dbbc0668a3be0b82b4948/content/browser/download/download_item_impl.h
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/32ac7ce1ac427406623e836e6fe886806d6a27ac commit 32ac7ce1ac427406623e836e6fe886806d6a27ac Author: Min Qin <qinmin@chromium.org> Date: Tue Jun 06 22:51:21 2017 Remove unreachable save package code OnDownloadCompleting() is only called by MaybeCompleteDownload() but MaybeCompleteDownload() is never reachable for save package download. SavePackage download has its own finish() call. BUG=726487 Change-Id: I984dbcb7d6e19fed82f5a258235206a76242a5cf Reviewed-on: https://chromium-review.googlesource.com/523672 Reviewed-by: Xing Liu <xingliu@chromium.org> Commit-Queue: Min Qin <qinmin@chromium.org> Cr-Commit-Position: refs/heads/master@{#477448} [modify] https://crrev.com/32ac7ce1ac427406623e836e6fe886806d6a27ac/content/browser/download/download_item_impl.cc
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/b4e374a794b807d5be18ffe95e1a087e9cbdecae commit b4e374a794b807d5be18ffe95e1a087e9cbdecae Author: Min Qin <qinmin@chromium.org> Date: Wed Jun 07 23:33:35 2017 Introduce DestinationInfo for DownloadItemImpl In the future, DownloadJobImpl will be responsible for getting the destination information. This CL introduces the DestinationInfo struct so that information related to the destination can be passed between DownloadJob and DownloadItemImpl. BUG=726487 Change-Id: I92aa6a3ef8b712d9db576002651435a5a80bca7f Reviewed-on: https://chromium-review.googlesource.com/526309 Commit-Queue: Min Qin <qinmin@chromium.org> Reviewed-by: David Trainor <dtrainor@chromium.org> Cr-Commit-Position: refs/heads/master@{#477793} [modify] https://crrev.com/b4e374a794b807d5be18ffe95e1a087e9cbdecae/content/browser/download/download_item_impl.cc [modify] https://crrev.com/b4e374a794b807d5be18ffe95e1a087e9cbdecae/content/browser/download/download_item_impl.h
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/5b912dc480674fc5778fe34bf30b435f03ef25a4 commit 5b912dc480674fc5778fe34bf30b435f03ef25a4 Author: Min Qin <qinmin@chromium.org> Date: Thu Jun 08 00:21:54 2017 Use DownloadJobImpl to cancel SavePackage download Currently SavePackage registers as a DownloadItemObserver. When DownloadItem is removed, it will cancel the download. And it also checks the DownloadItem's status to see if it should abort. This change allows DownloadItem to cancel SavePackage download directly. So SavePackage doesn't need to check DownloadItem status. BUG=726487 Change-Id: I593f5638709dfabef9d3e5cf5a27ddc7a4144508 Reviewed-on: https://chromium-review.googlesource.com/526294 Commit-Queue: Min Qin <qinmin@chromium.org> Reviewed-by: David Trainor <dtrainor@chromium.org> Cr-Commit-Position: refs/heads/master@{#477824} [modify] https://crrev.com/5b912dc480674fc5778fe34bf30b435f03ef25a4/content/browser/download/download_item_impl_unittest.cc [modify] https://crrev.com/5b912dc480674fc5778fe34bf30b435f03ef25a4/content/browser/download/download_job_impl.cc [modify] https://crrev.com/5b912dc480674fc5778fe34bf30b435f03ef25a4/content/browser/download/download_manager_impl.cc [modify] https://crrev.com/5b912dc480674fc5778fe34bf30b435f03ef25a4/content/browser/download/download_request_handle.cc [modify] https://crrev.com/5b912dc480674fc5778fe34bf30b435f03ef25a4/content/browser/download/download_request_handle.h [modify] https://crrev.com/5b912dc480674fc5778fe34bf30b435f03ef25a4/content/browser/download/download_worker.cc [modify] https://crrev.com/5b912dc480674fc5778fe34bf30b435f03ef25a4/content/browser/download/download_worker.h [modify] https://crrev.com/5b912dc480674fc5778fe34bf30b435f03ef25a4/content/browser/download/parallel_download_job.cc [modify] https://crrev.com/5b912dc480674fc5778fe34bf30b435f03ef25a4/content/browser/download/parallel_download_job_unittest.cc [modify] https://crrev.com/5b912dc480674fc5778fe34bf30b435f03ef25a4/content/browser/download/save_package.cc [modify] https://crrev.com/5b912dc480674fc5778fe34bf30b435f03ef25a4/content/browser/download/save_package.h [modify] https://crrev.com/5b912dc480674fc5778fe34bf30b435f03ef25a4/content/browser/download/url_downloader.cc
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/5b8d6939094b8aa2f96be68ebf5ebc253869376d commit 5b8d6939094b8aa2f96be68ebf5ebc253869376d Author: Min Qin <qinmin@chromium.org> Date: Mon Jun 12 19:55:26 2017 Add SavePackageDownloadJob class Currently SavePackage uses DownloadJobImpl as its DownloadJob implementation. However, DownloadJobImpl will be refactored to handle all regular downloads. So SavePackage should have its own DownloadJob implementation. BUG=726487 Change-Id: I18b842982c9fe89226868f822030463128cbc4b0 Reviewed-on: https://chromium-review.googlesource.com/528497 Reviewed-by: David Trainor <dtrainor@chromium.org> Reviewed-by: Bo Liu <boliu@chromium.org> Commit-Queue: Min Qin <qinmin@chromium.org> Cr-Commit-Position: refs/heads/master@{#478728} [modify] https://crrev.com/5b8d6939094b8aa2f96be68ebf5ebc253869376d/content/browser/BUILD.gn [modify] https://crrev.com/5b8d6939094b8aa2f96be68ebf5ebc253869376d/content/browser/download/download_item_impl.cc [modify] https://crrev.com/5b8d6939094b8aa2f96be68ebf5ebc253869376d/content/browser/download/download_item_impl.h [modify] https://crrev.com/5b8d6939094b8aa2f96be68ebf5ebc253869376d/content/browser/download/download_job.cc [modify] https://crrev.com/5b8d6939094b8aa2f96be68ebf5ebc253869376d/content/browser/download/download_job.h [modify] https://crrev.com/5b8d6939094b8aa2f96be68ebf5ebc253869376d/content/browser/download/download_job_factory.cc [modify] https://crrev.com/5b8d6939094b8aa2f96be68ebf5ebc253869376d/content/browser/download/download_job_factory.h [modify] https://crrev.com/5b8d6939094b8aa2f96be68ebf5ebc253869376d/content/browser/download/download_job_impl.cc [modify] https://crrev.com/5b8d6939094b8aa2f96be68ebf5ebc253869376d/content/browser/download/download_job_impl.h [modify] https://crrev.com/5b8d6939094b8aa2f96be68ebf5ebc253869376d/content/browser/download/mock_download_job.cc [add] https://crrev.com/5b8d6939094b8aa2f96be68ebf5ebc253869376d/content/browser/download/save_package_download_job.cc [add] https://crrev.com/5b8d6939094b8aa2f96be68ebf5ebc253869376d/content/browser/download/save_package_download_job.h
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/655aa58109202ef80771c61caa7dcfbefcc91192 commit 655aa58109202ef80771c61caa7dcfbefcc91192 Author: Min Qin <qinmin@chromium.org> Date: Fri Jul 14 01:25:24 2017 remove DownloadManager::OnSavePackageSuccessfullyFinished() This method is only used for test. And caller should inherit DownloadItem::Observer to monitor a download item, Rahther than listening to the DownloadManager to get download item state. So the functionality provided by this method is already covered by other calls. BUG=726487 Change-Id: I7f9f5fd223093faf5643eb3f8d5f0255b7ca8c15 Reviewed-on: https://chromium-review.googlesource.com/568906 Reviewed-by: Jochen Eisinger <jochen@chromium.org> Reviewed-by: David Trainor <dtrainor@chromium.org> Commit-Queue: Min Qin <qinmin@chromium.org> Cr-Commit-Position: refs/heads/master@{#486601} [modify] https://crrev.com/655aa58109202ef80771c61caa7dcfbefcc91192/chrome/browser/browser_encoding_browsertest.cc [modify] https://crrev.com/655aa58109202ef80771c61caa7dcfbefcc91192/chrome/browser/download/save_page_browsertest.cc [modify] https://crrev.com/655aa58109202ef80771c61caa7dcfbefcc91192/content/browser/download/download_item_impl.cc [modify] https://crrev.com/655aa58109202ef80771c61caa7dcfbefcc91192/content/browser/download/download_item_impl.h [modify] https://crrev.com/655aa58109202ef80771c61caa7dcfbefcc91192/content/browser/download/download_manager_impl.cc [modify] https://crrev.com/655aa58109202ef80771c61caa7dcfbefcc91192/content/browser/download/download_manager_impl.h [modify] https://crrev.com/655aa58109202ef80771c61caa7dcfbefcc91192/content/browser/download/save_package.cc [modify] https://crrev.com/655aa58109202ef80771c61caa7dcfbefcc91192/content/browser/download/save_package.h [modify] https://crrev.com/655aa58109202ef80771c61caa7dcfbefcc91192/content/browser/download/save_package_browsertest.cc [modify] https://crrev.com/655aa58109202ef80771c61caa7dcfbefcc91192/content/public/browser/download_manager.h [modify] https://crrev.com/655aa58109202ef80771c61caa7dcfbefcc91192/content/public/test/download_test_observer.cc [modify] https://crrev.com/655aa58109202ef80771c61caa7dcfbefcc91192/content/public/test/download_test_observer.h
Comment 1 by bugdroid1@chromium.org
, May 26 2017