Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1986673002:
[Offline Pages] Updated clearing logic in storage manager. (Closed)
|
Created:
4 years, 7 months ago by romax Modified:
4 years, 7 months ago Reviewers:
fgorski CC:
chromium-reviews, sdefresne+watchlist_chromium.org, droger+watchlist_chromium.org, romax+watch_chromium.org, fgorski+watch_chromium.org, dewittj+watch_chromium.org, petewil+watch_chromium.org, chili+watch_chromium.org, blundell+watchlist_chromium.org, dimich+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Offline Pages] Updated clearing logic in storage manager.
Updated clearing logic in storage manager, adapted to new changes from archive manager.
Also removed the storage manager related test in model_unittest, while
adding some tests in storage_manager_unittest.
BUG=609357
Committed: https://crrev.com/13aa507d5e0488ebbbe5e4f8afd0942e5435689b
Cr-Commit-Position: refs/heads/master@{#395384}
Patch Set 1 #Patch Set 2 : fixing trybots. #
Total comments: 19
Patch Set 3 : removed PrefService, using new clearing logic and adapted changes of archive manager. #Patch Set 4 : Adapt archiver changes, minor fixes and tests. #Patch Set 5 : Refactored to short circuit earlier if no need to clear. #
Total comments: 48
Patch Set 6 : Addressing comments. #
Total comments: 6
Patch Set 7 : more comments. #
Dependent Patchsets: Messages
Total messages: 18 (6 generated)
romax@chromium.org changed reviewers: + fgorski@chromium.org
Hi Filip, please take a look if I'm doing it right in the direction, so that I can stop before doing more wrong things. Thanks.
https://codereview.chromium.org/1986673002/diff/20001/components/offline_page... File components/offline_pages.gypi (right): https://codereview.chromium.org/1986673002/diff/20001/components/offline_page... components/offline_pages.gypi:23: 'user_prefs', Why is this not in sync with GN? Also: include bauerb@ for DEPs review. https://codereview.chromium.org/1986673002/diff/20001/components/offline_page... File components/offline_pages/offline_page_model.h (right): https://codereview.chromium.org/1986673002/diff/20001/components/offline_page... components/offline_pages/offline_page_model.h:372: // PrefService which should be used by the storage manager. Not owned. https://codereview.chromium.org/1986673002/diff/20001/components/offline_page... File components/offline_pages/offline_page_storage_manager.cc (right): https://codereview.chromium.org/1986673002/diff/20001/components/offline_page... components/offline_pages/offline_page_storage_manager.cc:141: // If it's been more than the pre-defined gap since the last time we clear the interval https://codereview.chromium.org/1986673002/diff/20001/components/offline_page... components/offline_pages/offline_page_storage_manager.cc:160: int64_t free_space = base::SysInfo::AmountOfFreeDiskSpace(pages[0].file_path); This requires IO Thread and to my knowledge you are on UI... https://codereview.chromium.org/1986673002/diff/20001/components/offline_page... components/offline_pages/offline_page_storage_manager.cc:164: return ClearMode::CLEAR_ALL; This is too aggressive. Why would you remove everything? https://codereview.chromium.org/1986673002/diff/20001/components/offline_page... File components/offline_pages/offline_page_storage_manager.h (right): https://codereview.chromium.org/1986673002/diff/20001/components/offline_page... components/offline_pages/offline_page_storage_manager.h:28: // of (50 MB, 10% of available storage space). where did the second number come from? https://codereview.chromium.org/1986673002/diff/20001/components/offline_page... components/offline_pages/offline_page_storage_manager.h:31: static const base::TimeDelta kClearStorageTimeGap = kClearStorageInterval https://codereview.chromium.org/1986673002/diff/20001/components/offline_page... components/offline_pages/offline_page_storage_manager.h:33: static const char kOfflinePageStorageLastClearedTime[] = isn't this defined already? If unaccessible, this can stay of course. https://codereview.chromium.org/1986673002/diff/20001/components/offline_page... components/offline_pages/offline_page_storage_manager.h:63: DELETE_FAIL, // Deletion failed. FAILURE? https://codereview.chromium.org/1986673002/diff/20001/components/offline_page... components/offline_pages/offline_page_storage_manager.h:89: NO_NEED, // No need to remove any page. Please explain that a bit better.
PTAL, removed PrefService, changed to the discussed logic, adapted changes related with ArchiveManager https://codereview.chromium.org/1986673002/diff/20001/components/offline_page... File components/offline_pages.gypi (right): https://codereview.chromium.org/1986673002/diff/20001/components/offline_page... components/offline_pages.gypi:23: 'user_prefs', On 2016/05/17 05:33:29, fgorski wrote: > Why is this not in sync with GN? > > Also: include bauerb@ for DEPs review. > it seems there's no prefs.gypi... but we're removing this https://codereview.chromium.org/1986673002/diff/20001/components/offline_page... File components/offline_pages/offline_page_model.h (right): https://codereview.chromium.org/1986673002/diff/20001/components/offline_page... components/offline_pages/offline_page_model.h:372: // PrefService which should be used by the storage manager. On 2016/05/17 05:33:29, fgorski wrote: > Not owned. removing https://codereview.chromium.org/1986673002/diff/20001/components/offline_page... File components/offline_pages/offline_page_storage_manager.cc (right): https://codereview.chromium.org/1986673002/diff/20001/components/offline_page... components/offline_pages/offline_page_storage_manager.cc:141: // If it's been more than the pre-defined gap since the last time we clear the On 2016/05/17 05:33:29, fgorski wrote: > interval Done. https://codereview.chromium.org/1986673002/diff/20001/components/offline_page... components/offline_pages/offline_page_storage_manager.cc:164: return ClearMode::CLEAR_ALL; On 2016/05/17 05:33:29, fgorski wrote: > This is too aggressive. Why would you remove everything? will change per discussion https://codereview.chromium.org/1986673002/diff/20001/components/offline_page... File components/offline_pages/offline_page_storage_manager.h (right): https://codereview.chromium.org/1986673002/diff/20001/components/offline_page... components/offline_pages/offline_page_storage_manager.h:28: // of (50 MB, 10% of available storage space). On 2016/05/17 05:33:29, fgorski wrote: > where did the second number come from? it's from the Offline Page Cache prd. second point under 'Manager Storage' https://codereview.chromium.org/1986673002/diff/20001/components/offline_page... components/offline_pages/offline_page_storage_manager.h:31: static const base::TimeDelta kClearStorageTimeGap = On 2016/05/17 05:33:29, fgorski wrote: > kClearStorageInterval Done. https://codereview.chromium.org/1986673002/diff/20001/components/offline_page... components/offline_pages/offline_page_storage_manager.h:33: static const char kOfflinePageStorageLastClearedTime[] = On 2016/05/17 05:33:29, fgorski wrote: > isn't this defined already? > If unaccessible, this can stay of course. yes it was unaccessible. removing since we no longer use it. https://codereview.chromium.org/1986673002/diff/20001/components/offline_page... components/offline_pages/offline_page_storage_manager.h:63: DELETE_FAIL, // Deletion failed. On 2016/05/17 05:33:29, fgorski wrote: > FAILURE? Done. https://codereview.chromium.org/1986673002/diff/20001/components/offline_page... components/offline_pages/offline_page_storage_manager.h:89: NO_NEED, // No need to remove any page. On 2016/05/17 05:33:29, fgorski wrote: > Please explain that a bit better. Done.
Description was changed from ========== [Offline Pages] Using PrefService to get/set last storage clear time. Using PrefService to store last clear time, so that the storage manager could clear pages after a certain amount of gap. Passing the prefs as an argument in the constructor of OfflinePageModel. Adding constant values in pref_names, and register in profile.cc. Also removed the storage manager related test in model_unittest, while adding some tests in storage_manager_unittest. BUG=609357 ========== to ========== [Offline Pages] Updated clearing logic in storage manager. Updated clearing logic in storage manager, adapted to new changes from archive manager. Also removed the storage manager related test in model_unittest, while adding some tests in storage_manager_unittest. BUG=609357 ==========
updated CL. PTAL
Refactored storage manager so that we can know there's no need to clear storage earlier than before.
https://codereview.chromium.org/1986673002/diff/80001/components/offline_page... File components/offline_pages/archive_manager.cc (right): https://codereview.chromium.org/1986673002/diff/80001/components/offline_page... components/offline_pages/archive_manager.cc:122: ArchiveManager::ArchiveManager() {} constructors go above methods. https://codereview.chromium.org/1986673002/diff/80001/components/offline_page... File components/offline_pages/offline_page_storage_manager.cc (right): https://codereview.chromium.org/1986673002/diff/80001/components/offline_page... components/offline_pages/offline_page_storage_manager.cc:6: #include <stdint.h> this is for int64_t https://codereview.chromium.org/1986673002/diff/80001/components/offline_page... components/offline_pages/offline_page_storage_manager.cc:9: #include "base/bind.h" needed? https://codereview.chromium.org/1986673002/diff/80001/components/offline_page... components/offline_pages/offline_page_storage_manager.cc:13: #include "base/time/time.h" time.h already included in header. https://codereview.chromium.org/1986673002/diff/80001/components/offline_page... components/offline_pages/offline_page_storage_manager.cc:14: #include "components/offline_pages/archive_manager.h" same https://codereview.chromium.org/1986673002/diff/80001/components/offline_page... components/offline_pages/offline_page_storage_manager.cc:18: #include "components/offline_pages/offline_page_types.h" same https://codereview.chromium.org/1986673002/diff/80001/components/offline_page... components/offline_pages/offline_page_storage_manager.cc:92: std::vector<int64_t>& offline_ids, Could you put this at the end. It feels much better when output parameters are at the end. https://codereview.chromium.org/1986673002/diff/80001/components/offline_page... components/offline_pages/offline_page_storage_manager.cc:101: std::vector<OfflinePageItem> pages_left; how about kept_pages? https://codereview.chromium.org/1986673002/diff/80001/components/offline_page... components/offline_pages/offline_page_storage_manager.cc:102: int64_t non_expired_usage = 0; how about kept_pages_size? https://codereview.chromium.org/1986673002/diff/80001/components/offline_page... components/offline_pages/offline_page_storage_manager.cc:129: for (int i = pos; i < page_list_size; i++) for (;pos < page_list_size; ++pos) https://codereview.chromium.org/1986673002/diff/80001/components/offline_page... components/offline_pages/offline_page_storage_manager.cc:141: std::sort(pages_left.begin(), pages_left.end(), make a comment that this lambda sorts in reverse order. https://codereview.chromium.org/1986673002/diff/80001/components/offline_page... components/offline_pages/offline_page_storage_manager.cc:165: if (total_size >= (total_size + free_space) * kOfflinePageStorageLimit) { nit: {} not needed. https://codereview.chromium.org/1986673002/diff/80001/components/offline_page... File components/offline_pages/offline_page_storage_manager.h (right): https://codereview.chromium.org/1986673002/diff/80001/components/offline_page... components/offline_pages/offline_page_storage_manager.h:16: #include "components/offline_pages/archive_manager.h" you can forward declare that one. https://codereview.chromium.org/1986673002/diff/80001/components/offline_page... components/offline_pages/offline_page_storage_manager.h:21: } } // namespace base https://codereview.chromium.org/1986673002/diff/80001/components/offline_page... components/offline_pages/offline_page_storage_manager.h:30: static const double kOfflinePageStorageLimit = 0.3; remove static, you are not inside of a class. Also is it important to expose these values outside of StorageManager? https://codereview.chromium.org/1986673002/diff/80001/components/offline_page... components/offline_pages/offline_page_storage_manager.h:61: enum class StorageClearResult { ClearStorageResult https://codereview.chromium.org/1986673002/diff/80001/components/offline_page... components/offline_pages/offline_page_storage_manager.h:70: typedef base::Callback<void(int, StorageClearResult)> ClearPageCallback; ClearPagesCallback https://codereview.chromium.org/1986673002/diff/80001/components/offline_page... components/offline_pages/offline_page_storage_manager.h:95: // pages and we're not exceeding the storage limit.) why do we have unnecessary and no need? https://codereview.chromium.org/1986673002/diff/80001/components/offline_page... components/offline_pages/offline_page_storage_manager.h:96: NO_NEED, NOT_NEEDED https://codereview.chromium.org/1986673002/diff/80001/components/offline_page... File components/offline_pages/offline_page_storage_manager_unittest.cc (right): https://codereview.chromium.org/1986673002/diff/80001/components/offline_page... components/offline_pages/offline_page_storage_manager_unittest.cc:85: }; nit: empty line https://codereview.chromium.org/1986673002/diff/80001/components/offline_page... components/offline_pages/offline_page_storage_manager_unittest.cc:90: callback.Run(DeletePageResult::STORE_FAILURE); consider putting return; here, skipping else https://codereview.chromium.org/1986673002/diff/80001/components/offline_page... components/offline_pages/offline_page_storage_manager_unittest.cc:100: } nit: empty line https://codereview.chromium.org/1986673002/diff/80001/components/offline_page... components/offline_pages/offline_page_storage_manager_unittest.cc:141: TestArchiveManager(StorageStats stats) mark explicit https://codereview.chromium.org/1986673002/diff/80001/components/offline_page... components/offline_pages/offline_page_storage_manager_unittest.cc:157: int64_t free_space_; Simply Stats stats_; ?
Addressed comments, PTAL. https://codereview.chromium.org/1986673002/diff/80001/components/offline_page... File components/offline_pages/archive_manager.cc (right): https://codereview.chromium.org/1986673002/diff/80001/components/offline_page... components/offline_pages/archive_manager.cc:122: ArchiveManager::ArchiveManager() {} On 2016/05/19 04:14:22, fgorski wrote: > constructors go above methods. Done. https://codereview.chromium.org/1986673002/diff/80001/components/offline_page... File components/offline_pages/offline_page_storage_manager.cc (right): https://codereview.chromium.org/1986673002/diff/80001/components/offline_page... components/offline_pages/offline_page_storage_manager.cc:6: On 2016/05/19 04:14:22, fgorski wrote: > #include <stdint.h> > > this is for int64_t Done. https://codereview.chromium.org/1986673002/diff/80001/components/offline_page... components/offline_pages/offline_page_storage_manager.cc:9: #include "base/bind.h" On 2016/05/19 04:14:22, fgorski wrote: > needed? forgot to remove... https://codereview.chromium.org/1986673002/diff/80001/components/offline_page... components/offline_pages/offline_page_storage_manager.cc:13: #include "base/time/time.h" On 2016/05/19 04:14:22, fgorski wrote: > time.h already included in header. Done. https://codereview.chromium.org/1986673002/diff/80001/components/offline_page... components/offline_pages/offline_page_storage_manager.cc:14: #include "components/offline_pages/archive_manager.h" On 2016/05/19 04:14:22, fgorski wrote: > same Done. https://codereview.chromium.org/1986673002/diff/80001/components/offline_page... components/offline_pages/offline_page_storage_manager.cc:18: #include "components/offline_pages/offline_page_types.h" On 2016/05/19 04:14:22, fgorski wrote: > same Done. https://codereview.chromium.org/1986673002/diff/80001/components/offline_page... components/offline_pages/offline_page_storage_manager.cc:92: std::vector<int64_t>& offline_ids, On 2016/05/19 04:14:22, fgorski wrote: > Could you put this at the end. It feels much better when output parameters are > at the end. Done. https://codereview.chromium.org/1986673002/diff/80001/components/offline_page... components/offline_pages/offline_page_storage_manager.cc:101: std::vector<OfflinePageItem> pages_left; On 2016/05/19 04:14:22, fgorski wrote: > how about kept_pages? Done. https://codereview.chromium.org/1986673002/diff/80001/components/offline_page... components/offline_pages/offline_page_storage_manager.cc:102: int64_t non_expired_usage = 0; On 2016/05/19 04:14:22, fgorski wrote: > how about kept_pages_size? Done. https://codereview.chromium.org/1986673002/diff/80001/components/offline_page... components/offline_pages/offline_page_storage_manager.cc:129: for (int i = pos; i < page_list_size; i++) On 2016/05/19 04:14:22, fgorski wrote: > for (;pos < page_list_size; ++pos) Done. https://codereview.chromium.org/1986673002/diff/80001/components/offline_page... components/offline_pages/offline_page_storage_manager.cc:141: std::sort(pages_left.begin(), pages_left.end(), On 2016/05/19 04:14:22, fgorski wrote: > make a comment that this lambda sorts in reverse order. Done. https://codereview.chromium.org/1986673002/diff/80001/components/offline_page... components/offline_pages/offline_page_storage_manager.cc:165: if (total_size >= (total_size + free_space) * kOfflinePageStorageLimit) { On 2016/05/19 04:14:22, fgorski wrote: > nit: {} not needed. Done. https://codereview.chromium.org/1986673002/diff/80001/components/offline_page... File components/offline_pages/offline_page_storage_manager.h (right): https://codereview.chromium.org/1986673002/diff/80001/components/offline_page... components/offline_pages/offline_page_storage_manager.h:16: #include "components/offline_pages/archive_manager.h" On 2016/05/19 04:14:22, fgorski wrote: > you can forward declare that one. I'm also using ArchiveManager::StorageStats, so I guess I have to include this header anyways? https://codereview.chromium.org/1986673002/diff/80001/components/offline_page... components/offline_pages/offline_page_storage_manager.h:21: } On 2016/05/19 04:14:22, fgorski wrote: > } // namespace base Done. https://codereview.chromium.org/1986673002/diff/80001/components/offline_page... components/offline_pages/offline_page_storage_manager.h:30: static const double kOfflinePageStorageLimit = 0.3; On 2016/05/19 04:14:22, fgorski wrote: > remove static, you are not inside of a class. > > Also is it important to expose these values outside of StorageManager? Done. Moved them into an anonymous namespace. https://codereview.chromium.org/1986673002/diff/80001/components/offline_page... components/offline_pages/offline_page_storage_manager.h:61: enum class StorageClearResult { On 2016/05/19 04:14:22, fgorski wrote: > ClearStorageResult Done. https://codereview.chromium.org/1986673002/diff/80001/components/offline_page... components/offline_pages/offline_page_storage_manager.h:70: typedef base::Callback<void(int, StorageClearResult)> ClearPageCallback; On 2016/05/19 04:14:22, fgorski wrote: > ClearPagesCallback Done. https://codereview.chromium.org/1986673002/diff/80001/components/offline_page... components/offline_pages/offline_page_storage_manager.h:95: // pages and we're not exceeding the storage limit.) On 2016/05/19 04:14:22, fgorski wrote: > why do we have unnecessary and no need? I just want to provide a way to identify that we're returning with doing nothing. I thought about when the return int in the callback is 0 it means we did nothing but then i realized there might be a deletion failure. So I'm having this. https://codereview.chromium.org/1986673002/diff/80001/components/offline_page... components/offline_pages/offline_page_storage_manager.h:96: NO_NEED, On 2016/05/19 04:14:22, fgorski wrote: > NOT_NEEDED Done. https://codereview.chromium.org/1986673002/diff/80001/components/offline_page... File components/offline_pages/offline_page_storage_manager_unittest.cc (right): https://codereview.chromium.org/1986673002/diff/80001/components/offline_page... components/offline_pages/offline_page_storage_manager_unittest.cc:85: }; On 2016/05/19 04:14:22, fgorski wrote: > nit: empty line Done. https://codereview.chromium.org/1986673002/diff/80001/components/offline_page... components/offline_pages/offline_page_storage_manager_unittest.cc:90: callback.Run(DeletePageResult::STORE_FAILURE); On 2016/05/19 04:14:23, fgorski wrote: > consider putting return; here, skipping else Done. https://codereview.chromium.org/1986673002/diff/80001/components/offline_page... components/offline_pages/offline_page_storage_manager_unittest.cc:100: } On 2016/05/19 04:14:22, fgorski wrote: > nit: empty line Done. https://codereview.chromium.org/1986673002/diff/80001/components/offline_page... components/offline_pages/offline_page_storage_manager_unittest.cc:141: TestArchiveManager(StorageStats stats) On 2016/05/19 04:14:22, fgorski wrote: > mark explicit Done. https://codereview.chromium.org/1986673002/diff/80001/components/offline_page... components/offline_pages/offline_page_storage_manager_unittest.cc:157: int64_t free_space_; On 2016/05/19 04:14:22, fgorski wrote: > Simply Stats stats_; ? Done.
lgtm with comments https://codereview.chromium.org/1986673002/diff/100001/components/offline_pag... File components/offline_pages/offline_page_storage_manager.cc (right): https://codereview.chromium.org/1986673002/diff/100001/components/offline_pag... components/offline_pages/offline_page_storage_manager.cc:7: #include <stdint.h> I just noticed that this is already in .h https://codereview.chromium.org/1986673002/diff/100001/components/offline_pag... components/offline_pages/offline_page_storage_manager.cc:145: while (space_to_release > 0) { && pos < kept_pages.size() https://codereview.chromium.org/1986673002/diff/100001/components/offline_pag... File components/offline_pages/offline_page_storage_manager.h (right): https://codereview.chromium.org/1986673002/diff/100001/components/offline_pag... components/offline_pages/offline_page_storage_manager.h:25: namespace { Interesting, are you using this outside of .cc file?
more comments and commit ready. https://codereview.chromium.org/1986673002/diff/100001/components/offline_pag... File components/offline_pages/offline_page_storage_manager.cc (right): https://codereview.chromium.org/1986673002/diff/100001/components/offline_pag... components/offline_pages/offline_page_storage_manager.cc:7: #include <stdint.h> On 2016/05/19 20:19:53, fgorski wrote: > I just noticed that this is already in .h Done. https://codereview.chromium.org/1986673002/diff/100001/components/offline_pag... components/offline_pages/offline_page_storage_manager.cc:145: while (space_to_release > 0) { On 2016/05/19 20:19:53, fgorski wrote: > && pos < kept_pages.size() Done. https://codereview.chromium.org/1986673002/diff/100001/components/offline_pag... File components/offline_pages/offline_page_storage_manager.h (right): https://codereview.chromium.org/1986673002/diff/100001/components/offline_pag... components/offline_pages/offline_page_storage_manager.h:25: namespace { On 2016/05/19 20:19:53, fgorski wrote: > Interesting, are you using this outside of .cc file? Done.
The CQ bit was checked by romax@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from fgorski@chromium.org Link to the patchset: https://codereview.chromium.org/1986673002/#ps120001 (title: "more comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1986673002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1986673002/120001
Message was sent while issue was closed.
Description was changed from ========== [Offline Pages] Updated clearing logic in storage manager. Updated clearing logic in storage manager, adapted to new changes from archive manager. Also removed the storage manager related test in model_unittest, while adding some tests in storage_manager_unittest. BUG=609357 ========== to ========== [Offline Pages] Updated clearing logic in storage manager. Updated clearing logic in storage manager, adapted to new changes from archive manager. Also removed the storage manager related test in model_unittest, while adding some tests in storage_manager_unittest. BUG=609357 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== [Offline Pages] Updated clearing logic in storage manager. Updated clearing logic in storage manager, adapted to new changes from archive manager. Also removed the storage manager related test in model_unittest, while adding some tests in storage_manager_unittest. BUG=609357 ========== to ========== [Offline Pages] Updated clearing logic in storage manager. Updated clearing logic in storage manager, adapted to new changes from archive manager. Also removed the storage manager related test in model_unittest, while adding some tests in storage_manager_unittest. BUG=609357 Committed: https://crrev.com/13aa507d5e0488ebbbe5e4f8afd0942e5435689b Cr-Commit-Position: refs/heads/master@{#395384} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/13aa507d5e0488ebbbe5e4f8afd0942e5435689b Cr-Commit-Position: refs/heads/master@{#395384} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
