Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
183743019:
delete file if resume mode is invalid. DCHECK fails in DownloadItemImpl::OnDownloadRenamedToFinalNa… (Closed)
|
Created:
6 years, 9 months ago by baranovich Modified:
6 years, 9 months ago Reviewers:
asanka CC:
chromium-reviews, benjhayden+dwatch_chromium.org, darin-cc_chromium.org, jam, joi+watch-content_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
Descriptiondelete file when doing Interrupt() if resume mode is invalid. The
partial file, if exists, can no longer be used to complete the download.
R=asanka@chromium.org
BUG=none
TEST=DownloadItemTest.UnresumableInterrupt
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=255506
Patch Set 1 #
Total comments: 1
Patch Set 2 : fix codereview remarks #Patch Set 3 : more readable comment #Patch Set 4 : unit tests for RESUME_MODE_INVALID interrupt #
Messages
Total messages: 21 (0 generated)
Hi all, not sure whether this should be fixed or DCHECK condition should be modified. If my patch is good, I'll add some tests. For now please just take a look.
https://codereview.chromium.org/183743019/diff/1/content/browser/download/dow... File content/browser/download/download_item_impl.cc (right): https://codereview.chromium.org/183743019/diff/1/content/browser/download/dow... content/browser/download/download_item_impl.cc:1451: (resume_mode == RESUME_MODE_INVALID && You can just check for resume_mode == RESUME_MODE_INVALID. The last_reason_ had better be something other than DOWNLOAD_INTERRUPT_REASON_NONE if Interrupt() was called :). Also update the comment above.
On 2014/03/04 21:17:49, asanka wrote: > https://codereview.chromium.org/183743019/diff/1/content/browser/download/dow... > File content/browser/download/download_item_impl.cc (right): > > https://codereview.chromium.org/183743019/diff/1/content/browser/download/dow... > content/browser/download/download_item_impl.cc:1451: (resume_mode == > RESUME_MODE_INVALID && > You can just check for resume_mode == RESUME_MODE_INVALID. The last_reason_ had > better be something other than DOWNLOAD_INTERRUPT_REASON_NONE if Interrupt() was > called :). Also update the comment above. Fixed, although there are no any restrictions on the |reason| value in Interrupt() :) Maybe DCHECK about that should be added.
On 2014/03/05 10:08:07, baranovich wrote: > On 2014/03/04 21:17:49, asanka wrote: > > > https://codereview.chromium.org/183743019/diff/1/content/browser/download/dow... > > File content/browser/download/download_item_impl.cc (right): > > > > > https://codereview.chromium.org/183743019/diff/1/content/browser/download/dow... > > content/browser/download/download_item_impl.cc:1451: (resume_mode == > > RESUME_MODE_INVALID && > > You can just check for resume_mode == RESUME_MODE_INVALID. The last_reason_ > had > > better be something other than DOWNLOAD_INTERRUPT_REASON_NONE if Interrupt() > was > > called :). Also update the comment above. > > Fixed, although there are no any restrictions on the |reason| value in > Interrupt() :) Maybe DCHECK about that should be added. Indeed. Could you add such a DCHECK to Interrupt()? Would you also mind adding a test case to download_item_impl_unittest.cc for this case (where the resume mode is INVALID)? You could copy DownloadItemTest_RestartAfterInterrupted and use one of the interrupt reasons that result in an invalid resume mode.
On 2014/03/05 21:46:33, asanka wrote: > On 2014/03/05 10:08:07, baranovich wrote: > > On 2014/03/04 21:17:49, asanka wrote: > > > > > > https://codereview.chromium.org/183743019/diff/1/content/browser/download/dow... > > > File content/browser/download/download_item_impl.cc (right): > > > > > > > > > https://codereview.chromium.org/183743019/diff/1/content/browser/download/dow... > > > content/browser/download/download_item_impl.cc:1451: (resume_mode == > > > RESUME_MODE_INVALID && > > > You can just check for resume_mode == RESUME_MODE_INVALID. The last_reason_ > > had > > > better be something other than DOWNLOAD_INTERRUPT_REASON_NONE if Interrupt() > > was > > > called :). Also update the comment above. > > > > Fixed, although there are no any restrictions on the |reason| value in > > Interrupt() :) Maybe DCHECK about that should be added. > > Indeed. Could you add such a DCHECK to Interrupt()? > > Would you also mind adding a test case to download_item_impl_unittest.cc for > this case (where the resume mode is INVALID)? You could copy > DownloadItemTest_RestartAfterInterrupted and use one of the interrupt reasons > that result in an invalid resume mode. Other than that, the change LG.
On 2014/03/05 21:47:07, asanka wrote: > On 2014/03/05 21:46:33, asanka wrote: > > On 2014/03/05 10:08:07, baranovich wrote: > > > On 2014/03/04 21:17:49, asanka wrote: > > > > > > > > > > https://codereview.chromium.org/183743019/diff/1/content/browser/download/dow... > > > > File content/browser/download/download_item_impl.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/183743019/diff/1/content/browser/download/dow... > > > > content/browser/download/download_item_impl.cc:1451: (resume_mode == > > > > RESUME_MODE_INVALID && > > > > You can just check for resume_mode == RESUME_MODE_INVALID. The > last_reason_ > > > had > > > > better be something other than DOWNLOAD_INTERRUPT_REASON_NONE if > Interrupt() > > > was > > > > called :). Also update the comment above. > > > > > > Fixed, although there are no any restrictions on the |reason| value in > > > Interrupt() :) Maybe DCHECK about that should be added. > > > > Indeed. Could you add such a DCHECK to Interrupt()? > > > > Would you also mind adding a test case to download_item_impl_unittest.cc for > > this case (where the resume mode is INVALID)? You could copy > > DownloadItemTest_RestartAfterInterrupted and use one of the interrupt reasons > > that result in an invalid resume mode. > > Other than that, the change LG. Done
LGTM. Thanks for doing this! Can you update the CL description before landing? The reason why the file is deleted when the resume mode is invalid is because the partial file, if it exists, can no longer be used to complete the download. The DCHECK was just there to verify this claim. We prefer CL descriptions to be wrapped at 72 columns. Running 'git cl description' should help with this (assuming you are using Git). Also avoid abbreviations in the description unless they are used in code (e.g. 'smth' => 'something').
The CQ bit was checked by baranovich@yandex-team.ru
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/baranovich@yandex-team.ru/183743019/50001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on win_rel for step(s) app_list_unittests, ash_unittests, aura_unittests, cacheinvalidation_unittests, cc_unittests, check_deps, check_deps2git, chrome_elf_unittests, chromedriver_unittests, components_unittests, compositor_unittests, content_browsertests, content_unittests, crypto_unittests, events_unittests, gfx_unittests, google_apis_unittests, gpu_unittests, installer_util_unittests, ipc_tests, jingle_unittests, media_unittests, mini_installer_test, nacl_integration, ppapi_unittests, printing_unittests, remoting_unittests, sql_unittests, sync_integration_tests, sync_unit_tests, telemetry_perf_unittests, telemetry_unittests, views_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
The CQ bit was checked by baranovich@yandex-team.ru
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/baranovich@yandex-team.ru/183743019/50001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on mac_rel for step(s) telemetry_perf_unittests, telemetry_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
The CQ bit was checked by baranovich@yandex-team.ru
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/baranovich@yandex-team.ru/183743019/50001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/baranovich@yandex-team.ru/183743019/50001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/baranovich@yandex-team.ru/183743019/50001
Message was sent while issue was closed.
Change committed as 255506 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
