Offline Page Cache uses user interaction triggers for saving pages.

Offline Page Cache uses user interaction triggers for saving pages.

This switches Offline Page Cache (aka last_n) to listen to user interaction
triggers instead of navigation events for starting page snapshots. The latter
are still monitored and important to allow the quality estimation of the page
being loaded but they just don't cause snapshot creation by themselves anymore.

Tests were also updated to work with this new mechanism.

BUG=678367
Review-Url: https://codereview.chromium.org/2602473004
Cr-Commit-Position: refs/heads/master@{#445183}
Committed: https://chromium.googlesource.com/chromium/src/+/6b26b7a9ba6b62730399c632dac0fcbfce0a15bd

[carlosk, 2016-12-23 02:35:57]

The CQ bit was checked by carlosk@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-23 02:36:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-23 03:50:04]

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

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

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

[carlosk, 2016-12-30 03:31:17]

The CQ bit was checked by carlosk@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-30 03:31:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-30 04:59:55]

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

[commit-bot: I haz the power, 2016-12-30 04:59:56]

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

[carlosk, 2017-01-04 20:43:03]

Description was changed from

==========
Offline Page Cache uses user interaction triggers for saving pages.

This is a first take on switching Offline Page Cache to use user interaction
triggers instead of the navigation based ones used until now. It still keep the
old method in place and being the default for comparison reasons. This might
not land on its current for as there might be some edge cases I haven't
forseen.

BUG=TBD
==========

to

==========
Offline Page Cache uses user interaction triggers for saving pages.

This is a first take on switching Offline Page Cache to use user interaction
triggers instead of the navigation based ones used until now. It still keep the
old method in place and being the default for comparison reasons.

For this method to become the new default we need to at least IMO:
- Add tests.
- Handle the "tab is about to be closed" case that happens on Android.

BUG=TBD
==========

[carlosk, 2017-01-04 20:49:31]

Description was changed from

==========
Offline Page Cache uses user interaction triggers for saving pages.

This is a first take on switching Offline Page Cache to use user interaction
triggers instead of the navigation based ones used until now. It still keep the
old method in place and being the default for comparison reasons.

For this method to become the new default we need to at least IMO:
- Add tests.
- Handle the "tab is about to be closed" case that happens on Android.

BUG=TBD
==========

to

==========
Offline Page Cache uses user interaction triggers for saving pages.

This is a first take on switching Offline Page Cache to use user interaction
triggers instead of the navigation based ones used until now. It still keep the
old method in place and being the default for comparison reasons.

For this method to become the new default we need to at least IMO:
- Add tests.
- Handle the "tab is about to be closed" case that happens on Android.

BUG=678367
==========

[carlosk, 2017-01-04 20:52:47]

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

[carlosk, 2017-01-04 20:52:48]

dimich@, dewittj@: PTAL.

You might consider this is not land-able on its current form. Let's see if it's
workable but otherwise we might need to start with the RecentTabHelper refactor.

Also there might be edge cases I haven't foreseen. Would appreciate your careful
analysis dimich@. ;)

It doesn't look like the linux_android_rel_ng failure -- two tests are crashing,
no logs -- is related to my changes. I'm able to repro it locally *with or
without* my patch applied so I couldn't pint point its cause.

See description and code comments for further details.

https://codereview.chromium.org/2602473004/diff/40001/chrome/browser/android/...
File chrome/browser/android/offline_pages/recent_tab_helper.cc (left):

https://codereview.chromium.org/2602473004/diff/40001/chrome/browser/android/...
chrome/browser/android/offline_pages/recent_tab_helper.cc:152: }
The same page check now happens earlier and the download_info_ check happens
inside ReportDownloadStatusToRequestCoordinator anyway.

https://codereview.chromium.org/2602473004/diff/40001/chrome/browser/android/...
File chrome/browser/android/offline_pages/recent_tab_helper.cc (right):

https://codereview.chromium.org/2602473004/diff/40001/chrome/browser/android/...
chrome/browser/android/offline_pages/recent_tab_helper.cc:82:
weak_ptr_factory_.InvalidateWeakPtrs();
This change is what justifies the change of if-checks to DCHECKs for the
callbacks further below. I couldn't find any use case where resetting
download_info_ (just below) wouldn't justify invalidating all weak pointers
(what IIUC would avoid any future scheduled callback calls).

https://codereview.chromium.org/2602473004/diff/40001/chrome/browser/android/...
chrome/browser/android/offline_pages/recent_tab_helper.cc:136:
navigation_handle->IsSamePage() || !navigation_handle->HasCommitted()) {
IsExternalProtocol: if a navigation is handled externally the current page
contents should not change so we should not snapshot IMO. It might be the case
that this and HasCommitted are muttually exclusive (I'm not sure) so this is
just-in-case.

IsSamePage: same page navigations shouldn't change the page contents either.
There's even some pages (I think YouTube video pages are one example) that
execute a same page navigation during a regular load through javascript.

https://codereview.chromium.org/2602473004/diff/40001/chrome/browser/android/...
chrome/browser/android/offline_pages/recent_tab_helper.cc:141: return;
It might be better to move this up even further. If snapshots_enabled_ is true
we won't be doing anything anyway.

[Dmitry Titov, 2017-01-05 21:50:03]

Initial comments.

I like the idea of adding a second separate flag for trigger-based capture.

However, the logic in recent_tab_helper is getting out of hand. As it is, it
might have a few bugs that are hard to see. What would it take to change the
helper along lines we have discussed, with main goal to make it
easy-to-understand?

https://codereview.chromium.org/2602473004/diff/40001/chrome/browser/android/...
File chrome/browser/android/offline_pages/recent_tab_helper.cc (left):

https://codereview.chromium.org/2602473004/diff/40001/chrome/browser/android/...
chrome/browser/android/offline_pages/recent_tab_helper.cc:152: }
On 2017/01/04 20:52:48, carlosk wrote:
> The same page check now happens earlier and the download_info_ check happens
> inside ReportDownloadStatusToRequestCoordinator anyway.

Acknowledged.

https://codereview.chromium.org/2602473004/diff/40001/chrome/browser/android/...
File chrome/browser/android/offline_pages/recent_tab_helper.cc (right):

https://codereview.chromium.org/2602473004/diff/40001/chrome/browser/android/...
chrome/browser/android/offline_pages/recent_tab_helper.cc:82:
weak_ptr_factory_.InvalidateWeakPtrs();
On 2017/01/04 20:52:48, carlosk wrote:
> This change is what justifies the change of if-checks to DCHECKs for the
> callbacks further below. I couldn't find any use case where resetting
> download_info_ (just below) wouldn't justify invalidating all weak pointers
> (what IIUC would avoid any future scheduled callback calls).

Acknowledged.

https://codereview.chromium.org/2602473004/diff/40001/chrome/browser/android/...
chrome/browser/android/offline_pages/recent_tab_helper.cc:141: return;
On 2017/01/04 20:52:48, carlosk wrote:
> It might be better to move this up even further. If snapshots_enabled_ is true
> we won't be doing anything anyway.

Acknowledged.

https://codereview.chromium.org/2602473004/diff/40001/chrome/browser/android/...
chrome/browser/android/offline_pages/recent_tab_helper.cc:179:
download_info_.reset();
I think this reset() should happen unconditionally before this big if(can_save).
Otherwise there is a branch in these ifs that keeps the old download_info_,
which should not happen if we actually navigated.

[dewittj, 2017-01-07 00:50:09]

agree with dmitry that some logic separation would be a big win here.

https://codereview.chromium.org/2602473004/diff/40001/chrome/browser/android/...
File chrome/browser/android/offline_pages/recent_tab_helper.cc (right):

https://codereview.chromium.org/2602473004/diff/40001/chrome/browser/android/...
chrome/browser/android/offline_pages/recent_tab_helper.cc:136:
navigation_handle->IsSamePage() || !navigation_handle->HasCommitted()) {
On 2017/01/04 20:52:48, carlosk wrote:
> IsExternalProtocol: if a navigation is handled externally the current page
> contents should not change so we should not snapshot IMO. It might be the case
> that this and HasCommitted are muttually exclusive (I'm not sure) so this is
> just-in-case.
> 
> IsSamePage: same page navigations shouldn't change the page contents either.
> There's even some pages (I think YouTube video pages are one example) that
> execute a same page navigation during a regular load through javascript.

This change should probably be extracted into a small CL

[carlosk, 2017-01-17 23:16:27]

Thanks. Just replying to comments and uploading last minor changes before fully
removing the listening of navigation events for last N.

https://codereview.chromium.org/2602473004/diff/40001/chrome/browser/android/...
File chrome/browser/android/offline_pages/recent_tab_helper.cc (right):

https://codereview.chromium.org/2602473004/diff/40001/chrome/browser/android/...
chrome/browser/android/offline_pages/recent_tab_helper.cc:136:
navigation_handle->IsSamePage() || !navigation_handle->HasCommitted()) {
On 2017/01/07 00:50:09, dewittj wrote:
> On 2017/01/04 20:52:48, carlosk wrote:
> > IsExternalProtocol: if a navigation is handled externally the current page
> > contents should not change so we should not snapshot IMO. It might be the
case
> > that this and HasCommitted are muttually exclusive (I'm not sure) so this is
> > just-in-case.
> > 
> > IsSamePage: same page navigations shouldn't change the page contents either.
> > There's even some pages (I think YouTube video pages are one example) that
> > execute a same page navigation during a regular load through javascript.
> 
> This change should probably be extracted into a small CL

Moved this and many others to a new CL: https://crrev.com/2635023005. Will
rebase into that change in the next patch set with the new approach.

https://codereview.chromium.org/2602473004/diff/40001/chrome/browser/android/...
chrome/browser/android/offline_pages/recent_tab_helper.cc:179:
download_info_.reset();
On 2017/01/05 21:50:03, Dmitry Titov wrote:
> I think this reset() should happen unconditionally before this big
if(can_save).
> Otherwise there is a branch in these ifs that keeps the old download_info_,
> which should not happen if we actually navigated.

Done. Indeed, every time we invalidate pointers we should also reset
download_info_.

[carlosk, 2017-01-19 02:43:39]

The CQ bit was checked by carlosk@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-19 02:44:04]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-19 02:45:55]

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

[commit-bot: I haz the power, 2017-01-19 02:45:56]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[carlosk, 2017-01-19 18:24:15]

Patchset #6 (id:100001) has been deleted

[carlosk, 2017-01-19 19:09:44]

Description was changed from

==========
Offline Page Cache uses user interaction triggers for saving pages.

This is a first take on switching Offline Page Cache to use user interaction
triggers instead of the navigation based ones used until now. It still keep the
old method in place and being the default for comparison reasons.

For this method to become the new default we need to at least IMO:
- Add tests.
- Handle the "tab is about to be closed" case that happens on Android.

BUG=678367
==========

to

==========
Offline Page Cache uses user interaction triggers for saving pages.

This switches Offline Page Cache (aka last_n) to listen to user interaction
triggers instead of navigation events for starting page snapshots. The latter
are still monitored and important to allow the quality estimation of the page
being loaded but they just don't cause snapshot creation by themselves anymore.

Tests were also updated to work with this new mechanism.

BUG=678367
==========

[carlosk, 2017-01-20 02:50:31]

The CQ bit was checked by carlosk@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-20 02:50:57]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[carlosk, 2017-01-20 02:52:43]

Patchset #7 (id:140001) has been deleted

[carlosk, 2017-01-20 02:56:03]

dimich@: PTAL.

dewittj@: FYI.

This patch set implements the new triggers and fully removes the navigation ones
for last_n.

Note that now the RecentTabHelper callback chain carries it's data within the
closures (especially the "info" instance) being almost stateless (until the very
last step where behavior forks). So simultaneous last_n and download snapshots
can now be processed.

[commit-bot: I haz the power, 2017-01-20 03:31:19]

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

[commit-bot: I haz the power, 2017-01-20 03:31:19]

Dry run: This issue passed the CQ dry run.

[Dmitry Titov, 2017-01-20 06:25:36]

As far as I can see it's LGTM.
I tried to 'simulate' several scenarios through this code and I did not find any
issues.

One question, just want to make sure I understand your intentions correctly
here:

https://codereview.chromium.org/2602473004/diff/160001/chrome/browser/android...
File chrome/browser/android/offline_pages/recent_tab_helper.cc (right):

https://codereview.chromium.org/2602473004/diff/160001/chrome/browser/android...
chrome/browser/android/offline_pages/recent_tab_helper.cc:213: current_quality
== last_n_latest_saved_quality_) {
This looks like we store the quality of the latest last_n snapshot and then
would snapshot again when quality goes higher. The latter would be a
'navigational trigger' and come from SnapshotController. I am not sure there is
the code that makes another last_n snapshot on a signal from SnapshotController.
Do you expect WasHidden to come potentially multiple times and opportunistically
capture a better snapshot?

[carlosk, 2017-01-20 07:49:41]

Thanks.

https://codereview.chromium.org/2602473004/diff/160001/chrome/browser/android...
File chrome/browser/android/offline_pages/recent_tab_helper.cc (right):

https://codereview.chromium.org/2602473004/diff/160001/chrome/browser/android...
chrome/browser/android/offline_pages/recent_tab_helper.cc:153:
weak_ptr_factory_.InvalidateWeakPtrs();
Note that now this is the only "normal" use case for pointers invalidation (full
callbacks interruption). Receiving a downloads request doesn't do it anymore.
This is a consequence of the new "stateless" callback implementations that allow
more isolation between the two requesters and between multiple downloads
requests.

Explanations to the quotes:

- "normal" invalidation: the single other non-normal usage of pointers
invalidation is on WebContentsDestroyed as at that moment we want to stop
everything.

- "stateless" callbacks: they are really stateless within RecentTabHelper for
the downloads case. They are not stateless for downloads as the final step still
access the info member variable but that is desirable as it is being used to
avoid overlapping snapshots.

https://codereview.chromium.org/2602473004/diff/160001/chrome/browser/android...
chrome/browser/android/offline_pages/recent_tab_helper.cc:213: current_quality
== last_n_latest_saved_quality_) {
On 2017/01/20 06:25:36, Dmitry Titov wrote:
> This looks like we store the quality of the latest last_n snapshot and then
> would snapshot again when quality goes higher. The latter would be a
> 'navigational trigger' and come from SnapshotController. I am not sure there
is
> the code that makes another last_n snapshot on a signal from
SnapshotController.
> Do you expect WasHidden to come potentially multiple times and
opportunistically
> capture a better snapshot?

Yes, this last statement is correct. With this implementation the only last_n
trigger for starting a snapshot is TabHidden and nothing else.

That quality value is stored when a TabHidden call creates a snapshot. It is
then compared to the updated page quality in further calls and another snapshot
is only allowed in case the latest quality is higher.

[carlosk, 2017-01-20 21:19:21]

The CQ bit was checked by carlosk@chromium.org

[carlosk, 2017-01-20 21:19:22]

The patchset sent to the CQ was uploaded after l-g-t-m from dimich@chromium.org
Link to the patchset: https://codereview.chromium.org/2602473004/#ps180001
(title: "Minor changes.")

[commit-bot: I haz the power, 2017-01-20 21:20:15]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-20 22:21:11]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1484947161608230,
"parent_rev": "b281bde16d4878454ed00e1335c80100c414a920", "commit_rev":
"6b26b7a9ba6b62730399c632dac0fcbfce0a15bd"}

[commit-bot: I haz the power, 2017-01-20 22:21:49]

Description was changed from

==========
Offline Page Cache uses user interaction triggers for saving pages.

This switches Offline Page Cache (aka last_n) to listen to user interaction
triggers instead of navigation events for starting page snapshots. The latter
are still monitored and important to allow the quality estimation of the page
being loaded but they just don't cause snapshot creation by themselves anymore.

Tests were also updated to work with this new mechanism.

BUG=678367
==========

to

==========
Offline Page Cache uses user interaction triggers for saving pages.

This switches Offline Page Cache (aka last_n) to listen to user interaction
triggers instead of navigation events for starting page snapshots. The latter
are still monitored and important to allow the quality estimation of the page
being loaded but they just don't cause snapshot creation by themselves anymore.

Tests were also updated to work with this new mechanism.

BUG=678367

Review-Url: https://codereview.chromium.org/2602473004
Cr-Commit-Position: refs/heads/master@{#445183}
Committed:
https://chromium.googlesource.com/chromium/src/+/6b26b7a9ba6b62730399c632dac0...
==========

[commit-bot: I haz the power, 2017-01-20 22:21:50]

Committed patchset #8 (id:180001) as
https://chromium.googlesource.com/chromium/src/+/6b26b7a9ba6b62730399c632dac0...

[dewittj, 2017-01-20 22:41:26]

generally lgtm

https://codereview.chromium.org/2602473004/diff/180001/chrome/browser/android...
File chrome/browser/android/offline_pages/recent_tab_helper.cc (right):

https://codereview.chromium.org/2602473004/diff/180001/chrome/browser/android...
chrome/browser/android/offline_pages/recent_tab_helper.cc:191:
downloads_latest_snapshot_info_.get());
reset both snapshot infos?

https://codereview.chromium.org/2602473004/diff/180001/chrome/browser/android...
chrome/browser/android/offline_pages/recent_tab_helper.cc:197: // saving a
snapshot is probably useless (low probability of the user undoing
Do you have a metric to back up your parenthetical?

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

https://codereview.chromium.org/2602473004/diff/180001/chrome/browser/android...
chrome/browser/android/offline_pages/recent_tab_helper.h:25: using PageQuality =
SnapshotController::PageQuality;
Does this pollute the namespace? in other words, could someone else refer to
offline_pages::PageQuality?  If so we should not alias this here.

[carlosk, 2017-01-20 23:04:35]

Thanks.

https://codereview.chromium.org/2602473004/diff/180001/chrome/browser/android...
File chrome/browser/android/offline_pages/recent_tab_helper.cc (right):

https://codereview.chromium.org/2602473004/diff/180001/chrome/browser/android...
chrome/browser/android/offline_pages/recent_tab_helper.cc:191:
downloads_latest_snapshot_info_.get());
On 2017/01/20 22:41:26, dewittj wrote:
> reset both snapshot infos?

This would be a 2nd step safe guard.

The line below is the 1st step safeguard as this instance should be destroyed
anyway along with the WebContents. I added it just in case the destruction
process takes more than one posted-task so we interrupt any ongoing operations.
Resetting those would only help if yet another request happened in the meantime.

https://codereview.chromium.org/2602473004/diff/180001/chrome/browser/android...
chrome/browser/android/offline_pages/recent_tab_helper.cc:197: // saving a
snapshot is probably useless (low probability of the user undoing
On 2017/01/20 22:41:26, dewittj wrote:
> Do you have a metric to back up your parenthetical?

Yes. Metrics show that tab closures are undone less than .5% of times.

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

https://codereview.chromium.org/2602473004/diff/180001/chrome/browser/android...
chrome/browser/android/offline_pages/recent_tab_helper.h:25: using PageQuality =
SnapshotController::PageQuality;
On 2017/01/20 22:41:26, dewittj wrote:
> Does this pollute the namespace? in other words, could someone else refer to
> offline_pages::PageQuality?  If so we should not alias this here.

I'm unsure but I agree it's a problem. I'll move this into the .CC and use the
full namespace name here.