Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2860573004:
[Offline Pages] Add cached offline page utils and show usage in settings. (Closed)
|
Created:
3 years, 7 months ago by romax Modified:
3 years, 7 months ago CC:
chromium-reviews, msramek+watch_chromium.org, dewittj+watch_chromium.org, fgorski+watch_chromium.org, romax+watch_chromium.org, petewil+watch_chromium.org, chili+watch_chromium.org, markusheintz_, dimich+watch_chromium.org, dullweber Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[Offline Pages] Add cached offline page utils and show usage in settings.
Added some utility functions for cached offline pages:
Get sizes of all cached offline pages;
Delete all cached offline pages.
Also added the usage of cached offline pages to the number shown at:
Chrome Settings -> Privacy -> Clear Browsing Data -> Cache
BUG=716142
Review-Url: https://codereview.chromium.org/2860573004
Cr-Commit-Position: refs/heads/master@{#472255}
Committed: https://chromium.googlesource.com/chromium/src/+/444db4b416dbc2ed7fed540cfbc0d6ec7774ed65
Patch Set 1 #
Total comments: 24
Patch Set 2 : comments from carlosk@. #
Total comments: 23
Patch Set 3 : comments. #Patch Set 4 : more tests. #Patch Set 5 : fixed unit tests but CBD tests flaky. #
Total comments: 9
Patch Set 6 : more comments. #Patch Set 7 : Tests fixed. #
Total comments: 12
Patch Set 8 : Comments from dewittj@ and dullweber@. #Patch Set 9 : fix build #
Total comments: 4
Patch Set 10 : more build fix. #Patch Set 11 : address missed comment #Messages
Total messages: 72 (48 generated)
romax@chromium.org changed reviewers: + carlosk@chromium.org, dewittj@chromium.org
dewittj@ PTAL carlosk@ FYI Thanks!
Thanks. https://codereview.chromium.org/2860573004/diff/1/components/offline_pages/co... File components/offline_pages/core/cached_offline_page_utils.cc (right): https://codereview.chromium.org/2860573004/diff/1/components/offline_pages/co... components/offline_pages/core/cached_offline_page_utils.cc:24: if (begin_time <= page.creation_time && page.creation_time <= end_time) nit: use < instead of <= to avoid overlapping ranges. https://codereview.chromium.org/2860573004/diff/1/components/offline_pages/co... components/offline_pages/core/cached_offline_page_utils.cc:41: callback.Run(0); Change this to a DCHECK instead. It should be the responsibility of the caller to deal with the case of an nonexistent model instead of receiving a zero value for the cache size and be ignorant the error. https://codereview.chromium.org/2860573004/diff/1/components/offline_pages/co... components/offline_pages/core/cached_offline_page_utils.cc:55: base::Time::Now()); Switch to base::Time::Max() for unbounded upper limits as done in other cache calculating code. https://codereview.chromium.org/2860573004/diff/1/components/offline_pages/co... components/offline_pages/core/cached_offline_page_utils.cc:63: callback.Run(DeletePageResult::STORE_FAILURE); nit: should probably change this to a DCHECK too? I'm unsure because so far there's no caller apart from test code. https://codereview.chromium.org/2860573004/diff/1/components/offline_pages/co... File components/offline_pages/core/cached_offline_page_utils.h (right): https://codereview.chromium.org/2860573004/diff/1/components/offline_pages/co... components/offline_pages/core/cached_offline_page_utils.h:20: // Get total size of all cached offline pages. nit: add a bullet symbol to these lines like a "*" or "-". IMO the extra leading space doesn't make it very clear. https://codereview.chromium.org/2860573004/diff/1/components/offline_pages/co... components/offline_pages/core/cached_offline_page_utils.h:22: class CachedOfflinePageUtils { It seems to be more common in Chromium to use non-member static functions inside a meaningful namespace instead of wrapping them into a class. As this is already in the offline_pages namespace the CachedOfflinePageUtils wrapper could be removed. Method names could need to be renamed but I think they are clear enough even without the wrapping class. https://codereview.chromium.org/2860573004/diff/1/components/offline_pages/co... File components/offline_pages/core/cached_offline_page_utils_unittest.cc (right): https://codereview.chromium.org/2860573004/diff/1/components/offline_pages/co... components/offline_pages/core/cached_offline_page_utils_unittest.cc:107: MultipleOfflinePageItemResult result; nit: use either MultipleOfflinePageItemResult (here) or std::vector<OfflinePageItem> (in GetPagesMatchingQuery) consistently. https://codereview.chromium.org/2860573004/diff/1/components/offline_pages/co... components/offline_pages/core/cached_offline_page_utils_unittest.cc:213: clock()->Advance(base::TimeDelta::FromHours(3)); Advance adds time to the current "now". So this is in fact 05:00:00. If you think it's clearer you could use absolute times and use SetNow instead. https://codereview.chromium.org/2860573004/diff/1/components/offline_pages/co... components/offline_pages/core/cached_offline_page_utils_unittest.cc:215: clock()->Now()); I'd also suggest adding an ulterior temporary entry to check the upper time limit is also being enforced. https://codereview.chromium.org/2860573004/diff/1/components/offline_pages/co... components/offline_pages/core/cached_offline_page_utils_unittest.cc:221: clock()->Now() - base::TimeDelta::FromHours(2), clock()->Now()); Given how Advance works this begin time will not include the persistent entry as I think you intended to do. https://codereview.chromium.org/2860573004/diff/1/components/offline_pages/co... components/offline_pages/core/cached_offline_page_utils_unittest.cc:237: ASSERT_EQ(3UL, GetAllPages().size()); As the test pages used in all current tests are the same move this code into SetUp and avoid duplication in both. https://codereview.chromium.org/2860573004/diff/1/components/offline_pages/co... components/offline_pages/core/cached_offline_page_utils_unittest.cc:239: // Delete cached offlin pages, and there will be one left. /offlin/offline/s
addressed comments from carlosk@. Removed the unused methods. PTAL, thanks! https://codereview.chromium.org/2860573004/diff/1/components/offline_pages/co... File components/offline_pages/core/cached_offline_page_utils.cc (right): https://codereview.chromium.org/2860573004/diff/1/components/offline_pages/co... components/offline_pages/core/cached_offline_page_utils.cc:24: if (begin_time <= page.creation_time && page.creation_time <= end_time) On 2017/05/03 18:13:52, carlosk wrote: > nit: use < instead of <= to avoid overlapping ranges. Done. https://codereview.chromium.org/2860573004/diff/1/components/offline_pages/co... components/offline_pages/core/cached_offline_page_utils.cc:41: callback.Run(0); On 2017/05/03 18:13:52, carlosk wrote: > Change this to a DCHECK instead. It should be the responsibility of the caller > to deal with the case of an nonexistent model instead of receiving a zero value > for the cache size and be ignorant the error. Done. https://codereview.chromium.org/2860573004/diff/1/components/offline_pages/co... components/offline_pages/core/cached_offline_page_utils.cc:55: base::Time::Now()); On 2017/05/03 18:13:52, carlosk wrote: > Switch to base::Time::Max() for unbounded upper limits as done in other cache > calculating code. Done. https://codereview.chromium.org/2860573004/diff/1/components/offline_pages/co... components/offline_pages/core/cached_offline_page_utils.cc:63: callback.Run(DeletePageResult::STORE_FAILURE); On 2017/05/03 18:13:52, carlosk wrote: > nit: should probably change this to a DCHECK too? I'm unsure because so far > there's no caller apart from test code. Removed the unused methods. https://codereview.chromium.org/2860573004/diff/1/components/offline_pages/co... File components/offline_pages/core/cached_offline_page_utils.h (right): https://codereview.chromium.org/2860573004/diff/1/components/offline_pages/co... components/offline_pages/core/cached_offline_page_utils.h:20: // Get total size of all cached offline pages. On 2017/05/03 18:13:52, carlosk wrote: > nit: add a bullet symbol to these lines like a "*" or "-". IMO the extra leading > space doesn't make it very clear. Done. https://codereview.chromium.org/2860573004/diff/1/components/offline_pages/co... components/offline_pages/core/cached_offline_page_utils.h:22: class CachedOfflinePageUtils { On 2017/05/03 18:13:52, carlosk wrote: > It seems to be more common in Chromium to use non-member static functions inside > a meaningful namespace instead of wrapping them into a class. > As this is already in the offline_pages namespace the CachedOfflinePageUtils > wrapper could be removed. Method names could need to be renamed but I think they > are clear enough even without the wrapping class. Done. https://codereview.chromium.org/2860573004/diff/1/components/offline_pages/co... File components/offline_pages/core/cached_offline_page_utils_unittest.cc (right): https://codereview.chromium.org/2860573004/diff/1/components/offline_pages/co... components/offline_pages/core/cached_offline_page_utils_unittest.cc:107: MultipleOfflinePageItemResult result; On 2017/05/03 18:13:52, carlosk wrote: > nit: use either MultipleOfflinePageItemResult (here) or > std::vector<OfflinePageItem> (in GetPagesMatchingQuery) consistently. Done. https://codereview.chromium.org/2860573004/diff/1/components/offline_pages/co... components/offline_pages/core/cached_offline_page_utils_unittest.cc:213: clock()->Advance(base::TimeDelta::FromHours(3)); On 2017/05/03 18:13:52, carlosk wrote: > Advance adds time to the current "now". So this is in fact 05:00:00. > If you think it's clearer you could use absolute times and use SetNow instead. Done. https://codereview.chromium.org/2860573004/diff/1/components/offline_pages/co... components/offline_pages/core/cached_offline_page_utils_unittest.cc:215: clock()->Now()); On 2017/05/03 18:13:52, carlosk wrote: > I'd also suggest adding an ulterior temporary entry to check the upper time > limit is also being enforced. Done. https://codereview.chromium.org/2860573004/diff/1/components/offline_pages/co... components/offline_pages/core/cached_offline_page_utils_unittest.cc:221: clock()->Now() - base::TimeDelta::FromHours(2), clock()->Now()); On 2017/05/03 18:13:52, carlosk wrote: > Given how Advance works this begin time will not include the persistent entry as > I think you intended to do. Acknowledged. https://codereview.chromium.org/2860573004/diff/1/components/offline_pages/co... components/offline_pages/core/cached_offline_page_utils_unittest.cc:237: ASSERT_EQ(3UL, GetAllPages().size()); On 2017/05/03 18:13:52, carlosk wrote: > As the test pages used in all current tests are the same move this code into > SetUp and avoid duplication in both. Done. https://codereview.chromium.org/2860573004/diff/1/components/offline_pages/co... components/offline_pages/core/cached_offline_page_utils_unittest.cc:239: // Delete cached offlin pages, and there will be one left. On 2017/05/03 18:13:52, carlosk wrote: > /offlin/offline/s Done.
https://codereview.chromium.org/2860573004/diff/20001/components/offline_page... File components/offline_pages/core/cached_offline_page_utils.h (right): https://codereview.chromium.org/2860573004/diff/20001/components/offline_page... components/offline_pages/core/cached_offline_page_utils.h:23: void GetCachedOfflinePageSizeBetween(OfflinePageModel* model, It might be better to move this to //components/offline_pages/content or //chrome/browser and to accept a browsercontext, so that the cache controller doesn't need to know about the OfflinePageModelFactory. https://codereview.chromium.org/2860573004/diff/20001/components/offline_page... components/offline_pages/core/cached_offline_page_utils.h:25: const base::Time& begin_time, nit: #include "base/time/time.h" https://codereview.chromium.org/2860573004/diff/20001/components/offline_page... File components/offline_pages/core/cached_offline_page_utils_unittest.cc (right): https://codereview.chromium.org/2860573004/diff/20001/components/offline_page... components/offline_pages/core/cached_offline_page_utils_unittest.cc:45: class CachedOfflinePageTestModel : public StubOfflinePageModel { no action required: I think we are overdue for a nicer fake model. https://codereview.chromium.org/2860573004/diff/20001/components/offline_page... components/offline_pages/core/cached_offline_page_utils_unittest.cc:138: std::unique_ptr<CachedOfflinePageTestModel> model_; I think this doesn't need to be a unique_ptr, just a CachedOfflinePageTestModel object. https://codereview.chromium.org/2860573004/diff/20001/components/offline_page... components/offline_pages/core/cached_offline_page_utils_unittest.cc:139: std::unique_ptr<base::SimpleTestClock> clock_; same, don't think unique_ptr is necessary. https://codereview.chromium.org/2860573004/diff/20001/components/offline_page... components/offline_pages/core/cached_offline_page_utils_unittest.cc:165: ASSERT_EQ(4UL, GetAllPages().size()); ASSERT in SetUp seems strange to me. https://codereview.chromium.org/2860573004/diff/20001/components/offline_page... components/offline_pages/core/cached_offline_page_utils_unittest.cc:169: model_.reset(); Don't think this is necessary. https://codereview.chromium.org/2860573004/diff/20001/components/offline_page... components/offline_pages/core/cached_offline_page_utils_unittest.cc:192: void CachedOfflinePageUtilsTest::OnSizeCalculated(int64_t size) { why do you have multiple paradigms for the callbacks (one uses |storage| and the other uses member variables) https://codereview.chromium.org/2860573004/diff/20001/components/offline_page... components/offline_pages/core/cached_offline_page_utils_unittest.cc:202: TEST_F(CachedOfflinePageUtilsTest, TestGetCachedOfflinePageSizeBetween) { nit: moar tests plz, eg: * no pages in model * no pages in range * all pages in range * invalid range https://codereview.chromium.org/2860573004/diff/20001/components/offline_page... File components/offline_pages/core/offline_page_types.h (right): https://codereview.chromium.org/2860573004/diff/20001/components/offline_page... components/offline_pages/core/offline_page_types.h:80: typedef base::Callback<void(int64_t)> SizeCalculatedCallback; nit: SizeInBytesCallback?
Just wanted to reply with these quickly. Will make another pass on the next patch set. https://codereview.chromium.org/2860573004/diff/20001/components/offline_page... File components/offline_pages/core/cached_offline_page_utils_unittest.cc (right): https://codereview.chromium.org/2860573004/diff/20001/components/offline_page... components/offline_pages/core/cached_offline_page_utils_unittest.cc:164: base::Time::Max()); Time::Max is not a good time for an entry; it should be a "normal"/valid time instead. Especially now that the upper limit is open this time would never be a match. https://codereview.chromium.org/2860573004/diff/20001/components/offline_page... components/offline_pages/core/cached_offline_page_utils_unittest.cc:165: ASSERT_EQ(4UL, GetAllPages().size()); On 2017/05/03 21:40:51, dewittj wrote: > ASSERT in SetUp seems strange to me. I disagree. This is testing an assumption that all tests using this data set depend on to work properly.
I've moved them into offline_page_utils, which seems a better place than a separate file. Also added and moved tests. Most of the commented codes in the test file were removed. PTAL, thanks! https://codereview.chromium.org/2860573004/diff/20001/components/offline_page... File components/offline_pages/core/cached_offline_page_utils.h (right): https://codereview.chromium.org/2860573004/diff/20001/components/offline_page... components/offline_pages/core/cached_offline_page_utils.h:23: void GetCachedOfflinePageSizeBetween(OfflinePageModel* model, On 2017/05/03 21:40:51, dewittj wrote: > It might be better to move this to //components/offline_pages/content or > //chrome/browser and to accept a browsercontext, so that the cache controller > doesn't need to know about the OfflinePageModelFactory. Done. I'm adding these into offline_page_utils then, since it looks like a better place. https://codereview.chromium.org/2860573004/diff/20001/components/offline_page... components/offline_pages/core/cached_offline_page_utils.h:25: const base::Time& begin_time, On 2017/05/03 21:40:51, dewittj wrote: > nit: #include "base/time/time.h" I thought a forward declaration would be enough? https://codereview.chromium.org/2860573004/diff/20001/components/offline_page... File components/offline_pages/core/cached_offline_page_utils_unittest.cc (right): https://codereview.chromium.org/2860573004/diff/20001/components/offline_page... components/offline_pages/core/cached_offline_page_utils_unittest.cc:45: class CachedOfflinePageTestModel : public StubOfflinePageModel { On 2017/05/03 21:40:51, dewittj wrote: > no action required: I think we are overdue for a nicer fake model. Acknowledged. https://codereview.chromium.org/2860573004/diff/20001/components/offline_page... components/offline_pages/core/cached_offline_page_utils_unittest.cc:138: std::unique_ptr<CachedOfflinePageTestModel> model_; On 2017/05/03 21:40:53, dewittj wrote: > I think this doesn't need to be a unique_ptr, just a CachedOfflinePageTestModel > object. Done. https://codereview.chromium.org/2860573004/diff/20001/components/offline_page... components/offline_pages/core/cached_offline_page_utils_unittest.cc:139: std::unique_ptr<base::SimpleTestClock> clock_; On 2017/05/03 21:40:53, dewittj wrote: > same, don't think unique_ptr is necessary. Done. https://codereview.chromium.org/2860573004/diff/20001/components/offline_page... components/offline_pages/core/cached_offline_page_utils_unittest.cc:164: base::Time::Max()); On 2017/05/03 21:54:35, carlosk wrote: > Time::Max is not a good time for an entry; it should be a "normal"/valid time > instead. Especially now that the upper limit is open this time would never be a > match. Done. https://codereview.chromium.org/2860573004/diff/20001/components/offline_page... components/offline_pages/core/cached_offline_page_utils_unittest.cc:165: ASSERT_EQ(4UL, GetAllPages().size()); On 2017/05/03 21:54:35, carlosk wrote: > On 2017/05/03 21:40:51, dewittj wrote: > > ASSERT in SetUp seems strange to me. > > I disagree. This is testing an assumption that all tests using this data set > depend on to work properly. THere's no easy way (and no one did) to get all pages in a sync way (in this file). To keep with tradition, i'm removing this assert. https://codereview.chromium.org/2860573004/diff/20001/components/offline_page... components/offline_pages/core/cached_offline_page_utils_unittest.cc:169: model_.reset(); On 2017/05/03 21:40:52, dewittj wrote: > Don't think this is necessary. Done. https://codereview.chromium.org/2860573004/diff/20001/components/offline_page... components/offline_pages/core/cached_offline_page_utils_unittest.cc:192: void CachedOfflinePageUtilsTest::OnSizeCalculated(int64_t size) { On 2017/05/03 21:40:52, dewittj wrote: > why do you have multiple paradigms for the callbacks (one uses |storage| and the > other uses member variables) Done. https://codereview.chromium.org/2860573004/diff/20001/components/offline_page... components/offline_pages/core/cached_offline_page_utils_unittest.cc:202: TEST_F(CachedOfflinePageUtilsTest, TestGetCachedOfflinePageSizeBetween) { On 2017/05/03 21:40:51, dewittj wrote: > nit: moar tests plz, eg: > * no pages in model > * no pages in range > * all pages in range > * invalid range Done. https://codereview.chromium.org/2860573004/diff/20001/components/offline_page... File components/offline_pages/core/offline_page_types.h (right): https://codereview.chromium.org/2860573004/diff/20001/components/offline_page... components/offline_pages/core/offline_page_types.h:80: typedef base::Callback<void(int64_t)> SizeCalculatedCallback; On 2017/05/03 21:40:53, dewittj wrote: > nit: SizeInBytesCallback? Done.
The CQ bit was checked by romax@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_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 dullweber@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...
Description was changed from ========== [Offline Pages] Add cached offline page utils and show usage in settings. Added some utility functions for cached offline pages: Get sizes of all cached offline pages; Delete all cached offline pages. Also added the usage of cached offline pages to the number shown at: Chrome Settings -> Privacy -> Clear Browsing Data -> Cache BUG=716142 ========== to ========== [Offline Pages] Add cached offline page utils and show usage in settings. Added some utility functions for cached offline pages: Get sizes of all cached offline pages; Delete all cached offline pages. Also added the usage of cached offline pages to the number shown at: Chrome Settings -> Privacy -> Clear Browsing Data -> Cache BUG=716142 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
Thanks. https://codereview.chromium.org/2860573004/diff/80001/chrome/browser/android/... File chrome/browser/android/offline_pages/offline_page_utils.cc (right): https://codereview.chromium.org/2860573004/diff/80001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_utils.cc:140: total_size += page.file_size; The interval here should be closed-open, where the start time is included and the end time is excluded. This is so that sequential intervals are both a) continuous and b) don't overlap. So the begin time comparison should use <= (closed limit) and the end time should use < (open limit). This might sound like nit picking but in the not so uncommon case of using "exact" times (like April 4th 2016, 00:00:00) for both entries and search ranges, then not being careful about this matter could cause entries to either not show up or appear twice in sequential ranges. https://codereview.chromium.org/2860573004/diff/80001/chrome/browser/android/... File chrome/browser/android/offline_pages/offline_page_utils_unittest.cc (right): https://codereview.chromium.org/2860573004/diff/80001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_utils_unittest.cc:367: static_cast<OfflinePageModelImpl*>(model)->set_testing_clock(&clock); Can these 3 initialization steps above that are repeated in every test be part of SetUp? https://codereview.chromium.org/2860573004/diff/80001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_utils_unittest.cc:385: // The clock will be at 03:00:00 after adding pages. This comment doesn't apply for this test and neither do the times mentioned in the following comments. https://codereview.chromium.org/2860573004/diff/80001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_utils_unittest.cc:470: Another test to add would have period limits that match entries' times so to test how those edge cases are handled.
addressed more comments from carlosk@. Figured out why the tests are flaky but blocked on getting a way to fix with dullweber@. Hopefully will have a fix soon. https://codereview.chromium.org/2860573004/diff/80001/chrome/browser/android/... File chrome/browser/android/offline_pages/offline_page_utils.cc (right): https://codereview.chromium.org/2860573004/diff/80001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_utils.cc:140: total_size += page.file_size; On 2017/05/05 18:20:27, carlosk wrote: > The interval here should be closed-open, where the start time is included and > the end time is excluded. This is so that sequential intervals are both a) > continuous and b) don't overlap. So the begin time comparison should use <= > (closed limit) and the end time should use < (open limit). > > This might sound like nit picking but in the not so uncommon case of using > "exact" times (like April 4th 2016, 00:00:00) for both entries and search > ranges, then not being careful about this matter could cause entries to either > not show up or appear twice in sequential ranges. Done. https://codereview.chromium.org/2860573004/diff/80001/chrome/browser/android/... File chrome/browser/android/offline_pages/offline_page_utils_unittest.cc (right): https://codereview.chromium.org/2860573004/diff/80001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_utils_unittest.cc:367: static_cast<OfflinePageModelImpl*>(model)->set_testing_clock(&clock); On 2017/05/05 18:20:27, carlosk wrote: > Can these 3 initialization steps above that are repeated in every test be part > of SetUp? I thought about it and I prefer not to touch existing tests by adding more pages into the model. Also these pages are only used to test these test cases so I prefer not to have them in SetUp(). https://codereview.chromium.org/2860573004/diff/80001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_utils_unittest.cc:385: // The clock will be at 03:00:00 after adding pages. On 2017/05/05 18:20:27, carlosk wrote: > This comment doesn't apply for this test and neither do the times mentioned in > the following comments. Done. https://codereview.chromium.org/2860573004/diff/80001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_utils_unittest.cc:470: On 2017/05/05 18:20:27, carlosk wrote: > Another test to add would have period limits that match entries' times so to > test how those edge cases are handled. Done.
LGTM. https://codereview.chromium.org/2860573004/diff/80001/chrome/browser/android/... File chrome/browser/android/offline_pages/offline_page_utils_unittest.cc (right): https://codereview.chromium.org/2860573004/diff/80001/chrome/browser/android/... chrome/browser/android/offline_pages/offline_page_utils_unittest.cc:367: static_cast<OfflinePageModelImpl*>(model)->set_testing_clock(&clock); On 2017/05/05 21:01:30, romax wrote: > On 2017/05/05 18:20:27, carlosk wrote: > > Can these 3 initialization steps above that are repeated in every test be part > > of SetUp? > > I thought about it and I prefer not to touch existing tests by adding more pages > into the model. Also these pages are only used to test these test cases so I > prefer not to have them in SetUp(). As discussed offline this would not include page creation but would set the mock clock for all tests. I'm OK with leaving as is.
The CQ bit was checked by romax@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...
romax@chromium.org changed reviewers: + msramek@chromium.org
+msramek for cache_counter owner review also friendly ping for dewittj@, since the cache counter fix has landed and unblocked this CL.
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_...) win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...) win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...) win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
Thanks for lots of tests! I'm not sure the size calculation really needs to be in chrome/browser/android since it only depends on components layer things. But maybe that's for later when we start trying to support non-android? https://codereview.chromium.org/2860573004/diff/120001/chrome/browser/browsin... File chrome/browser/browsing_data/cache_counter.cc (right): https://codereview.chromium.org/2860573004/diff/120001/chrome/browser/browsin... chrome/browser/browsing_data/cache_counter.cc:50: void CacheCounter::OnCacheSizeCalculated(int64_t cache_bytes, nit: rename to OnBrowsingDataCacheSizeCalculated https://codereview.chromium.org/2860573004/diff/120001/chrome/browser/browsin... chrome/browser/browsing_data/cache_counter.cc:54: return; It's not clear that we should return here anymore. I'm also not clear what the comment above the if is supposed to mean, can you explain? https://codereview.chromium.org/2860573004/diff/120001/chrome/browser/browsin... chrome/browser/browsing_data/cache_counter.cc:71: #if defined(OS_ANDROID) I'd not make this android-only, and just call it with 0 offline page bytes at the end of OnCacheSizeCalculated, removing the ReporrtResult duplication. https://codereview.chromium.org/2860573004/diff/120001/chrome/browser/browsin... chrome/browser/browsing_data/cache_counter.cc:73: int64_t cache_bytes, rename to cached_browsing_data_bytes https://codereview.chromium.org/2860573004/diff/120001/chrome/browser/browsin... chrome/browser/browsing_data/cache_counter.cc:74: bool is_upper_limit, do we need to compute the upper limit? Why is this passthrough?
https://codereview.chromium.org/2860573004/diff/120001/chrome/browser/browsin... File chrome/browser/browsing_data/cache_counter.cc (right): https://codereview.chromium.org/2860573004/diff/120001/chrome/browser/browsin... chrome/browser/browsing_data/cache_counter.cc:57: if (offline_pages::OfflinePageUtils::GetCachedOfflinePageSizeBetween( Could you start both cache calculations in parallel? Then you wouldn't have to pass the cache_bytes and is_upper_limit through your function. Both functions could have the same on_cache_size_calculated(size, upperlimit) callback, which accumulates the size and tracks if both tasks have returned. (upper_limit is used because some browser cache backends can't efficiently determine the size of a subset of the cache, so we just use the total size)
The CQ bit was checked by romax@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 keep the files in chrome/browser/android for now and maybe later I'll move them into components/offline_pages/content. Addressed the comment from dullweber@ which introduced a little refactoring in cache_counter.cc. Now it will reuse the same callback and track the calculated numbers/states using member variable. Since the callbacks are both on UI thread, no threading issue will be introduced. PTAL, thanks! https://codereview.chromium.org/2860573004/diff/120001/chrome/browser/browsin... File chrome/browser/browsing_data/cache_counter.cc (right): https://codereview.chromium.org/2860573004/diff/120001/chrome/browser/browsin... chrome/browser/browsing_data/cache_counter.cc:50: void CacheCounter::OnCacheSizeCalculated(int64_t cache_bytes, On 2017/05/09 01:29:54, dewittj wrote: > nit: rename to OnBrowsingDataCacheSizeCalculated Done. https://codereview.chromium.org/2860573004/diff/120001/chrome/browser/browsin... chrome/browser/browsing_data/cache_counter.cc:54: return; On 2017/05/09 01:29:53, dewittj wrote: > It's not clear that we should return here anymore. I'm also not clear what the > comment above the if is supposed to mean, can you explain? if this value is <0, it means there was an error during counting browsing data cache. In this case, it would just return which will keep a 'Calculating...' string on UI side. I'd still like to return here since there's one part which cannot be counted correctly and whatever number we show will be wrong, so why not just fail the whole counting? https://codereview.chromium.org/2860573004/diff/120001/chrome/browser/browsin... chrome/browser/browsing_data/cache_counter.cc:57: if (offline_pages::OfflinePageUtils::GetCachedOfflinePageSizeBetween( On 2017/05/09 07:56:01, dullweber wrote: > Could you start both cache calculations in parallel? Then you wouldn't have to > pass the cache_bytes and is_upper_limit through your function. > > Both functions could have the same on_cache_size_calculated(size, upperlimit) > callback, which accumulates the size and tracks if both tasks have returned. > > (upper_limit is used because some browser cache backends can't efficiently > determine the size of a subset of the cache, so we just use the total size) Done. But in order to reuse the callback I changed the order of the argument so that the base::Bind can work for the offline page counter. https://codereview.chromium.org/2860573004/diff/120001/chrome/browser/browsin... chrome/browser/browsing_data/cache_counter.cc:71: #if defined(OS_ANDROID) On 2017/05/09 01:29:53, dewittj wrote: > I'd not make this android-only, and just call it with 0 offline page bytes at > the end of OnCacheSizeCalculated, removing the ReporrtResult duplication. Done. https://codereview.chromium.org/2860573004/diff/120001/chrome/browser/browsin... chrome/browser/browsing_data/cache_counter.cc:73: int64_t cache_bytes, On 2017/05/09 01:29:53, dewittj wrote: > rename to cached_browsing_data_bytes Done. https://codereview.chromium.org/2860573004/diff/120001/chrome/browser/browsin... chrome/browser/browsing_data/cache_counter.cc:74: bool is_upper_limit, On 2017/05/09 01:29:54, dewittj wrote: > do we need to compute the upper limit? Why is this passthrough? we don't need to compute it (or it can be always false). It is only used by the browsing data cache for the reason described by dullweber@ above. This is passing through because the place where ReportResult is handled will have a special handling if is_upper_limit is true.
I'll keep the files in chrome/browser/android for now and maybe later I'll move them into components/offline_pages/content. Addressed the comment from dullweber@ which introduced a little refactoring in cache_counter.cc. Now it will reuse the same callback and track the calculated numbers/states using member variable. Since the callbacks are both on UI thread, no threading issue will be introduced. PTAL, thanks!
The CQ bit was checked by romax@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...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by romax@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...)
thanks, browsing_data/ lgtm but I can't approve it ;) https://codereview.chromium.org/2860573004/diff/160001/chrome/browser/browsin... File chrome/browser/browsing_data/cache_counter.cc (right): https://codereview.chromium.org/2860573004/diff/160001/chrome/browser/browsin... chrome/browser/browsing_data/cache_counter.cc:71: is_upper_limit_ = is_upper_limit_ || is_upper_limit; you could use |= here
The CQ bit was checked by romax@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: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by romax@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_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by romax@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, with a nit. https://codereview.chromium.org/2860573004/diff/160001/chrome/browser/browsin... File chrome/browser/browsing_data/cache_counter.cc (right): https://codereview.chromium.org/2860573004/diff/160001/chrome/browser/browsin... chrome/browser/browsing_data/cache_counter.cc:65: // A value less than 0 means a net error code. nit: add a DCHECK(pending_sources_ > 0)
romax@chromium.org changed reviewers: + dbeam@chromium.org
+dbeam@: Please review changes in c/b/ui/ Thanks! https://codereview.chromium.org/2860573004/diff/160001/chrome/browser/browsin... File chrome/browser/browsing_data/cache_counter.cc (right): https://codereview.chromium.org/2860573004/diff/160001/chrome/browser/browsin... chrome/browser/browsing_data/cache_counter.cc:71: is_upper_limit_ = is_upper_limit_ || is_upper_limit; On 2017/05/10 08:13:40, dullweber wrote: > you could use |= here Done.
LGTM. It's a bit strange for a method to return the secondary value (is_upper_limit) first, and the primary value second, but given that CacheCounter is the primary consumer that needs it in that order, I'm fine with leaving it. https://codereview.chromium.org/2860573004/diff/160001/chrome/browser/browsin... File chrome/browser/browsing_data/cache_counter.cc (right): https://codereview.chromium.org/2860573004/diff/160001/chrome/browser/browsin... chrome/browser/browsing_data/cache_counter.cc:71: is_upper_limit_ = is_upper_limit_ || is_upper_limit; On 2017/05/12 00:44:37, romax wrote: > On 2017/05/10 08:13:40, dullweber wrote: > > you could use |= here > > Done. I have a vague memory that using bitwise OR with logical values might be against the style guide, but I can't find it. But it should work correctly in C++, and disjunction in particular would work correctly even with ints, so I guess we can leave it as is.
c/b/ui/webui/options/ lgtm
On 2017/05/16 03:10:23, Dan Beam wrote: > c/b/ui/webui/options/ lgtm * and /settings/ ;)
The CQ bit was checked by romax@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from carlosk@chromium.org, dullweber@chromium.org Link to the patchset: https://codereview.chromium.org/2860573004/#ps200001 (title: "address missed comment")
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: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by romax@chromium.org
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": 200001, "attempt_start_ts": 1494978081409820,
"parent_rev": "07c2492c1970b0bca13093879d1681932c7976fa", "commit_rev":
"444db4b416dbc2ed7fed540cfbc0d6ec7774ed65"}
Message was sent while issue was closed.
Description was changed from ========== [Offline Pages] Add cached offline page utils and show usage in settings. Added some utility functions for cached offline pages: Get sizes of all cached offline pages; Delete all cached offline pages. Also added the usage of cached offline pages to the number shown at: Chrome Settings -> Privacy -> Clear Browsing Data -> Cache BUG=716142 ========== to ========== [Offline Pages] Add cached offline page utils and show usage in settings. Added some utility functions for cached offline pages: Get sizes of all cached offline pages; Delete all cached offline pages. Also added the usage of cached offline pages to the number shown at: Chrome Settings -> Privacy -> Clear Browsing Data -> Cache BUG=716142 Review-Url: https://codereview.chromium.org/2860573004 Cr-Commit-Position: refs/heads/master@{#472255} Committed: https://chromium.googlesource.com/chromium/src/+/444db4b416dbc2ed7fed540cfbc0... ==========
Message was sent while issue was closed.
Committed patchset #11 (id:200001) as https://chromium.googlesource.com/chromium/src/+/444db4b416dbc2ed7fed540cfbc0... |
