Honor hardware screen rotation

RESOLVED FIXED in mozilla15

Status

()

Core
Widget
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: cjones, Assigned: cjones)

Tracking

unspecified
mozilla15
ARM
Gonk (Firefox OS)
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Some devices have a default rotation that we need to respect.  Patch coming up.
Created attachment 626352 [details] [diff] [review]
Honor hardware screen rotation
Attachment #626352 - Flags: review?(mwu)

Comment 2

5 years ago
Comment on attachment 626352 [details] [diff] [review]
Honor hardware screen rotation

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

::: widget/gonk/nsWindow.cpp
@@ +572,5 @@
>  nsScreenGonk::nsScreenGonk(void *nativeScreen)
>  {
> +    char propValue[PROPERTY_VALUE_MAX];
> +    property_get("ro.sf.hwrotation", propValue, "0");
> +    sPhysicalScreenRotation = atoi(propValue) / 90;

If possible, do this in the constructor for nsWindow. That would seem less magical to me than relying on nsScreenGonk getting created before the first nsWindow.

@@ +638,5 @@
>          return NS_OK;
>  
>      sScreenRotation = aRotation;
>      sRotationMatrix.Reset();
> +    switch ((aRotation + sPhysicalScreenRotation) % (nsIScreen::ROTATION_270_DEG + 1)) {

nsIScreen::ROTATION_270_DEG + 1 is a little bizarre looking.. oh well.
Attachment #626352 - Flags: review?(mwu) → review+
(In reply to Michael Wu [:mwu] from comment #2)
> @@ +638,5 @@
> >          return NS_OK;
> >  
> >      sScreenRotation = aRotation;
> >      sRotationMatrix.Reset();
> > +    switch ((aRotation + sPhysicalScreenRotation) % (nsIScreen::ROTATION_270_DEG + 1)) {
> 
> nsIScreen::ROTATION_270_DEG + 1 is a little bizarre looking.. oh well.

Yeah.   A couple other options:
1)  "4":  IMO more difficult to grok at first glance.  ie, WTF is a 4?
2) "360 / 90": still 4 but makes it clear how that magic number was derived and matches the "atoi(propValue) / 90" elsewhere in the file, so a search for "90" will pick up both lines.
3) Add a new nsIScreen constant for the "nsIScreen::ROTATION_270_DEG + 1".  Seems like overkill.

I'm kindof liking #2 right now.

Comment 4

5 years ago
Sure #2 sounds good. If there's more people who want to use this constant, #3 might be better but #2 sounds fine for now.
Created attachment 626552 [details] [diff] [review]
Honor hardware screen rotation, v2

Requesting re-review to ensure nits were picked.
Attachment #626352 - Attachment is obsolete: true
Attachment #626552 - Flags: review?(mwu)

Updated

5 years ago
Attachment #626552 - Flags: review?(mwu) → review+
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/b9179adbbdb1
Keywords: checkin-needed
Flags: in-testsuite-
Target Milestone: --- → mozilla15
https://hg.mozilla.org/mozilla-central/rev/b9179adbbdb1
Assignee: nobody → jones.chris.g
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.