Closed Bug 739747 Opened 8 years ago Closed 8 years ago

Tab not put into background for subactivities

Categories

(Firefox for Android :: General, defect)

ARM
Android
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 14
Tracking Status
blocking-fennec1.0 --- +

People

(Reporter: bnicholson, Assigned: sriram)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
blocking-fennec1.0: --- → ?
Version: unspecified → Trunk
blocking-fennec1.0: ? → +
Assignee: nobody → sriram
Attached patch WIP (obsolete) — Splinter Review
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.
Attachment #610731 - Flags: review?(mark.finkle)
Depends on bug 721344, as the connectivity receiver crashes, when we try to unregister it twice during onPause() of GeckoApp and from TabsTray.
Depends on: 721344
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?
Attachment #610731 - Flags: review?(mark.finkle)
Attachment #610731 - Flags: review?(blassey.bugs)
Attachment #610731 - Flags: review+
Attached patch PatchSplinter Review
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.
Attachment #610731 - Attachment is obsolete: true
Attachment #610731 - Flags: review?(blassey.bugs)
Attachment #611552 - Flags: review?(mark.finkle)
Attachment #611552 - Flags: review?(blassey.bugs)
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
Attachment #611552 - Flags: review?(blassey.bugs) → review+
(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 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
Attachment #611552 - Flags: review?(mark.finkle) → review+
Blocks: 739729
https://hg.mozilla.org/mozilla-central/rev/5c941f58dd5e
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 14
You need to log in before you can comment on or make changes to this bug.