1253278 - Don't zombify tabs that are playing audio

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Audio/Video, defect)

Description

[Paul Adenot (:padenot)]

It's super annoying to have the sound suddenly stopped because the tab in which you were listening to music with gets zombified.

Comment 1

[Paul Adenot (:padenot)]

Attachment: Don't zombify tabs that are playing audio

Margaret, I haven't tested this yet, I'm not sure what triggers zombification
and all, but does that look reasonnable ?

Also, should I and how would I write a test for this ? I'm unfamiliar with how
front-end testing works in Fennec.

Comment 2

[:Margaret Leibovic]

Comment on attachment 8726253 [details] [diff] [review]
Don't zombify tabs that are playing audio

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

Thanks for the patch! We don't have great test coverage on Fennec, but if you want to take a stab at it, I don't think it would be that hard to make a new chrome mochitest for this:
https://wiki.mozilla.org/Mobile/Fennec/Android/Testing#mochitest_.28plain_and_chrome.29

Our current tests live in here:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/tests/browser/chrome/

We do already have some tests that include media:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/tests/browser/chrome/test_video_discovery.html?force=1

You could check to see if these have audio, and if they do make a new test that loads one in a new tab and plays it. You could test calling expireLruTab directly, although it looks like you would need to do some set-up to make sure that it doesn't return early.

Given the current testing situation, a test isn't strictly required for this fix, but it would be nice :)

::: mobile/android/chrome/content/browser.js
@@ +3511,5 @@
>        };
>        Messaging.sendRequest(message);
>  
>        this.overscrollController = new OverscrollController(this);
> +      this.playingAudio = false;

I think it would be more appropriate to declare this in the Tab constructor:
http://hg.mozilla.org/mozilla-central/annotate/bdde3fedb45b/mobile/android/chrome/content/browser.js#l3307

@@ +4178,4 @@
>          Messaging.sendRequest({
>            type: "Tab:AudioPlayingChange",
>            tabID: this.id,
> +          isAudioPlaying: this.playingAudio

This message is just used to update the state in Java, you don't need to include this unless you're using this data for the native Android UI.

@@ +7311,5 @@
>      // Find the least recently used non-zombie tab.
>      for (let i = 0; i < tabs.length; i++) {
> +      if (tabs[i] == selected ||
> +          tabs[i].browser.__SS_restore ||
> +          tabs[i].playingAudio) {

This seems reasonable to me, but kats should also take a look.

Comment 3

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8726253 [details] [diff] [review]
Don't zombify tabs that are playing audio

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

This looks good to me!

Comment 4

[Paul Adenot (:padenot)]

(In reply to :Margaret Leibovic from comment #2)
> @@ +4178,4 @@
> >          Messaging.sendRequest({
> >            type: "Tab:AudioPlayingChange",
> >            tabID: this.id,
> > +          isAudioPlaying: this.playingAudio
> 
> This message is just used to update the state in Java, you don't need to
> include this unless you're using this data for the native Android UI.

This is already present, I'm just moving the actual computation of the state to factor it with setting the new attribute I'm using.

Comment 5

[Paul Adenot (:padenot)]

Attachment: Don't zombify tabs that are playing audio

Comments addressed, I've looked into adding a test, but I'm really not familiar
with all that so I'll pass.

Comment 6

[:Margaret Leibovic]

(In reply to Paul Adenot (:padenot) from comment #4)
> (In reply to :Margaret Leibovic from comment #2)
> > @@ +4178,4 @@
> > >          Messaging.sendRequest({
> > >            type: "Tab:AudioPlayingChange",
> > >            tabID: this.id,
> > > +          isAudioPlaying: this.playingAudio
> > 
> > This message is just used to update the state in Java, you don't need to
> > include this unless you're using this data for the native Android UI.
> 
> This is already present, I'm just moving the actual computation of the state
> to factor it with setting the new attribute I'm using.

Oops, must have looked at that too quickly!

Comment 7

[Paul Adenot (:padenot)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f637f149610bb15fdbbda534417d6743877738f5
Bug 1253278 - Don't zombify tabs that are playing audio. r=margaret

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/f637f149610b