Make the timing of ManifestUpgradeDetectorFetcher::Start() call less subtle

Make the timing of ManifestUpgradeDetectorFetcher::Start() call less subtle

Currently ManifestUpgradeDetectorFetcher assumes that
ManifestUpgradeDetectorFetcher::Start() will be called prior to the initial page
load finishing (a lot of web apps are a single page sites so there is only
one page load ever).

This assumumption relies on:
- The specifics of when ChromeActivity#onDeferredStartup() is called.
- How long the WebappRegistry#getWebappDataStorage() invocation in
  WebApkUpdateManager#updateIfNeeded() takes to call the callback.

This CL makes ManifestUpgradeDetectorFetcher::Start() work even if it is called
after the initial page load finishes

BUG=639536
Committed: https://crrev.com/cd85a3b3a92fb04a89b586110bea55ce3f0bfa00
Cr-Commit-Position: refs/heads/master@{#425110}

[pkotwicz, 2016-10-10 02:06:08]

pkotwicz@chromium.org changed reviewers:
+ dominickn@chromium.org

[pkotwicz, 2016-10-10 02:06:09]

Dominick, can you please take a look?

[dominickn, 2016-10-10 10:12:45]

How often is DidStopLoading called in comparison to DidFinishLoad? And what
exactly is the behaviour of this change on single page apps? I'm just wary of
having this run either excessively, or not on some types of pages (e.g. what
about infinite scrolling or pages which defer loading so that the load event
never fires?)

[dominickn, 2016-10-10 10:13:04]

On 2016/10/10 10:12:45, dominickn wrote:
> How often is DidStopLoading called in comparison to DidFinishLoad? And what
> exactly is the behaviour of this change on single page apps? I'm just wary of
> having this run either excessively, or not on some types of pages (e.g. what
> about infinite scrolling or pages which defer loading so that the load event
> never fires?)

Also, stubtle -> subtle in CL title.

[dominickn, 2016-10-10 10:14:43]

For instance, would it be more reliable to make
ManifestUpgradeDetectorFetcher::Start run in the deferred startup handler (i.e.
check it once per WebAPK launch at some point in the background)?

[pkotwicz, 2016-10-10 16:42:38]

Description was changed from

==========
Make the timing of when ManifestUpgradeDetectorFetcher::Start() is called less
stubtle

Currently ManifestUpgradeDetectorFetcher assumes that
ManifestUpgradeDetectorFetcher::Start() will be called prior to the initial page
load finishing (a lot of web apps are a single page sites so there is only
one page load ever).

This assumumption relies on:
- The specifics of when ChromeActivity#onDeferredStartup() is called.
- How long the WebappRegistry#getWebappDataStorage() invocation in
  WebApkUpdateManager#updateIfNeeded() takes to call the callback.

This CL makes ManifestUpgradeDetectorFetcher::Start() work even if it is called
after the initial page load finishes

BUG=639536
==========

to

==========
Make the timing of when ManifestUpgradeDetectorFetcher::Start() is called less
stubtle

Currently ManifestUpgradeDetectorFetcher assumes that
ManifestUpgradeDetectorFetcher::Start() will be called prior to the initial page
load finishing (a lot of web apps are a single page sites so there is only
one page load ever).

This assumumption relies on:
- The specifics of when ChromeActivity#onDeferredStartup() is called.
- How long the WebappRegistry#getWebappDataStorage() invocation in
  WebApkUpdateManager#updateIfNeeded() takes to call the callback.

This CL makes ManifestUpgradeDetectorFetcher::Start() work even if it is called
after the initial page load finishes

BUG=639536
==========

[pkotwicz, 2016-10-10 17:36:30]

Sorry if the goal of the CL is unclear.
- This CL makes ManifestUpgradeDetectorFetcher::Start() work even if it is
called after the initial page load occurring. (In my testing we seem to be lucky
and ManifestUpgradeDetectorFetcher::Start() seems to be called prior to the
initial page load occurring)
- For single page apps, there is only ever one page load.
- I switched to using WebContentsObserver::DidStopLoading() from
WebContentsObserver::DidFinishLoad() because WebContents::IsLoading() matches up
with WebContentsObserver::DidStopLoading()

We can't call ManifestUpgradeDetectorFetcher::FetchInstallableData() at a random
point in time in the background. We want to call
ManifestUpgradeDetectorFetcher::FetchInstallableData() when a page load is not
in progress. For some web apps there is only one page load so we can't "wait for
the next one"

WebContentsObserver::DidStopLoading() fires whenever the spinner stops. It fires
2 - 3 times for www.amazon.com. I have not checked why this is the case but I
assume that this is due to how amazon loads reasources.
For regular pages DidStopLoading() should fire only once.

According to the comments in web_contents_observer.h
WebContentsObserver::DidFinishLoad() is only called after the window.onload
event fires. It seems that for infinite scrolling pages it would be possible for
neither WebContentsObserver::DidFinishLoad() or
WebContentsObserver::DidStopLoading() to fire. However, I have not found an
infinite scrolling page where either does not fire. I have tested with Google
image search and Google+.

[dominickn, 2016-10-11 04:59:58]

On 2016/10/10 17:36:30, pkotwicz wrote:
> Sorry if the goal of the CL is unclear.
> - This CL makes ManifestUpgradeDetectorFetcher::Start() work even if it is
> called after the initial page load occurring. (In my testing we seem to be
lucky
> and ManifestUpgradeDetectorFetcher::Start() seems to be called prior to the
> initial page load occurring)
> - For single page apps, there is only ever one page load.
> - I switched to using WebContentsObserver::DidStopLoading() from
> WebContentsObserver::DidFinishLoad() because WebContents::IsLoading() matches
up
> with WebContentsObserver::DidStopLoading()
> 
> We can't call ManifestUpgradeDetectorFetcher::FetchInstallableData() at a
random
> point in time in the background. We want to call
> ManifestUpgradeDetectorFetcher::FetchInstallableData() when a page load is not
> in progress. For some web apps there is only one page load so we can't "wait
for
> the next one"
> 
> WebContentsObserver::DidStopLoading() fires whenever the spinner stops. It
fires
> 2 - 3 times for http://www.amazon.com. I have not checked why this is the case
but I
> assume that this is due to how amazon loads reasources.
> For regular pages DidStopLoading() should fire only once.

This behaviour with Amazon is a little concerning. Can you test with various
pages at
pwa.rocks? Google Image Search / Google+ aren't really representative because
neither are PWAs, and PWAs are what we're targeting here.

If DidStopLoading can trigger multiple times for a number of PWAs (or never), I
don't think it's appropriate to use as the trigger for a an update. Ideally, I
think
we should check for an update once per each time the WebAPK is opened and run.

I think you can specify tasks in DeferredStartupHandler that run on the main
thread. That would let you run an update on the main thread at some point
after WebApkActivity starts right? Or am I being thick and missing a reason
for doing update checking more frequently? :)

[pkotwicz, 2016-10-12 04:06:47]

Sorry for the confusion. The goal of this CL is not to do the update check more
frequently.

The goal of this CL is to do in ManifestUpgradeDetectorFetcher::Start()
> if (!web_contents()->IsLoading())
>   FetchInstallableData();

I switched triggering the update check as a result of DidStopLoading() instead
of DidFinishLoad() because DidStopLoading() is compatible with
WebContents::IsLoading()
This CL adds logic to prevent doing the update check multiple times per page

[dominickn, 2016-10-12 22:02:24]

On 2016/10/12 04:06:47, pkotwicz wrote:
> Sorry for the confusion. The goal of this CL is not to do the update check
more
> frequently.
> 
> The goal of this CL is to do in ManifestUpgradeDetectorFetcher::Start()
> > if (!web_contents()->IsLoading())
> >   FetchInstallableData();
> 
> I switched triggering the update check as a result of DidStopLoading() instead
> of DidFinishLoad() because DidStopLoading() is compatible with
> WebContents::IsLoading()
> This CL adds logic to prevent doing the update check multiple times per page

What about PWAs which aren't single page apps? If they navigate you around to
different URLs, the update check will be triggered on each navigation right?
That seems excessive.

[pkotwicz, 2016-10-13 00:20:46]

In the common case, we do the update check only once because
ManifestUpgradeDetector#onGotManifestData() destroys the fetcher.

If a WebAPK no longer has a Web Manifest, we call InstallableManager::GetData()
on each page load. (This CL does not make this situation better or worse)

[dominickn, 2016-10-13 00:24:14]

On 2016/10/13 00:20:46, pkotwicz wrote:
> In the common case, we do the update check only once because
> ManifestUpgradeDetector#onGotManifestData() destroys the fetcher.
> 
> If a WebAPK no longer has a Web Manifest, we call
InstallableManager::GetData()
> on each page load. (This CL does not make this situation better or worse)

Thanks for the explanation. lgtm % please fix the first line of the description.
:)

[pkotwicz, 2016-10-13 17:18:52]

Description was changed from

==========
Make the timing of when ManifestUpgradeDetectorFetcher::Start() is called less
stubtle

Currently ManifestUpgradeDetectorFetcher assumes that
ManifestUpgradeDetectorFetcher::Start() will be called prior to the initial page
load finishing (a lot of web apps are a single page sites so there is only
one page load ever).

This assumumption relies on:
- The specifics of when ChromeActivity#onDeferredStartup() is called.
- How long the WebappRegistry#getWebappDataStorage() invocation in
  WebApkUpdateManager#updateIfNeeded() takes to call the callback.

This CL makes ManifestUpgradeDetectorFetcher::Start() work even if it is called
after the initial page load finishes

BUG=639536
==========

to

==========
Make the timing of ManifestUpgradeDetectorFetcher::Start() call less subtle

Currently ManifestUpgradeDetectorFetcher assumes that
ManifestUpgradeDetectorFetcher::Start() will be called prior to the initial page
load finishing (a lot of web apps are a single page sites so there is only
one page load ever).

This assumumption relies on:
- The specifics of when ChromeActivity#onDeferredStartup() is called.
- How long the WebappRegistry#getWebappDataStorage() invocation in
  WebApkUpdateManager#updateIfNeeded() takes to call the callback.

This CL makes ManifestUpgradeDetectorFetcher::Start() work even if it is called
after the initial page load finishes

BUG=639536
==========

[pkotwicz, 2016-10-13 18:14:27]

The CQ bit was checked by pkotwicz@chromium.org

[commit-bot: I haz the power, 2016-10-13 18:15:06]

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-13 18:52:11]

Description was changed from

==========
Make the timing of ManifestUpgradeDetectorFetcher::Start() call less subtle

Currently ManifestUpgradeDetectorFetcher assumes that
ManifestUpgradeDetectorFetcher::Start() will be called prior to the initial page
load finishing (a lot of web apps are a single page sites so there is only
one page load ever).

This assumumption relies on:
- The specifics of when ChromeActivity#onDeferredStartup() is called.
- How long the WebappRegistry#getWebappDataStorage() invocation in
  WebApkUpdateManager#updateIfNeeded() takes to call the callback.

This CL makes ManifestUpgradeDetectorFetcher::Start() work even if it is called
after the initial page load finishes

BUG=639536
==========

to

==========
Make the timing of ManifestUpgradeDetectorFetcher::Start() call less subtle

Currently ManifestUpgradeDetectorFetcher assumes that
ManifestUpgradeDetectorFetcher::Start() will be called prior to the initial page
load finishing (a lot of web apps are a single page sites so there is only
one page load ever).

This assumumption relies on:
- The specifics of when ChromeActivity#onDeferredStartup() is called.
- How long the WebappRegistry#getWebappDataStorage() invocation in
  WebApkUpdateManager#updateIfNeeded() takes to call the callback.

This CL makes ManifestUpgradeDetectorFetcher::Start() work even if it is called
after the initial page load finishes

BUG=639536
==========

[commit-bot: I haz the power, 2016-10-13 18:52:12]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-10-13 18:56:04]

Description was changed from

==========
Make the timing of ManifestUpgradeDetectorFetcher::Start() call less subtle

Currently ManifestUpgradeDetectorFetcher assumes that
ManifestUpgradeDetectorFetcher::Start() will be called prior to the initial page
load finishing (a lot of web apps are a single page sites so there is only
one page load ever).

This assumumption relies on:
- The specifics of when ChromeActivity#onDeferredStartup() is called.
- How long the WebappRegistry#getWebappDataStorage() invocation in
  WebApkUpdateManager#updateIfNeeded() takes to call the callback.

This CL makes ManifestUpgradeDetectorFetcher::Start() work even if it is called
after the initial page load finishes

BUG=639536
==========

to

==========
Make the timing of ManifestUpgradeDetectorFetcher::Start() call less subtle

Currently ManifestUpgradeDetectorFetcher assumes that
ManifestUpgradeDetectorFetcher::Start() will be called prior to the initial page
load finishing (a lot of web apps are a single page sites so there is only
one page load ever).

This assumumption relies on:
- The specifics of when ChromeActivity#onDeferredStartup() is called.
- How long the WebappRegistry#getWebappDataStorage() invocation in
  WebApkUpdateManager#updateIfNeeded() takes to call the callback.

This CL makes ManifestUpgradeDetectorFetcher::Start() work even if it is called
after the initial page load finishes

BUG=639536

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

[commit-bot: I haz the power, 2016-10-13 18:56:05]

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