Implementing recent pages snapshot capture.

Implementing recent pages snapshot capture.
This adds functinoality to RecenttabHelper that:
- uses SnapshotController to detect when page should be captrued.
- uses simple algorithm to remove previous snapshot and limit number of current snapshots
- captures the page using MHTML Archiver and Offline Page Model.

Also has a couple of simple tests.

BUG=606106
Committed: https://crrev.com/4410eaea5c56187d7a2d4c72694935245368ac29
Cr-Commit-Position: refs/heads/master@{#391702}

[Dmitry Titov, 2016-04-29 18:29:22]

dimich@chromium.org changed reviewers:
+ dewittj@chromium.org

[dewittj, 2016-04-29 20:29:31]

Sending out comments from ps 1, will review the most latest ps when you reply.

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

https://codereview.chromium.org/1936613002/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/recent_tab_helper.cc:93: // - snapshot the
current web contents
I wonder if the snapshot should not actually wait for the delete to finish,
since it involves querying a number of other entries from the model which could
take time on low-end devices and possibly miss this web contents entirely if the
user navigates again.

https://codereview.chromium.org/1936613002/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/recent_tab_helper.cc:104: if (!page_model_)
{
Any reason you do this lazily?

https://codereview.chromium.org/1936613002/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/recent_tab_helper.cc:144: // vector of ids.
This is probably bad behavior, please open a bug on the model.

https://codereview.chromium.org/1936613002/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/recent_tab_helper.cc:158:
test_archiver_.get() ? test_archiver_.release() :
I had to think about this a bit.  |test_archiver_| should really be
test_archiver_factory_, a delegate that returns new test archivers as necessary.

https://codereview.chromium.org/1936613002/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/recent_tab_helper.cc:169: // TODO(dimich):
add UMA, including result.
is there a bug for the uma?

https://codereview.chromium.org/1936613002/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/recent_tab_helper.cc:180:
std::vector<int64_t> pages_to_purge;
this name is a little ambiguous.  It's really a list of previous copies of the
current web contents.  It'd be a little clearer in the later code if that was
called out.

https://codereview.chromium.org/1936613002/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/recent_tab_helper.cc:182:
page_model_->GetOfflineIdsForClientId(client_id());
This would be the first usage of multiple offline pages with a single client ID.
 Wondering if we should just use unique client ids within a last-n namespace. 
You might need to update GetAllPages to take a namespace parameter to make it
work, though.

https://codereview.chromium.org/1936613002/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/recent_tab_helper.cc:194:
pages_to_purge.push_back(offline_id);
I think that we should wait for a successful snapshot before purging other
copies of this page.

https://codereview.chromium.org/1936613002/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/recent_tab_helper.cc:204: for (int count =
0; count < count_to_purge; ++count) {
int -> size_t

[Dmitry Titov, 2016-04-30 00:13:08]

Addressed/replied, PTAL

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

https://codereview.chromium.org/1936613002/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/recent_tab_helper.cc:93: // - snapshot the
current web contents
This is going to change as soon as cache pruning by Yafei is done. It'll be
asynchronous and not block the current thread.

For this case, my thinking for this one was to first avoid situation when for
some reason removal does not work and files simply accumulate as user browses.
That can eventually impair the device. In that case, not producing snapshots is
better than overflowing the device. Occasional missed snapshot is much lesser
evil.

https://codereview.chromium.org/1936613002/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/recent_tab_helper.cc:104: if (!page_model_)
{
On 2016/04/29 20:29:30, dewittj wrote:
> Any reason you do this lazily?

Mostly because of never_do_snapshot - some tab helpers will not need to do work
or cause page model to load. For example, current NTP is one.

https://codereview.chromium.org/1936613002/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/recent_tab_helper.cc:144: // vector of ids.
On 2016/04/29 20:29:30, dewittj wrote:
> This is probably bad behavior, please open a bug on the model.

Done. 608057 and TODO.

https://codereview.chromium.org/1936613002/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/recent_tab_helper.cc:158:
test_archiver_.get() ? test_archiver_.release() :
On 2016/04/29 20:29:30, dewittj wrote:
> I had to think about this a bit.  |test_archiver_| should really be
> test_archiver_factory_, a delegate that returns new test archivers as
necessary.

Done. Added a TestArchiverFactory as a local class.

https://codereview.chromium.org/1936613002/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/recent_tab_helper.cc:169: // TODO(dimich):
add UMA, including result.
On 2016/04/29 20:29:30, dewittj wrote:
> is there a bug for the uma?

Done, added bug number.

https://codereview.chromium.org/1936613002/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/recent_tab_helper.cc:180:
std::vector<int64_t> pages_to_purge;
On 2016/04/29 20:29:31, dewittj wrote:
> this name is a little ambiguous.  It's really a list of previous copies of the
> current web contents.  It'd be a little clearer in the later code if that was
> called out.

Hmm, this is a list of items not necessarily related to this web_contents, since
clientid of all lastN items is the same. It's a simpler variant of the algorithm
that Yafei's cleanup manager will be using. It collects oldest pages and pages
with the same URL. I'm adding comment to this effect.

https://codereview.chromium.org/1936613002/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/recent_tab_helper.cc:182:
page_model_->GetOfflineIdsForClientId(client_id());
On 2016/04/29 20:29:30, dewittj wrote:
> This would be the first usage of multiple offline pages with a single client
ID.
>  Wondering if we should just use unique client ids within a last-n namespace. 
> You might need to update GetAllPages to take a namespace parameter to make it
> work, though.

Coudl you clarify? Is there a plan to remove non-unique client ids? It looks
very convenient here, and having a special method to query for pages by client
Id seems good too.

https://codereview.chromium.org/1936613002/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/recent_tab_helper.cc:194:
pages_to_purge.push_back(offline_id);
On 2016/04/29 20:29:30, dewittj wrote:
> I think that we should wait for a successful snapshot before purging other
> copies of this page.

See comment above. It is probably safer to remove first and only if removal
succeeds to snapshot more. The risk of this is potentially missed snapshots. The
risk of 'snapshot first' is use of all the storage.

https://codereview.chromium.org/1936613002/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/recent_tab_helper.cc:204: for (int count =
0; count < count_to_purge; ++count) {
On 2016/04/29 20:29:30, dewittj wrote:
> int -> size_t
Note count_to_purge can be negative - then the loop should not run. However, the
clang will complain about comparing apples to oranges.

[dewittj, 2016-05-02 19:54:27]

lgtm

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

https://codereview.chromium.org/1936613002/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/recent_tab_helper.cc:93: // - snapshot the
current web contents
On 2016/04/30 00:13:08, Dmitry Titov wrote:
> This is going to change as soon as cache pruning by Yafei is done. It'll be
> asynchronous and not block the current thread.
> 
> For this case, my thinking for this one was to first avoid situation when for
> some reason removal does not work and files simply accumulate as user browses.
> That can eventually impair the device. In that case, not producing snapshots
is
> better than overflowing the device. Occasional missed snapshot is much lesser
> evil. 

Okay.  Could you document that decision here in the comments?

https://codereview.chromium.org/1936613002/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/recent_tab_helper.cc:182:
page_model_->GetOfflineIdsForClientId(client_id());
On 2016/04/30 00:13:08, Dmitry Titov wrote:
> On 2016/04/29 20:29:30, dewittj wrote:
> > This would be the first usage of multiple offline pages with a single client
> ID.
> >  Wondering if we should just use unique client ids within a last-n
namespace. 
> > You might need to update GetAllPages to take a namespace parameter to make
it
> > work, though.
> 
> Coudl you clarify? Is there a plan to remove non-unique client ids? It looks
> very convenient here, and having a special method to query for pages by client
> Id seems good too.

There isn't a plan to remove non-unique client ids.  OK to keep this as is.

https://codereview.chromium.org/1936613002/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/recent_tab_helper.cc:194:
pages_to_purge.push_back(offline_id);
On 2016/04/30 00:13:08, Dmitry Titov wrote:
> On 2016/04/29 20:29:30, dewittj wrote:
> > I think that we should wait for a successful snapshot before purging other
> > copies of this page.
> 
> See comment above. It is probably safer to remove first and only if removal
> succeeds to snapshot more. The risk of this is potentially missed snapshots.
The
> risk of 'snapshot first' is use of all the storage.

Acknowledged.

https://codereview.chromium.org/1936613002/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/recent_tab_helper.cc:204: for (int count =
0; count < count_to_purge; ++count) {
On 2016/04/30 00:13:08, Dmitry Titov wrote:
> On 2016/04/29 20:29:30, dewittj wrote:
> > int -> size_t
> Note count_to_purge can be negative - then the loop should not run. However,
the
> clang will complain about comparing apples to oranges.

Acknowledged.

https://codereview.chromium.org/1936613002/diff/80001/chrome/browser/android/...
File chrome/browser/android/offline_pages/recent_tab_helper.h (right):

https://codereview.chromium.org/1936613002/diff/80001/chrome/browser/android/...
chrome/browser/android/offline_pages/recent_tab_helper.h:52: // A factory that
supplies the instances of an OfflienPageArchiver.
nit: spelling offlien

[Dmitry Titov, 2016-05-03 17:04:04]

The CQ bit was checked by dimich@chromium.org

[Dmitry Titov, 2016-05-03 17:04:05]

The patchset sent to the CQ was uploaded after l-g-t-m from dewittj@chromium.org
Link to the patchset: https://codereview.chromium.org/1936613002/#ps100001
(title: "comment update")

[commit-bot: I haz the power, 2016-05-03 17:04:17]

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

[commit-bot: I haz the power, 2016-05-03 17:24:42]

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

[commit-bot: I haz the power, 2016-05-03 17:24:43]

Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Dmitry Titov, 2016-05-04 21:28:38]

The CQ bit was checked by dimich@chromium.org

[Dmitry Titov, 2016-05-04 21:28:38]

The patchset sent to the CQ was uploaded after l-g-t-m from dewittj@chromium.org
Link to the patchset: https://codereview.chromium.org/1936613002/#ps120001
(title: "updated")

[commit-bot: I haz the power, 2016-05-04 21:29:14]

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

[commit-bot: I haz the power, 2016-05-04 22:30:18]

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

[commit-bot: I haz the power, 2016-05-04 22:30:19]

Try jobs failed on following builders:
  android_clang_dbg_recipe on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)

[Dmitry Titov, 2016-05-04 22:31:38]

The CQ bit was checked by dimich@chromium.org

[Dmitry Titov, 2016-05-04 22:31:38]

The patchset sent to the CQ was uploaded after l-g-t-m from dewittj@chromium.org
Link to the patchset: https://codereview.chromium.org/1936613002/#ps140001
(title: "clang is my friend")

[commit-bot: I haz the power, 2016-05-04 22:32:28]

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

[commit-bot: I haz the power, 2016-05-05 00:01:54]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-05-05 00:02:49]

Description was changed from

==========
Implementing recent pages snapshot capture.
This adds functinoality to RecenttabHelper that:
- uses SnapshotController to detect when page should be captrued.
- uses simple algorithm to remove previous snapshot and limit number of current
snapshots
- captures the page using MHTML Archiver and Offline Page Model.

Also has a couple of simple tests.

BUG=606106
==========

to

==========
Implementing recent pages snapshot capture.
This adds functinoality to RecenttabHelper that:
- uses SnapshotController to detect when page should be captrued.
- uses simple algorithm to remove previous snapshot and limit number of current
snapshots
- captures the page using MHTML Archiver and Offline Page Model.

Also has a couple of simple tests.

BUG=606106

Committed: https://crrev.com/4410eaea5c56187d7a2d4c72694935245368ac29
Cr-Commit-Position: refs/heads/master@{#391702}
==========

[commit-bot: I haz the power, 2016-05-05 00:02:51]

Patchset 8 (id:??) landed as
https://crrev.com/4410eaea5c56187d7a2d4c72694935245368ac29
Cr-Commit-Position: refs/heads/master@{#391702}