Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2618743006:
Use previous target path when resuming a download after crash (Closed)
DescriptionUse previous target path when resuming a download after crash
When resuming a download after browser crash, Chrome will regenerate the target path.
This could reintroduce name conflict if user has resolved a conflict before the crash.
This CL reuses the previous target path Chrome has already generated for the download.
BUG=678835
Review-Url: https://codereview.chromium.org/2618743006
Cr-Commit-Position: refs/heads/master@{#446218}
Committed: https://chromium.googlesource.com/chromium/src/+/41231a3e20197bf9499332f02c69a426186f5fbe
Patch Set 1 #
Total comments: 2
Patch Set 2 : adding a helper function to get last used target path #
Total comments: 8
Patch Set 3 : remove GetLastTargetPath function #
Total comments: 4
Patch Set 4 : nits #Patch Set 5 : respect prompt reasons #Patch Set 6 : move the target path logic into if(!is_forced_path) statement #
Total comments: 2
Patch Set 7 : use virtual_path_ instead #Patch Set 8 : change conflict_action based on existing virtual path value #Patch Set 9 : add code for test #
Messages
Total messages: 47 (29 generated)
qinmin@chromium.org changed reviewers: + asanka@chromium.org, dtrainor@chromium.org
+asanka for OWNER and code review
https://codereview.chromium.org/2618743006/diff/1/chrome/browser/download/dow... File chrome/browser/download/download_target_determiner.cc (right): https://codereview.chromium.org/2618743006/diff/1/chrome/browser/download/dow... chrome/browser/download/download_target_determiner.cc:245: virtual_path_ = download_->GetForcedFilePath().empty() ? Since we're using this logic a lot, would it be cleaner to expose this as a method on DownloadItem?
https://codereview.chromium.org/2618743006/diff/1/chrome/browser/download/dow... File chrome/browser/download/download_target_determiner.cc (right): https://codereview.chromium.org/2618743006/diff/1/chrome/browser/download/dow... chrome/browser/download/download_target_determiner.cc:245: virtual_path_ = download_->GetForcedFilePath().empty() ? On 2017/01/06 23:26:41, David Trainor wrote: > Since we're using this logic a lot, would it be cleaner to expose this as a > method on DownloadItem? Done.
ping, asanka@, would you mind take a look? would like to merge this into M56.
https://codereview.chromium.org/2618743006/diff/20001/chrome/browser/download... File chrome/browser/download/download_target_determiner.cc (right): https://codereview.chromium.org/2618743006/diff/20001/chrome/browser/download... chrome/browser/download/download_target_determiner.cc:195: bool has_target_path = !download_->GetLastUsedTargetPath().empty(); Implement the GetForcedFilePath() || GetTargetFilePath() logic inline here. https://codereview.chromium.org/2618743006/diff/20001/content/browser/downloa... File content/browser/download/download_item_impl.cc (right): https://codereview.chromium.org/2618743006/diff/20001/content/browser/downloa... content/browser/download/download_item_impl.cc:630: base::FilePath DownloadItemImpl::GetLastUsedTargetPath() const { Is there a case where GetTargetFilePath() and GetForcedFilePath() differ? The only one I can think of is where the download gets destroyed or the devices crashes during the initial download target determination. IMHO, if things go south that close to the download initiation, it's much safer to discard the intermediate file and start from the beginning. Before the first download target determination, it's not accurate to refer to the value of GetForcedFilePath() as the last used target path. Let's get rid of this method if there is no other use case for it. https://codereview.chromium.org/2618743006/diff/20001/content/browser/downloa... File content/browser/download/download_item_impl_delegate.cc (right): https://codereview.chromium.org/2618743006/diff/20001/content/browser/downloa... content/browser/download/download_item_impl_delegate.cc:33: // target file path are null. Also note that this code path is unreachable in Chrome. It's just here since this is //content and can't rely on //chrome replacing this implementation. https://codereview.chromium.org/2618743006/diff/20001/content/public/browser/... File content/public/browser/download_item.h (right): https://codereview.chromium.org/2618743006/diff/20001/content/public/browser/... content/public/browser/download_item.h:287: virtual base::FilePath GetLastUsedTargetPath() const { = 0. But then again we shouldn't have this method at all.
PTAL again https://codereview.chromium.org/2618743006/diff/20001/chrome/browser/download... File chrome/browser/download/download_target_determiner.cc (right): https://codereview.chromium.org/2618743006/diff/20001/chrome/browser/download... chrome/browser/download/download_target_determiner.cc:195: bool has_target_path = !download_->GetLastUsedTargetPath().empty(); On 2017/01/10 19:34:19, asanka wrote: > Implement the GetForcedFilePath() || GetTargetFilePath() logic inline here. Done. https://codereview.chromium.org/2618743006/diff/20001/content/browser/downloa... File content/browser/download/download_item_impl.cc (right): https://codereview.chromium.org/2618743006/diff/20001/content/browser/downloa... content/browser/download/download_item_impl.cc:630: base::FilePath DownloadItemImpl::GetLastUsedTargetPath() const { On 2017/01/10 19:34:19, asanka wrote: > Is there a case where GetTargetFilePath() and GetForcedFilePath() differ? > > The only one I can think of is where the download gets destroyed or the devices > crashes during the initial download target determination. IMHO, if things go > south that close to the download initiation, it's much safer to discard the > intermediate file and start from the beginning. Before the first download target > determination, it's not accurate to refer to the value of GetForcedFilePath() as > the last used target path. > > Let's get rid of this method if there is no other use case for it. Done. https://codereview.chromium.org/2618743006/diff/20001/content/browser/downloa... File content/browser/download/download_item_impl_delegate.cc (right): https://codereview.chromium.org/2618743006/diff/20001/content/browser/downloa... content/browser/download/download_item_impl_delegate.cc:33: // target file path are null. On 2017/01/10 19:34:19, asanka wrote: > Also note that this code path is unreachable in Chrome. It's just here since > this is //content and can't rely on //chrome replacing this implementation. Done https://codereview.chromium.org/2618743006/diff/20001/content/public/browser/... File content/public/browser/download_item.h (right): https://codereview.chromium.org/2618743006/diff/20001/content/public/browser/... content/public/browser/download_item.h:287: virtual base::FilePath GetLastUsedTargetPath() const { On 2017/01/10 19:34:19, asanka wrote: > = 0. But then again we shouldn't have this method at all. Done.
lgtm https://codereview.chromium.org/2618743006/diff/40001/content/browser/downloa... File content/browser/download/download_item_impl_delegate.cc (right): https://codereview.chromium.org/2618743006/diff/40001/content/browser/downloa... content/browser/download/download_item_impl_delegate.cc:35: base::FilePath target_path = download->GetForcedFilePath().empty() ? The changes in this file are not necessary. The last-target-path logic is //chrome specific. https://codereview.chromium.org/2618743006/diff/40001/content/browser/downloa... File content/browser/download/download_manager_impl.cc (right): https://codereview.chromium.org/2618743006/diff/40001/content/browser/downloa... content/browser/download/download_manager_impl.cc:223: base::FilePath target_path = item->GetForcedFilePath().empty() ? Ditto. No need to change //content filename determination behavior. This is just here as a fallback in case the embedder doesn't provide an implementation.
https://codereview.chromium.org/2618743006/diff/40001/content/browser/downloa... File content/browser/download/download_item_impl_delegate.cc (right): https://codereview.chromium.org/2618743006/diff/40001/content/browser/downloa... content/browser/download/download_item_impl_delegate.cc:35: base::FilePath target_path = download->GetForcedFilePath().empty() ? On 2017/01/10 22:24:03, asanka wrote: > The changes in this file are not necessary. The last-target-path logic is > //chrome specific. ok, reverted the change https://codereview.chromium.org/2618743006/diff/40001/content/browser/downloa... File content/browser/download/download_manager_impl.cc (right): https://codereview.chromium.org/2618743006/diff/40001/content/browser/downloa... content/browser/download/download_manager_impl.cc:223: base::FilePath target_path = item->GetForcedFilePath().empty() ? On 2017/01/10 22:24:03, asanka wrote: > Ditto. No need to change //content filename determination behavior. This is just > here as a fallback in case the embedder doesn't provide an implementation. reverted
The CQ bit was checked by qinmin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from asanka@chromium.org Link to the patchset: https://codereview.chromium.org/2618743006/#ps60001 (title: "nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by qinmin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
The CQ bit was checked by qinmin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #6 (id:100001) has been deleted
The CQ bit was checked by qinmin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2618743006/diff/120001/chrome/browser/downloa... File chrome/browser/download/download_target_determiner.cc (right): https://codereview.chromium.org/2618743006/diff/120001/chrome/browser/downloa... chrome/browser/download/download_target_determiner.cc:198: if (!virtual_path_.empty() && HasPromptedForPath() && !is_forced_path) { Apologies. I didn't read this CL as carefully as I should have during the first round. virtual_path_ here should contain the current virtual path for a resumed download. I.e. for the cases where GetTargetFilePath() is non-empty, virtual_path_ would already contain the corresponding virtual path. [fyi: for all cases except Drive downloads on Chrome OS, virtual path is the same as the real path]. So, there should be no Chrome visible effect to the new code that's been added. The behavior changes at the DownloadTargetDeterminer level, but Chrome should never be invoking this with an empty virtual_path_ if there's a valid target path. Is that not what you are seeing? Does this CL fix the issue for you? In the latter case we'd need to dig in and find out what's really going on.
asanka@, please take another look. This change affects many unit tests as some tests incorrectly provided the initial virtual path, which makes them a resumption case. Additionally, the initial_virtual_path_ impacts the dangerous type and dangerous level determination. So some of the test expectations has to be corrected. There are lots of tests already covering the non-empty initial virtual path, so I removed the new unit test from previous patchset. https://codereview.chromium.org/2618743006/diff/120001/chrome/browser/downloa... File chrome/browser/download/download_target_determiner.cc (right): https://codereview.chromium.org/2618743006/diff/120001/chrome/browser/downloa... chrome/browser/download/download_target_determiner.cc:198: if (!virtual_path_.empty() && HasPromptedForPath() && !is_forced_path) { On 2017/01/12 18:58:35, asanka wrote: > Apologies. I didn't read this CL as carefully as I should have during the first > round. > > virtual_path_ here should contain the current virtual path for a resumed > download. I.e. for the cases where GetTargetFilePath() is non-empty, > virtual_path_ would already contain the corresponding virtual path. > > [fyi: for all cases except Drive downloads on Chrome OS, virtual path is the > same as the real path]. > > So, there should be no Chrome visible effect to the new code that's been added. > The behavior changes at the DownloadTargetDeterminer level, but Chrome should > never be invoking this with an empty virtual_path_ if there's a valid target > path. > > Is that not what you are seeing? Does this CL fix the issue for you? In the > latter case we'd need to dig in and find out what's really going on. Based on our offline discussions, a non empty virtual_path_ means the download is in resumption. So changed the CL to use virtual_path_ if it is provided. And HasPromptedForPath() should only determine whether we should change the conflict_action_
The CQ bit was checked by qinmin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
(I'll have a look at this later tonight).
Sorry about the delay. I'll lgtm this, but with a couple of caveats. * Another alternative is to allow target re-determination for the !HasPromptedBefore() case, and set the conflict action based on whether there was a virtual path. That will likely reduce the test churn and also avoid the case where a conflict with a uniquified path result in a name like "foo (1) (1).pdf" and instead produce "foo (2).pdf" * The tests are crafted such that the specific name generation heuristic is apparent from the result. This is why there's a lot of test churn. The above case (i.e. resulting in "foo (1) (1).pdf") is the reason why I made the tests verify which heuristic we used.
Thanks Asanka, the alternative looks much simpler. The patchset is now just a 2 line change for Android, PTAL again On 2017/01/19 20:05:53, asanka wrote: > Sorry about the delay. > > I'll lgtm this, but with a couple of caveats. > > * Another alternative is to allow target re-determination for the > !HasPromptedBefore() case, and set the conflict action based on whether there > was a virtual path. That will likely reduce the test churn and also avoid the > case where a conflict with a uniquified path result in a name like "foo (1) > (1).pdf" and instead produce "foo (2).pdf" > > * The tests are crafted such that the specific name generation heuristic is > apparent from the result. This is why there's a lot of test churn. The above > case (i.e. resulting in "foo (1) (1).pdf") is the reason why I made the tests > verify which heuristic we used.
The CQ bit was checked by qinmin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by qinmin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from asanka@chromium.org Link to the patchset: https://codereview.chromium.org/2618743006/#ps180001 (title: "add code for test")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 180001, "attempt_start_ts": 1485389694046220,
"parent_rev": "365b58f79079581b244a12f4bf5aaa6c0c54b305", "commit_rev":
"41231a3e20197bf9499332f02c69a426186f5fbe"}
Message was sent while issue was closed.
Description was changed from ========== Use previous target path when resuming a download after crash When resuming a download after browser crash, Chrome will regenerate the target path. This could reintroduce name conflict if user has resolved a conflict before the crash. This CL reuses the previous target path Chrome has already generated for the download. BUG=678835 ========== to ========== Use previous target path when resuming a download after crash When resuming a download after browser crash, Chrome will regenerate the target path. This could reintroduce name conflict if user has resolved a conflict before the crash. This CL reuses the previous target path Chrome has already generated for the download. BUG=678835 Review-Url: https://codereview.chromium.org/2618743006 Cr-Commit-Position: refs/heads/master@{#446218} Committed: https://chromium.googlesource.com/chromium/src/+/41231a3e20197bf9499332f02c69... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:180001) as https://chromium.googlesource.com/chromium/src/+/41231a3e20197bf9499332f02c69... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
