Closed Bug 1392157 Opened 2 years ago Closed 2 years ago

Show a burst across the tab when a page has finished loading

Categories

(Firefox :: Tabbed Browser, enhancement, P1)

enhancement

Tracking

()

VERIFIED FIXED
Firefox 57
Iteration:
57.2 - Aug 29
Tracking Status
firefox57 --- fixed

People

(Reporter: jaws, Assigned: jaws)

References

(Depends on 2 open bugs, Blocks 1 open bug)

Details

(Whiteboard: [reserve-photon-animation])

Attachments

(2 files)

This bug is split off from bug 1352119. Bug 1352119 specs a burst animation that should run when a page has finished loading. This bug is on file to fix up the performance of this animation so we can land it.

The burst animation and any performance improvements related to it will land as part of this bug.
Flags: qe-verify+
Component: General → Tabbed Browser
Iteration: --- → 57.2 - Aug 29
Priority: P3 → P1
QA Contact: stefan.georgiev
Whiteboard: [photon-animation] → [reserve-photon-animation]
This is the baseline try push. This push shows the results of the tab loading indicator without the burst animation: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f40c030b5faf9ecce397a9b1e5b3c91d79d45b18.

Comparing the baseline to the r+'d burst animation patch: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=f40c030b5faf9ecce397a9b1e5b3c91d79d45b18&newProject=try&newRevision=33eedaf8383870de6436001236035104cbba0d0e&framework=1

Fixing some review nits
Baseline vs burst + review nits: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=f40c030b5faf9ecce397a9b1e5b3c91d79d45b18&newProject=try&newRevision=f7b6eb2189678b5c939f1700a645d1b82f411c57&framework=1
Interdiff (Burst vs burst + review nits): https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=33eedaf8383870de6436001236035104cbba0d0e&newProject=try&newRevision=f7b6eb2189678b5c939f1700a645d1b82f411c57&framework=1

I have implemented a few different optimizations. Each one has separate compare-talos results. All of these build on top of the previous, for example #5 includes #1-4. I have included links to interdiffs comparing the current revision to the previous one, for example #3 vs #2 to see if there is local improvement.

1. Don't force a synchronous layout flush when re-running the animation:
Baseline vs series: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=f40c030b5faf9ecce397a9b1e5b3c91d79d45b18&newProject=try&newRevision=21781ac8d450d285f7d2cdb6453404efe9a96746&framework=1
Interdiff: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=f7b6eb2189678b5c939f1700a645d1b82f411c57&newProject=try&newRevision=21781ac8d450d285f7d2cdb6453404efe9a96746&framework=1

2. Don't run the burst animation if the page finishes loading within 150ms:
Baseline vs series: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=f40c030b5faf9ecce397a9b1e5b3c91d79d45b18&newProject=try&newRevision=0cb66da1c6c7d56381f03f5660ca55477f1f6ea8&framework=1
Interdiff: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=21781ac8d450d285f7d2cdb6453404efe9a96746&newProject=try&newRevision=0cb66da1c6c7d56381f03f5660ca55477f1f6ea8&framework=1

3. Don't run the burst animation if a tab animation is in progress:
Baseline vs series: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=f40c030b5faf9ecce397a9b1e5b3c91d79d45b18&newProject=try&newRevision=a2a274712b74ca33e32a7b216e16d40f50bf4f21&framework=1
Interdiff: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=0cb66da1c6c7d56381f03f5660ca55477f1f6ea8&newProject=try&newRevision=a2a274712b74ca33e32a7b216e16d40f50bf4f21&framework=1

4. Wait until the session has been restored before running the burst animation:
Baseline vs series: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=f40c030b5faf9ecce397a9b1e5b3c91d79d45b18&newProject=try&newRevision=a293684e733c84f10e442d414ae9b5f7c8d7de14&framework=1
Interdiff: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=a2a274712b74ca33e32a7b216e16d40f50bf4f21&newProject=try&newRevision=a293684e733c84f10e442d414ae9b5f7c8d7de14&framework=1

5. Remove the bursting attribute when a tab is closing:
Baseline vs series: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=f40c030b5faf9ecce397a9b1e5b3c91d79d45b18&newProject=try&newRevision=00cd5bad7e63062895ec937bb84be9e4f9ebd769&framework=1
Interdiff: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=a293684e733c84f10e442d414ae9b5f7c8d7de14&newProject=try&newRevision=00cd5bad7e63062895ec937bb84be9e4f9ebd769&framework=1
Will we have a pref to disable this and the new tab animation(look nice but still want to disable it) which landed today?
It's distracting and just want to disable both of them.

Animations are pretty good looking but don't want animations so an option to disable it on my slow netbook will be appreciated.
(In reply to shellye from comment #3)
> Will we have a pref to disable this and the new tab animation(look nice but
> still want to disable it) which landed today?
> It's distracting and just want to disable both of them.
> 
> Animations are pretty good looking but don't want animations so an option to
> disable it on my slow netbook will be appreciated.

Yes, you can go to about:config and set toolkit.cosmeticAnimations.enabled to `false`. Doing so will disable tab animations, the burst added by this bug, stop/reload animation, notification bar animations, and potentially some others. We do not have more fine-grained prefs for disabling animations.
In optimization #2 above, a TART regression is introduced. I believe some of that may be due to the usage of performance.now(), which is about 17% slower than Date.now() on my local machine as measured by https://jsperf.com/perf-vs-date/1

8. Switch to using Date.now() from performance.now() since Date.now() is roughly 17% faster on my local machine and for tab loading times we don't need to worry about microsecond precision and a monotonically increasing clock isn't required for this calculation:
Baseline vs series: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=f40c030b5faf9ecce397a9b1e5b3c91d79d45b18&newProject=try&newRevision=893ae001aa0703b263b6b77b640101f54fac54be&framework=1
Interdiff: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=52c8d72ea59e73ab040bee7b2a4c0ba9b15c6424&newProject=try&newRevision=893ae001aa0703b263b6b77b640101f54fac54be&framework=1
(In reply to shellye from comment #5)
> Have set that pref but still tab throbber animates.

The tab throbber will continue to animate even if the pref is set to false, because the tab throbber is essential to knowing when a page is loading and has completed loading. We also will not disable the download animation even if this pref is set to false because otherwise the user will not know download progress.
The patch from bug 1352119 had an animation-duration of 1.8s, of which about 1.4s was spent animating the burst outside of the tab shape due to the animation-timing-function that was being used. I talked to UX and we were able to adjust the timing-function and in turn lower the duration of the animation from 1.8s to 375ms.

Now that the duration has been lowered to 375ms, we are much less likely to have overlapping burst animations. This also means that if one is playing we can simply skip playing an overlapping animation, simplifying our code as well as removing some code that was shown to add some large regressions in #1.

Through logging I measured that gBrowser.tabAnimationsInProgress was truthy for 85% of TART. We saw the largest regression in TART when I added the 150ms check in #2.

The CSS animation uses the `transform` property, and we can use the `will-change` property to hint that this will change and optimize for the animation. I don't expect this to show any changes in the performance but it is worthwhile to get confirmation.

9. Reduce the animation-duration to 375ms and tweak the timing-function:
Baseline vs series: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=f40c030b5faf9ecce397a9b1e5b3c91d79d45b18&newProject=try&newRevision=22f39bf16623e8a0e144b979e633461cd2003a0e&framework=1
Interdiff: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=893ae001aa0703b263b6b77b640101f54fac54be&newProject=try&newRevision=22f39bf16623e8a0e144b979e633461cd2003a0e&framework=1

10. Don't restart the animation if it is already in progress:
Baseline vs series: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=f40c030b5faf9ecce397a9b1e5b3c91d79d45b18&newProject=try&newRevision=106b5b9e6dd05b2a905630d438a9e4453ef7534a&framework=1
Interdiff: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=22f39bf16623e8a0e144b979e633461cd2003a0e&newProject=try&newRevision=106b5b9e6dd05b2a905630d438a9e4453ef7534a&framework=1

11. Don't take into account how long it took to load a page before showing the burst:
Baseline vs series: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=f40c030b5faf9ecce397a9b1e5b3c91d79d45b18&newProject=try&newRevision=82af5c25dce708448f939b569b964e1326db6fe5&framework=1
Interdiff: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=106b5b9e6dd05b2a905630d438a9e4453ef7534a&newProject=try&newRevision=82af5c25dce708448f939b569b964e1326db6fe5&framework=1

12. Use the `will-change` CSS property:
Baseline vs series: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=f40c030b5faf9ecce397a9b1e5b3c91d79d45b18&newProject=try&newRevision=74721376c4ee870db20702284104c76af9af90cd&framework=1
Interdiff: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=82af5c25dce708448f939b569b964e1326db6fe5&newProject=try&newRevision=74721376c4ee870db20702284104c76af9af90cd&framework=1
Some of the regressions shown in the previous pushes are regressions in memory use. After talking to erahm and jmaher, I triggered AWSY tests to run because they are more reliable than the tp5 test suite for memory tracking.

This is a link to the AWSY perfherder results comparing the baseline vs #7 change from above. The comparison shows no important (2%+) change:

https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=f40c030b5faf9ecce397a9b1e5b3c91d79d45b18&newProject=try&newRevision=52c8d72ea59e73ab040bee7b2a4c0ba9b15c6424&framework=4&showOnlyImportant=0
Comment on attachment 8900730 [details]
Bug 1392157 - Implement new page loading indicator animation, part 2: add a "burst" when loading finishes.

https://reviewboard.mozilla.org/r/172172/#review177428
Attachment #8900730 - Flags: review?(jaws) → review+
Comment on attachment 8900731 [details]
Bug 1392157 - Performance improvements to burst animation.

https://reviewboard.mozilla.org/r/172174/#review177424

::: browser/base/content/tabbrowser.xml:734
(Diff revision 1)
>  
>                    // Only animate the "burst" indicating the page has loaded if
>                    // the top-level page is the one that finished loading.
>                    if (aWebProgress.isTopLevel && !aWebProgress.isLoadingDocument &&
> +                      !this.mTabBrowser.tabAnimationsInProgress &&
> +                      Date.now() - this.mTab.timeWhenStartedLoading > 150 &&

I don't understand how this makes sense from a user's perspective. Assuming the burst animation is supposed to signal to the user that a page has finished loading, then it seems this should be done consistently even for fast-loading pages.

::: browser/base/content/tabbrowser.xml:7674
(Diff revision 1)
> -            // Stop the animation if it's already playing so we can restart it.
> +            return;
> -            this.removeAttribute("bursting");
> -            let burst = document.getAnonymousElementByAttribute(this, "anonid", "tab-loading-burst");
> -            window.getComputedStyle(burst).animationName;
>            }
>            this.setAttribute("bursting", "true");

At this point you can just remove this method and do tab.setAttribute("bursting") on the call site.
(In reply to Dão Gottwald [::dao] from comment #14)
> Comment on attachment 8900731 [details]
> Bug 1392157 - Performance improvements to burst animation.
> 
> https://reviewboard.mozilla.org/r/172174/#review177424
> 
> ::: browser/base/content/tabbrowser.xml:734
> (Diff revision 1)
> >  
> >                    // Only animate the "burst" indicating the page has loaded if
> >                    // the top-level page is the one that finished loading.
> >                    if (aWebProgress.isTopLevel && !aWebProgress.isLoadingDocument &&
> > +                      !this.mTabBrowser.tabAnimationsInProgress &&
> > +                      Date.now() - this.mTab.timeWhenStartedLoading > 150 &&
> 
> I don't understand how this makes sense from a user's perspective. Assuming
> the burst animation is supposed to signal to the user that a page has
> finished loading, then it seems this should be done consistently even for
> fast-loading pages.

I removed this in #11 above, but removing this reintroduced the bloom_basic and damp regression. It's possible that we don't care about these regressions for bloom_basic and damp. bloom_basic loads many pages very fast in the same tab, too fast to actually see the tab contents changing. DAMP loads many tabs and loads the devtools in the tab, acting as a poor cousin of TART.
Comment on attachment 8900731 [details]
Bug 1392157 - Performance improvements to burst animation.

https://reviewboard.mozilla.org/r/172174/#review177474

::: browser/base/content/tabbrowser.xml:735
(Diff revision 2)
>                    // the top-level page is the one that finished loading.
>                    if (aWebProgress.isTopLevel && !aWebProgress.isLoadingDocument &&
> +                      !this.mTabBrowser.tabAnimationsInProgress &&
>                        Services.prefs.getBoolPref("toolkit.cosmeticAnimations.enabled")) {
> -                    this.mTab.animateLoadingBurst();
> +                    if (!this.mTab.hasAttribute("bursting")) {
> +                      this.mTab.setAttribute("bursting", "true");

nit: you can remove the hasAttribute check, it shouldn't make a difference

I'm not sure about the tabAnimationsInProgress check. If we're lucky, this is enough of an edge case that users won't run into it or just won't notice. If we're not lucky, this introduces inconsistent behavior.
Comment on attachment 8900731 [details]
Bug 1392157 - Performance improvements to burst animation.

https://reviewboard.mozilla.org/r/172174/#review177548

::: browser/base/content/tabbrowser.xml:735
(Diff revision 2)
>                    // the top-level page is the one that finished loading.
>                    if (aWebProgress.isTopLevel && !aWebProgress.isLoadingDocument &&
> +                      !this.mTabBrowser.tabAnimationsInProgress &&
>                        Services.prefs.getBoolPref("toolkit.cosmeticAnimations.enabled")) {
> -                    this.mTab.animateLoadingBurst();
> +                    if (!this.mTab.hasAttribute("bursting")) {
> +                      this.mTab.setAttribute("bursting", "true");

If you don't end up removing this for some reason, you should probably fold the hasAttribute check into the outer conditional. I suspect dao's right that it doesn't change much, but happy to hear otherwise.

::: browser/base/content/tabbrowser.xml
(Diff revision 2)
> -        <body><![CDATA[
> -          if (this.hasAttribute("bursting")) {
> -            // Stop the animation if it's already playing so we can restart it.
> -            this.removeAttribute("bursting");
> -            let burst = document.getAnonymousElementByAttribute(this, "anonid", "tab-loading-burst");
> -            window.getComputedStyle(burst).animationName;

Hooray!
Attachment #8900731 - Flags: review?(mconley) → review+
Comment on attachment 8900731 [details]
Bug 1392157 - Performance improvements to burst animation.

https://reviewboard.mozilla.org/r/172174/#review177474

> nit: you can remove the hasAttribute check, it shouldn't make a difference
> 
> I'm not sure about the tabAnimationsInProgress check. If we're lucky, this is enough of an edge case that users won't run into it or just won't notice. If we're not lucky, this introduces inconsistent behavior.

I'd like to keep the tabAnimationsInProgress for now. We know that removing it regresses TART significantly, and Eric is OK with not running it in this case. I think it's something that we could land now and if it does become a problem it will be easy to remove this later.
Pushed by jwein@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4a8b63a9298d
Implement new page loading indicator animation, part 2: add a "burst" when loading finishes. r=jaws
https://hg.mozilla.org/integration/autoland/rev/ce5d0cb837b1
Performance improvements to burst animation. r=mconley
https://hg.mozilla.org/mozilla-central/rev/4a8b63a9298d
https://hg.mozilla.org/mozilla-central/rev/ce5d0cb837b1
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Depends on: 1394060
Depends on: 1394189
User Agent    Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:57.0) Gecko/20100101 Firefox/57.0
Build   ID    20170830100230

This bug fix is Verified with latest Nightly 57.0a1.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Depends on: 1398442
Depends on: 1399111
Depends on: 1399130
Depends on: 1399458
Depends on: 1398142
Depends on: 1447657
You need to log in before you can comment on or make changes to this bug.