Closed Bug 951959 Opened 7 years ago Closed 6 years ago

GeckoScreenOrientationListener is invoked hundreds of times during page load

Categories

(Firefox for Android :: General, defect)

29 Branch
All
Android
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 30

People

(Reporter: rnewman, Assigned: blassey)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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.
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
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?
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;
        }
(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)
Does this look better in terms of time in your profile?
Assignee: nobody → blassey.bugs
Attachment #8360221 - Flags: review?(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.
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 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.
Attachment #8360221 - Flags: review?(rnewman) → review+
https://hg.mozilla.org/mozilla-central/rev/ac3d8111a806
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 30
You need to log in before you can comment on or make changes to this bug.