[Offline Pages] Adding more expiration logic.

[Offline Pages] Adding more expiration logic.

BUG=609357
Committed: https://crrev.com/84146c7d410b92010d8599b7fea3f9de345dfdbc
Cr-Commit-Position: refs/heads/master@{#393397}

[romax, 2016-05-12 00:07:09]

romax@chromium.org changed reviewers:
+ fgorski@chromium.org

[romax, 2016-05-12 00:07:09]

PTAL

[fgorski, 2016-05-12 14:49:32]

https://codereview.chromium.org/1970953002/diff/40001/components/offline_page...
File components/offline_pages/offline_page_storage_manager.cc (right):

https://codereview.chromium.org/1970953002/diff/40001/components/offline_page...
components/offline_pages/offline_page_storage_manager.cc:51: base::Time now =
base::Time::Now();
use base::Clock for tracking time. ability to inject the clock allows you to
control the time in tests.

https://codereview.chromium.org/1970953002/diff/40001/components/offline_page...
components/offline_pages/offline_page_storage_manager.cc:55: // min{size(),
page_limit} should be expired. And then start iterating
did you consider an alternative, where you sort by namespace and
last_access_time?

https://codereview.chromium.org/1970953002/diff/40001/components/offline_page...
components/offline_pages/offline_page_storage_manager.cc:58: pages_map.clear();
is this necessary?

https://codereview.chromium.org/1970953002/diff/40001/components/offline_page...
components/offline_pages/offline_page_storage_manager.cc:60: for (const auto&
page : pages) {
nit: single condition with single line if/for/while/.. don't require {}, but you
can leave it in. (This is based Chromium style guide). 

Applies to the while below (where braces are required)
and inner for where they are optional.

https://codereview.chromium.org/1970953002/diff/40001/components/offline_page...
components/offline_pages/offline_page_storage_manager.cc:73: });
nit: put an empty line here, and remove the one before while, these variables
are about that while loop mostly.

https://codereview.chromium.org/1970953002/diff/40001/components/offline_page...
components/offline_pages/offline_page_storage_manager.cc:100: bool
OfflinePageStorageManager::IsPageExpired(const base::Time& now,
Can you change the name to ShouldBeExpired? This will give us a leg up on two
step expiration.

https://codereview.chromium.org/1970953002/diff/40001/components/offline_page...
File components/offline_pages/offline_page_storage_manager_unittest.cc (right):

https://codereview.chromium.org/1970953002/diff/40001/components/offline_page...
components/offline_pages/offline_page_storage_manager_unittest.cc:46:
pages_.clear();
why?

https://codereview.chromium.org/1970953002/diff/40001/components/offline_page...
components/offline_pages/offline_page_storage_manager_unittest.cc:160:
{{kBookmarkNamespace, 1, 1}, {kLastNNamespace, 1, 1}}));
nit: alignment
Run git cl format, please

[romax, 2016-05-12 22:20:04]

Addressing comments from Filip. Not changing from map to vector though...

https://codereview.chromium.org/1970953002/diff/40001/components/offline_page...
File components/offline_pages/offline_page_storage_manager.cc (right):

https://codereview.chromium.org/1970953002/diff/40001/components/offline_page...
components/offline_pages/offline_page_storage_manager.cc:51: base::Time now =
base::Time::Now();
On 2016/05/12 14:49:32, fgorski wrote:
> use base::Clock for tracking time. ability to inject the clock allows you to
> control the time in tests.

Done.

https://codereview.chromium.org/1970953002/diff/40001/components/offline_page...
components/offline_pages/offline_page_storage_manager.cc:55: // min{size(),
page_limit} should be expired. And then start iterating
On 2016/05/12 14:49:32, fgorski wrote:
> did you consider an alternative, where you sort by namespace and
> last_access_time?

I thought it's more convenient and intuitive to use a map...

https://codereview.chromium.org/1970953002/diff/40001/components/offline_page...
components/offline_pages/offline_page_storage_manager.cc:58: pages_map.clear();
On 2016/05/12 14:49:32, fgorski wrote:
> is this necessary?

it's not

https://codereview.chromium.org/1970953002/diff/40001/components/offline_page...
components/offline_pages/offline_page_storage_manager.cc:60: for (const auto&
page : pages) {
On 2016/05/12 14:49:32, fgorski wrote:
> nit: single condition with single line if/for/while/.. don't require {}, but
you
> can leave it in. (This is based Chromium style guide). 
> 
> Applies to the while below (where braces are required)
> and inner for where they are optional. 

Done.

https://codereview.chromium.org/1970953002/diff/40001/components/offline_page...
components/offline_pages/offline_page_storage_manager.cc:73: });
On 2016/05/12 14:49:32, fgorski wrote:
> nit: put an empty line here, and remove the one before while, these variables
> are about that while loop mostly.

Done.

https://codereview.chromium.org/1970953002/diff/40001/components/offline_page...
components/offline_pages/offline_page_storage_manager.cc:100: bool
OfflinePageStorageManager::IsPageExpired(const base::Time& now,
On 2016/05/12 14:49:32, fgorski wrote:
> Can you change the name to ShouldBeExpired? This will give us a leg up on two
> step expiration.

Done.

https://codereview.chromium.org/1970953002/diff/40001/components/offline_page...
File components/offline_pages/offline_page_storage_manager_unittest.cc (right):

https://codereview.chromium.org/1970953002/diff/40001/components/offline_page...
components/offline_pages/offline_page_storage_manager_unittest.cc:46:
pages_.clear();
On 2016/05/12 14:49:32, fgorski wrote:
> why?

i always want to make sure it's empty... but seems constructor would guarantee
that. removed.

https://codereview.chromium.org/1970953002/diff/40001/components/offline_page...
components/offline_pages/offline_page_storage_manager_unittest.cc:160:
{{kBookmarkNamespace, 1, 1}, {kLastNNamespace, 1, 1}}));
On 2016/05/12 14:49:32, fgorski wrote:
> nit: alignment
> Run git cl format, please

this is what 'git cl format' gave me... how should I align it to make it right?

[fgorski, 2016-05-12 22:34:25]

lgtm after comment addressed.

https://codereview.chromium.org/1970953002/diff/40001/components/offline_page...
File components/offline_pages/offline_page_storage_manager_unittest.cc (right):

https://codereview.chromium.org/1970953002/diff/40001/components/offline_page...
components/offline_pages/offline_page_storage_manager_unittest.cc:160:
{{kBookmarkNamespace, 1, 1}, {kLastNNamespace, 1, 1}}));
On 2016/05/12 22:20:03, romax wrote:
> On 2016/05/12 14:49:32, fgorski wrote:
> > nit: alignment
> > Run git cl format, please
> 
> this is what 'git cl format' gave me... how should I align it to make it
right?

no, it's good if git cl format did that.

https://codereview.chromium.org/1970953002/diff/60001/components/offline_page...
File components/offline_pages/offline_page_storage_manager.h (right):

https://codereview.chromium.org/1970953002/diff/60001/components/offline_page...
components/offline_pages/offline_page_storage_manager.h:87: void
SetClockForTesting(std::unique_ptr<base::Clock> clock);
make this public and don't friend the test class.

[romax, 2016-05-12 23:31:20]

more comments addressed and checking in.

https://codereview.chromium.org/1970953002/diff/60001/components/offline_page...
File components/offline_pages/offline_page_storage_manager.h (right):

https://codereview.chromium.org/1970953002/diff/60001/components/offline_page...
components/offline_pages/offline_page_storage_manager.h:87: void
SetClockForTesting(std::unique_ptr<base::Clock> clock);
On 2016/05/12 22:34:25, fgorski wrote:
> make this public and don't friend the test class.

Done.

[romax, 2016-05-12 23:31:25]

The CQ bit was checked by romax@chromium.org

[romax, 2016-05-12 23:31:26]

The patchset sent to the CQ was uploaded after l-g-t-m from fgorski@chromium.org
Link to the patchset: https://codereview.chromium.org/1970953002/#ps80001
(title: "More comments.")

[commit-bot: I haz the power, 2016-05-12 23:31:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1970953002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1970953002/80001

[commit-bot: I haz the power, 2016-05-12 23:35:05]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-05-12 23:37:24]

Description was changed from

==========
[Offline Pages] Adding more expiration logic.

BUG=606088
==========

to

==========
[Offline Pages] Adding more expiration logic.

BUG=606088

Committed: https://crrev.com/84146c7d410b92010d8599b7fea3f9de345dfdbc
Cr-Commit-Position: refs/heads/master@{#393397}
==========

[commit-bot: I haz the power, 2016-05-12 23:37:26]

Patchset 5 (id:??) landed as
https://crrev.com/84146c7d410b92010d8599b7fea3f9de345dfdbc
Cr-Commit-Position: refs/heads/master@{#393397}

[romax, 2016-05-12 23:53:07]

Description was changed from

==========
[Offline Pages] Adding more expiration logic.

BUG=606088

Committed: https://crrev.com/84146c7d410b92010d8599b7fea3f9de345dfdbc
Cr-Commit-Position: refs/heads/master@{#393397}
==========

to

==========
[Offline Pages] Adding more expiration logic.

BUG=609357

Committed: https://crrev.com/84146c7d410b92010d8599b7fea3f9de345dfdbc
Cr-Commit-Position: refs/heads/master@{#393397}
==========