742216 - Move the code to enable/disable Android services to onApplicationPause/onApplicationResume

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

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

We have code in GeckoApp.onPause / onResume to disable/enable some Android services, like network monitoring and screen orientation. These will fire too frequently now that onApplicationXxx methods are used to handle "application" background/foreground control.

We should move the code.

Comment 1

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch (1/3): Disable/Enable sensors in onApplicationPause()/Resume()

This patch moves the code to onApplicationPause() and Resume()

Comment 2

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch (2/3): Remove onPause()

Because of first patch, onPause() became redundant. This can be removed now.

Comment 3

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch (3/3): Battery sensor cleanup.

Some cleanup on Battery sensor in the similar lines of GeckoConnectivityReceiver.

Comment 4

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

Comment on attachment 612345 [details] [diff] [review]
Patch (1/3): Disable/Enable sensors in onApplicationPause()/Resume()

>+        mConnectivityReceiver = new GeckoConnectivityReceiver();
>+        mConnectivityReceiver.registerFor(mAppContext);
>+
>         GeckoNetworkManager.getInstance().init();
>+        GeckoNetworkManager.getInstance().start();
>+
>+        GeckoScreenOrientationListener.getInstance().start();

What affect does this have on startup time?

>-        mMainHandler.post(new Runnable() {
>-            public void run() {
>-                mConnectivityReceiver.registerFor(mAppContext);
>-                GeckoNetworkManager.getInstance().start();
>-                GeckoScreenOrientationListener.getInstance().start();
>-            }
>-        });

This was put in a runnable to try to avoid any startup time costs. If you can show that startup is not affected (using Try server and the logging times) we could think about avoiding the runnable.

r- until we are sure about the startup time data

Comment 5

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

Comment on attachment 612349 [details] [diff] [review]
Patch (3/3): Battery sensor cleanup.

This code my exist in embedding/android too, but I don't think we need to move the cleanup there too.

Comment 6

[Sriram Ramasubramanian [:sriram]]

> 
> What affect does this have on startup time?
> 
> >-        mMainHandler.post(new Runnable() {
> >-            public void run() {
> >-                mConnectivityReceiver.registerFor(mAppContext);
> >-                GeckoNetworkManager.getInstance().start();
> >-                GeckoScreenOrientationListener.getInstance().start();
> >-            }
> >-        });
> 
> This was put in a runnable to try to avoid any startup time costs. If you
> can show that startup is not affected (using Try server and the logging
> times) we could think about avoiding the runnable.
> 
> r- until we are sure about the startup time data

This particular piece of code was in onResume(), ideally executing "after" intialize() is done. I've moved this "into" intialize() to the end of it. Also, initialize() is run on UI thread, hence a Runnable is not needed there. I don't feel there would be any regression on startup time.

Comment 7

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

Comment on attachment 612345 [details] [diff] [review]
Patch (1/3): Disable/Enable sensors in onApplicationPause()/Resume()

r+, thanks for the bit of detail. Also, you need to keep an eye on Ts when this lands on inbound to make sure nothig was regressed. You could push the Try first and check it there too.

Comment 8

[Sriram Ramasubramanian [:sriram]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/ee64f3c826df
http://hg.mozilla.org/integration/mozilla-inbound/rev/fccc5ee8c51b
http://hg.mozilla.org/integration/mozilla-inbound/rev/7580c5db101f

Comment 9

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/ee64f3c826df
http://hg.mozilla.org/mozilla-central/rev/fccc5ee8c51b
http://hg.mozilla.org/mozilla-central/rev/7580c5db101f