679927 - Use a two-stage throbber in Fennec

Status: RESOLVED WONTFIX

(Firefox for Android Graveyard :: General, defect)

Description

[Matt Brubeck (:mbrubeck)]

Attachment: WIP

Desktop firefox uses a two-state throbber to indicate whether a tab is still connecting, or has connected successfully and is loading (bug 602964).  Doing the same for Fennec would have the following benefits:

* Provides feedback on what is happening when pages don't load quickly
* Brand/UX consistency with desktop Firefox

This patch is a proof of concept.  There's still some work left if we want to adopt this in Fennec:

* We need new artwork.  The patch uses artwork from the Mac "pinstripe" theme (scaled up from 16px to 32px), which is the wrong resolution and doesn't work well against our Fenenc's blue or green favicon backgrounds for SSL sites.
* We don't want not to regress bug 539495 (high-frame-rate animated throbber uses too much CPU on mobile).
* This patch replaces the current throbber.png which is also used in about:home and in the Add-on Manager when they are loading content.  Do we want these to stay consistent with the page-load throbber, or do we want to keep using the old Fennec throbber in those locations?
* I'm not sure if I like how this patch adds a message to the browser binding.

There's a test Android build at https://people.mozilla.com/~mbrubeck/fennec-throbber.apk if anyone wants to try out this patch.

Comment 1

[Matt Brubeck (:mbrubeck)]

In bug 539495, we reduced the throbber's frame rate to 4fps because the previous version was using 50% CPU on N900.

The current 4fps Fennec version uses about 5% CPU on my G2 while the desktop version (I don't know the frame rate) uses around 20-25% CPU on my G2.  I pushed the patch to Try to see if this level of CPU usage has any effect on page load performance.

Comment 2

[Matt Brubeck (:mbrubeck)]

Using the desktop throbber does not seem to impact page load, so I think we can safely increase our frame rate to whatever they are using.  The throbber shouldn't be visible long enough during normal use to impact battery life noticeably.
http://tbpl.allizom.org/?tree=Try&usebuildbot=1&rev=3e7893fdaa51

Comment 3

[Matt Brubeck (:mbrubeck)]

Adding Ian to request artwork.  We will need two 32x32 animated PNGs, one similar to the "connecting" animation from desktop Firefox:

https://mxr.mozilla.org/mozilla-central/source/browser/themes/gnomestripe/browser/tabbrowser/connecting.png

and one similar to the "loading" animation which is different on each platform:

https://mxr.mozilla.org/mozilla-central/source/browser/themes/pinstripe/browser/tabbrowser/loading.png
https://mxr.mozilla.org/mozilla-central/source/browser/themes/winstripe/browser/tabbrowser/loading.png
https://mxr.mozilla.org/mozilla-central/source/browser/themes/gnomestripe/browser/tabbrowser/loading.png

As far as I can tell, we should be able to use the same frame rate as the desktop versions (see comment 1 and comment 2).

This animations appear in the urlbar end-cap, which has a light background for normal sites and a dark green or blue background for SSL sites.

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

Do we have tools to create animated PNGs? The last tool I used was Dolske's animated PNG add-on. You pass it some JS and it "makes" the animated PNG.

Comment 5

[:Margaret Leibovic]

Stephen made the images we use in the desktop browser. He probably knows what should be done, and also probably has the files that were used for the desktop throbbers.

Comment 6

[Ian Barlow (:ibarlow)]

Mark, I have tools I've used in the past to build animated PNGs. I think it might actually be a firefox plugin where you upload images and it produces the animated png. 

Which might, in fact, be Dolske's add on :)

Comment 7

[Stephen Horlander [:shorlander]]

I have been using japng https://www.reto-hoehener.ch/japng/

Comment 9

[Max Stepin]

I have a bunch of command-line APNG tools, feel free to use them:

http://sourceforge.net/projects/apngasm/
http://sourceforge.net/projects/apngdis/
http://sourceforge.net/projects/gif2apng/
http://sourceforge.net/projects/apng2gif/

Comment 10

[Ian Barlow (:ibarlow)]

Attachment: Activity Spinners (animated PNGs)

I've included two sizes here: 24x24 and 32x32

I believe the 32x32 spinners will look best on the phone versions of Fennec, but we may need to scale down to 24x24 on tablet. Let's try them and see.

Comment 11

[Matt Brubeck (:mbrubeck)]

Attachment: patch

This patch uses the 24px icons on Honeycomb, and the 32px icons on Froyo and Gingerbread.  There is a test build at http://people.mozilla.com/~mbrubeck/fennec-throbber2.apk

The new spinners are hard to see against the blue favicon background for DV sites, and look a little weird against the green favicon background for EV sites.  Maybe we could keep the background grey while the throbber is animating?

I'll wait for a design decision from Ian before checking this in, but I think the code parts are ready for review.

Comment 12

[Ian Barlow (:ibarlow)]

Comment on attachment 561522 [details] [diff] [review]
patch

There are no words for the joy I feel seeing these spinners. Let's just change our favicon background colours a bit so the spinners show up better.

Blue - #b2bcf1

Green - #c4eb8a

Comment 13

[Matt Brubeck (:mbrubeck)]

(In reply to Ian Barlow (:ibarlow) from comment #12)
> There are no words for the joy I feel seeing these spinners. Let's just
> change our favicon background colours a bit so the spinners show up better.

In the gingerbread and honeycomb themes, these are images and have an active state when pressed, for example:

http://hg.mozilla.org/mozilla-central/raw-file/4ea7e2080666/mobile/themes/core/images/endcap-ssl-default-bg.png
http://hg.mozilla.org/mozilla-central/raw-file/4ea7e2080666/mobile/themes/core/images/endcap-ssl-active-bg.png

Do you want to provide updated images, or replace them with the same flat colors as the honeycomb theme..?

Comment 14

[Max Stepin]

Some of the proposed spinners waste almost 1kb for iTXT comments.

Comment 15

[Ian Barlow (:ibarlow)]

Matt, I would like to update the background images, but in a separate bug please :)

Comment 16

[Matt Brubeck (:mbrubeck)]

(In reply to Max Stepin from comment #14)
> Some of the proposed spinners waste almost 1kb for iTXT comments.

Good catch, thanks!  I'll make sure to pngcrush the files before pushing this patch.

Comment 17

[Max Stepin]

Don't forget to rename extension to .apng before pngcrush, 
and then rename it back to .png after pngcrush.

That way pngcrush will not remove apng chunks.

Comment 18

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 561522 [details] [diff] [review]
patch

I don't want to introduce "progress" events/callbacks unless we also start using the C++ browser filter (bug 474464). That should also wait for Fx10 IMO.

For Fx9, UX would be happy with just using "the blue one" (ibarlow's words)

Comment 19

[Ian Barlow (:ibarlow)]

Attachment: background images for froyo and gingerbread

Comment 20

[Matt Brubeck (:mbrubeck)]

Attachment: patch v2

Same code, rebased on top of the patches in bug 688689.

> I don't want to introduce "progress" events/callbacks unless we also start
> using the C++ browser filter (bug 474464). That should also wait for Fx10
> IMO.

That bug has a non-working patch since 2009; it looks like the filter code isn't e10s-ready (see my comment in bug 474464).  It's not clear to me that it's necessary either.  When I pushed the patch in this bug to Try and ran the tp4m test several times, I did not see any noticeable regression.  (The noise in that test makes it hard to say for sure without a lot more samples though.)

This post says that OnProgress messages should be free from an IPC point of view, so the only perf impact would be from the JavaScript code in the content process (which 99% of the time will just return immediately in the first "if" expression):
https://groups.google.com/d/msg/mozilla.dev.planning/Y72S3srGW2E/2PPdm4cheoEJ

I'm happy to wait until Fx10 to land this, but I think we should try it as-is and back it out if there turns out to be a real performance hit.

Comment 21

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 562184 [details] [diff] [review]
patch v2

We can't take this as is. The backout for the 20fps spinner showed Android was affected on startup (Ts regressed ~200ms). I know for a fact that even empty OnProgress listeners hurt Tp on Android and Maemo.

We need to use a different FPS on the spinner and we need the browser filter to throttle the OnProgress in C++. Even with those two changes I bet Ts and Tp are still affected - but hopefully not significantly.