[Offline Pages] Filtering expired pages and fix consistency check.

[Offline Pages] Filtering expired pages and fix consistency check.

Filtering expired pages when querying offline pages. Using expiring
instead of removing page entries in consistency check. Also will remove
files in archive folder to which have no page entry pointing.

BUG=616622
Committed: https://crrev.com/b057d1e7c41c04cad80cbd7232ebcfed293c9ee7
Cr-Commit-Position: refs/heads/master@{#398744}

[romax, 2016-06-07 20:14:19]

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

[romax, 2016-06-07 20:14:20]

PTAL.

[fgorski, 2016-06-07 22:46:15]

https://codereview.chromium.org/2041983006/diff/1/chrome/browser/android/offl...
File chrome/browser/android/offline_pages/offline_page_utils_unittest.cc
(right):

https://codereview.chromium.org/2041983006/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/offline_page_utils_unittest.cc:224:
profile(), GURL(kTestPage3Url)));
I am not sure I understand this line.
Except for constructing new GURL it does the same.
Did you mean to add test for page 4 here?

https://codereview.chromium.org/2041983006/diff/1/components/offline_pages/of...
File components/offline_pages/offline_page_model_impl.cc (right):

https://codereview.chromium.org/2041983006/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_model_impl.cc:874: void
OfflinePageModelImpl::ScanForMissingArchiveFiles(
I think you added functionality to this method and it no longer matches the
name.

I suggest 2 separate methods instead. I know that you will iterate twice, but I
don't see that as a problem.

https://codereview.chromium.org/2041983006/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_model_impl.cc:877:
std::set<base::FilePath> unused_archives(archive_paths);
I like the idea you used here.
I think a comment, such as: Archives are considered unused until proven
otherwise. Will explain this thing nicely.

https://codereview.chromium.org/2041983006/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_model_impl.cc:896:
std::vector<base::FilePath> headless_archives(unused_archives.begin(),
orphaned_archives?

https://codereview.chromium.org/2041983006/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_model_impl.cc:908:
UMA_HISTOGRAM_COUNTS("OfflinePages.Consistency.ArchivelessPageCount",
how about .PagesMissingArchiveFiles for the last segment?

https://codereview.chromium.org/2041983006/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_model_impl.cc:914: void
OfflinePageModelImpl::OnDeleteHeadlessArchivesDone(
Again Orphaned feel better here and below.

https://codereview.chromium.org/2041983006/diff/1/components/offline_pages/of...
File components/offline_pages/offline_page_model_impl.h (right):

https://codereview.chromium.org/2041983006/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_model_impl.h:199: void
OnDeleteHeadlessArchivesDone(const std::vector<base::FilePath>& archives,
Please explain what "headless" means here and why you chose it.

https://codereview.chromium.org/2041983006/diff/1/components/offline_pages/of...
File components/offline_pages/offline_page_model_impl_unittest.cc (right):

https://codereview.chromium.org/2041983006/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_model_impl_unittest.cc:765:
EXPECT_TRUE(base::PathExists(path));
Could you also add this check before CheckForExternalFileDeletion?

[romax, 2016-06-08 01:31:26]

Sorry for adding more files since I've changed some names... And please let me
know if you still prefer 'orphaned' to 'headless' with explaination so I can
change it.
Thanks!

https://codereview.chromium.org/2041983006/diff/1/chrome/browser/android/offl...
File chrome/browser/android/offline_pages/offline_page_utils_unittest.cc
(right):

https://codereview.chromium.org/2041983006/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/offline_page_utils_unittest.cc:224:
profile(), GURL(kTestPage3Url)));
On 2016/06/07 22:46:15, fgorski wrote:
> I am not sure I understand this line.
> Except for constructing new GURL it does the same.
> Did you mean to add test for page 4 here?

yeah.. copy paste.. (╯‵□′)╯︵┻━┻

https://codereview.chromium.org/2041983006/diff/1/components/offline_pages/of...
File components/offline_pages/offline_page_model_impl.cc (right):

https://codereview.chromium.org/2041983006/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_model_impl.cc:874: void
OfflinePageModelImpl::ScanForMissingArchiveFiles(
On 2016/06/07 22:46:15, fgorski wrote:
> I think you added functionality to this method and it no longer matches the
> name.
> 
> I suggest 2 separate methods instead. I know that you will iterate twice, but
I
> don't see that as a problem.

Done.

https://codereview.chromium.org/2041983006/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_model_impl.cc:877:
std::set<base::FilePath> unused_archives(archive_paths);
On 2016/06/07 22:46:15, fgorski wrote:
> I like the idea you used here.
> I think a comment, such as: Archives are considered unused until proven
> otherwise. Will explain this thing nicely.

Done.

https://codereview.chromium.org/2041983006/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_model_impl.cc:896:
std::vector<base::FilePath> headless_archives(unused_archives.begin(),
On 2016/06/07 22:46:15, fgorski wrote:
> orphaned_archives?

i explained in previous comments, but I'm willing to change all names (and
Histogram ones) once we settled with a name if that doesn't make sense to you.

https://codereview.chromium.org/2041983006/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_model_impl.cc:908:
UMA_HISTOGRAM_COUNTS("OfflinePages.Consistency.ArchivelessPageCount",
On 2016/06/07 22:46:15, fgorski wrote:
> how about .PagesMissingArchiveFiles for the last segment?

Done.

https://codereview.chromium.org/2041983006/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_model_impl.cc:914: void
OfflinePageModelImpl::OnDeleteHeadlessArchivesDone(
On 2016/06/07 22:46:15, fgorski wrote:
> Again Orphaned feel better here and below.

Acknowledged.

https://codereview.chromium.org/2041983006/diff/1/components/offline_pages/of...
File components/offline_pages/offline_page_model_impl.h (right):

https://codereview.chromium.org/2041983006/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_model_impl.h:199: void
OnDeleteHeadlessArchivesDone(const std::vector<base::FilePath>& archives,
On 2016/06/07 22:46:15, fgorski wrote:
> Please explain what "headless" means here and why you chose it. 

added some explaination in comment..I'm choosing headless since i was always
considering page item and archive files like pointers and objects... page items
in metadata store are pointers and archives are objects...or linked list? I
think there's a similar term in linked list.
But TBH I'd like some suggestions for names. However I'd like to have some
'terms' for things we have here within our group which we know what they are
while they're not too long to write.

https://codereview.chromium.org/2041983006/diff/1/components/offline_pages/of...
File components/offline_pages/offline_page_model_impl_unittest.cc (right):

https://codereview.chromium.org/2041983006/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_model_impl_unittest.cc:765:
EXPECT_TRUE(base::PathExists(path));
On 2016/06/07 22:46:15, fgorski wrote:
> Could you also add this check before CheckForExternalFileDeletion?

Done.

[fgorski, 2016-06-08 20:27:06]

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

[fgorski, 2016-06-08 20:27:07]

lgtm with nits and one extra comment.
Also adding Jian Li, as it seems that scheme will still depend on this code.

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

https://codereview.chromium.org/2041983006/diff/20001/components/offline_page...
components/offline_pages/offline_page_model_impl.cc:886: }
nit: could you put a space here for better readability?

https://codereview.chromium.org/2041983006/diff/20001/components/offline_page...
components/offline_pages/offline_page_model_impl.cc:888: return;
nit: same here (by space I mean empty line of course)

https://codereview.chromium.org/2041983006/diff/20001/components/offline_page...
components/offline_pages/offline_page_model_impl.cc:909: for (const auto&
id_page_pair : offline_pages_) {
nit: you don't need {} for one-liner, and in our team we agreed to not use it,
even thou it is optional.

https://codereview.chromium.org/2041983006/diff/20001/components/offline_page...
components/offline_pages/offline_page_model_impl.cc:914: return;
nit: extra line would be useful for readability.

https://codereview.chromium.org/2041983006/diff/20001/components/offline_page...
components/offline_pages/offline_page_model_impl.cc:917:
archive_manager_->DeleteMultipleArchives(
I had one more thought about this. What if an archive is being created as we are
running metadata consistency check and there is no entry for it yet. Can we only
proceed with deleting the file, if it is older than 10 second or something like
that?  Chances we hit it are slim, but still. Would be great to at least think
through that scenario. (This would have to be done inside the delete method, as
you need to stat the file. Perhaps an extra parameter is in order, but you work
it out)

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

https://codereview.chromium.org/2041983006/diff/20001/components/offline_page...
components/offline_pages/offline_page_model_impl_unittest.cc:769: ResetModel();
If there is implicit stuff going on, can you add a comment?

[jianli, 2016-06-08 21:10:35]

lgtm

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

https://codereview.chromium.org/2041983006/diff/20001/components/offline_page...
components/offline_pages/offline_page_model_impl.cc:877:
DeleteHeadlessArchives(archive_paths);
I think "Orphaned" is better than "Headless".

https://codereview.chromium.org/2041983006/diff/20001/components/offline_page...
components/offline_pages/offline_page_model_impl.cc:907: // Archives are
considered unused unless they are pointed to by some pages.
unused => orphaned

https://codereview.chromium.org/2041983006/diff/20001/components/offline_page...
components/offline_pages/offline_page_model_impl.cc:917:
archive_manager_->DeleteMultipleArchives(
On 2016/06/08 20:27:07, fgorski wrote:
> I had one more thought about this. What if an archive is being created as we
are
> running metadata consistency check and there is no entry for it yet. Can we
only
> proceed with deleting the file, if it is older than 10 second or something
like
> that?  Chances we hit it are slim, but still. Would be great to at least think
> through that scenario. (This would have to be done inside the delete method,
as
> you need to stat the file. Perhaps an extra parameter is in order, but you
work
> it out)

I agree with Filip that we need to handle this case. We probably don't need to
delete a file that was created a couple hours ago. The 10 second is not enough,
considering potential DST change. We could do this in another patch.

https://codereview.chromium.org/2041983006/diff/20001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2041983006/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:35034: +    Result of deleting archives
without metadata entry when checking
Whether an attempt to delete archive files without metadata was successful.

https://codereview.chromium.org/2041983006/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:35043: +    Result of expiring pages
without archive file on disk when checking
Whether an attempt to expire pages without archives was successful.

[romax, 2016-06-08 22:18:13]

Description was changed from

==========
[Offline Pages] Filtering expired pages and fix consistency check.

Filtering expired pages when querying offline pages. Using expiring
instead of removing page entries in consistency check. Also will remove
files in archive folder to which have no page entry pointing.

BUG=616622
==========

to

==========
[Offline Pages] Filtering expired pages and fix consistency check.

Filtering expired pages when querying offline pages. Using expiring
instead of removing page entries in consistency check. Also will remove
files in archive folder to which have no page entry pointing.

BUG=616622
==========

[romax, 2016-06-08 22:18:13]

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

[romax, 2016-06-08 22:18:59]

Addressed comments.
Also adding holte@ for histograms.xml owner review.

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

https://codereview.chromium.org/2041983006/diff/20001/components/offline_page...
components/offline_pages/offline_page_model_impl.cc:877:
DeleteHeadlessArchives(archive_paths);
On 2016/06/08 21:10:35, jianli wrote:
> I think "Orphaned" is better than "Headless".

sure, i've changed to orphaned.

https://codereview.chromium.org/2041983006/diff/20001/components/offline_page...
components/offline_pages/offline_page_model_impl.cc:886: }
On 2016/06/08 20:27:07, fgorski wrote:
> nit: could you put a space here for better readability?

Done.

https://codereview.chromium.org/2041983006/diff/20001/components/offline_page...
components/offline_pages/offline_page_model_impl.cc:888: return;
On 2016/06/08 20:27:07, fgorski wrote:
> nit: same here (by space I mean empty line of course)

Done.

https://codereview.chromium.org/2041983006/diff/20001/components/offline_page...
components/offline_pages/offline_page_model_impl.cc:907: // Archives are
considered unused unless they are pointed to by some pages.
On 2016/06/08 21:10:35, jianli wrote:
> unused => orphaned

Done.

https://codereview.chromium.org/2041983006/diff/20001/components/offline_page...
components/offline_pages/offline_page_model_impl.cc:909: for (const auto&
id_page_pair : offline_pages_) {
On 2016/06/08 20:27:07, fgorski wrote:
> nit: you don't need {} for one-liner, and in our team we agreed to not use it,
> even thou it is optional.

Done.

https://codereview.chromium.org/2041983006/diff/20001/components/offline_page...
components/offline_pages/offline_page_model_impl.cc:914: return;
On 2016/06/08 20:27:07, fgorski wrote:
> nit: extra line would be useful for readability.

Done.

https://codereview.chromium.org/2041983006/diff/20001/components/offline_page...
components/offline_pages/offline_page_model_impl.cc:917:
archive_manager_->DeleteMultipleArchives(
On 2016/06/08 21:10:35, jianli wrote:
> On 2016/06/08 20:27:07, fgorski wrote:
> > I had one more thought about this. What if an archive is being created as we
> are
> > running metadata consistency check and there is no entry for it yet. Can we
> only
> > proceed with deleting the file, if it is older than 10 second or something
> like
> > that?  Chances we hit it are slim, but still. Would be great to at least
think
> > through that scenario. (This would have to be done inside the delete method,
> as
> > you need to stat the file. Perhaps an extra parameter is in order, but you
> work
> > it out)
> 
> I agree with Filip that we need to handle this case. We probably don't need to
> delete a file that was created a couple hours ago. The 10 second is not
enough,
> considering potential DST change. We could do this in another patch.

This seems not a trivial issue. Will have another patch for this.

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

https://codereview.chromium.org/2041983006/diff/20001/components/offline_page...
components/offline_pages/offline_page_model_impl_unittest.cc:769: ResetModel();
On 2016/06/08 20:27:07, fgorski wrote:
> If there is implicit stuff going on, can you add a comment?

Done.

https://codereview.chromium.org/2041983006/diff/20001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2041983006/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:35034: +    Result of deleting archives
without metadata entry when checking
On 2016/06/08 21:10:35, jianli wrote:
> Whether an attempt to delete archive files without metadata was successful.

Done.

https://codereview.chromium.org/2041983006/diff/20001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:35043: +    Result of expiring pages
without archive file on disk when checking
On 2016/06/08 21:10:35, jianli wrote:
> Whether an attempt to expire pages without archives was successful.

Done.

[Steven Holte, 2016-06-08 23:50:29]

histograms lgtm

[romax, 2016-06-09 00:01:20]

The CQ bit was checked by romax@chromium.org

[romax, 2016-06-09 00:01:20]

The patchset sent to the CQ was uploaded after l-g-t-m from jianli@chromium.org,
fgorski@chromium.org
Link to the patchset: https://codereview.chromium.org/2041983006/#ps60001
(title: "minor name changes.")

[commit-bot: I haz the power, 2016-06-09 00:01:52]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2041983006/60001

[commit-bot: I haz the power, 2016-06-09 00:10:09]

Description was changed from

==========
[Offline Pages] Filtering expired pages and fix consistency check.

Filtering expired pages when querying offline pages. Using expiring
instead of removing page entries in consistency check. Also will remove
files in archive folder to which have no page entry pointing.

BUG=616622
==========

to

==========
[Offline Pages] Filtering expired pages and fix consistency check.

Filtering expired pages when querying offline pages. Using expiring
instead of removing page entries in consistency check. Also will remove
files in archive folder to which have no page entry pointing.

BUG=616622
==========

[commit-bot: I haz the power, 2016-06-09 00:10:10]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-06-09 00:12:25]

Description was changed from

==========
[Offline Pages] Filtering expired pages and fix consistency check.

Filtering expired pages when querying offline pages. Using expiring
instead of removing page entries in consistency check. Also will remove
files in archive folder to which have no page entry pointing.

BUG=616622
==========

to

==========
[Offline Pages] Filtering expired pages and fix consistency check.

Filtering expired pages when querying offline pages. Using expiring
instead of removing page entries in consistency check. Also will remove
files in archive folder to which have no page entry pointing.

BUG=616622

Committed: https://crrev.com/b057d1e7c41c04cad80cbd7232ebcfed293c9ee7
Cr-Commit-Position: refs/heads/master@{#398744}
==========

[commit-bot: I haz the power, 2016-06-09 00:12:26]

Patchset 4 (id:??) landed as
https://crrev.com/b057d1e7c41c04cad80cbd7232ebcfed293c9ee7
Cr-Commit-Position: refs/heads/master@{#398744}