Closed Bug 1232357 Opened 9 years ago Closed 8 years ago

Delay hiding the sound indicator icon by a few seconds after audio has stopped

Categories

(Firefox :: Tabbed Browser, defect)

Product:
Firefox
Component:
Tabbed Browser
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 52
Tracking Flags:
Tracking Status
firefox45 --- affected
firefox52 --- fixed

People

(Reporter: jaws, Assigned: jaws)

References

Details

Attachments

(1 file)

Bug 1232357 - Delay hiding the sound indicator icon by a few seconds after audio has stopped.
58 bytes, text/x-review-board-request
Gijs
: review+
Details 
Some tabs create short bursts of sound, but we remove the sound indicator icon immediately once the sound has stopped. For example, Gmail will play a sound when you get an instant message over Hangouts, and so will Facebook when you get an IM.

Other sites may play a short burst of audio when a video starts playing then pauses to buffer more content.

We should delay hiding the audio indicator (muted and unmuted) by about three seconds to help users in finding where the audio is coming from.
See Also: → 1239372
I first suggested it in bug 486262 comment 186, while bug 486262 comment 188 says:
> The indicator being visible when the tab is not playing sound is even more confusing
However, Ehsan didn't completely reject this idea in bug 486262 comment 192

Then, in bug 1239372 comment 5 I came up with scenario (opposite to "a short burst of audio") that still causes blinking of sound indicator (as well as the other scenario, when a new portion of media is buffered and it starts producing sound just after sound indicator was hidden).
I hope all that will be taken into account in this bug, if it won't be closed.
Hiro, browser_audioTabIcon.js times out with this patch when waiting for the two TabAttrModified events. Do you see what I am doing wrong in this patch?
Flags: needinfo?(hiikezoe)
Assignee: nobody → jaws
Status: NEW → ASSIGNED
Though I am not a right person to reviewing patches in this field, I could help you something.

Adding SetTimeout(0) spins a main loop, so some events are handled in the loop.  You might need to change the test codes or its implementation carefully.
Flags: needinfo?(hiikezoe)
Comment on attachment 8720652 [details]
Bug 1232357 - Delay hiding the sound indicator icon by a few seconds after audio has stopped.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35409/diff/1-2/
Attachment #8720652 - Attachment description: MozReview Request: Bug 1232357 - Delay hiding the sound indicator icon by a few seconds after audio has stopped. r?hiro → MozReview Request: Bug 1232357 - Delay hiding the sound indicator icon by a few seconds after audio has stopped. r?dao
Attachment #8720652 - Flags: review?(hiikezoe) → review?(dao)
I don't particularly like that the tests will use a slightly different code path. Let me know if you have ideas on how to make this better.
Comment on attachment 8720652 [details]
Bug 1232357 - Delay hiding the sound indicator icon by a few seconds after audio has stopped.

Can you please get this ui-reviewed first?
Attachment #8720652 - Flags: review?(dao)
Stephen, can you give your thoughts about the changes I proposed in comment #0?
Flags: needinfo?(shorlander)
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #10)
> Stephen, can you give your thoughts about the changes I proposed in comment
> #0?

Seems reasonable to me. Gives people enough time to react, but not so long that it's likely to conflict with actively playing tabs.
Flags: needinfo?(shorlander)
I did try the patch now.  It seems not to work properly when swapBrowsersAndCloseOther is called.  Though I am not reviewer, I am going to leave some comments.
https://reviewboard.mozilla.org/r/35409/#review32381

::: browser/base/content/tabbrowser.xml:2534
(Diff revision 2)
> +              this._tabAttrModified(aOtherTab, ["soundplaying"]);

This mubt be 'otherBrowser._tabModified(aOtherTab, ["soundplaying"]);'

::: browser/base/content/tabbrowser.xml:4641
(Diff revision 2)
> +          tab.mSoundPlayingAttrRemovalTimer = 0;

When swapBrowsersAndCloseOther is called, DOMAudioPlaybackStopped is called and then DOMAudioPlaybackStarted is called.  So if the timeout value is set and the tab has still "soundplaying" attribute (that means the "soundplaying" is actually stopped at this time),  _tabAttrModified(tab, ["soundplaying"]) does not invoked at all.

So, we should do something like this:

if (tab.mSoundPlayingAttrRemovalTimer) {
  clearTimeout(tab.mSoundPlayingAttrRemovalTimer);
  tab.mSoundPlayingAttrRemovalTimer = 0;
}

if (!tab.hasAttribute("soundplaying")) {
  tab.setAttribute("soundplaying", true);
}
this._tabAttrModified(tab, ["soundplaying"];

With this changes, we have to change the test codes to wait just one TabAttrModified there.

::: browser/base/content/tabbrowser.xml:4663
(Diff revision 2)
> +              this.mSoundPlayingAttrRemovalTimer = setTimeout(() => {

I think tab.mSoundPlayingAttrRemovalTimer = setTimeout(...

::: browser/base/content/tabbrowser.xml:4671
(Diff revision 2)
>            }

With above changes, this else block is not needed any more.
NI Ehsan, because he might have interest to know this change.
Flags: needinfo?(ehsan)
https://reviewboard.mozilla.org/r/35409/#review32943

This looks good to me overall, a couple of drive-by comments!

::: browser/app/profile/firefox.js:461
(Diff revision 2)
> +pref("browser.tabs.delayHidingAudioPlayingIconMS", 3000);

Do we want the same thing on Android?  It's worth at least filing a bug about it.

::: browser/base/content/tabbrowser.xml:4662
(Diff revision 2)
> +            if (removalDelay) {

Is there a good reason to have this branch here?  Why not just eliminate it and let us do a setTimeout(0) when the pref is set to 0?  That should help us test something closer to what we ship...
Thanks for the needinfo!

Another thing to think about.  Chrome uses a 10 second delay.  I'm curious to know where the 3 second value came from, and whether we considered if it's better to make the value longer or shorter.  (I personally don't feel strongly about the value of the timeout, just bringing this up!)
Flags: needinfo?(ehsan)
Attachment #8720652 - Attachment description: MozReview Request: Bug 1232357 - Delay hiding the sound indicator icon by a few seconds after audio has stopped. r?dao → MozReview Request: Bug 1232357 - Delay hiding the sound indicator icon by a few seconds after audio has stopped. r?gijs
Attachment #8720652 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8720652 [details]
Bug 1232357 - Delay hiding the sound indicator icon by a few seconds after audio has stopped.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35409/diff/2-3/
Comment on attachment 8720652 [details]
Bug 1232357 - Delay hiding the sound indicator icon by a few seconds after audio has stopped.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35409/diff/3-4/
I've switched to a 10 second delay to match Chrome. This should help to keep the indicator visible between videos that are auto-advancing on YouTube, for example. It also doesn't seem to be a point that we should differentiate on unless we hear that this delay is bothering users.
Comment on attachment 8720652 [details]
Bug 1232357 - Delay hiding the sound indicator icon by a few seconds after audio has stopped.

https://reviewboard.mozilla.org/r/35409/#review33119

This seems generally sane, but see my ridiculously long somewhat-stream-of-consciousness comment about the test change. :-\

You get r+, but it would be a good idea, IMHO, to write a simple test for the functionality as written here, that just verifies that we don't remove the attribute synchronously when playback stops.

::: browser/base/content/tabbrowser.xml:2507
(Diff revision 4)
> +              this._tabAttrModified(aOtherTab, ["soundplaying"]);

This will invoke the CustomEvent constructor in this window, but aOtherTab might be in another window. I think this will Just Work, as the event is dispatched on the other tab anyway, but it might be saner to use the other tab's gBrowser? You could move this block after the next declaration and use `remoteBrowser._tabAttrModified` etc.

::: browser/base/content/test/general/browser_audioTabIcon.js:220
(Diff revision 4)
> -  let receivedSoundPlaying = 0;
> -  // We need to receive two TabAttrModified events with 'soundplaying'
> -  // because swapBrowsersAndCloseOther involves nsDocument::OnPageHide and
> -  // nsDocument::OnPageShow. Each methods lead to TabAttrModified event.
> +  // We only receive one TabAttrModified event with 'soundplaying' because
> +  // the attribute is persisted on a timeout that lives longer than the
> +  // DOMAudioPlaybackStopped and DOMAudioPlaybackStarted that is triggered
> +  // by the nsDocument::onPageHide and nsDocument::OnPageShow, respectively.

I'm not familiar with this test, so this is confusing to me.

The swap happens a bit earlier, and we wait for an attribute change there. Why would there be another soundplaying change after the initial one where both the soundplaying and muted attribute are modified?

Maybe the comment is adding to my confusion, because it implies the old comment, but obviously that's no longer there.

OK, so can you confirm that this is what happens:

1. we swap the browsers. `swapBrowsersAndCloseOther` fires the attrchange thing here: https://dxr.mozilla.org/mozilla-central/rev/d848a5628d801a460a7244cbcdea22d328d8b310/browser/base/content/tabbrowser.xml#2516-2525,2578-2579 (note 2 selected regions).
2. pagehide is triggered *after this happens* (by swapBrowsersAndCloseOther?) and triggers (how/where?) the audio to stop, which fires the `DOMAudioPlaybackStopped` handler this patch modifies. That used to change the attribute immediately and fire off another TabAttrModified, and now it does that on a 0-millisecond delay (which gets clamped to actually run an event loop and fires after ~10ms or whatever).
3. pageshow is triggered (how/where?) and triggers (how/where?) the audio to restart, which fires `DOMAudioPlaybackStarted` and even though the attribute is still there, your patch modified the code so this will (a) clear the timeout from (2) and (b) fire another TabAttrModified.

So I guess I don't understand how we're firing pagehide/pageshow (and both in the same event loop cycle? Is that guaranteed, because if not this will race because the removal delay is set to 0 seconds...) and why those cause these events when the audio is muted anyway. But assuming that the above is what happens, this seems sane, assuming also that the pagehide/pageshow and their triggered playbackstopped/started events are guaranteed to fire before the 0-ms timeout fires.

If this races, I guess we would continue "too soon" as we'd only have had an event for 'stop' and not for 'start', and the tab might intermittently not be in the right state for the test assertions that follow this yield statement.

Maybe inside the event matching function that used to count to 2, we can now only return true if the current value/presence of the attribute on the tab is correct? That would ensure that even if this races now or in the future (due to the 0-second timeout), the test will pass.

::: browser/base/content/test/general/browser_audioTabIcon.js:226
(Diff revision 4)
> -    if (event.detail.changed.indexOf("soundplaying") >= 0) {
> +    return event.detail.changed.indexOf("soundplaying") >= 0;

Nit: changed.includes("soundplaying")

if we're changing this anyway.
Attachment #8720652 - Flags: review?(gijskruitbosch+bugs) → review+
(In reply to :Ehsan Akhgari from comment #16)
> Another thing to think about.  Chrome uses a 10 second delay.  I'm curious
> to know where the 3 second value came from
Just in case: Chrome 48.0.2564.116 m  uses 3-4 seconds delay, which I find too huge delay actually
Since you decided to follow irreproachable Chrome behavior, please make delay at least 3s, not 10s
Screencast:
> https://dl.dropboxusercontent.com/s/9zmxw1g72ebeeyn/bug%201232357%20comment%2021.webm?dl=0
Comment on attachment 8720652 [details]
Bug 1232357 - Delay hiding the sound indicator icon by a few seconds after audio has stopped.

>      <field name="mSoundPlayingAttrRemovalTimer">0</field>

The m prefix is only used in ancient front-end code. Please use _ instead.
Hi, Jared,
Do we have any plan to land this bug?
Thanks!
Flags: needinfo?(jaws)
Yes, I need to respond to the feedback from comment #20 still. Would you like to take over this bug?
Flags: needinfo?(jaws) → needinfo?(alwu)
Hi, Jared,
Sorry for the delay reply.
Since I don't understand well about the usage of "swapBrowsersAndCloseOther" and what you did in this function, I'm afraid of making some mistakes for that if I take over this bug.
Maybe you can continue to finish it when you have available time?
Thanks!
Flags: needinfo?(alwu)
Blocks: 1284830
Jared, what's needed to move forward with this bug?
Flags: needinfo?(jaws)
(In reply to :Gijs Kruitbosch from comment #26)
> Jared, what's needed to move forward with this bug?

Basically just time. Comment #20 still needs to be addressed.
Flags: needinfo?(jaws)
Flags: needinfo?(gijskruitbosch+bugs)
I'll start working on this now.
Flags: needinfo?(gijskruitbosch+bugs)
I looked back at the test and the tabAttrModified call. It looks like I might not have fully understood how the code flowed at the time, but it seems much clearer to me now. I removed the second block from the test because by-design there will be no event or change after the AttrChangePromise is resolved.

I can still add a test to confirm the behavior of this change.
In the latest revision I included a a test that that verifies that we don't remove the attribute synchronously when playback stops.
Comment on attachment 8720652 [details]
Bug 1232357 - Delay hiding the sound indicator icon by a few seconds after audio has stopped.

https://reviewboard.mozilla.org/r/35409/#review85626

::: browser/base/content/test/general/browser_audioTabIcon.js:32
(Diff revision 6)
> +  let awaitDOMAudioPlaybackStopped =
> +    BrowserTestUtils.waitForEvent(browser, "DOMAudioPlaybackStopped", "DOMAudioPlaybackStopped event should get fired after pause");
>    yield ContentTask.spawn(browser, {}, function* () {
>      let audio = content.document.querySelector("audio");
>      audio.pause();
>    });
> +  ok(tab.hasAttribute("soundplaying"), "The tab should still have the soundplaying attribute immediately after pausing");
> +
> +  yield awaitDOMAudioPlaybackStopped;
> +  ok(tab.hasAttribute("soundplaying"), "The tab should still have the soundplaying attribute immediately after DOMAudioPlaybackStopped");

This feels racy. You're setting the delay to 0 with the test prefs - if there's other stuff happening on the main thread, it seems possible that by the time either of these promises (ie contenttask.spawn's rv and the domaudioplaybackstopped one) yield, the tab playing event has also fired because the timer has fired as well.

One way to fix this would be to create the tab playing promise earlier, and to compare the hasAttribute state with the resolution state of the promise. You could do this by adding a .then() to the tab playing event promise that writes to a local variable, and then you can is() the hasAttribute result to the sound playing variables.

The downside is, of course, that this makes it impossible to guarantee that immediately after the pause has executed, we don't immediately remove the attribute... (ie if wait_for_tab_playing_event resolves too soon, we're not actually testing the delay here).

So it feels like we should do the comparison I suggested above in the general case to avoid intermittents, and add a separate test where we override the delay to something larger (than 3s, or the 0 you're using in prefs_general) for 1 specific test, and then assert that immediately after a .pause(), we don't update the attribute. Does that sound sensible?

::: browser/base/content/test/general/browser_audioTabIcon.js
(Diff revision 6)
> -  ok(newTab.hasAttribute("muted"), "Expected the correct muted attribute on the new tab");
> -  is(newTab.muteReason, null, "Expected the correct muteReason property on the new tab");
> -  ok(newTab.hasAttribute("soundplaying"), "Expected the correct soundplaying attribute on the new tab");

Shouldn't we still check that the tab in the new window has the right attributes?
Flags: needinfo?(jaws)
Comment on attachment 8720652 [details]
Bug 1232357 - Delay hiding the sound indicator icon by a few seconds after audio has stopped.

https://reviewboard.mozilla.org/r/35409/#review85626

> Shouldn't we still check that the tab in the new window has the right attributes?

Yes, we're testing that it has the right attributes above this also. Since we now don't have a TabAttrModified event there is nothing to wait for. These were removed because they are duplicated right above this (once the yield is removed).
Depends on: 1311446
Comment on attachment 8720652 [details]
Bug 1232357 - Delay hiding the sound indicator icon by a few seconds after audio has stopped.

https://reviewboard.mozilla.org/r/35409/#review32943

> Do we want the same thing on Android?  It's worth at least filing a bug about it.

I filed bug 1311446 for Android support.
Flags: needinfo?(jaws)
https://hg.mozilla.org/integration/fx-team/rev/8bd0d7e850dd1e62fa088d99821a2072a9273c09
Bug 1232357 - Delay hiding the sound indicator icon by a few seconds after audio has stopped. r=gijs
https://hg.mozilla.org/mozilla-central/rev/8bd0d7e850dd
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
Do we have any plan to add extra effects (eg. fade out) for sound indicator? (Chrome uses fading out) 
Since it looks little strange because disappearing very suddenly.
Flags: needinfo?(jaws)
(In reply to Alastor Wu [:alwu] from comment #39)
> Do we have any plan to add extra effects (eg. fade out) for sound indicator?
> (Chrome uses fading out) 
> Since it looks little strange because disappearing very suddenly.

No defined plans for other changes, but we could fade the icon out when it is in the "about-to-get-removed" stage. Can you file a bug for that?
Flags: needinfo?(jaws)
Depends on: 1311799
(In reply to Alastor Wu [:alwu] from comment #39)
> Do we have any plan to add extra effects (eg. fade out) for sound indicator?
> (Chrome uses fading out) 
> Since it looks little strange because disappearing very suddenly.

I filed bug 1311799 for this.
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #41)
> I filed bug 1311799 for this.

Thanks a lots :)
Depends on: 1345440
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: