Refactor cache counting into a separate helper class

Refactor cache counting code as a preparation for a more exact implementation.

Move code from StoragePartitionHttpCacheRemover to CacheCountingHelper. Improve tests by adding a CacheTestUtil class for common functionality of cache counting and cache deletion tests.

BUG=671196
TBR=achuith@chromium.org

Committed: https://crrev.com/75321ac0828dd8f7dd38944234b867ea98598656
Cr-Commit-Position: refs/heads/master@{#441399}

[dullweber, 2016-12-08 08:45:45]

The CQ bit was checked by dullweber@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-08 08:46:01]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-08 09:02:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-08 09:02:28]

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-...)

[dullweber, 2016-12-08 09:21:09]

Description was changed from

==========
WIP!
Currently cache size in "Clear browsing data" dialog is shown as "Less than X"
when selecting a time range, where X is the total size of the cache.
To fix this, count the cache size depending on chosen time range. The cache can
be very big and we want a responsive ui, so if counting the cache takes too long
(more than 1s) show the estimate instead.

BUG=671196
==========

to

==========
WIP!
Currently cache size in "Clear browsing data" dialog is shown as "Less than X"
when selecting a time range, where X is the total size of the cache.
To fix this, count the cache size depending on chosen time range. The cache can
be very big and we want a responsive ui, so if counting the cache takes too long
(more than 1s) show the estimate instead.

BUG=671196
==========

[dullweber, 2016-12-08 12:55:15]

The CQ bit was checked by dullweber@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-08 12:55:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-08 13:13:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-08 13:13:44]

Dry run: Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)

[dullweber, 2016-12-08 14:54:31]

The CQ bit was checked by dullweber@chromium.org to run a CQ dry run

[dullweber, 2016-12-08 14:54:56]

Description was changed from

==========
WIP!
Currently cache size in "Clear browsing data" dialog is shown as "Less than X"
when selecting a time range, where X is the total size of the cache.
To fix this, count the cache size depending on chosen time range. The cache can
be very big and we want a responsive ui, so if counting the cache takes too long
(more than 1s) show the estimate instead.

BUG=671196
==========

to

==========
WIP!
Currently cache size in "Clear browsing data" dialog is shown as "Less than X"
when selecting a time range (X is the total size of the cache).
To fix this, I count the cache size depending on a chosen time range. 
The cache can be very big and we want a responsive ui, so if counting the cache
takes too long (currently more than 1s) I show the upper estimate instead.

As a future improvement I would like to propose a specialized version for
SimpleBackendImpl. It could determine the size of a subset of the cache from the
EntryMetadata stored in memory. This should be fast enough for any cache size.

BUG=671196
==========

[commit-bot: I haz the power, 2016-12-08 14:55:00]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-08 15:13:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-08 15:13:39]

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-...)

[dullweber, 2016-12-08 15:54:33]

The CQ bit was checked by dullweber@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-08 15:54:49]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dullweber, 2016-12-08 15:57:01]

The CQ bit was checked by dullweber@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-08 15:57:16]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-08 16:57:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-08 16:57:36]

Dry run: This issue passed the CQ dry run.

[dullweber, 2016-12-09 10:43:57]

dullweber@chromium.org changed reviewers:
+ msramek@chromium.org

[dullweber, 2016-12-09 10:43:58]

Martin, please have a look at this CL. What do you think is a good value for the
timeout after which we return the estimate and how should it be configured?

[dullweber, 2016-12-09 10:45:18]

Description was changed from

==========
WIP!
Currently cache size in "Clear browsing data" dialog is shown as "Less than X"
when selecting a time range (X is the total size of the cache).
To fix this, I count the cache size depending on a chosen time range. 
The cache can be very big and we want a responsive ui, so if counting the cache
takes too long (currently more than 1s) I show the upper estimate instead.

As a future improvement I would like to propose a specialized version for
SimpleBackendImpl. It could determine the size of a subset of the cache from the
EntryMetadata stored in memory. This should be fast enough for any cache size.

BUG=671196
==========

to

==========
Currently cache size in "Clear browsing data" dialog is shown as "Less than X"
when selecting a time range (Where X is the total size of the cache).
To fix this, I count the cache size depending on a chosen time range. 
The cache can be very big and we want a responsive ui, so if counting the cache
takes too long I show the upper estimate instead.

As a future improvement I would like to propose a specialized version for
SimpleBackendImpl. It could determine the size of a subset of the cache from the
EntryMetadata stored in memory. This should be fast enough for any cache size.

BUG=671196
==========

[msramek, 2016-12-09 13:00:50]

On 2016/12/09 10:43:58, dullweber wrote:
> Martin, please have a look at this CL. What do you think is a good value for
the
> timeout after which we return the estimate and how should it be configured?

StoragePartitionHttpCacheDataRemover should just perform the requested task
without making judgements on how long it will take. It already contains
time-range-based deletion, which can take even longer than counting, and there
is no timeout handling; the counting code should mirror that.

CacheCounter is the class whose responsibility is to provide an answer that the
UI si happy with. Therefore, it should be the CacheCounter that handles the
timing and compares the different answer.

Therefore, CacheCounter should do:
- if period is all time:
  - return CalculateSizeOfAllEntries
- else
  - run the timer
  - run StoragePartitionHttpCacheDataRemover
  - run CalculateSizeOfAllEntries
  - return the answer according to the timing of the callbacks

Note that this design will also mean you won't have to make changes to the
chromeos/ storage code.

As an estimate, I would use 3-5 seconds. We can discuss the exact timeout with
Max.

[dullweber, 2016-12-09 13:46:15]

The CQ bit was checked by dullweber@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-09 13:46:37]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-09 14:01:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-09 14:01:54]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[dullweber, 2016-12-09 15:20:58]

The CQ bit was checked by dullweber@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-09 15:21:15]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-09 16:15:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-09 16:15:41]

Dry run: This issue passed the CQ dry run.

[dullweber, 2016-12-12 15:18:22]

The CQ bit was checked by dullweber@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-12 15:18:39]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-12 16:29:11]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-12 16:29:12]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[dullweber, 2016-12-13 10:41:43]

The CQ bit was checked by dullweber@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-13 10:42:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-13 12:02:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-13 12:02:30]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[dullweber, 2016-12-14 09:42:34]

The CQ bit was checked by dullweber@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-14 09:42:45]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dullweber, 2016-12-14 10:53:41]

The CQ bit was checked by dullweber@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-14 10:53:50]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-14 12:10:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-14 12:10:08]

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_...)

[dullweber, 2016-12-14 12:37:01]

The CQ bit was checked by dullweber@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-14 12:37:18]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-14 15:03:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-14 15:03:17]

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_...)

[dullweber, 2016-12-15 13:27:01]

The CQ bit was checked by dullweber@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-15 13:27:18]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dullweber, 2016-12-15 14:34:57]

On 2016/12/09 13:00:50, msramek wrote:
> On 2016/12/09 10:43:58, dullweber wrote:
> > Martin, please have a look at this CL. What do you think is a good value for
> the
> > timeout after which we return the estimate and how should it be configured?
> 
> StoragePartitionHttpCacheDataRemover should just perform the requested task
> without making judgements on how long it will take. It already contains
> time-range-based deletion, which can take even longer than counting, and there
> is no timeout handling; the counting code should mirror that.
> 
> CacheCounter is the class whose responsibility is to provide an answer that
the
> UI si happy with. Therefore, it should be the CacheCounter that handles the
> timing and compares the different answer.
> 
> Therefore, CacheCounter should do:
> - if period is all time:
>   - return CalculateSizeOfAllEntries
> - else
>   - run the timer
>   - run StoragePartitionHttpCacheDataRemover
>   - run CalculateSizeOfAllEntries
>   - return the answer according to the timing of the callbacks
> 
> Note that this design will also mean you won't have to make changes to the
> chromeos/ storage code.
> 
> As an estimate, I would use 3-5 seconds. We can discuss the exact timeout with
> Max.

I changed it to work this way and fixed all bugs that caused failing unittests.
I also moved the counting code from StoragePartitionHttpCacheDataRemover to a
new class "ConditionalCacheCountingHelper".

[commit-bot: I haz the power, 2016-12-15 14:54:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-15 14:54:42]

Dry run: This issue passed the CQ dry run.

[msramek, 2016-12-15 15:58:51]

Very nice!

The architecture looks good now; I still have some polishing comments though.

https://codereview.chromium.org/2556363003/diff/300001/chrome/browser/browsin...
File chrome/browser/browsing_data/cache_counter.cc (right):

https://codereview.chromium.org/2556363003/diff/300001/chrome/browser/browsin...
chrome/browser/browsing_data/cache_counter.cc:12: using content::BrowserThread;
style nit: Please use namespaces explicitly in non-test files when possible.

https://codereview.chromium.org/2556363003/diff/300001/chrome/browser/browsin...
chrome/browser/browsing_data/cache_counter.cc:15:
CacheCounter::CacheResult::CacheResult(const CacheCounter* source,
style nit: please keep the ":" from initializers under the first letter of the
first parameter; that is, break the parameters on the next line

https://codereview.chromium.org/2556363003/diff/300001/chrome/browser/browsin...
chrome/browser/browsing_data/cache_counter.cc:44: // Counting a subset of the
cache can take a long time. If it doesn't succeed
nit: The following block is wrapped in GetPeriodStart().is_null(); please
explain that the counting doesn't take long time in that case.

https://codereview.chromium.org/2556363003/diff/300001/chrome/browser/browsin...
chrome/browser/browsing_data/cache_counter.cc:48: base::TimeDelta
cache_counter_timeout = base::TimeDelta::FromSeconds(3);
nit: Please pull this up as kCacheCounterTimeout constant in an anonymous
namespace.

https://codereview.chromium.org/2556363003/diff/300001/chrome/browser/browsin...
chrome/browser/browsing_data/cache_counter.cc:69:
weak_ptr_factory_.GetWeakPtr(), true));
|is_upper_limit| is not always true; it depends on whether the time range is
bounded.

https://codereview.chromium.org/2556363003/diff/300001/chrome/browser/browsin...
chrome/browser/browsing_data/cache_counter.cc:77: pending_ = false;
nit: Please re-add the empty line for readability.

https://codereview.chromium.org/2556363003/diff/300001/chrome/browser/browsin...
File chrome/browser/browsing_data/cache_counter_browsertest.cc (right):

https://codereview.chromium.org/2556363003/diff/300001/chrome/browser/browsin...
chrome/browser/browsing_data/cache_counter_browsertest.cc:255: // Tests that the
counting is restarted when the time period changes.
Can you add tests for the new behavior?

At minimum, we should test |is_upper_limit|.

Ideally, the test should also be able to inject a smaller time limit and test
the switch from exact computation to the estimate.

https://codereview.chromium.org/2556363003/diff/300001/chrome/browser/browsin...
File
chrome/browser/browsing_data/conditional_cache_counting_helper_browsertest.cc
(right):

https://codereview.chromium.org/2556363003/diff/300001/chrome/browser/browsin...
chrome/browser/browsing_data/conditional_cache_counting_helper_browsertest.cc:28:
class ConditionalCacheCountingHelperBrowserTest : public InProcessBrowserTest {
Optional: I wonder if we could extract the common parts (mainly all the helper
functions) from this and CCDH browsertest to something like a
cache_iteration_test_utils.h file.

https://codereview.chromium.org/2556363003/diff/300001/chrome/browser/browsin...
chrome/browser/browsing_data/conditional_cache_counting_helper_browsertest.cc:32:
const int64_t kTimeoutMs = 1;
Let's try 10ms. The CCDH browsertest got a lot of attention from sheriffs for
being flaky due to a short timeout.

https://codereview.chromium.org/2556363003/diff/300001/chrome/browser/browsin...
chrome/browser/browsing_data/conditional_cache_counting_helper_browsertest.cc:158:
// Miscellaneous. ------------------------------------------------------------
This section looks empty :)

https://codereview.chromium.org/2556363003/diff/300001/chrome/browser/browsin...
chrome/browser/browsing_data/conditional_cache_counting_helper_browsertest.cc:175:
CountingWithTimeout) {
What does "Timeout" refer to?

https://codereview.chromium.org/2556363003/diff/300001/components/browsing_da...
File components/browsing_data/content/conditional_cache_counting_helper.cc
(right):

https://codereview.chromium.org/2556363003/diff/300001/components/browsing_da...
components/browsing_data/content/conditional_cache_counting_helper.cc:151: //
TODO(dullweber): Implement faster counting for SimpleBackendImpl.
You already implemented it in this CL, just didn't call it from here. To keep
the CL cleaner, I suggest either:

a) resolving this TODO immediately
b) splitting up the cache changes to a separate CL

I recommend b), as smaller CLs are easier and faster to review.

https://codereview.chromium.org/2556363003/diff/300001/components/browsing_da...
components/browsing_data/content/conditional_cache_counting_helper.cc:168:
default: {
Instead of default: NOTREACHED(), we could just remove default entirely. The
compiler will then check that all values are listed in the switch.

https://codereview.chromium.org/2556363003/diff/300001/components/browsing_da...
components/browsing_data/content/conditional_cache_counting_helper.cc:190:
iterator_.reset(nullptr);
nit: reset()

https://codereview.chromium.org/2556363003/diff/300001/components/browsing_da...
File components/browsing_data/content/conditional_cache_counting_helper.h
(right):

https://codereview.chromium.org/2556363003/diff/300001/components/browsing_da...
components/browsing_data/content/conditional_cache_counting_helper.h:12:
#include "net/disk_cache/disk_cache.h"
nit: unnecessary include

https://codereview.chromium.org/2556363003/diff/300001/components/browsing_da...
File
components/browsing_data/content/storage_partition_http_cache_data_remover.h
(right):

https://codereview.chromium.org/2556363003/diff/300001/components/browsing_da...
components/browsing_data/content/storage_partition_http_cache_data_remover.h:59:
STATE_PROCESS_MAIN,
Optional: This was originally called STATE_DELETE_MAIN, STATE_DELETE_MEDIA,
before I added the counting mechanism. You could rename it back so that the name
is more explicit, and also rename it in CCCH into STATE_COUNT_MAIN,
STATE_COUNT_MEDIA.

[dullweber, 2016-12-16 16:39:06]

The CQ bit was checked by dullweber@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-16 16:39:39]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dullweber, 2016-12-16 16:41:11]

Thanks for the comments. I extracted the common methods from the browser_tests
into a util class that can create and retrieve cache entries by key.

https://codereview.chromium.org/2556363003/diff/300001/chrome/browser/browsin...
File chrome/browser/browsing_data/cache_counter.cc (right):

https://codereview.chromium.org/2556363003/diff/300001/chrome/browser/browsin...
chrome/browser/browsing_data/cache_counter.cc:12: using content::BrowserThread;
On 2016/12/15 15:58:50, msramek wrote:
> style nit: Please use namespaces explicitly in non-test files when possible.

Done.

https://codereview.chromium.org/2556363003/diff/300001/chrome/browser/browsin...
chrome/browser/browsing_data/cache_counter.cc:15:
CacheCounter::CacheResult::CacheResult(const CacheCounter* source,
On 2016/12/15 15:58:50, msramek wrote:
> style nit: please keep the ":" from initializers under the first letter of the
> first parameter; that is, break the parameters on the next line

clangformat doesn't let me do this.

https://codereview.chromium.org/2556363003/diff/300001/chrome/browser/browsin...
chrome/browser/browsing_data/cache_counter.cc:44: // Counting a subset of the
cache can take a long time. If it doesn't succeed
On 2016/12/15 15:58:50, msramek wrote:
> nit: The following block is wrapped in GetPeriodStart().is_null(); please
> explain that the counting doesn't take long time in that case.

Done.

https://codereview.chromium.org/2556363003/diff/300001/chrome/browser/browsin...
chrome/browser/browsing_data/cache_counter.cc:48: base::TimeDelta
cache_counter_timeout = base::TimeDelta::FromSeconds(3);
On 2016/12/15 15:58:50, msramek wrote:
> nit: Please pull this up as kCacheCounterTimeout constant in an anonymous
> namespace.

Done.

https://codereview.chromium.org/2556363003/diff/300001/chrome/browser/browsin...
chrome/browser/browsing_data/cache_counter.cc:69:
weak_ptr_factory_.GetWeakPtr(), true));
On 2016/12/15 15:58:50, msramek wrote:
> |is_upper_limit| is not always true; it depends on whether the time range is
> bounded.

This code is only executed if the timer finishes before the counter is responds
for a restricted time range, so this result is always an estimate.

https://codereview.chromium.org/2556363003/diff/300001/chrome/browser/browsin...
chrome/browser/browsing_data/cache_counter.cc:77: pending_ = false;
On 2016/12/15 15:58:50, msramek wrote:
> nit: Please re-add the empty line for readability.

Done.

https://codereview.chromium.org/2556363003/diff/300001/chrome/browser/browsin...
File chrome/browser/browsing_data/cache_counter_browsertest.cc (right):

https://codereview.chromium.org/2556363003/diff/300001/chrome/browser/browsin...
chrome/browser/browsing_data/cache_counter_browsertest.cc:255: // Tests that the
counting is restarted when the time period changes.
On 2016/12/15 15:58:50, msramek wrote:
> Can you add tests for the new behavior?
> 
> At minimum, we should test |is_upper_limit|.
> 
> Ideally, the test should also be able to inject a smaller time limit and test
> the switch from exact computation to the estimate.

I added a test that sets timeout to 0 and checks if is_upper_limit is true. I
also tested that is_upper_limit returns false for the other cases.

https://codereview.chromium.org/2556363003/diff/300001/chrome/browser/browsin...
File
chrome/browser/browsing_data/conditional_cache_counting_helper_browsertest.cc
(right):

https://codereview.chromium.org/2556363003/diff/300001/chrome/browser/browsin...
chrome/browser/browsing_data/conditional_cache_counting_helper_browsertest.cc:28:
class ConditionalCacheCountingHelperBrowserTest : public InProcessBrowserTest {
On 2016/12/15 15:58:50, msramek wrote:
> Optional: I wonder if we could extract the common parts (mainly all the helper
> functions) from this and CCDH browsertest to something like a
> cache_iteration_test_utils.h file.

I extracted creating and retrieving cache entries into a separate class

https://codereview.chromium.org/2556363003/diff/300001/chrome/browser/browsin...
chrome/browser/browsing_data/conditional_cache_counting_helper_browsertest.cc:32:
const int64_t kTimeoutMs = 1;
On 2016/12/15 15:58:50, msramek wrote:
> Let's try 10ms. The CCDH browsertest got a lot of attention from sheriffs for
> being flaky due to a short timeout.

Done.

https://codereview.chromium.org/2556363003/diff/300001/chrome/browser/browsin...
chrome/browser/browsing_data/conditional_cache_counting_helper_browsertest.cc:158:
// Miscellaneous. ------------------------------------------------------------
On 2016/12/15 15:58:50, msramek wrote:
> This section looks empty :)

Done.

https://codereview.chromium.org/2556363003/diff/300001/chrome/browser/browsin...
chrome/browser/browsing_data/conditional_cache_counting_helper_browsertest.cc:175:
CountingWithTimeout) {
On 2016/12/15 15:58:50, msramek wrote:
> What does "Timeout" refer to?

It doesn't refer to anything anymore, I forgot to remove it.

https://codereview.chromium.org/2556363003/diff/300001/components/browsing_da...
File components/browsing_data/content/conditional_cache_counting_helper.cc
(right):

https://codereview.chromium.org/2556363003/diff/300001/components/browsing_da...
components/browsing_data/content/conditional_cache_counting_helper.cc:151: //
TODO(dullweber): Implement faster counting for SimpleBackendImpl.
On 2016/12/15 15:58:51, msramek wrote:
> You already implemented it in this CL, just didn't call it from here. To keep
> the CL cleaner, I suggest either:
> 
> a) resolving this TODO immediately
> b) splitting up the cache changes to a separate CL
> 
> I recommend b), as smaller CLs are easier and faster to review.

I added the code to get the size of a single entry from the entry but I didn't
add code for the SimpleBackendImpl to calculate its size from the metadata in
the index. I can split the GetEntrySize() code into a separate cl, that

https://codereview.chromium.org/2556363003/diff/300001/components/browsing_da...
components/browsing_data/content/conditional_cache_counting_helper.cc:168:
default: {
On 2016/12/15 15:58:51, msramek wrote:
> Instead of default: NOTREACHED(), we could just remove default entirely. The
> compiler will then check that all values are listed in the switch.

Done.

https://codereview.chromium.org/2556363003/diff/300001/components/browsing_da...
components/browsing_data/content/conditional_cache_counting_helper.cc:190:
iterator_.reset(nullptr);
On 2016/12/15 15:58:50, msramek wrote:
> nit: reset()

Done.

https://codereview.chromium.org/2556363003/diff/300001/components/browsing_da...
File components/browsing_data/content/conditional_cache_counting_helper.h
(right):

https://codereview.chromium.org/2556363003/diff/300001/components/browsing_da...
components/browsing_data/content/conditional_cache_counting_helper.h:12:
#include "net/disk_cache/disk_cache.h"
On 2016/12/15 15:58:51, msramek wrote:
> nit: unnecessary include

The forward declaration of disk_cache::Backend is not needed but I need this
include for std::unique_ptr<disk_cache::Backend::Iterator>. (At least I think
that I can't forward declare an inner class?)

https://codereview.chromium.org/2556363003/diff/300001/components/browsing_da...
File
components/browsing_data/content/storage_partition_http_cache_data_remover.h
(right):

https://codereview.chromium.org/2556363003/diff/300001/components/browsing_da...
components/browsing_data/content/storage_partition_http_cache_data_remover.h:59:
STATE_PROCESS_MAIN,
On 2016/12/15 15:58:51, msramek wrote:
> Optional: This was originally called STATE_DELETE_MAIN, STATE_DELETE_MEDIA,
> before I added the counting mechanism. You could rename it back so that the
name
> is more explicit, and also rename it in CCCH into STATE_COUNT_MAIN,
> STATE_COUNT_MEDIA.

Done.

[commit-bot: I haz the power, 2016-12-16 19:46:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-16 19:46:49]

Dry run: This issue passed the CQ dry run.

[msramek, 2016-12-20 01:03:00]

Thanks for extracting out the util class, I think it helped the readability of
the tests a lot.

We're getting there, but I still have some comments :)

https://codereview.chromium.org/2556363003/diff/300001/chrome/browser/browsin...
File chrome/browser/browsing_data/cache_counter.cc (right):

https://codereview.chromium.org/2556363003/diff/300001/chrome/browser/browsin...
chrome/browser/browsing_data/cache_counter.cc:69:
weak_ptr_factory_.GetWeakPtr(), true));
On 2016/12/16 16:41:10, dullweber wrote:
> On 2016/12/15 15:58:50, msramek wrote:
> > |is_upper_limit| is not always true; it depends on whether the time range is
> > bounded.
> 
> This code is only executed if the timer finishes before the counter is
responds
> for a restricted time range, so this result is always an estimate.

Acknowledged.

https://codereview.chromium.org/2556363003/diff/300001/chrome/browser/browsin...
File
chrome/browser/browsing_data/conditional_cache_counting_helper_browsertest.cc
(right):

https://codereview.chromium.org/2556363003/diff/300001/chrome/browser/browsin...
chrome/browser/browsing_data/conditional_cache_counting_helper_browsertest.cc:28:
class ConditionalCacheCountingHelperBrowserTest : public InProcessBrowserTest {
On 2016/12/16 16:41:10, dullweber wrote:
> On 2016/12/15 15:58:50, msramek wrote:
> > Optional: I wonder if we could extract the common parts (mainly all the
helper
> > functions) from this and CCDH browsertest to something like a
> > cache_iteration_test_utils.h file.
> 
> I extracted creating and retrieving cache entries into a separate class

Thanks!

https://codereview.chromium.org/2556363003/diff/300001/components/browsing_da...
File components/browsing_data/content/conditional_cache_counting_helper.cc
(right):

https://codereview.chromium.org/2556363003/diff/300001/components/browsing_da...
components/browsing_data/content/conditional_cache_counting_helper.cc:151: //
TODO(dullweber): Implement faster counting for SimpleBackendImpl.
On 2016/12/16 16:41:11, dullweber wrote:
> On 2016/12/15 15:58:51, msramek wrote:
> > You already implemented it in this CL, just didn't call it from here. To
keep
> > the CL cleaner, I suggest either:
> > 
> > a) resolving this TODO immediately
> > b) splitting up the cache changes to a separate CL
> > 
> > I recommend b), as smaller CLs are easier and faster to review.
> 
> I added the code to get the size of a single entry from the entry but I didn't
> add code for the SimpleBackendImpl to calculate its size from the metadata in
> the index. I can split the GetEntrySize() code into a separate cl, that 

You seem not to have finished that sentence; however, I agree that a separate CL
would be better.

https://codereview.chromium.org/2556363003/diff/300001/components/browsing_da...
File components/browsing_data/content/conditional_cache_counting_helper.h
(right):

https://codereview.chromium.org/2556363003/diff/300001/components/browsing_da...
components/browsing_data/content/conditional_cache_counting_helper.h:12:
#include "net/disk_cache/disk_cache.h"
On 2016/12/16 16:41:11, dullweber wrote:
> On 2016/12/15 15:58:51, msramek wrote:
> > nit: unnecessary include
> 
> The forward declaration of disk_cache::Backend is not needed but I need this
> include for std::unique_ptr<disk_cache::Backend::Iterator>. (At least I think
> that I can't forward declare an inner class?)

Acknowledged. I didn't notice that; yes, AFAIK you can't forward declare an
inner class.

https://codereview.chromium.org/2556363003/diff/320001/chrome/browser/browsin...
File chrome/browser/browsing_data/cache_test_util.cc (right):

https://codereview.chromium.org/2556363003/diff/320001/chrome/browser/browsin...
chrome/browser/browsing_data/cache_test_util.cc:18: void
CacheTestUtil::SetUpOnMainThread() {
Couldn't this method's body be merged into the constructor and the following
one's into the destructor?

The SetUp() and TearDown() methods are used by tests so that they can be
inherited and specialized; however, this is not a test. If it's held by the test
in a std::unique_ptr, you have perfect control over when the constructor and
destructor are run (by calling MakeUnique and reset), so it seems there's no
reason to separate this.

https://codereview.chromium.org/2556363003/diff/320001/chrome/browser/browsin...
chrome/browser/browsing_data/cache_test_util.cc:113: // Check cache content
nit: Period at the end of the sentence.

https://codereview.chromium.org/2556363003/diff/320001/chrome/browser/browsin...
File chrome/browser/browsing_data/cache_test_util.h (right):

https://codereview.chromium.org/2556363003/diff/320001/chrome/browser/browsin...
chrome/browser/browsing_data/cache_test_util.h:48: void SetUpOnIOThread();
Ordering: Methods before attributes (if you got inspired for this wrong ordering
from CacheCounter, sorry for that; feel free to fix it there as well).

https://codereview.chromium.org/2556363003/diff/320001/chrome/browser/browsin...
File
chrome/browser/browsing_data/conditional_cache_counting_helper_browsertest.cc
(right):

https://codereview.chromium.org/2556363003/diff/320001/chrome/browser/browsin...
chrome/browser/browsing_data/conditional_cache_counting_helper_browsertest.cc:41:
// Counting helpers. ---------------------------------------------------------
nit: The section delimiter is not needed now that there's only one small
section.

https://codereview.chromium.org/2556363003/diff/320001/chrome/browser/browsin...
chrome/browser/browsing_data/conditional_cache_counting_helper_browsertest.cc:85:
std::unique_ptr<CacheTestUtil> cache_util_;
It's unusual to inherit member variables. The naming is technically also wrong,
as _$ means private.

Could you make this private and instead expose

protected:
 CacheTestUtil* GetCacheTestUtil();

?

https://codereview.chromium.org/2556363003/diff/320001/chrome/browser/browsin...
File
chrome/browser/browsing_data/conditional_cache_deletion_helper_browsertest.cc
(right):

https://codereview.chromium.org/2556363003/diff/320001/chrome/browser/browsin...
chrome/browser/browsing_data/conditional_cache_deletion_helper_browsertest.cc:82:
if (run_loop_) {
Why the switch from WaitableEvent to base::RunLoop?

WaitableEvent was introduced in https://codereview.chromium.org/2405803003 as a
bug fix; this change is a regression.

Would you mind also fixing that in the util class?

https://codereview.chromium.org/2556363003/diff/320001/chrome/browser/browsin...
chrome/browser/browsing_data/conditional_cache_deletion_helper_browsertest.cc:96:
base::Callback<void(int)> done_callback_;
Ditto.

https://codereview.chromium.org/2556363003/diff/320001/chrome/test/BUILD.gn
File chrome/test/BUILD.gn (right):

https://codereview.chromium.org/2556363003/diff/320001/chrome/test/BUILD.gn#n...
chrome/test/BUILD.gn:1332: "../browser/browsing_data/cache_test_util.cc",
Also add .h?

https://codereview.chromium.org/2556363003/diff/320001/components/browsing_da...
File components/browsing_data/content/conditional_cache_counting_helper.cc
(right):

https://codereview.chromium.org/2556363003/diff/320001/components/browsing_da...
components/browsing_data/content/conditional_cache_counting_helper.cc:100: //
-->
nit: Why the line break?

https://codereview.chromium.org/2556363003/diff/320001/components/browsing_da...
File
components/browsing_data/content/storage_partition_http_cache_data_remover.cc
(right):

https://codereview.chromium.org/2556363003/diff/320001/components/browsing_da...
components/browsing_data/content/storage_partition_http_cache_data_remover.cc:100:
// -->
nit: Ditto.

[dullweber, 2016-12-20 09:27:44]

The CQ bit was checked by dullweber@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-20 09:27:59]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-20 10:25:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-20 10:25:37]

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_...)

[dullweber, 2016-12-21 09:29:22]

Description was changed from

==========
Currently cache size in "Clear browsing data" dialog is shown as "Less than X"
when selecting a time range (Where X is the total size of the cache).
To fix this, I count the cache size depending on a chosen time range. 
The cache can be very big and we want a responsive ui, so if counting the cache
takes too long I show the upper estimate instead.

As a future improvement I would like to propose a specialized version for
SimpleBackendImpl. It could determine the size of a subset of the cache from the
EntryMetadata stored in memory. This should be fast enough for any cache size.

BUG=671196
==========

to

==========
Currently cache size in "Clear browsing data" dialog is shown as "Less than X"
when selecting a time range (Where X is the total size of the cache).
To fix this, I count the cache size depending on a chosen time range. 
The cache can be very big and we want a responsive ui, so if counting the cache
takes too long I show the upper estimate instead.

As a future improvement I would like to propose a specialized version for
SimpleBackendImpl. It could determine the size of a subset of the cache from the
EntryMetadata stored in memory. This should be fast enough for any cache size.

BUG=671196
CQ-DEPEND=CL:2591743002
==========

[dullweber, 2016-12-21 10:22:04]

The CQ bit was checked by dullweber@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-21 10:22:12]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dullweber, 2016-12-21 10:29:20]

Thanks for the comments, I moved GetEntrySize() to http://crrev.com/2591743002
and fixed the other issues as well.

https://codereview.chromium.org/2556363003/diff/300001/components/browsing_da...
File components/browsing_data/content/conditional_cache_counting_helper.cc
(right):

https://codereview.chromium.org/2556363003/diff/300001/components/browsing_da...
components/browsing_data/content/conditional_cache_counting_helper.cc:151: //
TODO(dullweber): Implement faster counting for SimpleBackendImpl.
On 2016/12/20 01:02:59, msramek wrote:
> On 2016/12/16 16:41:11, dullweber wrote:
> > On 2016/12/15 15:58:51, msramek wrote:
> > > You already implemented it in this CL, just didn't call it from here. To
> keep
> > > the CL cleaner, I suggest either:
> > > 
> > > a) resolving this TODO immediately
> > > b) splitting up the cache changes to a separate CL
> > > 
> > > I recommend b), as smaller CLs are easier and faster to review.
> > 
> > I added the code to get the size of a single entry from the entry but I
didn't
> > add code for the SimpleBackendImpl to calculate its size from the metadata
in
> > the index. I can split the GetEntrySize() code into a separate cl, that 
> 
> You seem not to have finished that sentence; however, I agree that a separate
CL
> would be better.

I created a separate cl and moved the GetEntrySize() changes there

https://codereview.chromium.org/2556363003/diff/320001/chrome/browser/browsin...
File chrome/browser/browsing_data/cache_test_util.cc (right):

https://codereview.chromium.org/2556363003/diff/320001/chrome/browser/browsin...
chrome/browser/browsing_data/cache_test_util.cc:18: void
CacheTestUtil::SetUpOnMainThread() {
On 2016/12/20 01:02:59, msramek wrote:
> Couldn't this method's body be merged into the constructor and the following
> one's into the destructor?
> 
> The SetUp() and TearDown() methods are used by tests so that they can be
> inherited and specialized; however, this is not a test. If it's held by the
test
> in a std::unique_ptr, you have perfect control over when the constructor and
> destructor are run (by calling MakeUnique and reset), so it seems there's no
> reason to separate this.

yes, sounds good.

https://codereview.chromium.org/2556363003/diff/320001/chrome/browser/browsin...
File chrome/browser/browsing_data/cache_test_util.h (right):

https://codereview.chromium.org/2556363003/diff/320001/chrome/browser/browsin...
chrome/browser/browsing_data/cache_test_util.h:48: void SetUpOnIOThread();
On 2016/12/20 01:02:59, msramek wrote:
> Ordering: Methods before attributes (if you got inspired for this wrong
ordering
> from CacheCounter, sorry for that; feel free to fix it there as well).

Fixed here and in cache counter. I guess that was my inspiration for this :D

https://codereview.chromium.org/2556363003/diff/320001/chrome/browser/browsin...
File
chrome/browser/browsing_data/conditional_cache_counting_helper_browsertest.cc
(right):

https://codereview.chromium.org/2556363003/diff/320001/chrome/browser/browsin...
chrome/browser/browsing_data/conditional_cache_counting_helper_browsertest.cc:41:
// Counting helpers. ---------------------------------------------------------
On 2016/12/20 01:03:00, msramek wrote:
> nit: The section delimiter is not needed now that there's only one small
> section.

Done.

https://codereview.chromium.org/2556363003/diff/320001/chrome/browser/browsin...
chrome/browser/browsing_data/conditional_cache_counting_helper_browsertest.cc:85:
std::unique_ptr<CacheTestUtil> cache_util_;
On 2016/12/20 01:02:59, msramek wrote:
> It's unusual to inherit member variables. The naming is technically also
wrong,
> as _$ means private.
> 
> Could you make this private and instead expose
> 
> protected:
>  CacheTestUtil* GetCacheTestUtil();
> 
> ?

Done.

https://codereview.chromium.org/2556363003/diff/320001/chrome/browser/browsin...
File
chrome/browser/browsing_data/conditional_cache_deletion_helper_browsertest.cc
(right):

https://codereview.chromium.org/2556363003/diff/320001/chrome/browser/browsin...
chrome/browser/browsing_data/conditional_cache_deletion_helper_browsertest.cc:82:
if (run_loop_) {
On 2016/12/20 01:03:00, msramek wrote:
> Why the switch from WaitableEvent to base::RunLoop?
> 
> WaitableEvent was introduced in https://codereview.chromium.org/2405803003 as
a
> bug fix; this change is a regression.
> 
> Would you mind also fixing that in the util class?

Oh, thanks. I will revert it here and in the util class but I didn't find a way
to make it work with waitable_event in the cache_counting class because it calls
the callback on the UI thread. But runloop should be ok if I only call
runloop.Quit() from the UI thread because it can't be executed before
runloop.run()? I will make sure that there are DCHECKs to ensure this.

https://codereview.chromium.org/2556363003/diff/320001/chrome/test/BUILD.gn
File chrome/test/BUILD.gn (right):

https://codereview.chromium.org/2556363003/diff/320001/chrome/test/BUILD.gn#n...
chrome/test/BUILD.gn:1332: "../browser/browsing_data/cache_test_util.cc",
On 2016/12/20 01:03:00, msramek wrote:
> Also add .h?

Done.

https://codereview.chromium.org/2556363003/diff/320001/components/browsing_da...
File components/browsing_data/content/conditional_cache_counting_helper.cc
(right):

https://codereview.chromium.org/2556363003/diff/320001/components/browsing_da...
components/browsing_data/content/conditional_cache_counting_helper.cc:100: //
-->
On 2016/12/20 01:03:00, msramek wrote:
> nit: Why the line break?

Oh, I guess changing to an enum class made the line longer and git cl format
broke it afterwards. Fixed

https://codereview.chromium.org/2556363003/diff/320001/components/browsing_da...
File
components/browsing_data/content/storage_partition_http_cache_data_remover.cc
(right):

https://codereview.chromium.org/2556363003/diff/320001/components/browsing_da...
components/browsing_data/content/storage_partition_http_cache_data_remover.cc:100:
// -->
On 2016/12/20 01:03:00, msramek wrote:
> nit: Ditto.

Done.

[commit-bot: I haz the power, 2016-12-21 11:31:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-21 11:31:48]

Dry run: This issue passed the CQ dry run.

[msramek, 2017-01-03 14:44:36]

LGTM! Thanks for your patience :)

It looks like the base CL https://codereview.chromium.org/2591743002 might take
longer. If I understand correctly, the only dependency is GetEntrySize() in
ConditionalCacheCountingHelper::IterateOverEntries(). Would it make sense to
perhaps remove IterateOverEntries(), CountEntries(), and add a TODOs in
DoCountCache() to implement the iteration?

Then, we could land the CL with the current functionality. Once
https://codereview.chromium.org/2591743002 is resolved, we can re-add the code
(basically copy-paste it back from this code review).

While that misses the primary purpose of this CL, we do a lot of cleanup in
SPHCDR, CCDH, and their tests, which I think are valuable to land right now.

https://codereview.chromium.org/2556363003/diff/320001/chrome/browser/browsin...
File
chrome/browser/browsing_data/conditional_cache_deletion_helper_browsertest.cc
(right):

https://codereview.chromium.org/2556363003/diff/320001/chrome/browser/browsin...
chrome/browser/browsing_data/conditional_cache_deletion_helper_browsertest.cc:82:
if (run_loop_) {
On 2016/12/21 10:29:19, dullweber wrote:
> On 2016/12/20 01:03:00, msramek wrote:
> > Why the switch from WaitableEvent to base::RunLoop?
> > 
> > WaitableEvent was introduced in https://codereview.chromium.org/2405803003
as
> a
> > bug fix; this change is a regression.
> > 
> > Would you mind also fixing that in the util class?
> 
> Oh, thanks. I will revert it here and in the util class but I didn't find a
way
> to make it work with waitable_event in the cache_counting class because it
calls
> the callback on the UI thread. But runloop should be ok if I only call
> runloop.Quit() from the UI thread because it can't be executed before
> runloop.run()? I will make sure that there are DCHECKs to ensure this.

I agree - because Run() and Quit() are both on the UI thread, and because CCDCH
will never respond within the same UI thread task (it always makes a UI->IO->UI
jump), we know that Run() will always happen before Quit().

[dullweber, 2017-01-04 12:45:18]

The CQ bit was checked by dullweber@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-04 12:45:27]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dullweber, 2017-01-04 12:55:14]

On 2017/01/03 14:44:36, msramek wrote:
> LGTM! Thanks for your patience :)
> 
> It looks like the base CL https://codereview.chromium.org/2591743002 might
take
> longer. If I understand correctly, the only dependency is GetEntrySize() in
> ConditionalCacheCountingHelper::IterateOverEntries(). Would it make sense to
> perhaps remove IterateOverEntries(), CountEntries(), and add a TODOs in
> DoCountCache() to implement the iteration?
> 
> Then, we could land the CL with the current functionality. Once
> https://codereview.chromium.org/2591743002 is resolved, we can re-add the code
> (basically copy-paste it back from this code review).
> 
> While that misses the primary purpose of this CL, we do a lot of cleanup in
> SPHCDR, CCDH, and their tests, which I think are valuable to land right now.
> 
>
https://codereview.chromium.org/2556363003/diff/320001/chrome/browser/browsin...
> File
> chrome/browser/browsing_data/conditional_cache_deletion_helper_browsertest.cc
> (right):
> 
>
https://codereview.chromium.org/2556363003/diff/320001/chrome/browser/browsin...
>
chrome/browser/browsing_data/conditional_cache_deletion_helper_browsertest.cc:82:
> if (run_loop_) {
> On 2016/12/21 10:29:19, dullweber wrote:
> > On 2016/12/20 01:03:00, msramek wrote:
> > > Why the switch from WaitableEvent to base::RunLoop?
> > > 
> > > WaitableEvent was introduced in https://codereview.chromium.org/2405803003
> as
> > a
> > > bug fix; this change is a regression.
> > > 
> > > Would you mind also fixing that in the util class?
> > 
> > Oh, thanks. I will revert it here and in the util class but I didn't find a
> way
> > to make it work with waitable_event in the cache_counting class because it
> calls
> > the callback on the UI thread. But runloop should be ok if I only call
> > runloop.Quit() from the UI thread because it can't be executed before
> > runloop.run()? I will make sure that there are DCHECKs to ensure this.
> 
> I agree - because Run() and Quit() are both on the UI thread, and because
CCDCH
> will never respond within the same UI thread task (it always makes a
UI->IO->UI
> jump), we know that Run() will always happen before Quit().

Ok, I removed all code depending on GetEntrySize() and rebased everything to
master. The remaining parts are mostly refactoring of all counting code into a
helper class and cleanup for the test. When the other CL is finished a simple
revert of this commit will hopefully be enough and everything works again :D

TODO: revert 93d040e376daa3daad1df03d6d02449117a0d70e

[msramek, 2017-01-04 14:55:58]

Still LGTM, thank you!

Consider uploading that revert as a CL already so that it won't get lost :)

[dullweber, 2017-01-04 15:16:32]

Description was changed from

==========
Currently cache size in "Clear browsing data" dialog is shown as "Less than X"
when selecting a time range (Where X is the total size of the cache).
To fix this, I count the cache size depending on a chosen time range. 
The cache can be very big and we want a responsive ui, so if counting the cache
takes too long I show the upper estimate instead.

As a future improvement I would like to propose a specialized version for
SimpleBackendImpl. It could determine the size of a subset of the cache from the
EntryMetadata stored in memory. This should be fast enough for any cache size.

BUG=671196
CQ-DEPEND=CL:2591743002
==========

to

==========
Refactor cache counting code as a preparation for a more exact implementation.

Move code from StoragePartitionHttpCacheRemover to CacheCountingHelper. Improve
tests by adding a CacheTestUtil class for common functionality of cache counting
and cache deletion tests.

BUG=671196
TBR=achuith@chromium.org
==========

[dullweber, 2017-01-04 15:23:16]

On 2017/01/04 14:55:58, msramek wrote:
> Still LGTM, thank you!
> 
> Consider uploading that revert as a CL already so that it won't get lost :)

Achuith: adding you as TBR for small changes in
chrome/browser/ui/webui/options/chromeos/

[dullweber, 2017-01-04 15:23:33]

The CQ bit was checked by dullweber@chromium.org

[commit-bot: I haz the power, 2017-01-04 15:23:52]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-04 17:05:22]

CQ is committing da patch.

Bot data: {"patchset_id": 460001, "attempt_start_ts": 1483543413111170,
"parent_rev": "c9b1f40b946857d8f2229414dfb7eea890ecefde", "commit_rev":
"82516c572dbb7eeaed2ad688205a10dfd9294e7f"}

[commit-bot: I haz the power, 2017-01-04 17:05:51]

Description was changed from

==========
Refactor cache counting code as a preparation for a more exact implementation.

Move code from StoragePartitionHttpCacheRemover to CacheCountingHelper. Improve
tests by adding a CacheTestUtil class for common functionality of cache counting
and cache deletion tests.

BUG=671196
TBR=achuith@chromium.org
==========

to

==========
Refactor cache counting code as a preparation for a more exact implementation.

Move code from StoragePartitionHttpCacheRemover to CacheCountingHelper. Improve
tests by adding a CacheTestUtil class for common functionality of cache counting
and cache deletion tests.

BUG=671196
TBR=achuith@chromium.org

Review-Url: https://codereview.chromium.org/2556363003
==========

[commit-bot: I haz the power, 2017-01-04 17:05:53]

Committed patchset #24 (id:460001)

[commit-bot: I haz the power, 2017-01-04 17:08:43]

Description was changed from

==========
Refactor cache counting code as a preparation for a more exact implementation.

Move code from StoragePartitionHttpCacheRemover to CacheCountingHelper. Improve
tests by adding a CacheTestUtil class for common functionality of cache counting
and cache deletion tests.

BUG=671196
TBR=achuith@chromium.org

Review-Url: https://codereview.chromium.org/2556363003
==========

to

==========
Refactor cache counting code as a preparation for a more exact implementation.

Move code from StoragePartitionHttpCacheRemover to CacheCountingHelper. Improve
tests by adding a CacheTestUtil class for common functionality of cache counting
and cache deletion tests.

BUG=671196
TBR=achuith@chromium.org

Committed: https://crrev.com/75321ac0828dd8f7dd38944234b867ea98598656
Cr-Commit-Position: refs/heads/master@{#441399}
==========

[commit-bot: I haz the power, 2017-01-04 17:08:45]

Patchset 24 (id:??) landed as
https://crrev.com/75321ac0828dd8f7dd38944234b867ea98598656
Cr-Commit-Position: refs/heads/master@{#441399}