Last Comment Bug 738859 - Increment mActivityDepth for all activities launched from GeckoApp
: Increment mActivityDepth for all activities launched from GeckoApp
Status: RESOLVED FIXED
: perf
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: unspecified
: ARM Android
: -- normal (vote)
: Firefox 14
Assigned To: Brian Nicholson (:bnicholson)
:
: Sebastian Kaspari (:sebastian)
Mentors:
Depends on:
Blocks: 739578
  Show dependency treegraph
 
Reported: 2012-03-23 17:01 PDT by Brian Nicholson (:bnicholson)
Modified: 2016-07-29 14:23 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
-


Attachments
patch (2.37 KB, patch)
2012-03-23 17:01 PDT, Brian Nicholson (:bnicholson)
no flags Details | Diff | Splinter Review
patch v2 (3.53 KB, patch)
2012-03-23 17:17 PDT, Brian Nicholson (:bnicholson)
blassey.bugs: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Brian Nicholson (:bnicholson) 2012-03-23 17:01:46 PDT
Created attachment 608919 [details] [diff] [review]
patch

In bug 706822, mActivityDepth is used to prevent saving state (and taking a screenshot) when showing the AwesomeScreen. I think want to do this for all activities we launch (GeckoPreferences, the tabs tray, the file picker, etc.) to improve performance.
Comment 1 Brian Nicholson (:bnicholson) 2012-03-23 17:17:49 PDT
Created attachment 608923 [details] [diff] [review]
patch v2

One problem with not saving the state in these cases is that the following can happen:

1) user opens subactivity (AwesomeScreen, preferences, or tabs tray)
2) user clicks home to put fennec in background
3) fennec is OOM killed

Since the state wasn't saved in step 1, the last title/screenshot/session boolean won't get saved. This patch always saves these to the bundle, but only runs the SessionSnapshotRunnable() if the depth is 0.
Comment 2 Brad Lassey [:blassey] (use needinfo?) 2012-03-26 18:11:11 PDT
Comment on attachment 608923 [details] [diff] [review]
patch v2

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

r=blassey for overriding startActivity and startActivityForResult, as well as making the depth count private.

::: mobile/android/base/GeckoApp.java
@@ +550,5 @@
> +
> +        if (mOwnActivityDepth > 0)
> +            return; // we're showing one of our own activities and likely won't get paged out
> +
> +        new SessionSnapshotRunnable(null).run();

this needs to be called before putting the mLastScreen in the bundle.

The changes to this method are wrong in general, drop them. There's an argument to be made that we should save what we can to the bundle even if the depth is more than 0, but let's have that argument in a different bug.
Comment 3 Brian Nicholson (:bnicholson) 2012-03-27 12:28:04 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/43b9f25a14b0
Comment 4 Brian Nicholson (:bnicholson) 2012-03-27 12:49:16 PDT
Filed bug 739742 and bug 739747.
Comment 5 Brian Nicholson (:bnicholson) 2012-03-28 11:28:38 PDT
Comment on attachment 608923 [details] [diff] [review]
patch v2

[Approval Request Comment]
Improves performance for Gecko subactivities.
Comment 6 Ed Morley [:emorley] 2012-03-28 14:20:26 PDT
https://hg.mozilla.org/mozilla-central/rev/43b9f25a14b0
Comment 7 Alex Keybl [:akeybl] 2012-03-28 14:44:22 PDT
Comment on attachment 608923 [details] [diff] [review]
patch v2

[Triage Comment]
Mobile only - approved for Aurora 13.
Comment 8 Brian Nicholson (:bnicholson) 2012-03-30 09:54:25 PDT
http://hg.mozilla.org/releases/mozilla-aurora/rev/0e479285c04b
Comment 9 Damon Sicore (:damons) 2012-04-02 12:18:08 PDT
Why are we fixing bugs that we have explicitly said will not block the release?
Comment 10 Mark Finkle (:mfinkle) (use needinfo?) 2012-04-02 12:40:54 PDT
(In reply to Damon Sicore (:damons) from comment #9)
> Why are we fixing bugs that we have explicitly said will not block the
> release?

Because at one point Brian ran out of blockers, so I "OK'ed" this bug since it has performance impact.

Note You need to log in before you can comment on or make changes to this bug.