Closed Bug 971012 Opened 10 years ago Closed 10 years ago

Use onConfigurationChanged for orientation changes

Categories

(Firefox for Android Graveyard :: General, defect)

ARM
Android
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 30

People

(Reporter: snorp, Assigned: esawin)

Details

Attachments

(1 file, 6 obsolete files)

Right now we use GeckoScreenOrientationListener to deliver orientation change events to Gecko. Bug 951959 indicates that there are several problems (both performance and correctness) with this. The Right Way is to use an onConfigurationChanged listener. http://developer.android.com/reference/android/app/Activity.html#onConfigurationChanged%28android.content.res.Configuration%29
Assignee: nobody → esawin
Finished:
* handle orientation change events in onConfigurationChange
* move functionality into separate class
* fix orientation detection for landscape devices
* fix primary and secondary mode classification (primary is counter-clockwise rotated)

Working on:
* redirect all calls to GeckoScreenOrientation (GeckoAppShell)
* enforce consistent orientation type values (ActivityInfo.* vs. eScreenOrientation_*)
* comments and tests

Open questions:
* should we keep the custom orientation type values (eScreenOrientation_*), use ActivityInfo.SCREEN_ORIENTATION_* values instead or let the custom values match the ActivityInfo values?
Attachment #8377518 - Flags: feedback?(snorp)
Attachment #8377518 - Attachment is obsolete: true
Attachment #8377518 - Flags: feedback?(snorp)
Attachment #8377519 - Flags: feedback?(snorp)
Regarding the orientation type constants, it looks like the eScreenOrientation_* values are DOM constants, so we have to maintain a mapping between these values.

Add to working on:
* establish consistent mapping between eScreenOrientation_* and ActivityInfo.SCREEN_ORIENTATION_*
Comment on attachment 8377519 [details] [diff] [review]
Handle orientation events in onConfigurationChange (WIP)

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

Headed in the right direction, just a few minor issues.

::: mobile/android/base/GeckoScreenOrientation.java
@@ +19,5 @@
> +public class GeckoScreenOrientation {
> +    private static final String LOGTAG = "GeckoScreenOrientation";
> +
> +    // Make sure that any change in dom/base/ScreenOrientation.h happens here too.
> +    public static final int eScreenOrientation_None               = 0;

We should be able to use an enum for these now.

@@ +42,5 @@
> +    public GeckoScreenOrientation() {
> +        PrefsHelper.getPref(DEFAULT_SCREEN_ORIENTATION_PREF, new PrefsHelper.PrefHandlerBase() {
> +            @Override public void prefValue(String pref, String value) {
> +                mDefaultScreenOrientation = orientationFromStringArray(value);
> +                unlock();

Unlock without a prior lock is wrong. Add some helper to set the orientation if that's what you need to do.

@@ +62,5 @@
> +    public boolean update(int aOrientation) {
> +      return _update(getScreenOrientation(aOrientation, getRotation()));
> +    }
> +
> +    private boolean _update(int aScreenOrientation) {

No _ for private methods. The 'private' keyword is enough. Name it updateInternal or something instead?

@@ +75,5 @@
> +      return true;
> +    }
> +
> +    public int getOrientation() {
> +        if (mScreenOrientation > 2) {

What is 2? No magic values please.

@@ +78,5 @@
> +    public int getOrientation() {
> +        if (mScreenOrientation > 2) {
> +            return 2;
> +        }
> +        return 1;

Same

@@ +115,5 @@
> +        default:
> +            Log.e(LOGTAG, "Unexpected value received! (" + aScreenOrientation + ")");
> +            return;
> +        }
> +        if (GeckoAppShell.getContext() instanceof Activity) {

This is ugly and also returns the Application context, I believe, which is not what you want. Look at GeckoAppShell.GeckoInterface instead.

@@ +123,5 @@
> +    }
> +
> +    public void unlock() {
> +        Log.d(LOGTAG, "### GSO unlockscreen ");
> +        if (!(GeckoAppShell.getContext() instanceof Activity))

Same as above
Attachment #8377519 - Flags: feedback?(snorp) → feedback+
(In reply to Eugen Sawin (:esawin) <esawin@mozilla.com> from comment #1)
> Created attachment 8377518 [details] [diff] [review]
> Handle orientation events in onConfigurationChange (WIP)
> 
> Finished:
> * handle orientation change events in onConfigurationChange
> * move functionality into separate class
> * fix orientation detection for landscape devices
> * fix primary and secondary mode classification (primary is
> counter-clockwise rotated)
> 
> Working on:
> * redirect all calls to GeckoScreenOrientation (GeckoAppShell)
> * enforce consistent orientation type values (ActivityInfo.* vs.
> eScreenOrientation_*)
> * comments and tests
> 
> Open questions:
> * should we keep the custom orientation type values (eScreenOrientation_*),
> use ActivityInfo.SCREEN_ORIENTATION_* values instead or let the custom
> values match the ActivityInfo values?

I think we should continue using our own values here. That way they aren't tied to one platform if we use that code elsewhere in the platform (Metro or something).
Here is the current status.

Finished:
* handle orientation change events in onConfigurationChange
* fix orientation detection for landscape devices
* fix primary and secondary mode classification
* establish consistent mapping between eScreenOrientation_* and ActivityInfo.SCREEN_ORIENTATION_*
* redirect all calls to GeckoScreenOrientation (GeckoAppShell)

Working on:
* comments and testing

Issues:
* onConfigurationChange events are not fired for 180 degree flips, because the orientation does not change (landscape to landscape or portrait to portrait)
Attachment #8377519 - Attachment is obsolete: true
Patch for review.

Try results: https://tbpl.mozilla.org/?tree=Try&rev=be932218db73

Open issue:
onConfigurationChange is not invoked on 180 degree rotations, so from landscape to landscape and from portrait to portrait mode by flipping the device over or by rotating fast enough. onSizeChange and onLayoutChange (API level 11) are not invoked either, since the size/layout doesn't change.

Effect:
No Gecko events are produced for 180 degree rotations. Validated screen rotations for getScreenRotation() can still be activated by enableValidateOnGet().

Since 180 degree rotations don't change anything about the rendering, I don't think this is a big issue, but it should be noted.
Attachment #8378312 - Attachment is obsolete: true
Attachment #8379067 - Flags: review?(snorp)
Fixed patch title.
Attachment #8379067 - Attachment is obsolete: true
Attachment #8379067 - Flags: review?(snorp)
Attachment #8379072 - Flags: review?(snorp)
Comment on attachment 8379072 [details] [diff] [review]
Handle orientation events in onConfigurationChange

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

Looks pretty good

::: mobile/android/base/GeckoScreenOrientation.java
@@ +30,5 @@
> +        PORTRAIT_PRIMARY(1),
> +        PORTRAIT_SECONDARY(2),
> +        LANDSCAPE_PRIMARY(4),
> +        LANDSCAPE_SECONDARY(8),
> +        DEFAULT(16);

I like to see the bitshift method here (1 << 4) for assigning values.

@@ +105,5 @@
> +
> +    /*
> +     * Enable screen orientation update on retrieval.
> +     */
> +    public void enableValidateOnGet() {

Does this actually get used anywhere?
Attachment #8379072 - Flags: review?(snorp) → review+
Attachment #8379072 - Attachment is obsolete: true
Attachment #8379103 - Flags: review?(snorp)
Updated patch according to review feedback.

Patch for check-in.
Attachment #8379103 - Attachment is obsolete: true
Attachment #8379103 - Flags: review?(snorp)
Attachment #8379801 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/5ecc62d2cd7b
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 30
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: