Last Comment Bug 481082 - Video controls should listen for |stalled| event
: Video controls should listen for |stalled| event
Status: RESOLVED FIXED
:
Product: Toolkit
Classification: Components
Component: Video/Audio Controls (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla8
Assigned To: (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2009-03-02 16:47 PST by Justin Dolske [:Dolske]
Modified: 2011-12-22 20:06 PST (History)
6 users (show)
bzbarsky: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch for bug 481082 (5.10 KB, patch)
2011-07-08 12:11 PDT, (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back)
no flags Details | Diff | Review
Patch for bug 481082 (5.89 KB, patch)
2011-07-08 12:16 PDT, (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back)
no flags Details | Diff | Review
Patch for bug 481082 (62.02 KB, patch)
2011-07-13 17:49 PDT, (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back)
dolske: review-
Details | Diff | Review
Patch for bug 481082 v2 (60.28 KB, patch)
2011-07-20 16:24 PDT, (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back)
dolske: review+
Details | Diff | Review

Description Justin Dolske [:Dolske] 2009-03-02 16:47:02 PST
Spun off from bug 470983.

When a video fires a |stalled| event (because it's waiting on data from the network, but nothing in coming in), it would be good to show some UI to indicate this state. We should already be showing the throbber by the time |stalled| fires, but it seems like we should differentiate between "data's coming in, sit tight and we'll be right with you" and "aww, crap, nothing's happening, go grab some coffee and a Snicker's bar".

Possible ideas:
 * Add text below the throbber. EG "Waiting", "Server not responding", etc.
 * Replace fast-spinning throbber with a slow-spinning throbber?

Localization of the text will be a bit of a pain, because the unprivileged video control code can't just load up a string bundle (as normal chrome would do). XUL does generated content now (on 1.9.2, at least, via bug 472500), so it might suffice to pull in some localized CSS.
Comment 1 (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back) 2011-07-08 12:11:45 PDT
Created attachment 544878 [details] [diff] [review]
Patch for bug 481082

With this patch, the throbber will spin backwards with the occurrence of the |stalled| event. We may want to change the graphic (or slow down the spinning).
Comment 2 (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back) 2011-07-08 12:16:17 PDT
Created attachment 544879 [details] [diff] [review]
Patch for bug 481082

With this patch, the throbber will spin backwards with the occurrence of the |stalled| event. We may want to change the graphic (or slow down the spinning).

[Previous patch was missing updates for pinstripe theme]
Comment 3 (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back) 2011-07-13 17:49:20 PDT
Created attachment 545792 [details] [diff] [review]
Patch for bug 481082

Updated the graphic to be a slower spinning throbber moving in the backwards direction.
Comment 4 Justin Dolske [:Dolske] 2011-07-19 18:17:21 PDT
Comment on attachment 545792 [details] [diff] [review]
Patch for bug 481082

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

::: toolkit/content/widgets/videocontrols.xml
@@ +315,5 @@
>                  setupStatusFader : function(immediate) {
>                      var show = false;
>                      if (this.video.seeking ||
>                          this.video.error ||
> +                        this.stalled ||

I don't think we want to check for stalled here, since you could be both in a stalled state and currently playing from a previously-buffered region.

We need to track the stalled state, but can just rely on the existing "waiting" event handling to trigger the throbber overlay.

Since there are a lot of other events setting the overlay type to "throbber", I suspect it would be easiest to just set an attribute ("stalled") on the throbber overlay (upon receiving stalled, and clear it upon receiving progress). And let CSS handle which image to show.

EG:

.statusIcon[type="throbber"] {
    background: url(chrome://global/skin/media/throbber.png) no-repeat center;
}
.statusIcon[type="throbber"][stalled] {
    background-image: url(stalled.png);
}
Comment 5 (Away 6/25-7/4) Jared Wein [:jaws] (reviews and needinfo disabled until back) 2011-07-20 16:24:28 PDT
Created attachment 547288 [details] [diff] [review]
Patch for bug 481082 v2

I made the recommended changes and was able to remove the |this.stalled| property since it was redundant.
Comment 6 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-07-29 11:30:51 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/ec66137aed05
Comment 7 Marco Bonardo [::mak] 2011-08-01 07:51:21 PDT
http://hg.mozilla.org/mozilla-central/rev/ec66137aed05

Note You need to log in before you can comment on or make changes to this bug.