[Offline pages] Adding expiration capability to OfflinePageModel

[Offline pages] Adding expiration capability to OfflinePageModel

* Adds expiration_time to offline page item
* Adds capability to expire pages to the model
* Updates both stores and tests with the expiration_time

BUG=605615
Committed: https://crrev.com/48904d1cdbc4f70fd961efdc69199897be8cad84
Cr-Commit-Position: refs/heads/master@{#395638}

[fgorski, 2016-05-18 22:59:45]

fgorski@chromium.org changed reviewers:
+ bburns@chromium.org, jianli@chromium.org, romax@chromium.org

[fgorski, 2016-05-18 22:59:45]

PTAL
bburns for SQL
jianli for general idea
romax for things needed by storage manager.

[jianli, 2016-05-19 20:39:54]

https://codereview.chromium.org/1993953002/diff/20001/components/offline_page...
File components/offline_pages/offline_page_metadata_store_impl_unittest.cc
(right):

https://codereview.chromium.org/1993953002/diff/20001/components/offline_page...
components/offline_pages/offline_page_metadata_store_impl_unittest.cc:197:
EXPECT_EQ(offline_page.expiration_time,
Please set an expiration time before calling AddOrUpdateOfflinePage.

https://codereview.chromium.org/1993953002/diff/20001/components/offline_page...
File components/offline_pages/offline_page_model.h (right):

https://codereview.chromium.org/1993953002/diff/20001/components/offline_page...
components/offline_pages/offline_page_model.h:221: base::Time expiration_time);
nit: const &

https://codereview.chromium.org/1993953002/diff/20001/components/offline_page...
components/offline_pages/offline_page_model.h:339: base::Time expiration_time,
nit: ditto

https://codereview.chromium.org/1993953002/diff/20001/components/offline_page...
File components/offline_pages/offline_page_model_unittest.cc (right):

https://codereview.chromium.org/1993953002/diff/20001/components/offline_page...
components/offline_pages/offline_page_model_unittest.cc:960: // We will save 3
pages - two for the same URL, and one for a different URL.
The comment "two for the same URL" does not match with the code.

https://codereview.chromium.org/1993953002/diff/20001/components/offline_page...
components/offline_pages/offline_page_model_unittest.cc:963:
std::pair<SavePageResult, int64_t> saved_pages[3];
I found out that this pattern makes the code harder to understand. Since
probably all of the time we only care about offline id, why not just having
SavePage return it? If some tests need to check the result, we could have
another helper function.

I know this behavior has nothing to do with your patch and I am not asking to
fix it now. We should probably consider the change in another patch.

https://codereview.chromium.org/1993953002/diff/20001/components/offline_page...
components/offline_pages/offline_page_model_unittest.cc:966: saved_pages[1] =
SavePage(kTestUrl3, kTestClientId3);
[1] => [2]?

https://codereview.chromium.org/1993953002/diff/20001/components/offline_page...
components/offline_pages/offline_page_model_unittest.cc:982: const
std::vector<OfflinePageItem>& offline_pages = GetStore()->GetAllPages();
I think it would be better to verify against the in-memory cache since the cache
is updated after the store update. We should either verify both store and cache
or just cache.

https://codereview.chromium.org/1993953002/diff/20001/components/offline_page...
File components/offline_pages/proto/offline_pages.proto (right):

https://codereview.chromium.org/1993953002/diff/20001/components/offline_page...
components/offline_pages/proto/offline_pages.proto:66: // Last access time of
the offline archive.
Update comment

[fgorski, 2016-05-20 17:54:23]

Updated and tested.

https://codereview.chromium.org/1993953002/diff/20001/components/offline_page...
File components/offline_pages/offline_page_metadata_store_impl_unittest.cc
(right):

https://codereview.chromium.org/1993953002/diff/20001/components/offline_page...
components/offline_pages/offline_page_metadata_store_impl_unittest.cc:197:
EXPECT_EQ(offline_page.expiration_time,
On 2016/05/19 20:39:54, jianli wrote:
> Please set an expiration time before calling AddOrUpdateOfflinePage.

I am testing for that below, in a different test.
This one uses a basic constructor.

https://codereview.chromium.org/1993953002/diff/20001/components/offline_page...
File components/offline_pages/offline_page_model.h (right):

https://codereview.chromium.org/1993953002/diff/20001/components/offline_page...
components/offline_pages/offline_page_model.h:221: base::Time expiration_time);
On 2016/05/19 20:39:54, jianli wrote:
> nit: const &

Done.

https://codereview.chromium.org/1993953002/diff/20001/components/offline_page...
components/offline_pages/offline_page_model.h:339: base::Time expiration_time,
On 2016/05/19 20:39:54, jianli wrote:
> nit: ditto

Done.

https://codereview.chromium.org/1993953002/diff/20001/components/offline_page...
File components/offline_pages/offline_page_model_unittest.cc (right):

https://codereview.chromium.org/1993953002/diff/20001/components/offline_page...
components/offline_pages/offline_page_model_unittest.cc:960: // We will save 3
pages - two for the same URL, and one for a different URL.
On 2016/05/19 20:39:54, jianli wrote:
> The comment "two for the same URL" does not match with the code.

Done.

https://codereview.chromium.org/1993953002/diff/20001/components/offline_page...
components/offline_pages/offline_page_model_unittest.cc:963:
std::pair<SavePageResult, int64_t> saved_pages[3];
On 2016/05/19 20:39:54, jianli wrote:
> I found out that this pattern makes the code harder to understand. Since
> probably all of the time we only care about offline id, why not just having
> SavePage return it? If some tests need to check the result, we could have
> another helper function.
> 
> I know this behavior has nothing to do with your patch and I am not asking to
> fix it now. We should probably consider the change in another patch.

Acknowledged.

https://codereview.chromium.org/1993953002/diff/20001/components/offline_page...
components/offline_pages/offline_page_model_unittest.cc:966: saved_pages[1] =
SavePage(kTestUrl3, kTestClientId3);
On 2016/05/19 20:39:54, jianli wrote:
> [1] => [2]?

Done.

https://codereview.chromium.org/1993953002/diff/20001/components/offline_page...
components/offline_pages/offline_page_model_unittest.cc:982: const
std::vector<OfflinePageItem>& offline_pages = GetStore()->GetAllPages();
On 2016/05/19 20:39:54, jianli wrote:
> I think it would be better to verify against the in-memory cache since the
cache
> is updated after the store update. We should either verify both store and
cache
> or just cache.

I can do it right now, but GetAllPages will filter out expired pages starting
the next patch...

https://codereview.chromium.org/1993953002/diff/20001/components/offline_page...
File components/offline_pages/proto/offline_pages.proto (right):

https://codereview.chromium.org/1993953002/diff/20001/components/offline_page...
components/offline_pages/proto/offline_pages.proto:66: // Last access time of
the offline archive.
On 2016/05/19 20:39:54, jianli wrote:
> Update comment

Done.

[bburns, 2016-05-20 18:23:27]

generally looks good, some small nits.

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

https://codereview.chromium.org/1993953002/diff/40001/components/offline_page...
components/offline_pages/offline_page_metadata_store_impl.cc:101: if
(item_proto.has_expiration_time()) {
remove curly braces for one line ifs ?

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

https://codereview.chromium.org/1993953002/diff/40001/components/offline_page...
components/offline_pages/offline_page_model.cc:523:
weak_ptr_factory_.GetWeakPtr(), offline_id,
personally, I would place one parameter on each line.

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

https://codereview.chromium.org/1993953002/diff/40001/components/offline_page...
components/offline_pages/offline_page_model_unittest.cc:968:
std::get<0>(save_result));
does this fit on the previous line?

https://codereview.chromium.org/1993953002/diff/40001/components/offline_page...
components/offline_pages/offline_page_model_unittest.cc:972:
std::vector<int64_t> pages_to_expire = {std::get<1>(saved_pages[0]),
personally I would break the line at the '='

[bburns, 2016-05-20 18:24:53]

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

https://codereview.chromium.org/1993953002/diff/40001/components/offline_page...
components/offline_pages/offline_page_metadata_store_sql.cc:46: "
expiration_time INTEGER NOT NULL"
One of the SQL feedbacks was to pack the integers for things we expect to access
a lot to avoid loading extra pages.

[romax, 2016-05-20 19:05:00]

I think all looks good, but I have 2 questions.

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

https://codereview.chromium.org/1993953002/diff/40001/components/offline_page...
components/offline_pages/offline_page_item.cc:72: bool
OfflinePageItem::IsExpired() const {
seems like when you've set a value for expiration_time, it would be expired...
So I don't know why comparing the values here. And why not use last_access_time?

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

https://codereview.chromium.org/1993953002/diff/40001/components/offline_page...
components/offline_pages/offline_page_model.cc:511: void
OfflinePageModel::ExpirePages(const std::vector<int64_t>& offline_ids,
In .h you mentioned to clear files here but seems not doing it?

[fgorski, 2016-05-20 20:54:54]

PTAL

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

https://codereview.chromium.org/1993953002/diff/40001/components/offline_page...
components/offline_pages/offline_page_item.cc:72: bool
OfflinePageItem::IsExpired() const {
On 2016/05/20 19:05:00, romax wrote:
> seems like when you've set a value for expiration_time, it would be expired...
> So I don't know why comparing the values here. And why not use
last_access_time?

first: It is easier to compare it against creaiton_time
second: last_access_time is commonly updated. Race condition on the update would
cause trouble.

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

https://codereview.chromium.org/1993953002/diff/40001/components/offline_page...
components/offline_pages/offline_page_metadata_store_impl.cc:101: if
(item_proto.has_expiration_time()) {
On 2016/05/20 18:23:27, bburns wrote:
> remove curly braces for one line ifs ?

notice the if content does not fit in a single line, hence no.

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

https://codereview.chromium.org/1993953002/diff/40001/components/offline_page...
components/offline_pages/offline_page_metadata_store_sql.cc:46: "
expiration_time INTEGER NOT NULL"
On 2016/05/20 18:24:53, bburns wrote:
> One of the SQL feedbacks was to pack the integers for things we expect to
access
> a lot to avoid loading extra pages.

Done.

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

https://codereview.chromium.org/1993953002/diff/40001/components/offline_page...
components/offline_pages/offline_page_model.cc:511: void
OfflinePageModel::ExpirePages(const std::vector<int64_t>& offline_ids,
On 2016/05/20 19:05:00, romax wrote:
> In .h you mentioned to clear files here but seems not doing it?

Step by step. You'll be adding this code and it may better to do it directly
from StorageManager.

https://codereview.chromium.org/1993953002/diff/40001/components/offline_page...
components/offline_pages/offline_page_model.cc:523:
weak_ptr_factory_.GetWeakPtr(), offline_id,
On 2016/05/20 18:23:27, bburns wrote:
> personally, I would place one parameter on each line.

I am with you, it takes running git cl format once to get it back here and I
don't like to argue with machines.

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

https://codereview.chromium.org/1993953002/diff/40001/components/offline_page...
components/offline_pages/offline_page_model_unittest.cc:968:
std::get<0>(save_result));
On 2016/05/20 18:23:27, bburns wrote:
> does this fit on the previous line?

misses it by 3. git cl format argument again.

https://codereview.chromium.org/1993953002/diff/40001/components/offline_page...
components/offline_pages/offline_page_model_unittest.cc:972:
std::vector<int64_t> pages_to_expire = {std::get<1>(saved_pages[0]),
On 2016/05/20 18:23:27, bburns wrote:
> personally I would break the line at the '='

git cl format

[jianli, 2016-05-20 20:58:56]

lgtm

[romax, 2016-05-20 21:31:11]

lgtm

[bburns, 2016-05-23 16:50:16]

lgtm.

https://codereview.chromium.org/1993953002/diff/60001/components/offline_page...
File components/offline_pages/offline_page_model.cc (right):

https://codereview.chromium.org/1993953002/diff/60001/components/offline_page...
components/offline_pages/offline_page_model.cc:523:
weak_ptr_factory_.GetWeakPtr(), offline_id,
nit: one parameter per line.

[fgorski, 2016-05-23 17:50:20]

https://codereview.chromium.org/1993953002/diff/60001/components/offline_page...
File components/offline_pages/offline_page_model.cc (right):

https://codereview.chromium.org/1993953002/diff/60001/components/offline_page...
components/offline_pages/offline_page_model.cc:523:
weak_ptr_factory_.GetWeakPtr(), offline_id,
On 2016/05/23 16:50:16, bburns wrote:
> nit: one parameter per line.

again. this is git cl formatted.

[fgorski, 2016-05-24 16:45:24]

The CQ bit was checked by fgorski@chromium.org

[fgorski, 2016-05-24 16:45:24]

The patchset sent to the CQ was uploaded after l-g-t-m from bburns@chromium.org,
romax@chromium.org, jianli@chromium.org
Link to the patchset: https://codereview.chromium.org/1993953002/#ps80001
(title: "Rebasing")

[commit-bot: I haz the power, 2016-05-24 16:45:39]

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

[commit-bot: I haz the power, 2016-05-24 17:56:49]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-05-24 17:58:33]

Description was changed from

==========
[Offline pages] Adding expiration capability to OfflinePageModel

* Adds expiration_time to offline page item
* Adds capability to expire pages to the model
* Updates both stores and tests with the expiration_time

BUG=605615
==========

to

==========
[Offline pages] Adding expiration capability to OfflinePageModel

* Adds expiration_time to offline page item
* Adds capability to expire pages to the model
* Updates both stores and tests with the expiration_time

BUG=605615

Committed: https://crrev.com/48904d1cdbc4f70fd961efdc69199897be8cad84
Cr-Commit-Position: refs/heads/master@{#395638}
==========

[commit-bot: I haz the power, 2016-05-24 17:58:34]

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