739747 - Tab not put into background for subactivities

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Brian Nicholson (:bnicholson)]

When GeckoApp is paused/stopped, we use the activity depth to determine whether to fire application-background notifications. If the user opens a subactivity (like the AwesomeScreen or tabs tray), then pushes the home button, tabs will not be put into the background.

Comment 1

[Martijn Wargers (dead)]

Simple testcase for it here: http://people.mozilla.com/~mwargers/tests/dom/visibilitychange.htm

Comment 2

[Sriram Ramasubramanian [:sriram]]

Attachment: WIP

This patch uses onSubactivityPause() and onSubactivityResume() to track the activity pausing and resuming. mOwnActivityDepth was unnecessarily being used as an int. I've changed it to boolean so that tracking the subactivities is easy. This is just for TabsTray currently. If the approach sounds good, I can extend it to other activities as well.

I somehow feel, this should be tracked on Application level, and the logic in onPause() of GeckoApp should be handled based on when Application goes to background, comes to foreground. That will be best approach for this. Adding an onPause() and onResume() for every activity and making sure to inform GeckoApp sounds painful. Someone writing a new activity wouldn't know this.

Comment 3

[Sriram Ramasubramanian [:sriram]]

Depends on bug 721344, as the connectivity receiver crashes, when we try to unregister it twice during onPause() of GeckoApp and from TabsTray.

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 610731 [details] [diff] [review]
WIP

Looks OK to me. I still wonder if we could get into situations where a boolean wouldn't work for activity depth.

I want Brad to review this too.

I assume we want to add the onSubactivityXxx calls to Awesomebar and other activities too, right?

Comment 5

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch

This patch listens from all the activities for onPause() and onResume(), and informs the listeners if the application is sent to background or not. I have another version of patch with onActivityPaused(Activity whichActivity).. but, it looks too complicated to understand. So I just removed all unwanted parts and made the Application decide if it's in foreground or background.

I haven't subclassed Sync's activities to be from GeckoActivity. So, we would go to background there. Also, I am not pretty sure about messages in onStart() and onStop() states in GeckoApp. So, there might be some problems.

Other than that, this works for all other activities going to background and coming back.

Comment 6

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

Comment on attachment 611552 [details] [diff] [review]
Patch

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

Let's re-evaluate why we're tracking activity depth in the first place. We're not saving the screenshot anymore, so we don't need it for that. I think that the selected tab *should* be put in the background when the tabs tray or awesome bar comes up. It might be good to identify what we don't want to happen when its our own activity pausing us.

The one thing that comes to mind is writing out our preferences, which seems like a decent enough justification for this change. Please file a follow up to to audit what is and isn't done when our own activity pauses us.

::: mobile/android/base/GeckoActivity.java.in
@@ +17,5 @@
> +
> +    @Override
> +    public void onCreate(Bundle bundle) {
> +        super.onCreate(bundle);
> +    }

no need to override this

@@ +25,5 @@
> +        super.onPause();
> +
> +        // Avoid pause notifications in destroy path.
> +        if (!isFinishing())
> +            ((GeckoApplication) getApplication()).onActivityPause(this);

let's protect this with an instanceof check

@@ +34,5 @@
> +        super.onResume();
> +
> +        // Avoid resume notifications in startup path.
> +        if (hasStarted) {
> +            ((GeckoApplication) getApplication()).onActivityResume(this);

same instanceof check

Comment 7

[Sriram Ramasubramanian [:sriram]]

(In reply to Brad Lassey [:blassey] from comment #6)
> ::: mobile/android/base/GeckoActivity.java.in
> @@ +17,5 @@
> > +
> > +    @Override
> > +    public void onCreate(Bundle bundle) {
> > +        super.onCreate(bundle);
> > +    }
> 
> no need to override this

Ahh! I had some initialization here. But later removed them, but forgot to remove this. Will clear this.

> 
> @@ +25,5 @@
> > +        super.onPause();
> > +
> > +        // Avoid pause notifications in destroy path.
> > +        if (!isFinishing())
> > +            ((GeckoApplication) getApplication()).onActivityPause(this);
> 
> let's protect this with an instanceof check

Do you want getApplication() to have instanceof check?

Comment 8

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 611552 [details] [diff] [review]
Patch

>diff --git a/mobile/android/base/CrashReporter.java.in b/mobile/android/base/CrashReporter.java.in

>-public class CrashReporter extends Activity
>+public class CrashReporter extends GeckoActivity

Do we care about this activity? Should it be included in the App stack? I don't think we should include it.

>diff --git a/mobile/android/base/LauncherShortcuts.java.in b/mobile/android/base/LauncherShortcuts.java.in

>-public class LauncherShortcuts extends Activity {
>+public class LauncherShortcuts extends GeckoActivity {

Do we care about this activity? Should it be included in the App stack? It's the widget. I don't think we should include it.

r+ with the crash reporter and the launcher reverted

Comment 9

[Sriram Ramasubramanian [:sriram]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/5c941f58dd5e

Comment 10

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/5c941f58dd5e