Consider adding onProgressChanged method for smoother loading appearance

RESOLVED FIXED in Firefox 62

Status

defect
P1
normal
RESOLVED FIXED
Last year
6 months ago

People

(Reporter: ekager, Assigned: esawin)

Tracking

(Blocks 2 bugs)

unspecified
mozilla63
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 wontfix, firefox-esr60 wontfix, firefox61 wontfix, firefox62 fixed, firefox63 fixed)

Details

(Whiteboard: [geckoview:klar:p1][qf:p1])

Attachments

(2 attachments, 6 obsolete attachments)

Reporter

Description

Last year
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
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?
Flags: needinfo?(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.
Flags: needinfo?(bzbarsky)
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.
Priority: -- → P3
Whiteboard: [geckoview:klar]
See Also: → 1456153
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.
(In reply to Sebastian Kaspari (:sebastian) from comment #4)
> like in Fennec?

Which would mean intermediate points for onLocationChange and DOMContentLoaded.
Duplicate of this bug: 1456153
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.
Assignee

Comment 8

Last year
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.
Assignee: nobody → esawin
Attachment #8985239 - Flags: feedback?(snorp)
Attachment #8985239 - Flags: feedback?(droeh)
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
Attachment #8985239 - Flags: feedback?(droeh) → feedback+
Assignee

Comment 10

Last year
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.
(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 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
Whiteboard: [geckoview:klar] → [geckoview:klar] [geckoview] [qf:p1]
Assignee

Comment 13

Last year
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) 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.
Assignee

Comment 14

Last year
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.
Attachment #8985239 - Attachment is obsolete: true
Attachment #8985239 - Flags: feedback?(snorp)
Attachment #8991661 - Flags: feedback?(snorp)
Attachment #8991661 - Flags: feedback?(nchen)
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
Attachment #8991661 - Flags: feedback?(snorp) → feedback+
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?
Attachment #8991661 - Flags: feedback?(nchen) → feedback+
Assignee

Comment 17

11 months ago
Addressed all comments.

(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) 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.
Attachment #8991661 - Attachment is obsolete: true
Attachment #8992304 - Flags: review?(nchen)
Attachment #8992304 - Flags: review?(droeh)
Given the QF prioritization I'm bumping this to P1.
Priority: P5 → P1
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?
Attachment #8992304 - Flags: review?(droeh) → review+
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.
Attachment #8992304 - Flags: review?(nchen) → review+
Assignee

Comment 21

11 months ago
Addressed comments and increased first paint and page show signal weights for a smoother load progression.
Attachment #8992304 - Attachment is obsolete: true
Attachment #8992607 - Flags: review?(snorp)
Assignee

Comment 22

11 months ago
Update custom tabs to use the new progress events.
Attachment #8992610 - Flags: review?(droeh)
Assignee

Comment 23

11 months ago
Missed the updateCanStop call.
Attachment #8992610 - Attachment is obsolete: true
Attachment #8992610 - Flags: review?(droeh)
Attachment #8992643 - Flags: review?(droeh)
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
Attachment #8992643 - Flags: review?(droeh) → review+
Assignee

Comment 25

11 months ago
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.
Attachment #8992607 - Attachment is obsolete: true
Attachment #8992607 - Flags: review?(snorp)
Attachment #8992749 - Flags: review?(snorp)
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.
Attachment #8992749 - Flags: review?(snorp) → review+
Assignee

Comment 27

11 months ago
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) 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().
(In reply to Eugen Sawin [:esawin] from comment #27)
> (In reply to James Willcox (:snorp) (jwillcox@mozilla.com) 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.
[geckoview:klar:p1] because Eugen is working on this.
Whiteboard: [geckoview:klar] [geckoview] [qf:p1] → [geckoview:klar:p1][qf:p1]

Comment 30

11 months ago
Pushed by esawin@mozilla.com:
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
Assignee

Comment 31

11 months ago
Fixed linter and test issues. r=me.
Attachment #8992749 - Attachment is obsolete: true
Attachment #8995259 - Flags: review+
Assignee

Updated

11 months ago
Blocks: 1470299
Assignee

Updated

11 months ago
Blocks: 1454477

Comment 32

11 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d05c4d2a0ef8
https://hg.mozilla.org/mozilla-central/rev/e25320d4cca7
Status: NEW → RESOLVED
Closed: 11 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
Assignee

Comment 33

11 months ago
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.
Attachment #8995259 - Flags: approval-mozilla-beta?
Comment on attachment 8995259 [details] [diff] [review]
0001-Bug-1437988-1.5-Add-progress-tracking-and-expose-pro.patch

GeckoView related, Beta62+
Attachment #8995259 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Updated

6 months ago
Product: Firefox for Android → GeckoView
Target Milestone: Firefox 63 → mozilla63
You need to log in before you can comment on or make changes to this bug.