951959 - GeckoScreenOrientationListener is invoked hundreds of times during page load

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Richard Newman [:rnewman]]

org.mozilla.gecko.GeckoScreenOrientationListener.updateScreenOrientation(GeckoScreenOrientationListener.java:152)
org.mozilla.gecko.GeckoScreenOrientationListener.access$000(GeckoScreenOrientationListener.java:18)
org.mozilla.gecko.GeckoScreenOrientationListener$OrientationEventListenerImpl.onOrientationChanged(GeckoScreenOrientationListener.java:28)
android.view.OrientationEventListener$SensorEventListenerImpl.onSensorChanged(OrientationEventListener.java:143)
android.hardware.SystemSensorManager$SensorEventQueue.dispatchSensorEvent(SystemSensorManager.java:423)
android.os.MessageQueue.nativePollOnce(Native Method)
android.os.MessageQueue.next(MessageQueue.java:137)
android.os.Looper.loop(Looper.java:135)
android.app.ActivityThread.main(ActivityThread.java:5851)
java.lang.reflect.Method.invokeNative(Native Method)
java.lang.reflect.Method.invoke(Method.java:515)
com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:1067)
com.android.internal.os.ZygoteInit.main(ZygoteInit.java:883)
dalvik.system.NativeStart.main(Native Method)


This call path is invoked something like 90 times when a page loads. And then it keeps getting invoked. I opened Reddit and just sat there for a few seconds, and counted 272 calls, as close together as 50msec, and as far apart as 13 seconds.

Given that updateScreenOrientation does a context dance, a WindowManager dance, and sometimes sends an event to Gecko, we should probably investigate this further in case it's affecting page load times.

Comment 1

[Richard Newman [:rnewman]]

Apparently this is known.

"Basically, Screen.getRotation returns the same result for portrait and reverse portrait, checked in log, but the orientation returned by the listener is alright. Will this affect the performance as it's running all the time, getting called 10+ times every second?"

http://helpdesk.metaio.com/questions/17105/billboards-upside-down-in-reverse-portrait-configuration


We might want to specify a rate:

http://developer.android.com/reference/android/view/OrientationEventListener.html#OrientationEventListener%28android.content.Context,%20int%29

Comment 2

[Richard Newman [:rnewman]]

Secondly, perhaps we should just handle orientation in onConfigurationChanged, rather than watching for fine-grained rotations? Or do we need to provide that to the web layer?

Comment 3

[Richard Newman [:rnewman]]

Also, this class is wrong.

http://chrisrisner.com/31-Days-of-Android--Day-11%E2%80%93Device-Orientation
---
“You can safely assume that ROTATION_0 and ROTATION_180 are portrait and ROTATION_90 and ROTATION_270 are landscape.“…Not
on an HTC ChaCha (Blackberry form factor android with a normally landscape
screen).  On this device, ROTATION_0 is actually landscape.  Also
when you rotate the screen to be portrait, it says “Not Portrait, Landscape!” and “Rotation
90”.
---

Look what we do (after I've tidied up but otherwise not changed the code):

        if (rotation == Surface.ROTATION_0) {
            mOrientation = SCREEN_ORIENTATION_PORTRAIT_PRIMARY;
        } else if (rotation == Surface.ROTATION_180) {
            mOrientation = SCREEN_ORIENTATION_PORTRAIT_SECONDARY;
        } else if (rotation == Surface.ROTATION_270) {
            mOrientation = SCREEN_ORIENTATION_LANDSCAPE_SECONDARY;
        } else if (rotation == Surface.ROTATION_90) {
            mOrientation = SCREEN_ORIENTATION_LANDSCAPE_PRIMARY;
        } else {
            Log.e(LOGTAG, "Unexpected value received! (" + rotation + ")");
            return;
        }

Comment 4

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

(In reply to Richard Newman [:rnewman] from comment #1)

> We might want to specify a rate:
> 
> http://developer.android.com/reference/android/view/OrientationEventListener.
> html#OrientationEventListener%28android.content.Context,%20int%29
The default is the slowest rate (I'm assuming UI is faster than NORMAL)

Comment 5

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

Attachment: use_orientation.patch

Does this look better in terms of time in your profile?

Comment 6

[Richard Newman [:rnewman]]

Comment on attachment 8360221 [details] [diff] [review]
use_orientation.patch

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

I'll try to find time to test this out soon.

::: mobile/android/base/GeckoScreenOrientationListener.java
@@ +157,5 @@
> +        }
> +        updateScreenOrientation(rotation * 90);
> +    }
> +
> +    private void updateScreenOrientation(int aOrienation) {

Nit: aOrien*t*ation.

Comment 7

[Richard Newman [:rnewman]]

Interesting. On my Moto x86 phone, onOrientationChanged is only ever called (a) on first launch of the activity, (b) if the activity is resumed after a screen orientation event.

Will have to test on my HTC ARM device so I can repro.

Oh, Android.

Comment 8

[Richard Newman [:rnewman]]

Comment on attachment 8360221 [details] [diff] [review]
use_orientation.patch

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

This looks good to me; it drops the handling time for each occurrence from 1-5msec (avg 2-3) down to 0-1msec.

r+ with typo addressed.

Thanks for tackling this!

::: mobile/android/base/GeckoScreenOrientationListener.java
@@ +157,5 @@
> +        }
> +        updateScreenOrientation(rotation * 90);
> +    }
> +
> +    private void updateScreenOrientation(int aOrienation) {

Nit: aOrien*t*ation.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ac3d8111a806