[Offline Pages] SnapshotController support for delay after onLoadCompleted and also parameterized c…

[Offline Pages] SnapshotController support for delay after onLoadCompleted and also parameterized constructor so different uses could tune delays.

BUG=649711
Committed: https://crrev.com/869b93ac0e1bfc4789055555d9b194d2986bdaa6
Cr-Commit-Position: refs/heads/master@{#426892}

[dougarnett, 2016-09-29 16:22:35]

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

[commit-bot: I haz the power, 2016-09-29 16:22:47]

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

[dougarnett, 2016-09-29 16:26:54]

dougarnett@chromium.org changed reviewers:
+ dimich@chromium.org, petewil@chromium.org

[dougarnett, 2016-09-29 16:26:54]

Pete, if not viable to leverage FirstMeaningfulPaint for 55, here is a
prospective update to SnapshotController to make it more configurable that you
might leverage to set different delays for Background Loading (from Downloads
tab helper or Last-N).

[Dmitry Titov, 2016-09-29 16:30:54]

I have a feeling it could be better if we had an interface for
SnapshotController and 2 Impls - one for automatic snmapshot (last1) and one for
background snapshot - since apparently those are different scenarios.

It might be better than effectively bake 2 behaviors into one code. Also may be
easier for testing.

WDYT?

[dougarnett, 2016-09-29 16:40:27]

On 2016/09/29 16:30:54, Dmitry Titov wrote:
> I have a feeling it could be better if we had an interface for
> SnapshotController and 2 Impls - one for automatic snmapshot (last1) and one
for
> background snapshot - since apparently those are different scenarios.
> 
> It might be better than effectively bake 2 behaviors into one code. Also may
be
> easier for testing.
> 
> WDYT?

Sure, it would make it easier to swap in better ones later too.
This was just resuscitating the change I had sitting around in case helpful.

[commit-bot: I haz the power, 2016-09-29 17:32:37]

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

[commit-bot: I haz the power, 2016-09-29 17:32:39]

Dry run: This issue passed the CQ dry run.

[Pete Williamson, 2016-10-21 00:39:16]

On 2016/09/29 16:40:27, dougarnett wrote:
> On 2016/09/29 16:30:54, Dmitry Titov wrote:
> > I have a feeling it could be better if we had an interface for
> > SnapshotController and 2 Impls - one for automatic snmapshot (last1) and one
> for
> > background snapshot - since apparently those are different scenarios.
> > 
> > It might be better than effectively bake 2 behaviors into one code. Also may
> be
> > easier for testing.
> > 
> > WDYT?
> 
> Sure, it would make it easier to swap in better ones later too.
> This was just resuscitating the change I had sitting around in case helpful.

This is more advanced than what I have, let's proceed with this changelist. It
looks good to me so far, but I'll wait until you've addressed Dimich's comments
before stamping it.

[dougarnett, 2016-10-21 15:37:10]

On 2016/10/21 00:39:16, Pete Williamson wrote:
> On 2016/09/29 16:40:27, dougarnett wrote:
> > On 2016/09/29 16:30:54, Dmitry Titov wrote:
> > > I have a feeling it could be better if we had an interface for
> > > SnapshotController and 2 Impls - one for automatic snmapshot (last1) and
one
> > for
> > > background snapshot - since apparently those are different scenarios.
> > > 
> > > It might be better than effectively bake 2 behaviors into one code. Also
may
> > be
> > > easier for testing.
> > > 
> > > WDYT?
> > 
> > Sure, it would make it easier to swap in better ones later too.
> > This was just resuscitating the change I had sitting around in case helpful.
> 
> This is more advanced than what I have, let's proceed with this changelist. It
> looks good to me so far, but I'll wait until you've addressed Dimich's
comments
> before stamping it.

Ok.

Dmitry, I think parameterizing this SnapshotController is a simpler change and
allows us to move forward (vs. creating a separate implementation and possibly
putting behind an interface).

Once we really have an more different approach (different signals) and
implementation
may be the time to decide if interface makes vs. just two different
implementations
used separately for foreground and background.

[Dmitry Titov, 2016-10-21 18:21:03]

lgtm with a nit

https://codereview.chromium.org/2380093002/diff/1/components/offline_pages/sn...
File components/offline_pages/snapshot_controller.cc (right):

https://codereview.chromium.org/2380093002/diff/1/components/offline_pages/sn...
components/offline_pages/snapshot_controller.cc:74: // is up, or if the "load"
event is dispatched in the main frame.
This comment should be updated. Maybe just removed :)

[Pete Williamson, 2016-10-21 18:29:20]

lgtm

[dougarnett, 2016-10-21 18:40:48]

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

[dougarnett, 2016-10-21 18:41:07]

https://codereview.chromium.org/2380093002/diff/1/components/offline_pages/sn...
File components/offline_pages/snapshot_controller.cc (right):

https://codereview.chromium.org/2380093002/diff/1/components/offline_pages/sn...
components/offline_pages/snapshot_controller.cc:74: // is up, or if the "load"
event is dispatched in the main frame.
On 2016/10/21 18:21:03, Dmitry Titov wrote:
> This comment should be updated. Maybe just removed :)

Done.

[dougarnett, 2016-10-21 18:41:19]

The CQ bit was unchecked by dougarnett@chromium.org

[dougarnett, 2016-10-21 18:41:24]

The CQ bit was checked by dougarnett@chromium.org

[dougarnett, 2016-10-21 18:41:24]

The patchset sent to the CQ was uploaded after l-g-t-m from
petewil@chromium.org, dimich@chromium.org
Link to the patchset: https://codereview.chromium.org/2380093002/#ps20001
(title: "Fixed a comment")

[commit-bot: I haz the power, 2016-10-21 18:41:29]

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-10-21 21:34:43]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-10-21 21:37:30]

Description was changed from

==========
[Offline Pages] SnapshotController support for delay after onLoadCompleted and
also parameterized constructor so different uses could tune delays.

BUG=649711
==========

to

==========
[Offline Pages] SnapshotController support for delay after onLoadCompleted and
also parameterized constructor so different uses could tune delays.

BUG=649711

Committed: https://crrev.com/869b93ac0e1bfc4789055555d9b194d2986bdaa6
Cr-Commit-Position: refs/heads/master@{#426892}
==========

[commit-bot: I haz the power, 2016-10-21 21:37:31]

Patchset 2 (id:??) landed as
https://crrev.com/869b93ac0e1bfc4789055555d9b194d2986bdaa6
Cr-Commit-Position: refs/heads/master@{#426892}