1437988 - Consider adding onProgressChanged method for smoother loading appearance

Status: RESOLVED FIXED

(GeckoView :: General, defect, P1)

Description

[Emily Kager [:ekager]]

Focus uses onProgressChanged in WebView to update the progress bar for smoother loading appearance. Currently in the GeckoView implementation we hard code the progress from 10, 25, 100 leading to chunky loading appearance and some lag on the progress bar. 
 
Relevant API is here :
https://developer.android.com/reference/android/webkit/WebChromeClient.html#onProgressChanged(android.webkit.WebView,%20int)

See GH issue https://github.com/mozilla-mobile/focus-android/issues/2099

Comment 1

[James Willcox (:snorp) ([e-mail]) (he/him)]

There are a few reasons we don't have an onProgressChanged() equivalent right now:

1) It's difficult to really judge what percentage of a page has loaded. Also, since some resources can then pull in more resources once loaded, the progress can actually go backwards, and we know users won't want to see that.

2) Doing very detailed page progress monitoring (with accompanied UI) can hurt page load performance.

We can revisit these if desired, esp. 2). One thing we could consider is adding specific checkpoints which would then be reflected as some percentage progress in the UI. Maybe something like

* HTML fetched
* CSS loaded
* First paint
* Scripts loaded
* Images loaded
* All resources loaded

Of course the details of that might be pretty murky, esp. considering iframes, but some of them like "First paint" would actually have usefulness outside of showing a page load meter.

bz, do you have an opinion on page load progress APIs?

Comment 2

[Boris Zbarsky [:bzbarsky]]

#2 (perf issues due to progress notifications) has _definitely_ been a problem in Firefox UI in the past....

The fundamental problem, as you say, is that it's really impossible to say how must work is left to do on the pageload.  I figuring out some way to create a smooth-looking progress bar out of what is fundamentally non-smooth unknown data would be an interesting research project, of course.

Comment 3

[Chris Peterson [:cpeterson]]

Showing an incomplete progress bar that never goes away after the page has completed loading is much, much worse than closing the progress bar "too soon" while the page is still loading.

Comment 4

[Sebastian Kaspari (:sebastian; :pocmo)]

Could we get at least something very rough like in Fennec? Right now we have nothing and can only have a 2-step (start-load / stop-load) progress which is pretty bad.

Comment 5

[Jan Henning [:JanH]]

(In reply to Sebastian Kaspari (:sebastian) from comment #4)
> like in Fennec?

Which would mean intermediate points for onLocationChange and DOMContentLoaded.

Comment 7

[No-Jun Park [:njpark]]

On a slower device (Nexus 5 with Android 6.0, for example) complex sites (i.e. buzzfeed.com) are loaded, but the progressbar is stuck on 25%.  This times out nimbledroid tests on Geckoview.  This behavior can be seen on x86 simulator as well.  

This bug blocks the nimbledroid geckoview tests.

Comment 8

[Eugen Sawin [:esawin]]

Attachment: 0001-Bug-1437988-1.0-Add-progress-tracking-and-expose-pro.patch

Estimating page load duration is difficult. We have to balance between the significance, reliability and smoothness of the reported progress value.

As the first step, we can experiment with different signals and weights and see how it feels.

If we want to approach this empirically, we can add specific telemetry probes for the different signals and their durations, if that's no already being done.

With this patch I'm adding the following weighted signals:
1. Page load start (+10%)
2. DOM loaded (+10%)
3. First paint (+10%)
4. Content loaded (+10%)
5. Overall channel download progress (up to +50%)
6. Page load stop (= 100%)

This is an improvement over the current two signals (page load start and stop) but has its issues.

Some of the events are closely related, we can adjust weights or reduce signals to account for that.

#5 is currently difficult to handle because we don't seem to be able to intercept the progress of all the resources in the required detail.

I'm not sure if better support for #5 would solve this, but currently we have a signal gap between the other signals and #6. Visually, #6 is fired too late and not when the user perceives the page to has finished loading.
This needs further investigation, maybe in bug 1454477.

Comment 9

[Dylan Roeh (:droeh) (he/him)]

Comment on attachment 8985239 [details] [diff] [review]
0001-Bug-1437988-1.0-Add-progress-tracking-and-expose-pro.patch

Review of attachment 8985239 [details] [diff] [review]:
-----------------------------------------------------------------

The patch looks fine, but I'd like to see profiling before we actually go forward with something like this -- I think adding a ProgressDelegate already has an effect on performance, and listening for onProgressChange is probably going to worsen that. It's also worth pointing out that Fennec only uses four(!) events for the progress bar, and fakes interpolation for the rest[0] -- this is a problem that might be better solved by UI code than by trying to provide continuous progress feedback from GeckoView.

[0]: https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/widget/AnimatedProgressBar.java?q=AnimatedProgressBar.java&redirect_type=direct#124

Comment 10

[Eugen Sawin [:esawin]]

Thanks for the feedback!

(In reply to Dylan Roeh (:droeh) from comment #9)
> The patch looks fine, but I'd like to see profiling before we actually go
> forward with something like this -- I think adding a ProgressDelegate
> already has an effect on performance, and listening for onProgressChange is
> probably going to worsen that.

Good point! The onProgressChange events are already stacked so we only get one event per small to mid-sized resources and a few more for big files. I don't expect that to be a noticeable hit on load performance, but I will check once we agree on the general approach and the signal selection.

> It's also worth pointing out that Fennec only
> uses four(!) events for the progress bar, and fakes interpolation for the
> rest[0] -- this is a problem that might be better solved by UI code than by
> trying to provide continuous progress feedback from GeckoView.

I'm not planning to do any interpolation in the progress tracker, but the UI can't perform meaningful interpolation if we don't provide any signals to do so. The progress value should take some burden off the app developer in collecting and understanding the different signals and provide standardized load progress information that we control.

Side note: I think Fennec is probably not the appropriate baseline for GV feature evaluation.

Comment 11

[Dylan Roeh (:droeh) (he/him)]

(In reply to Eugen Sawin [:esawin] from comment #10) 
> I'm not planning to do any interpolation in the progress tracker, but the UI
> can't perform meaningful interpolation if we don't provide any signals to do
> so. The progress value should take some burden off the app developer in
> collecting and understanding the different signals and provide standardized
> load progress information that we control.

I was thinking more like we add onProgressChanged but essentially have it report discrete values based on other events -- to mimic Fennec (not that we necessarily want to) we could pass 10% when we hit onLoadRequest, 20% when we hit onPageStart, 60% when we hit onLocationChange, 80% when we hit DOMContentLoaded, and 100% for onPageStart; then let the interpolation be done by the UI (and possibly have an Android Component that does just that).
 
> Side note: I think Fennec is probably not the appropriate baseline for GV
> feature evaluation.

Yeah, I'm just referring to Fennec here because it has a much smoother progress bar than Focus+GV at the moment without requiring listening to onProgressChange; we can definitely improve on it (adding more events, doing telemetry to get an idea of what events should actually correspond to, etc).

Comment 12

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8985239 [details] [diff] [review]
0001-Bug-1437988-1.0-Add-progress-tracking-and-expose-pro.patch

Review of attachment 8985239 [details] [diff] [review]:
-----------------------------------------------------------------

I think the overall progress tracking algorithm could be ok, but I think we should try to do it in DocShell or somewhere else that is not-JS. Worth consulting bz here, I think.

::: mobile/android/modules/geckoview/GeckoViewProgress.jsm
@@ +184,5 @@
> +    this.clear();
> +    let that = this;
> +
> +    Services.obs.addObserver(this.onOpeningRequest,
> +                             "http-on-opening-request");

I'm pretty concerned with this. We found a while back that even handling the user agent string stuff in JS added something like 3ms for every request. On a page like cnn.com with 200 requests, it really adds up. I would like to avoid any per-request JS code.

@@ +306,5 @@
> +    // debug `ProgressTracker onOpeningRequest`;
> +
> +    let that = this;
> +    const channel = aSubject.QueryInterface(Ci.nsIChannel);
> +    if (!channel.notificationCallbacks) {

We first need to make sure this channel is related to our load

Comment 13

[Eugen Sawin [:esawin]]

(In reply to James Willcox (:snorp) ([e-mail]) from comment #12)
> I think the overall progress tracking algorithm could be ok, but I think we
> should try to do it in DocShell or somewhere else that is not-JS. Worth
> consulting bz here, I think.

I think once we agree on the signals to use, it will be clearer at which level we want to track those.

> ::: mobile/android/modules/geckoview/GeckoViewProgress.jsm
> @@ +184,5 @@
> > +    this.clear();
> > +    let that = this;
> > +
> > +    Services.obs.addObserver(this.onOpeningRequest,
> > +                             "http-on-opening-request");
> 
> I'm pretty concerned with this. We found a while back that even handling the
> user agent string stuff in JS added something like 3ms for every request. On
> a page like cnn.com with 200 requests, it really adds up. I would like to
> avoid any per-request JS code.

Given the performance concerns and the fact that we still get some (the accumulated sum?) of the networking progress through the onProgressChange event, I propose we drop per-channel tracking for now with the option to revisit it in future iterations, should the need arise.

Comment 14

[Eugen Sawin [:esawin]]

Attachment: 0001-Bug-1437988-1.1-Add-progress-tracking-and-expose-pro.patch

I've removed the per-channel "http-on-opening-request" observers due to performance concerns.

With this patch we track page start, first paint, pageshow, DOMContentLoaded, page stop and nsIWebProgressListener::onProgressChange.

A load is considered finished (100) if either page stop has been fired or all other signals are positive and all data has been received.
The latter is currently not fully supported in this patch, making that signal a somewhat unreliable source for progression tracking.

Comment 15

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8991661 [details] [diff] [review]
0001-Bug-1437988-1.1-Add-progress-tracking-and-expose-pro.patch

Review of attachment 8991661 [details] [diff] [review]:
-----------------------------------------------------------------

Can you do some local testing to see how this affects pageload perf? Sadly I don't know if we have anything running against GeckoView right now in automation that will do what we want.

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoSession.java
@@ +1774,5 @@
>          * @param success Whether the page loaded successfully or an error occurred.
>          */
>          void onPageStop(GeckoSession session, boolean success);
>  
> +        void onProgressChange(GeckoSession session, int progress);

Documentation

::: mobile/android/modules/geckoview/GeckoViewProgress.jsm
@@ +189,5 @@
> +
> +    this.clear();
> +
> +    if (aUri === "about:blank") {
> +      this._data.uri = null;

just early return here and lose the `else` block below

@@ +228,5 @@
> +    }
> +
> +    switch (aMsg.name) {
> +      case "GeckoView:DOMContentLoaded": {
> +        this.messageManager.removeMessageListener("GeckoView:DOMContentLoaded", this);

You can move this and the updateProgress() out of each switch block, e.g. `removeMessageListener(aMsg.name, this);`

@@ +264,5 @@
> +      }
> +    }
> +  },
> +
> +  clear: function() {

I think you  may want to removeMessageListener() the stuff that gets added in start(). It doesn't look like they will all be removed in all cases (like a page doesn't completely load)

@@ +295,5 @@
> +      totalExpected: this._data.totalExpected,
> +    }
> +  },
> +
> +  handleProgress: function(aUri, aProgress, aMax) {

am I misunderstanding or is this the per-request monitoring that we thought would be too costly?

@@ +312,5 @@
> +        expected: (aMax > 0 ? aMax : aProgress * 2),
> +        lastUpdate: this.window.performance.now(),
> +      };
> +    }
> +    data.channels[aUri].received = aProgress;

It seems like we'd want an `updateProgress()` call here

Comment 16

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8991661 [details] [diff] [review]
0001-Bug-1437988-1.1-Add-progress-tracking-and-expose-pro.patch

Review of attachment 8991661 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/chrome/geckoview/GeckoViewProgressContent.js
@@ +41,5 @@
> +      case "DOMContentLoaded":
> +        sendAsyncMessage("GeckoView:DOMContentLoaded");
> +        break;
> +      case "pageshow":
> +        sendAsyncMessage("GeckoView:PageShow");

I think these should be consolidated under a single "GeckoView:ProgressEvent" message, otherwise it seems like they're likely to conflict with messages from other modules.

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoSession.java
@@ +1774,5 @@
>          * @param success Whether the page loaded successfully or an error occurred.
>          */
>          void onPageStop(GeckoSession session, boolean success);
>  
> +        void onProgressChange(GeckoSession session, int progress);

Also need to update "Callbacks.kt"

::: mobile/android/modules/geckoview/GeckoViewProgress.jsm
@@ +391,5 @@
>      debug `onEnable`;
>  
> +    this._hostChanged = false;
> +
> +    ProgressTracker.onInit(this);

`onEnable` for consistency.

@@ +393,5 @@
> +    this._hostChanged = false;
> +
> +    ProgressTracker.onInit(this);
> +
> +    this.registerListener([

You don't really need the proxying stuff here. Regular `eventDispatcher.registerListener` should be okay and you can call that inside `ProgressTracker`. Also call `unregisterListener` inside `onDisable`.

@@ +423,5 @@
>  
> +  onEvent(aEvent, aData, aCallback) {
> +    debug `onEvent ${aEvent} ${aData}`;
> +
> +    ProgressTracker.onEvent.apply(ProgressTracker, arguments);

> ProgressTracker.onEvent(...arguments);

@@ +461,5 @@
>          uri: uriSpec,
>        };
>  
>        this.eventDispatcher.sendRequest(message);
> +      this.messageManager.sendAsyncMessage("GeckoView:PageStart");

Not used?

Comment 17

[Eugen Sawin [:esawin]]

Attachment: 0001-Bug-1437988-1.2-Add-progress-tracking-and-expose-pro.patch

Addressed all comments.

(In reply to James Willcox (:snorp) ([e-mail]) from comment #15)
> Can you do some local testing to see how this affects pageload perf? Sadly I
> don't know if we have anything running against GeckoView right now in
> automation that will do what we want.

Based on local testing, the current signals are not enough to capture progress on complex sites well, we tend to reach 100 before visual completion.
But it does seem to give reasonable results for small and medium-sized sites.

It should definitely improve the current situation on Focus by providing more intermediate progression values (compared to 0 - 25 - 100).

This is difficult to get right without more extensive testing on release builds.

> @@ +295,5 @@
> > +      totalExpected: this._data.totalExpected,
> > +    }
> > +  },
> > +
> > +  handleProgress: function(aUri, aProgress, aMax) {
> 
> am I misunderstanding or is this the per-request monitoring that we thought
> would be too costly?

It's the function to handle per-channel updates, but I've removed the listeners for that. We only call it from onProgressChange which is only called once per page load.

> @@ +312,5 @@
> > +        expected: (aMax > 0 ? aMax : aProgress * 2),
> > +        lastUpdate: this.window.performance.now(),
> > +      };
> > +    }
> > +    data.channels[aUri].received = aProgress;
> 
> It seems like we'd want an `updateProgress()` call here

We may want to stack handleProgress calls in future (if we wan't to re-enable per-channel tracking) before calling updateProgress which will iterate over all channel data.

Comment 18

[David Bolter [:davidb] (NeedInfo me for attention)]

Given the QF prioritization I'm bumping this to P1.

Comment 19

[Dylan Roeh (:droeh) (he/him)]

Comment on attachment 8992304 [details] [diff] [review]
0001-Bug-1437988-1.2-Add-progress-tracking-and-expose-pro.patch

Review of attachment 8992304 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good! Just one nit.

::: mobile/android/base/java/org/mozilla/gecko/customtabs/CustomTabsActivity.java
@@ +666,5 @@
>          updateProgress(100);
>      }
>  
>      @Override
> +    public void onProgressChange(GeckoSession session, int progress) {

Can you update this so we actually use onProgressChange() to set the progress bar and get rid of the old updateProgress() calls?

Comment 20

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8992304 [details] [diff] [review]
0001-Bug-1437988-1.2-Add-progress-tracking-and-expose-pro.patch

Review of attachment 8992304 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/chrome/geckoview/GeckoViewProgressContent.js
@@ +17,5 @@
> +
> +  onEnable() {
> +    debug `onEnable`;
> +
> +    addEventListener("MozAfterPaint", this, false);

Use `{ capture: false, mozSystemGroup: true }` for each of the addEventListener/removeEventListener calls.

Comment 21

[Eugen Sawin [:esawin]]

Attachment: 0001-Bug-1437988-1.3-Add-progress-tracking-and-expose-pro.patch

Addressed comments and increased first paint and page show signal weights for a smoother load progression.

Comment 22

[Eugen Sawin [:esawin]]

Attachment: 0002-Bug-1437988-2.0-Use-progress-tracking-events-for-cus.patch

Update custom tabs to use the new progress events.

Comment 23

[Eugen Sawin [:esawin]]

Attachment: 0002-Bug-1437988-2.0-Use-progress-tracking-events-for-cus.patch

Missed the updateCanStop call.

Comment 24

[Dylan Roeh (:droeh) (he/him)]

Comment on attachment 8992643 [details] [diff] [review]
0002-Bug-1437988-2.0-Use-progress-tracking-events-for-cus.patch

Review of attachment 8992643 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM

Comment 25

[Eugen Sawin [:esawin]]

Attachment: 0001-Bug-1437988-1.4-Add-progress-tracking-and-expose-pro.patch

I've moved ProgressTracker out into the content module to avoid handling of unrelated MozAfterPaint events.

I've also added the onLocationChange signal.

With this setup, progression values look more reasonable.
The pageshow event seems to be fired too late on some terrible sites like cnn.com, but then it's never really clear when we finish loading those sites in general.

Comment 26

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8992749 [details] [diff] [review]
0001-Bug-1437988-1.4-Add-progress-tracking-and-expose-pro.patch

Review of attachment 8992749 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/chrome/geckoview/GeckoViewProgressContent.js
@@ +98,5 @@
> +  start: function(aUri) {
> +    debug `ProgressTracker start ${aUri}`;
> +
> +    addEventListener("MozAfterPaint", this,
> +                     { capture: false, mozSystemGroup: true });

I remembered you can put "once: true" here to avoid having to remove the listener when you receive the event.

Comment 27

[Eugen Sawin [:esawin]]

(In reply to James Willcox (:snorp) ([e-mail]) from comment #26)
> I remembered you can put "once: true" here to avoid having to remove the
> listener when you receive the event.

We can't use once because we need the URI-based filtering in ProgressTracker.handleEvent().

Comment 28

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Eugen Sawin [:esawin] from comment #27)
> (In reply to James Willcox (:snorp) ([e-mail]) from comment #26)
> > I remembered you can put "once: true" here to avoid having to remove the
> > listener when you receive the event.
> 
> We can't use once because we need the URI-based filtering in
> ProgressTracker.handleEvent().

Ah, indeed. Thanks.

Comment 29

[Chris Peterson [:cpeterson]]

[geckoview:klar:p1] because Eugen is working on this.

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d05c4d2a0ef8
[1.5] Add progress tracking and expose progress via a delegate callback. r=jchen,snorp,droeh
https://hg.mozilla.org/integration/mozilla-inbound/rev/e25320d4cca7
[2.0] Use progress tracking events for custom tabs progress bar. r=droeh

Comment 31

[Eugen Sawin [:esawin]]

Attachment: 0001-Bug-1437988-1.5-Add-progress-tracking-and-expose-pro.patch

Fixed linter and test issues. r=me.

Comment 32

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/d05c4d2a0ef8
https://hg.mozilla.org/mozilla-central/rev/e25320d4cca7

Comment 33

[Eugen Sawin [:esawin]]

Comment on attachment 8995259 [details] [diff] [review]
0001-Bug-1437988-1.5-Add-progress-tracking-and-expose-pro.patch

Approval Request Comment
[Feature/Bug causing the regression]: GeckoView page load progress tracking API.
[User impact if declined]: A GeckoView app may not have enough signals to show page load progress and the progress bar may get stuck without ever finishing, e.g., bug 1470299.
[Is this code covered by automated tests?]: No (Nimbledroid only runs Focus with GV 62).
[Has the fix been verified in Nightly?]: Verified through Fennec's custom tabs.
[Needs manual test from QE? If yes, steps to reproduce]: STR: Open a page and verify that the progress bar matches the visual load progress of the site. Currently, the API is only used for Fennec's custom tabs.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: Somewhat.
[Why is the change risky/not risky?]: It's risky because we don't have automated tests for progress tracking (besides Nimbledroid, which only runs on GV 62) yet. However, the implementation only affects the load progress bar, which is currently blocking the Focus/GV release.
[String changes made/needed]: None.

Comment 34

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8995259 [details] [diff] [review]
0001-Bug-1437988-1.5-Add-progress-tracking-and-expose-pro.patch

GeckoView related, Beta62+

Comment 35

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/releases/mozilla-beta/rev/9dcf2844400a
https://hg.mozilla.org/releases/mozilla-beta/rev/94572bfa7c3b