703601 - Flash content continues to play regardless of set_screen_state 0

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect, P2)

Description

[Aaron Train [:aaronmt]]

Phone was idle on my desk with the display off, yet heard a sound-clip faintly playing. Awoke my phone and saw that the CNN Video page I was testing earlier was continuing to loop and play videos from their stream.

ER: Once my phone's display is off (i.e., set_screen_state 0) and phone is idle, pause or kill all Flash content (please do not drain device battery).

Resume at will once set_screen_state is set to 1, and when Fennec is in the foreground.

--
Samsung Nexus S (Android 2.3.6)
Flash 11.1
20111118040220
http://hg.mozilla.org/projects/birch/rev/9999a423d8ab

Comment 1

[Brad Lassey [:blassey] (use needinfo?)]

bumping to a P2 on the assumption that this is a life cycle event we need to send

Comment 2

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Fix a bunch of lifecycle issues with Flash on Android

Comment 3

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 597061 [details] [diff] [review]
Fix a bunch of lifecycle issues with Flash on Android

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

r- for mapping pause and resume onto onPause() and onResume()

::: mobile/android/base/GeckoApp.java
@@ +2731,5 @@
> +            status = false;
> +
> +        setScreenStatus(status);
> +    }
> +}

pretty much all of these changes to GeckoApp.java seem unnesesary if you just hook up to onPause() and onResume() for the application-foreground/background notifcations.

::: mobile/android/base/GeckoAppShell.java
@@ +1988,5 @@
>          return decodeBase64(s.getBytes(), flags);
>      }
> +
> +    public static void notifyScreenChanged(boolean on) {
> +        sendEventToGecko(new GeckoEvent(on ? GeckoEvent.SCREEN_ON : GeckoEvent.SCREEN_OFF));

this is bit-rotted now, the new pattern is:
GeckoEvent.createScreenStateEvent(on);

::: mobile/android/base/GeckoEvent.java
@@ +87,5 @@
>      public static final int VISITED = 21;
>      public static final int NETWORK_CHANGED = 22;
>      public static final int PROXIMITY_EVENT = 23;
> +    public static final int SCREEN_OFF = 24;
> +    public static final int SCREEN_ON = 25;

please just have SCREEN_STATE and use the flags on the event to indicate on or off

::: widget/android/nsAppShell.cpp
@@ +485,5 @@
>  
> +    case AndroidGeckoEvent::SCREEN_OFF: {
> +        nsCOMPtr<nsIObserverService> obsServ =
> +            mozilla::services::GetObserverService();
> +        obsServ->NotifyObservers(nsnull, "application-background", nsnull);

pretty sure you want to be sending this from here:
https://mxr.mozilla.org/mozilla-central/source/widget/android/nsAppShell.cpp#397

@@ +492,5 @@
> +
> +    case AndroidGeckoEvent::SCREEN_ON: {
> +        nsCOMPtr<nsIObserverService> obsServ =
> +            mozilla::services::GetObserverService();
> +        obsServ->NotifyObservers(nsnull, "application-foreground", nsnull);

and this from here (sorta):
https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/GeckoApp.java#2076

You'll need to send an ACTIVITY_RESUMING event to gecko so its on the right thread

Comment 4

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Brad Lassey [:blassey] from comment #3)
> Comment on attachment 597061 [details] [diff] [review]
> Fix a bunch of lifecycle issues with Flash on Android
> 
> Review of attachment 597061 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r- for mapping pause and resume onto onPause() and onResume()
> 
> ::: mobile/android/base/GeckoApp.java
> @@ +2731,5 @@
> > +            status = false;
> > +
> > +        setScreenStatus(status);
> > +    }
> > +}
> 
> pretty much all of these changes to GeckoApp.java seem unnesesary if you
> just hook up to onPause() and onResume() for the
> application-foreground/background notifcations.

That's what I tried at first, too, but it's problematic. When you open the tab tray, for instance, it pauses the main activity then resumes when you close it.

> 
> ::: mobile/android/base/GeckoAppShell.java
> @@ +1988,5 @@
> >          return decodeBase64(s.getBytes(), flags);
> >      }
> > +
> > +    public static void notifyScreenChanged(boolean on) {
> > +        sendEventToGecko(new GeckoEvent(on ? GeckoEvent.SCREEN_ON : GeckoEvent.SCREEN_OFF));
> 
> this is bit-rotted now, the new pattern is:
> GeckoEvent.createScreenStateEvent(on);

Fixed

> 
> ::: mobile/android/base/GeckoEvent.java
> @@ +87,5 @@
> >      public static final int VISITED = 21;
> >      public static final int NETWORK_CHANGED = 22;
> >      public static final int PROXIMITY_EVENT = 23;
> > +    public static final int SCREEN_OFF = 24;
> > +    public static final int SCREEN_ON = 25;
> 
> please just have SCREEN_STATE and use the flags on the event to indicate on
> or off

Fixed

> 
> ::: widget/android/nsAppShell.cpp
> @@ +485,5 @@
> >  
> > +    case AndroidGeckoEvent::SCREEN_OFF: {
> > +        nsCOMPtr<nsIObserverService> obsServ =
> > +            mozilla::services::GetObserverService();
> > +        obsServ->NotifyObservers(nsnull, "application-background", nsnull);
> 
> pretty sure you want to be sending this from here:
> https://mxr.mozilla.org/mozilla-central/source/widget/android/nsAppShell.
> cpp#397
> 
> @@ +492,5 @@
> > +
> > +    case AndroidGeckoEvent::SCREEN_ON: {
> > +        nsCOMPtr<nsIObserverService> obsServ =
> > +            mozilla::services::GetObserverService();
> > +        obsServ->NotifyObservers(nsnull, "application-foreground", nsnull);
> 
> and this from here (sorta):
> https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/GeckoApp.
> java#2076
> 
> You'll need to send an ACTIVITY_RESUMING event to gecko so its on the right
> thread

Comment 5

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Fix a bunch of lifecycle issues with Flash on Android

Comment 6

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to James Willcox (:snorp) ([e-mail]) from comment #4)
> (In reply to Brad Lassey [:blassey] from comment #3)
> > Comment on attachment 597061 [details] [diff] [review]
> > Fix a bunch of lifecycle issues with Flash on Android
> > 
> > Review of attachment 597061 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > r- for mapping pause and resume onto onPause() and onResume()
> > 
> > ::: mobile/android/base/GeckoApp.java
> > @@ +2731,5 @@
> > > +            status = false;
> > > +
> > > +        setScreenStatus(status);
> > > +    }
> > > +}
> > 
> > pretty much all of these changes to GeckoApp.java seem unnesesary if you
> > just hook up to onPause() and onResume() for the
> > application-foreground/background notifcations.
> 
> That's what I tried at first, too, but it's problematic. When you open the
> tab tray, for instance, it pauses the main activity then resumes when you
> close it.

Isn't that what we want? If not that's something that can be solved with checking mOwnActivityDepth (or in this case including it in the PAUSING/RESUMING event).

http://mxr.mozilla.org/mozilla-central/ident?i=mOwnActivityDepth

Comment 7

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 597134 [details] [diff] [review]
Fix a bunch of lifecycle issues with Flash on Android

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

as discussed on IRC, this should map on onPause/onResult

Comment 8

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Fix a bunch of lifecycle issues with Flash on Android

Comment 9

[James Willcox (:snorp) ([e-mail]) (he/him)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/7c004a416b2a

Comment 10

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/7c004a416b2a

Comment 11

[Cristian Nicolae (:xti)]

Verified fixed on:

Firefox 13.0a1 (2012-03-06)
20120306031101
http://hg.mozilla.org/mozilla-central/rev/7d0d1108a14e

--
Device: HTC Desire
OS: Android 2.2