Use onConfigurationChanged for orientation changes

RESOLVED FIXED in Firefox 30

Status

()

Firefox for Android
General
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: snorp, Assigned: esawin)

Tracking

Trunk
Firefox 30
ARM
Android
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 6 obsolete attachments)

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
(Assignee)

Comment 1

4 years ago
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?
Attachment #8377518 - Flags: feedback?(snorp)
(Assignee)

Comment 2

4 years ago
Created attachment 8377519 [details] [diff] [review]
Handle orientation events in onConfigurationChange (WIP)
Attachment #8377518 - Attachment is obsolete: true
Attachment #8377518 - Flags: feedback?(snorp)
Attachment #8377519 - Flags: feedback?(snorp)
(Assignee)

Comment 3

4 years ago
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).
(Assignee)

Comment 6

4 years ago
Created attachment 8378312 [details] [diff] [review]
Handle orientation events in onConfigurationChange (WIP)

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
(Assignee)

Comment 7

4 years ago
Created attachment 8379067 [details] [diff] [review]
Handle orientation events in onConfigurationChange

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)
(Assignee)

Comment 8

4 years ago
Created attachment 8379072 [details] [diff] [review]
Handle orientation events in onConfigurationChange

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+
(Assignee)

Comment 10

4 years ago
Created attachment 8379103 [details] [diff] [review]
Handle orientation events in onConfigurationChange
Attachment #8379072 - Attachment is obsolete: true
Attachment #8379103 - Flags: review?(snorp)
(Assignee)

Comment 11

4 years ago
Created attachment 8379801 [details] [diff] [review]
Handle orientation events in onConfigurationChange

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+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/5ecc62d2cd7b
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 30
You need to log in before you can comment on or make changes to this bug.