SnapshotController implementation. It will be used in WebContentsObservers for Offline Pages - to d…

SnapshotController implementation. It will be used in WebContentsObservers for Offline Pages - to detect the right moment to make a snapshot.
It will take input and generate a sequence of StartSnapshot calls (typically 1 or 2) to its Client.
This is initial implementation, in the future we might update the logic to rely not on a timer and onload event,
but rather on some more specific indicators (meaningful layout or proportion of resources loaded).

BUG=606106
Committed: https://crrev.com/18a6b28cc90dfc6ecfeb075804a67f74e5c42f9d
Cr-Commit-Position: refs/heads/master@{#389843}

[Dmitry Titov, 2016-04-23 02:04:12]

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

[fgorski, 2016-04-25 04:28:46]

https://codereview.chromium.org/1920603002/diff/60001/components/offline_page...
File components/offline_pages/snapshot_controller.cc (right):

https://codereview.chromium.org/1920603002/diff/60001/components/offline_page...
components/offline_pages/snapshot_controller.cc:8: #include
"base/files/file_util.h"
do you use it here?

https://codereview.chromium.org/1920603002/diff/60001/components/offline_page...
components/offline_pages/snapshot_controller.cc:14: // Delay, in milliseconds,
between the main document parsed and snapshot.
nit: main document parsed event and

https://codereview.chromium.org/1920603002/diff/60001/components/offline_page...
components/offline_pages/snapshot_controller.cc:28: state_(State::kStateReady),
I see a peculiarity here, since this is an enum class, you will use it scoped
with State:: always, why not:
State::kReady
State::kSnapshotPending
...

Instead of repeating State?

https://codereview.chromium.org/1920603002/diff/60001/components/offline_page...
components/offline_pages/snapshot_controller.cc:43: if (state_ !=
State::kStateStopped)
It feels a bit weird that you can have a snapshot in progress in both Pending
and Stopped states.
Was there no reason to have it be more precise?

https://codereview.chromium.org/1920603002/diff/60001/components/offline_page...
File components/offline_pages/snapshot_controller.h (right):

https://codereview.chromium.org/1920603002/diff/60001/components/offline_page...
components/offline_pages/snapshot_controller.h:21: //   abandoned w/o waiting
for the completion of the current one.
The part staring with "w/o" is not clear.
I presume you mean: the currently worked on snapshot is allowed to complete,
while the new one is ignored.

https://codereview.chromium.org/1920603002/diff/60001/components/offline_page...
components/offline_pages/snapshot_controller.h:22: // - Some signals cause
'last' snapshot - no signals after those will cause
some signals prevent more snapshots to be taken.

https://codereview.chromium.org/1920603002/diff/60001/components/offline_page...
components/offline_pages/snapshot_controller.h:55: // invoked from the
SnapshotController until current session is Reset().
Is this meant to be called by the client? (I think so, based on the next
question.)

If not, is there a way to tell client that snapshot controller stopped?

Please document.

https://codereview.chromium.org/1920603002/diff/60001/components/offline_page...
components/offline_pages/snapshot_controller.h:60: void
PreviousSnapshotCompleted();
Since your state is kStateSnapshotPending I suggest this be named:
PendingSnapshotCompleteted for consistency.

https://codereview.chromium.org/1920603002/diff/60001/components/offline_page...
File components/offline_pages/snapshot_controller_unittest.cc (right):

https://codereview.chromium.org/1920603002/diff/60001/components/offline_page...
components/offline_pages/snapshot_controller_unittest.cc:1: // Copyright 2015
The Chromium Authors. All rights reserved.
2016

[Dmitry Titov, 2016-04-26 01:45:30]

Thanks for review!  
Addressed, PTAL.

https://codereview.chromium.org/1920603002/diff/60001/components/offline_page...
File components/offline_pages/snapshot_controller.cc (right):

https://codereview.chromium.org/1920603002/diff/60001/components/offline_page...
components/offline_pages/snapshot_controller.cc:8: #include
"base/files/file_util.h"
On 2016/04/25 04:28:45, fgorski wrote:
> do you use it here?

Done.

https://codereview.chromium.org/1920603002/diff/60001/components/offline_page...
components/offline_pages/snapshot_controller.cc:14: // Delay, in milliseconds,
between the main document parsed and snapshot.
On 2016/04/25 04:28:45, fgorski wrote:
> nit: main document parsed event and 

Done.

https://codereview.chromium.org/1920603002/diff/60001/components/offline_page...
components/offline_pages/snapshot_controller.cc:28: state_(State::kStateReady),
On 2016/04/25 04:28:45, fgorski wrote:
> I see a peculiarity here, since this is an enum class, you will use it scoped
> with State:: always, why not:
> State::kReady
> State::kSnapshotPending
> ...
> 
> Instead of repeating State?

Done.

https://codereview.chromium.org/1920603002/diff/60001/components/offline_page...
components/offline_pages/snapshot_controller.cc:43: if (state_ !=
State::kStateStopped)
On 2016/04/25 04:28:45, fgorski wrote:
> It feels a bit weird that you can have a snapshot in progress in both Pending
> and Stopped states.
> Was there no reason to have it be more precise?

Stopped is the terminal state which prevents new snapshots from being started.
The snapshot started before may still be in progress. Added comment and used
early return in hope it'll be clearer.

https://codereview.chromium.org/1920603002/diff/60001/components/offline_page...
File components/offline_pages/snapshot_controller.h (right):

https://codereview.chromium.org/1920603002/diff/60001/components/offline_page...
components/offline_pages/snapshot_controller.h:21: //   abandoned w/o waiting
for the completion of the current one.
On 2016/04/25 04:28:45, fgorski wrote:
> The part staring with "w/o" is not clear.
> I presume you mean: the currently worked on snapshot is allowed to complete,
> while the new one is ignored.

Done.

https://codereview.chromium.org/1920603002/diff/60001/components/offline_page...
components/offline_pages/snapshot_controller.h:22: // - Some signals cause
'last' snapshot - no signals after those will cause
On 2016/04/25 04:28:45, fgorski wrote:
> some signals prevent more snapshots to be taken.

Done.

https://codereview.chromium.org/1920603002/diff/60001/components/offline_page...
components/offline_pages/snapshot_controller.h:55: // invoked from the
SnapshotController until current session is Reset().
On 2016/04/25 04:28:46, fgorski wrote:
> Is this meant to be called by the client? (I think so, based on the next
> question.)
> 
> If not, is there a way to tell client that snapshot controller stopped?
> 
> Please document.

Done.

https://codereview.chromium.org/1920603002/diff/60001/components/offline_page...
components/offline_pages/snapshot_controller.h:60: void
PreviousSnapshotCompleted();
On 2016/04/25 04:28:45, fgorski wrote:
> Since your state is kStateSnapshotPending I suggest this be named:
> PendingSnapshotCompleteted for consistency.

Done.

https://codereview.chromium.org/1920603002/diff/60001/components/offline_page...
File components/offline_pages/snapshot_controller_unittest.cc (right):

https://codereview.chromium.org/1920603002/diff/60001/components/offline_page...
components/offline_pages/snapshot_controller_unittest.cc:1: // Copyright 2015
The Chromium Authors. All rights reserved.
On 2016/04/25 04:28:46, fgorski wrote:
> 2016

Done.

[fgorski, 2016-04-26 02:45:57]

lgtm, please fix the const name.

https://codereview.chromium.org/1920603002/diff/80001/components/offline_page...
File components/offline_pages/snapshot_controller.cc (right):

https://codereview.chromium.org/1920603002/diff/80001/components/offline_page...
components/offline_pages/snapshot_controller.cc:16: const size_t
DELAY_AFTER_DOCUMENT_AVAILABLE = 7000;
kDelayAfterDocumentAvailable  -- I believe style guide mandates that for C++

[Dmitry Titov, 2016-04-26 18:20:21]

The CQ bit was checked by dimich@chromium.org

[Dmitry Titov, 2016-04-26 18:20:21]

The patchset sent to the CQ was uploaded after l-g-t-m from fgorski@chromium.org
Link to the patchset: https://codereview.chromium.org/1920603002/#ps100001
(title: "renaming constant")

[commit-bot: I haz the power, 2016-04-26 18:20:45]

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

[commit-bot: I haz the power, 2016-04-26 19:15:18]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-04-26 19:16:50]

Description was changed from

==========
SnapshotController implementation. It will be used in WebContentsObservers for
Offline Pages - to detect the right moment to make a snapshot.
It will take input and generate a sequence of StartSnapshot calls (typically 1
or 2) to its Client.
This is initial implementation, in the future we might update the logic to rely
not on a timer and onload event,
but rather on some more specific indicators (meaningful layout or proportion of
resources loaded).

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

to

==========
SnapshotController implementation. It will be used in WebContentsObservers for
Offline Pages - to detect the right moment to make a snapshot.
It will take input and generate a sequence of StartSnapshot calls (typically 1
or 2) to its Client.
This is initial implementation, in the future we might update the logic to rely
not on a timer and onload event,
but rather on some more specific indicators (meaningful layout or proportion of
resources loaded).

BUG=606106

Committed: https://crrev.com/18a6b28cc90dfc6ecfeb075804a67f74e5c42f9d
Cr-Commit-Position: refs/heads/master@{#389843}
==========

[commit-bot: I haz the power, 2016-04-26 19:16:51]

Patchset 6 (id:??) landed as
https://crrev.com/18a6b28cc90dfc6ecfeb075804a67f74e5c42f9d
Cr-Commit-Position: refs/heads/master@{#389843}