Closed Bug 1355242 Opened 7 years ago Closed 7 years ago

Mute tabs while closing them

Categories

(Firefox :: Tabbed Browser, defect, P1)

Product:
Firefox
Component:
Tabbed Browser
Type:
defect

Tracking

()

Status:
RESOLVED WORKSFORME
Milestone:
Firefox 55
Iteration:
55.7 - Jun 12
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: florian, Assigned: past)

References

(Blocks 1 open bug)

Details

(Whiteboard: [reserve-photon-performance])

Attachments

(1 file)

Mute audio immediately on tab closure (bug 1355242).
59 bytes, text/x-review-board-request
florian
: review+
Details 
I just closed a tab that was playing a video, and the sound stopped one or two seconds after the tab had disappeared from the tab strip. (I suspect the sound stopped at the next GC). This felt really slow, and I think we could easily hide it by muting the tab while closing it if we notice it's one causing sound.
Flags: qe-verify?
Priority: -- → P2
Whiteboard: [photon-performance]
No longer blocks: photon-performance-triage
Is this what you had in mind? And can you test it on the reference device to see if it makes a difference? The change seems imperceptible in my fast laptop.
Assignee: nobody → past
Status: NEW → ASSIGNED
Comment on attachment 8866946 [details]
Mute audio immediately on tab closure (bug 1355242).

https://reviewboard.mozilla.org/r/138556/#review141836

::: browser/base/content/tabbrowser.xml:7471
(Diff revision 1)
>        <method name="_tabOnTabClose">
>          <parameter name="aEvent"/>
>          <body><![CDATA[
>            var tab = aEvent.target;
> +          // Mute audio immediately to improve perceived speed of tab closure.
> +          tab.linkedBrowser.mute();

This _tabOnTabClose method is inside the tabbrowser-alltabs-popup binding, I don't think we want to do this here.

I would suggest doing this near http://searchfox.org/mozilla-central/rev/cd8c561106d804e26bc09389f18f361846d005eb/browser/base/content/tabbrowser.xml#2747

Apparently calling browser.mute sends an async message to the content process: http://searchfox.org/mozilla-central/rev/cd8c561106d804e26bc09389f18f361846d005eb/toolkit/content/widgets/browser.xml#763
I'm afraid that means that if the content process is unresponsive, sound will keep playing until the content process becomes responsive (and the content process being unresponsive may even be the reason for the bug in the first place :-().

I wonder if there's a risk of .linkedBrowser to force a lazy browser to be loaded. Adding a tab.hasAttribute("soundplaying") check would avoid that.
Attachment #8866946 - Flags: review?(florian) → review-
Comment on attachment 8866946 [details]
Mute audio immediately on tab closure (bug 1355242).

https://reviewboard.mozilla.org/r/138556/#review141838

::: browser/base/content/tabbrowser.xml:7471
(Diff revision 1)
>        <method name="_tabOnTabClose">
>          <parameter name="aEvent"/>
>          <body><![CDATA[
>            var tab = aEvent.target;
> +          // Mute audio immediately to improve perceived speed of tab closure.
> +          tab.linkedBrowser.mute();

This will only mute the tab when clicking the close button but not for Accel+W, for instance. Should probably do this in removeTab or _beginRemoveTab and maybe only bother if we're animating.
(In reply to Dão Gottwald [::dao] from comment #5)
> This will only mute the tab when clicking the close button but not for
> Accel+W, for instance.

Ignore this part, I didn't look carefully enough what _tabOnTabClose is. Florian is right.
Priority: P2 → P1
Flags: qe-verify? → qe-verify+
QA Contact: adrian.florinescu
Comment on attachment 8866946 [details]
Mute audio immediately on tab closure (bug 1355242).

https://reviewboard.mozilla.org/r/138556/#review145104

Sorry for the delay here :-(.

::: browser/base/content/tabbrowser.xml:2757
(Diff revision 2)
> +            // Mute audio immediately to improve perceived speed of tab closure.
> +            if (aTab.hasAttribute("soundplaying")) {
> +              aTab.linkedBrowser.mute();
> +              // Don't persist the muted state as this wasn't a user action.
> +              // This lets undo-close-tab return it to an unmuted state.
> +              aTab.linkedBrowser._audioMuted = false;

This makes me a bit nervous, as I'm afraid someone could rename the _audioMuted field during a browser.xml refactoring and we would never notice this broke (there would not even be a JS strict warning, as assigning a value to a non-existent property is fine).

I think we should either get this covered by a test (but that may be more work than it's worth), or implement this in a way that's less likely to get broken. Eg. I think adding an optional parameter to browser.xml's 'mute' method to skip the "this._audioMuted = true;" line would be safer.
Attachment #8866946 - Flags: review?(florian) → review-
Whiteboard: [photon-performance] → [reserve-photon-performance]
Comment on attachment 8866946 [details]
Mute audio immediately on tab closure (bug 1355242).

https://reviewboard.mozilla.org/r/138556/#review147562

Looks good to me, thanks!
Attachment #8866946 - Flags: review?(florian) → review+
Comment on attachment 8866946 [details]
Mute audio immediately on tab closure (bug 1355242).

https://reviewboard.mozilla.org/r/138556/#review147620

::: browser/base/content/tabbrowser.xml:2859
(Diff revision 3)
> +            // Mute audio immediately to improve perceived speed of tab closure.
> +            if (aTab.hasAttribute("soundplaying")) {
> +              // Don't persist the muted state as this wasn't a user action.
> +              // This lets undo-close-tab return it to an unmuted state.
> +              aTab.linkedBrowser.mute(true);
> +            }

nit: please add a blank line after this
Pushed by pastithas@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/06cf542e5724
Mute audio immediately on tab closure . r=florian
https://hg.mozilla.org/mozilla-central/rev/06cf542e5724
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Iteration: --- → 55.7 - Jun 12
Depends on: 1369248
Hello! 
I was not able to reproduce this issue on Firefox 55.0a1 (2017-04-10), under Windows 10x64. 
Do you have an example of a video that I can use in order to reproduce the issue? Or is the issue reproducible only on a certain OS? 
Any other information would be useful.
Flags: needinfo?(florian)
I don't have good steps to reproduce, but I assume this should be easier to reproduce if the tab being closed has a beforeunload event listener, and if the content process (or CPU more generally) is otherwise being very busy.

I saw this on Mac when I filed the bug, but I have no reason to believe this bug is specific to one platform.
Flags: needinfo?(florian)
Since I was not able to reproduce the issue on the affected build, or on Firefox 57.0, Firefox 58.0a1 (2017-11-12), I'm marking this issue as WORKSFORME. 
The issue was verified under Widows 10x64 and under macOS 10.13.
Flags: qe-verify+
Resolution: FIXED → WORKSFORME
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: