Integrates the SnapshotController into the PrerenderingLoader to provide logic for determining when…

Integrates the SnapshotController into the PrerenderingLoader to provide logic for determining when the right moment is to consider the page loaded and ready for snapshotting.
BUG=601173
Committed: https://crrev.com/d83899ec45e736c2a9289c1fe0b61572e31a112d
Cr-Commit-Position: refs/heads/master@{#398994}

[dougarnett, 2016-06-08 23:55:39]

Description was changed from

==========
Integrates the SnapshotController into the PrerenderingLoader to provide logic
for determining when the right moment is to consider the page loaded and ready
for snapshotting.
BUG=601173
==========

to

==========
Integrates the SnapshotController into the PrerenderingLoader to provide logic
for determining when the right moment is to consider the page loaded and ready
for snapshotting.
BUG=601173
==========

[dougarnett, 2016-06-08 23:55:39]

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

[Pete Williamson, 2016-06-09 00:22:51]

https://codereview.chromium.org/2049743004/diff/20001/chrome/browser/android/...
File chrome/browser/android/offline_pages/prerendering_loader.cc (right):

https://codereview.chromium.org/2049743004/diff/20001/chrome/browser/android/...
chrome/browser/android/offline_pages/prerendering_loader.cc:54:
snapshot_controller_->Reset();
Why reset the controller here?  I would guess that this is when we are asking
the prerenderer to load a page, not a callback telling us that the prerenderer
successfully loaded.

https://codereview.chromium.org/2049743004/diff/20001/chrome/browser/android/...
chrome/browser/android/offline_pages/prerendering_loader.cc:97:
snapshot_controller_->DocumentOnLoadCompletedInMainFrame();
(For my education as a reviewer of this code):
What is the difference between OnPrerendererStopLoading() and
OnPrerendererDomContentLoaded()?  Do we get both in some cases?

Also does the snapshot_controller have some kind of built in Start + N seconds
functionality where it takes a snapshot in case we never get the page loaded
event, or are we relying on the prerender stop loading event for our timeout?

It looks like this is set up for single snapshot, right?  Do we anticipate being
able to move to several snapshots someday, or does the implementation of the
prerenderer make that not feasible?

[dougarnett, 2016-06-09 00:42:34]

https://codereview.chromium.org/2049743004/diff/20001/chrome/browser/android/...
File chrome/browser/android/offline_pages/prerendering_loader.cc (right):

https://codereview.chromium.org/2049743004/diff/20001/chrome/browser/android/...
chrome/browser/android/offline_pages/prerendering_loader.cc:54:
snapshot_controller_->Reset();
On 2016/06/09 00:22:51, Pete Williamson wrote:
> Why reset the controller here?  I would guess that this is when we are asking
> the prerenderer to load a page, not a callback telling us that the prerenderer
> successfully loaded.

This is to prepare (or arm or clear or Reset()) the state of the controller to
be ready to determine when to snapshot. A subsequent Stop() call on it will
freeze it until it is Reset again.

https://codereview.chromium.org/2049743004/diff/20001/chrome/browser/android/...
chrome/browser/android/offline_pages/prerendering_loader.cc:97:
snapshot_controller_->DocumentOnLoadCompletedInMainFrame();
On 2016/06/09 00:22:51, Pete Williamson wrote:
> (For my education as a reviewer of this code):
> What is the difference between OnPrerendererStopLoading() and
> OnPrerendererDomContentLoaded()?  Do we get both in some cases?
> 
> Also does the snapshot_controller have some kind of built in Start + N seconds
> functionality where it takes a snapshot in case we never get the page loaded
> event, or are we relying on the prerender stop loading event for our timeout?
> 
> It looks like this is set up for single snapshot, right?  Do we anticipate
being
> able to move to several snapshots someday, or does the implementation of the
> prerenderer make that not feasible?

This code is single shot at the moment however the SnapshotController has
handshaking to support multiple ones. We can decide whether to utilize that here
down the road (let's get single shot working first!).

The SnapshotController currently sets a 7 second delay on the DocumentAvailable
(or DomContentLoaded) event and no delay for the OnLoadCompleted (or
OnPrerenderStopLoading) event. So we expect to get the HandleLoadEvent() call
once for whichever happens first: OnPrerenderStopLoading or
OnPrerenderDomContentLoaded+7secs

[Pete Williamson, 2016-06-09 00:49:05]

https://codereview.chromium.org/2049743004/diff/20001/chrome/browser/android/...
File chrome/browser/android/offline_pages/prerendering_loader.cc (right):

https://codereview.chromium.org/2049743004/diff/20001/chrome/browser/android/...
chrome/browser/android/offline_pages/prerendering_loader.cc:54:
snapshot_controller_->Reset();
On 2016/06/09 00:42:33, dougarnett wrote:
> On 2016/06/09 00:22:51, Pete Williamson wrote:
> > Why reset the controller here?  I would guess that this is when we are
asking
> > the prerenderer to load a page, not a callback telling us that the
prerenderer
> > successfully loaded.
> 
> This is to prepare (or arm or clear or Reset()) the state of the controller to
> be ready to determine when to snapshot. A subsequent Stop() call on it will
> freeze it until it is Reset again.

Ah, this is a bit confusing, since 'reset'-ing a smart pointer is how you clear
it!  At the very least we should comment here, better still  might be to rename
the reset method.

https://codereview.chromium.org/2049743004/diff/20001/chrome/browser/android/...
chrome/browser/android/offline_pages/prerendering_loader.cc:97:
snapshot_controller_->DocumentOnLoadCompletedInMainFrame();
On 2016/06/09 00:42:33, dougarnett wrote:
> On 2016/06/09 00:22:51, Pete Williamson wrote:
> > (For my education as a reviewer of this code):
> > What is the difference between OnPrerendererStopLoading() and
> > OnPrerendererDomContentLoaded()?  Do we get both in some cases?
> > 
> > Also does the snapshot_controller have some kind of built in Start + N
seconds
> > functionality where it takes a snapshot in case we never get the page loaded
> > event, or are we relying on the prerender stop loading event for our
timeout?
> > 
> > It looks like this is set up for single snapshot, right?  Do we anticipate
> being
> > able to move to several snapshots someday, or does the implementation of the
> > prerenderer make that not feasible?
> 
> This code is single shot at the moment however the SnapshotController has
> handshaking to support multiple ones. We can decide whether to utilize that
here
> down the road (let's get single shot working first!).
> 
> The SnapshotController currently sets a 7 second delay on the
DocumentAvailable
> (or DomContentLoaded) event and no delay for the OnLoadCompleted (or
> OnPrerenderStopLoading) event. So we expect to get the HandleLoadEvent() call
> once for whichever happens first: OnPrerenderStopLoading or
> OnPrerenderDomContentLoaded+7secs

OK, code is good as is, then, but let's document this.

[dougarnett, 2016-06-09 15:12:27]

https://codereview.chromium.org/2049743004/diff/20001/chrome/browser/android/...
File chrome/browser/android/offline_pages/prerendering_loader.cc (right):

https://codereview.chromium.org/2049743004/diff/20001/chrome/browser/android/...
chrome/browser/android/offline_pages/prerendering_loader.cc:54:
snapshot_controller_->Reset();
On 2016/06/09 00:49:05, Pete Williamson wrote:
> On 2016/06/09 00:42:33, dougarnett wrote:
> > On 2016/06/09 00:22:51, Pete Williamson wrote:
> > > Why reset the controller here?  I would guess that this is when we are
> asking
> > > the prerenderer to load a page, not a callback telling us that the
> prerenderer
> > > successfully loaded.
> > 
> > This is to prepare (or arm or clear or Reset()) the state of the controller
to
> > be ready to determine when to snapshot. A subsequent Stop() call on it will
> > freeze it until it is Reset again.
> 
> Ah, this is a bit confusing, since 'reset'-ing a smart pointer is how you
clear
> it!  At the very least we should comment here, better still  might be to
rename
> the reset method.

Added comment. 

I wasn't part of the SnapshotController review so not sure what API naming
discussion might have occurred but dimich@ is reviewer here and so he might
comment on its naming. I agree it is a bit confusing when contained in
unique_ptr.

https://codereview.chromium.org/2049743004/diff/20001/chrome/browser/android/...
chrome/browser/android/offline_pages/prerendering_loader.cc:97:
snapshot_controller_->DocumentOnLoadCompletedInMainFrame();
On 2016/06/09 00:49:05, Pete Williamson wrote:
> On 2016/06/09 00:42:33, dougarnett wrote:
> > On 2016/06/09 00:22:51, Pete Williamson wrote:
> > > (For my education as a reviewer of this code):
> > > What is the difference between OnPrerendererStopLoading() and
> > > OnPrerendererDomContentLoaded()?  Do we get both in some cases?
> > > 
> > > Also does the snapshot_controller have some kind of built in Start + N
> seconds
> > > functionality where it takes a snapshot in case we never get the page
loaded
> > > event, or are we relying on the prerender stop loading event for our
> timeout?
> > > 
> > > It looks like this is set up for single snapshot, right?  Do we anticipate
> > being
> > > able to move to several snapshots someday, or does the implementation of
the
> > > prerenderer make that not feasible?
> > 
> > This code is single shot at the moment however the SnapshotController has
> > handshaking to support multiple ones. We can decide whether to utilize that
> here
> > down the road (let's get single shot working first!).
> > 
> > The SnapshotController currently sets a 7 second delay on the
> DocumentAvailable
> > (or DomContentLoaded) event and no delay for the OnLoadCompleted (or
> > OnPrerenderStopLoading) event. So we expect to get the HandleLoadEvent()
call
> > once for whichever happens first: OnPrerenderStopLoading or
> > OnPrerenderDomContentLoaded+7secs
> 
> OK, code is good as is, then, but let's document this.

Done - let me know if not clear enough.

[Pete Williamson, 2016-06-09 16:49:19]

lgtm

[Dmitry Titov, 2016-06-09 17:41:00]

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

[Dmitry Titov, 2016-06-09 17:41:01]

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

https://codereview.chromium.org/2049743004/diff/40001/chrome/browser/android/...
chrome/browser/android/offline_pages/prerendering_loader.cc:55:
snapshot_controller_->Reset();
this is only needed if the LoadPage can be called on the same PrerendererLoader
instance multiple times to load multiple pages and get snapshots from them. If
it is not really possible, it'd be enough to just create an instance here, store
it in unique_ptr and then instead of Stop() just reset that unique_ptr... This
way, you don't have an instance of controller longer then you need and don't
need to worry about stopping/resetting it.

https://codereview.chromium.org/2049743004/diff/40001/chrome/browser/android/...
chrome/browser/android/offline_pages/prerendering_loader.cc:96: if (!IsLoaded())
{
I don't think these checks are really needed, especially repeated several times
in several functions. It woudl be simpler to just directly call methods on the
controller. It is designed to avoid multiple StartSnapshot until you Reset it or
report that previous snapshot is done. And it won't start another if you Stop -
so all the checks here are probably not needed.

https://codereview.chromium.org/2049743004/diff/40001/chrome/browser/android/...
chrome/browser/android/offline_pages/prerendering_loader.cc:107: if
(!IsLoaded()) {
Same as above

https://codereview.chromium.org/2049743004/diff/40001/chrome/browser/android/...
chrome/browser/android/offline_pages/prerendering_loader.cc:128:
DCHECK(!IsLoaded());
This is confusing. Why start snapshot on something that is not loaded? It may be
a naming issue, but this code is hard to parse.

Also, how about renaming HandleLoadEvent into StartSnapshot and not having this
method?

https://codereview.chromium.org/2049743004/diff/40001/chrome/browser/android/...
chrome/browser/android/offline_pages/prerendering_loader.cc:145:
snapshot_controller_->Stop();
No need ot call Stop here, while you didn't report the previous snapshot
actually doen (via SnapshotController::PendingSnapshotCompleted), the new one
won't be started anyways.

[dougarnett, 2016-06-09 18:25:47]

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

https://codereview.chromium.org/2049743004/diff/40001/chrome/browser/android/...
chrome/browser/android/offline_pages/prerendering_loader.cc:55:
snapshot_controller_->Reset();
On 2016/06/09 17:41:01, Dmitry Titov wrote:
> this is only needed if the LoadPage can be called on the same
PrerendererLoader
> instance multiple times to load multiple pages and get snapshots from them. If
> it is not really possible, it'd be enough to just create an instance here,
store
> it in unique_ptr and then instead of Stop() just reset that unique_ptr... This
> way, you don't have an instance of controller longer then you need and don't
> need to worry about stopping/resetting it.

Ok, creating new instance each LoadPage now

https://codereview.chromium.org/2049743004/diff/40001/chrome/browser/android/...
chrome/browser/android/offline_pages/prerendering_loader.cc:96: if (!IsLoaded())
{
On 2016/06/09 17:41:01, Dmitry Titov wrote:
> I don't think these checks are really needed, especially repeated several
times
> in several functions. It woudl be simpler to just directly call methods on the
> controller. It is designed to avoid multiple StartSnapshot until you Reset it
or
> report that previous snapshot is done. And it won't start another if you Stop
-
> so all the checks here are probably not needed.

Done.

https://codereview.chromium.org/2049743004/diff/40001/chrome/browser/android/...
chrome/browser/android/offline_pages/prerendering_loader.cc:107: if
(!IsLoaded()) {
On 2016/06/09 17:41:01, Dmitry Titov wrote:
> Same as above

Done.

https://codereview.chromium.org/2049743004/diff/40001/chrome/browser/android/...
chrome/browser/android/offline_pages/prerendering_loader.cc:128:
DCHECK(!IsLoaded());
On 2016/06/09 17:41:01, Dmitry Titov wrote:
> This is confusing. Why start snapshot on something that is not loaded? It may
be
> a naming issue, but this code is hard to parse.
> 
> Also, how about renaming HandleLoadEvent into StartSnapshot and not having
this
> method?

Precondition check, we are now waiting for StartSnapshot() call to tells us that
we should transition to LOADED state. When we support a second snapshot in the
future, we should refactor the interface with the Offliner (or join the Offliner
and Loader together).

I agree we can collapse StartSnapshot and HandleLoadEvent. This is an
integration step where the SnapshotController and BackgroundLoader were not
designed together so there is impedance and terminology mismatch here. For now I
think I will just directly call with no other code and do more refactoring
later.

https://codereview.chromium.org/2049743004/diff/40001/chrome/browser/android/...
chrome/browser/android/offline_pages/prerendering_loader.cc:145:
snapshot_controller_->Stop();
On 2016/06/09 17:41:01, Dmitry Titov wrote:
> No need ot call Stop here, while you didn't report the previous snapshot
> actually doen (via SnapshotController::PendingSnapshotCompleted), the new one
> won't be started anyways.

Done.

[Dmitry Titov, 2016-06-09 18:42:14]

lgtm

[dougarnett, 2016-06-09 19:15:49]

dougarnett@chromium.org changed reviewers:
- dimich@google.com

[dougarnett, 2016-06-09 19:16:19]

The CQ bit was checked by dougarnett@chromium.org

[dougarnett, 2016-06-09 19:16:20]

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/2049743004/#ps80001
(title: "Replaces Stop() calls with unique_ptr.reset for better instance
handling")

[commit-bot: I haz the power, 2016-06-09 19:16:59]

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

[commit-bot: I haz the power, 2016-06-09 20:18:22]

Description was changed from

==========
Integrates the SnapshotController into the PrerenderingLoader to provide logic
for determining when the right moment is to consider the page loaded and ready
for snapshotting.
BUG=601173
==========

to

==========
Integrates the SnapshotController into the PrerenderingLoader to provide logic
for determining when the right moment is to consider the page loaded and ready
for snapshotting.
BUG=601173
==========

[commit-bot: I haz the power, 2016-06-09 20:18:25]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-06-09 20:19:31]

Description was changed from

==========
Integrates the SnapshotController into the PrerenderingLoader to provide logic
for determining when the right moment is to consider the page loaded and ready
for snapshotting.
BUG=601173
==========

to

==========
Integrates the SnapshotController into the PrerenderingLoader to provide logic
for determining when the right moment is to consider the page loaded and ready
for snapshotting.
BUG=601173

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

[commit-bot: I haz the power, 2016-06-09 20:19:32]

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