Closed
Bug 1061372
Opened 10 years ago
Closed 10 years ago
Crash with any value passed to mozLockOrientation() but portrait-primary and portrait-secondary
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 36
People
(Reporter: mfinkle, Assigned: esawin)
References
Details
(Keywords: crash)
Attachments
(2 files, 4 obsolete files)
1.16 KB,
patch
|
blassey
:
review+
|
Details | Diff | Splinter Review |
7.95 KB,
patch
|
esawin
:
review+
|
Details | Diff | Splinter Review |
Filing from Twitter :)
https://twitter.com/AurelioDeRosa/status/506579100728057856
Test page here: http://aurelio.audero.it/demo/screen-orientation-api-demo.html
Updated•10 years ago
|
OS: Linux → Android
Hardware: x86_64 → All
Comment 1•10 years ago
|
||
09-01 19:16:11.909 D/GeckoScreenOrientation( 5721): locking to NONE
09-01 19:16:11.909 D/GeckoScreenOrientation( 5721): updating to new orientation NONE
09-01 19:16:12.029 D/MediaRouterService( 751): Client org.mozilla.fennec_rnewman (pid 5721): Died!
OOPS
Reporter | ||
Comment 2•10 years ago
|
||
Maybe I'm crazy for asking such a question, but why on earth do we even have a NONE condition?
Reporter | ||
Comment 3•10 years ago
|
||
When first added, we did not actually try to change the orientation to NONE:
http://hg.mozilla.org/mozilla-central/rev/8bf3120ed0f8
But refactorings at some point have introduced the problem.
Comment 4•10 years ago
|
||
NONE wasn't present in the original Bug 720795. It was introduced in Bug 740190. Bug 971012 moved this stuff out of GeckoAppShell.
Perhaps Mounir or Doug can remember why we have a NONE in the first place?
Reporter | ||
Comment 5•10 years ago
|
||
(In reply to Mark Finkle (:mfinkle) from comment #3)
> When first added, we did not actually try to change the orientation to NONE:
> http://hg.mozilla.org/mozilla-central/rev/8bf3120ed0f8
Actually, we did set the orientation when using NONE in that patch:
>+ case eScreenOrientation_None:
>+ orientation = ActivityInfo.SCREEN_ORIENTATION_SENSOR;
>+ break;
But now we do:
> case DEFAULT:
> case NONE:
> return ActivityInfo.SCREEN_ORIENTATION_UNSPECIFIED;
> default:
> return ActivityInfo.SCREEN_ORIENTATION_NOSENSOR;
Reporter | ||
Comment 6•10 years ago
|
||
bug 757791 and bug 766904 tweak it into our current situation
Assignee | ||
Comment 7•10 years ago
|
||
Looks like there are multiple things going wrong here.
There are 4 different orientation representations, the DOM strings (e.g. "portrait-primary", "landscape-secondary"), Gecko screen orientations (eScreenOrientation_*) and two Android types (ActivityInfo.SCREEN_ORIENTATION_* and Configuration.ORIENTATION_*).
It happens that except for the DOM strings, all of the representations are integer constants, so type-safe interfaces are not an option.
This lead to a type mismatch when interfacing with GeckoScreenOrientation.lock. GeckoScreenOrientation has to speak all of it, since it processes requests with Gecko and Android types.
Additionally, we don't seem to support orientation lock *sets*, so we should probably fallback to the primary options for "portrait" and "landscape" respectively.
Assignee | ||
Comment 8•10 years ago
|
||
We never delete (we do remove it, but keep the reference) the listener for the "mozfullscreenchange" events, so this patch fixes it.
Assignee | ||
Comment 9•10 years ago
|
||
GeckoScreenOrientation.lock expects an Android screen orientation type, but gets called with the Gecko-internal type, this should fix it.
Assignee | ||
Comment 10•10 years ago
|
||
The crashing on lock is caused by the type mismatch, so these patches will fix bug 1061373, too.
Updated•10 years ago
|
Assignee | ||
Updated•10 years ago
|
Attachment #8501787 -
Attachment description: (WIP) orientation-unlock-crash.patch → orientation-unlock-crash.patch
Attachment #8501787 -
Flags: review?(mounir)
Assignee | ||
Comment 11•10 years ago
|
||
Attachment #8501789 -
Attachment is obsolete: true
Attachment #8501810 -
Flags: review?(snorp)
Assignee | ||
Comment 12•10 years ago
|
||
Fixed comments.
Attachment #8501810 -
Attachment is obsolete: true
Attachment #8501810 -
Flags: review?(snorp)
Attachment #8501813 -
Flags: review?(snorp)
Comment 13•10 years ago
|
||
Comment on attachment 8501813 [details] [diff] [review]
orientation-lock-mapping.patch
Review of attachment 8501813 [details] [diff] [review]:
-----------------------------------------------------------------
Looks mostly alright, but that one switch block needs fixed up.
::: mobile/android/base/GeckoScreenOrientation.java
@@ +44,4 @@
> switch (value) {
> case (1 << 0): return PORTRAIT_PRIMARY;
> case (1 << 1): return PORTRAIT_SECONDARY;
> + case ((1 << 0) | (1 << 1)): return PORTRAIT;
There has to be a better way to do this. Can't we just use ScreenOrientation.PORTRAIT? What is the point of having the enum if we just use the raw values?
Attachment #8501813 -
Flags: review?(snorp) → review-
Assignee | ||
Comment 14•10 years ago
|
||
Replaced the switch statement.
Attachment #8501813 -
Attachment is obsolete: true
Attachment #8504113 -
Flags: review?(snorp)
Comment 15•10 years ago
|
||
Comment on attachment 8504113 [details] [diff] [review]
orientation-lock-mapping.patch
Review of attachment 8504113 [details] [diff] [review]:
-----------------------------------------------------------------
::: mobile/android/base/GeckoScreenOrientation.java
@@ +45,5 @@
> + public static ScreenOrientation get(int value) {
> + for (ScreenOrientation orient: ScreenOrientation.values) {
> + if (orient.value == value) {
> + return orient;
> + }
That is a huge improvement, thanks.
Attachment #8504113 -
Flags: review?(snorp) → review+
Assignee | ||
Comment 16•10 years ago
|
||
Just added a static variable prefix.
Attachment #8504113 -
Attachment is obsolete: true
Attachment #8504129 -
Flags: review+
Assignee | ||
Updated•10 years ago
|
Attachment #8501787 -
Flags: review?(mounir) → review?(blassey.bugs)
Updated•10 years ago
|
Attachment #8501787 -
Flags: review?(blassey.bugs) → review+
Assignee | ||
Comment 17•10 years ago
|
||
Comment 18•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/4de85788d5a6
https://hg.mozilla.org/mozilla-central/rev/317187728774
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 36
Updated•4 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•