[Offline pages] Selecting pages for clean up

[Offline pages] Selecting pages for clean

Changes how pages that will be cleaned up to free up space
are selected: using last_access_time instead of creation_time
as a basis for kicking them out.

Also changes a string that is shown in Free Up Space dialog.

BUG=491352
Committed: https://crrev.com/9c1bd5dc2637bbf4f3d490818f6d4846ea00cee9
Cr-Commit-Position: refs/heads/master@{#352451}

[fgorski, 2015-09-30 17:37:49]

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

[fgorski, 2015-09-30 17:37:49]

This is a simple change for LRU Clean up, or to be more accurate: Pages used
recently will not be removed.

[jianli, 2015-09-30 20:42:22]

This is not exactly LRU. Probably better to update the description as "based on
last access time".

Also, do you need to update the message IDS_OFFLINE_PAGES_FREE_UP_SPACE_MESSAGE
since "saved pages that are older than 1 month" sounds more like checking the
creation time?

[fgorski, 2015-10-02 21:36:10]

On 2015/09/30 20:42:22, jianli wrote:
> This is not exactly LRU. Probably better to update the description as "based
on
> last access time".
> 
> Also, do you need to update the message
IDS_OFFLINE_PAGES_FREE_UP_SPACE_MESSAGE
> since "saved pages that are older than 1 month" sounds more like checking the
> creation time?

Jian,
Description is updated and I proposed a change to the string to PM/UX. I'll send
it for review to Newt, once they approve.

Any other comments?

[fgorski, 2015-10-05 16:09:19]

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

[fgorski, 2015-10-05 16:09:20]

newt@ could you take a look at the string update?

This is after UX/PM discussion. 2 things happening here is:
* we are changing the policy to be based on usage of offline copies
* we are stressing the fact that only copy is removed and not the saved page
(bookmark)

[newt (away), 2015-10-05 16:45:21]

lgtm after spelling nit

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

https://codereview.chromium.org/1378883002/diff/40001/components/offline_page...
components/offline_pages/offline_page_model_unittest.cc:717: base::Time
fourty_days_ago = now - base::TimeDelta::FromDays(40);
spelling: "forty"

[fgorski, 2015-10-05 16:58:36]

Addressed.

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

https://codereview.chromium.org/1378883002/diff/40001/components/offline_page...
components/offline_pages/offline_page_model_unittest.cc:717: base::Time
fourty_days_ago = now - base::TimeDelta::FromDays(40);
On 2015/10/05 16:45:21, newt wrote:
> spelling: "forty"

Wow. You live you learn :)

[newt (away), 2015-10-05 17:42:22]

On 2015/10/05 16:58:36, fgorski wrote:
> Addressed.
> 
>
https://codereview.chromium.org/1378883002/diff/40001/components/offline_page...
> File components/offline_pages/offline_page_model_unittest.cc (right):
> 
>
https://codereview.chromium.org/1378883002/diff/40001/components/offline_page...
> components/offline_pages/offline_page_model_unittest.cc:717: base::Time
> fourty_days_ago = now - base::TimeDelta::FromDays(40);
> On 2015/10/05 16:45:21, newt wrote:
> > spelling: "forty"
> 
> Wow. You live you learn :)

The never-ending gift of English.

still lgtm.

[jianli, 2015-10-05 22:10:01]

lgtm

[fgorski, 2015-10-05 22:10:52]

The CQ bit was checked by fgorski@chromium.org

[commit-bot: I haz the power, 2015-10-05 22:11:21]

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

[commit-bot: I haz the power, 2015-10-05 22:47:38]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-10-05 22:48:21]

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