Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1414753005:
Evict drive cache with file download from website if necessary. (Closed)
|
Created:
5 years, 1 month ago by yawano Modified:
5 years, 1 month ago Reviewers:
hashimoto CC:
chromium-reviews, tfarina, oshima+watch_chromium.org, davemoore+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionEvict drive cache with file download from website if necessary.
BUG=524910
TEST=unit_tests:DownloadHandlerTest.*
Committed: https://crrev.com/4712d90aab1107b95daa715ff23951b836cf1d88
Cr-Commit-Position: refs/heads/master@{#358234}
Patch Set 1 #Patch Set 2 : Free disk space immediately in OnDownloadCreated. #
Total comments: 10
Patch Set 3 : Support multiple in-progress downloads. #Patch Set 4 : Request 0 byte for unknown size download. #Patch Set 5 : Correct spell. #
Total comments: 12
Patch Set 6 : Address comments. #
Total comments: 6
Patch Set 7 : Fix nits. #
Messages
Total messages: 17 (2 generated)
yawano@chromium.org changed reviewers: + hashimoto@chromium.org
PTAL. Thank you!
@hashimoto: Changed to immediately free disk space in OnDownloadCreated. PTAL. Thank you!
https://codereview.chromium.org/1414753005/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/drive/download_handler.cc (right): https://codereview.chromium.org/1414753005/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/drive/download_handler.cc:38: const int64 kDefaultRequestSpace = 512 * (1 << 20); // 512MB How was this value chosen? (based on some statistics?) https://codereview.chromium.org/1414753005/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/drive/download_handler.cc:253: // TODO(yawano): support multiple in-progress downloads. Why don't you support multiple downloads now? https://codereview.chromium.org/1414753005/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/drive/download_handler.cc:260: const int64 request_space = total_bytes == 0 Please add a comment why we need to handle total_bytes==0 case. https://codereview.chromium.org/1414753005/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/drive/download_handler.cc:261: ? kDefaultRequestSpace IIUC, when the download is 99% complete, this can result in requesting needed bytes + 512MB Is this really what we want? https://codereview.chromium.org/1414753005/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/drive/download_handler.h (right): https://codereview.chromium.org/1414753005/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/drive/download_handler.h:79: bool immediate); Generally, boolean parameters make the code less readable. (cf. "nullptr/NULL, true/false, 1, 2, 3..." section of our style guide https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Implementati...) How about passing base::TimeDelta instead?
Thank you for the review! Reply to some comments. https://codereview.chromium.org/1414753005/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/drive/download_handler.cc (right): https://codereview.chromium.org/1414753005/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/drive/download_handler.cc:38: const int64 kDefaultRequestSpace = 512 * (1 << 20); // 512MB No, this is not based on some statistics. This value should be - small as possible since we don't want to evict Drive cache unnecessary. - large enough as not to be used up until the next FreeDiskSpace (5 secs). https://codereview.chromium.org/1414753005/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/drive/download_handler.cc:253: // TODO(yawano): support multiple in-progress downloads. I thought it will make this CL larger. But I think this will be the best timing to support it. I'll try to implement it in this CL. https://codereview.chromium.org/1414753005/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/drive/download_handler.cc:261: ? kDefaultRequestSpace This value is used when we don't know total bytes of Download. When content_length is not available, total bytes become 0. https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/do...
Support multiple in-progress downloads with new patch set. PTAL again. Thank you! https://codereview.chromium.org/1414753005/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/drive/download_handler.cc (right): https://codereview.chromium.org/1414753005/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/drive/download_handler.cc:260: const int64 request_space = total_bytes == 0 On 2015/10/28 09:06:15, hashimoto wrote: > Please add a comment why we need to handle total_bytes==0 case. Done. https://codereview.chromium.org/1414753005/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/drive/download_handler.h (right): https://codereview.chromium.org/1414753005/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/drive/download_handler.h:79: bool immediate); Split a method into two methods, FreeDiskSpaceIfNeeded and FreeDiskSpaceIfNeededImmediately.
@hashimoto: As we talked offline, changed to request 0 byte for unknown size download. We can use drive::internal::kMinFreeSpaceInBytes temporally for it. PTAL. Thank you!
Sorry for being slow to respond. https://codereview.chromium.org/1414753005/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/drive/download_handler.cc (right): https://codereview.chromium.org/1414753005/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/drive/download_handler.cc:291: notifier_->GetManager()->GetAllDownloads(&downloads); GetManager() can return nullptr. Don't we need to null-check? https://codereview.chromium.org/1414753005/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/drive/download_handler.cc:293: notifier_incognito_->GetManager()->GetAllDownloads(&downloads); qq: IIUC this code is intended to get all downloads from one profile, and its off-the-record profile, but it cannot correctly calculate the required disk space if downloads are happening on multiple profiles. Is this intended? If so, it'd be nice to have a comment about it (or todo comment to support this case). https://codereview.chromium.org/1414753005/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/drive/download_handler_unittest.cc (right): https://codereview.chromium.org/1414753005/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/drive/download_handler_unittest.cc:68: content::DownloadManager::DownloadVector test_downloads; nit: test_downloads_; https://codereview.chromium.org/1414753005/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/drive/download_handler_unittest.cc:226: content::MockDownloadItem download_item_a; Could you implement something like DownloadHandlerTestDownloadManager for DownloadItem? We've deprecated use of gmock (http://crbug.com/256537). https://codereview.chromium.org/1414753005/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/drive/download_handler_unittest.cc:241: download_handler_->FreeDiskSpaceIfNeededImmediately(); Please describe the intention of each FreeDiskSpaceIfNeeded()/FreeDiskSpaceIfNeededImmediately() call. https://codereview.chromium.org/1414753005/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/drive/download_handler_unittest.cc:311: ASSERT_EQ(290, download_handler_->CalculateRequestSpace(downloads)); Please describe the intention of each CalculateRequestSpace().
PTAL again. Thank you! https://codereview.chromium.org/1414753005/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/drive/download_handler.cc (right): https://codereview.chromium.org/1414753005/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/drive/download_handler.cc:291: notifier_->GetManager()->GetAllDownloads(&downloads); Yes, we should. Thank you! https://codereview.chromium.org/1414753005/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/drive/download_handler.cc:293: notifier_incognito_->GetManager()->GetAllDownloads(&downloads); This gets all downloads of current profile and its off-the-record profile. But this doesn't consider downloads of other profiles. I added TODO. https://codereview.chromium.org/1414753005/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/drive/download_handler_unittest.cc (right): https://codereview.chromium.org/1414753005/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/drive/download_handler_unittest.cc:68: content::DownloadManager::DownloadVector test_downloads; On 2015/10/30 09:33:43, hashimoto wrote: > nit: test_downloads_; Done. https://codereview.chromium.org/1414753005/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/drive/download_handler_unittest.cc:226: content::MockDownloadItem download_item_a; On 2015/10/30 09:33:43, hashimoto wrote: > Could you implement something like DownloadHandlerTestDownloadManager for > DownloadItem? > We've deprecated use of gmock (http://crbug.com/256537). Done. https://codereview.chromium.org/1414753005/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/drive/download_handler_unittest.cc:241: download_handler_->FreeDiskSpaceIfNeededImmediately(); On 2015/10/30 09:33:43, hashimoto wrote: > Please describe the intention of each > FreeDiskSpaceIfNeeded()/FreeDiskSpaceIfNeededImmediately() call. Done. https://codereview.chromium.org/1414753005/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/drive/download_handler_unittest.cc:311: ASSERT_EQ(290, download_handler_->CalculateRequestSpace(downloads)); On 2015/10/30 09:33:43, hashimoto wrote: > Please describe the intention of each CalculateRequestSpace(). Done.
lgtm https://codereview.chromium.org/1414753005/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/drive/download_handler.cc (right): https://codereview.chromium.org/1414753005/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/drive/download_handler.cc:292: if (notifier_ != nullptr && notifier_->GetManager() != nullptr) { nit: You can write "if (notifier_ && notifier_->GetManager())" to make this shorter. The same goes for notifier_incognito_. https://codereview.chromium.org/1414753005/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/drive/download_handler_unittest.cc (right): https://codereview.chromium.org/1414753005/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/drive/download_handler_unittest.cc:249: ASSERT_EQ(90, test_file_system_.free_disk_space_if_needed_for_num_bytes_[0]); nit: How about replacing "90" with download_item_a.total_bytes - download_item_a.received_bytes to make the relationship between these values clearer? The same goes for others. https://codereview.chromium.org/1414753005/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/drive/download_handler_unittest.cc:313: // 290 = 100 (download_item_a) + 190 (download_item_b) nit:You can write this relationship directly into the code. ASSERT_EQ(download_item_a.total_bytes - download_item_a.received_bytes + ...); The same goes for others.
Thank you for the review! https://codereview.chromium.org/1414753005/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/drive/download_handler.cc (right): https://codereview.chromium.org/1414753005/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/drive/download_handler.cc:292: if (notifier_ != nullptr && notifier_->GetManager() != nullptr) { On 2015/11/02 20:57:59, hashimoto wrote: > nit: You can write "if (notifier_ && notifier_->GetManager())" to make this > shorter. The same goes for notifier_incognito_. Done. https://codereview.chromium.org/1414753005/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/drive/download_handler_unittest.cc (right): https://codereview.chromium.org/1414753005/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/drive/download_handler_unittest.cc:249: ASSERT_EQ(90, test_file_system_.free_disk_space_if_needed_for_num_bytes_[0]); On 2015/11/02 20:57:59, hashimoto wrote: > nit: How about replacing "90" with download_item_a.total_bytes - > download_item_a.received_bytes to make the relationship between these values > clearer? > The same goes for others. Done. https://codereview.chromium.org/1414753005/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/drive/download_handler_unittest.cc:313: // 290 = 100 (download_item_a) + 190 (download_item_b) On 2015/11/02 20:57:59, hashimoto wrote: > nit:You can write this relationship directly into the code. > ASSERT_EQ(download_item_a.total_bytes - download_item_a.received_bytes + ...); > The same goes for others. Done.
lgtm
On 2015/11/05 19:13:54, hashimoto wrote: > lgtm Thank you!
The CQ bit was checked by yawano@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1414753005/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1414753005/120001
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/4712d90aab1107b95daa715ff23951b836cf1d88 Cr-Commit-Position: refs/heads/master@{#358234} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
