Last Comment Bug 757758 - Honor hardware screen rotation
: Honor hardware screen rotation
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Widget (show other bugs)
: unspecified
: ARM Gonk (Firefox OS)
: -- normal (vote)
: mozilla15
Assigned To: Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-05-23 00:24 PDT by Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
Modified: 2012-05-24 09:27 PDT (History)
2 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Honor hardware screen rotation (3.75 KB, patch)
2012-05-23 00:25 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
mwu.code: review+
Details | Diff | Splinter Review
Honor hardware screen rotation, v2 (3.46 KB, patch)
2012-05-23 12:36 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
mwu.code: review+
Details | Diff | Splinter Review

Description Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-05-23 00:24:17 PDT
Some devices have a default rotation that we need to respect.  Patch coming up.
Comment 1 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-05-23 00:25:40 PDT
Created attachment 626352 [details] [diff] [review]
Honor hardware screen rotation
Comment 2 Michael Wu [:mwu] 2012-05-23 00:45:14 PDT
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.
Comment 3 Michael Vines [:m1] [:evilmachines] 2012-05-23 10:10:05 PDT
(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 Michael Wu [:mwu] 2012-05-23 10:26:36 PDT
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.
Comment 5 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-05-23 12:36:19 PDT
Created attachment 626552 [details] [diff] [review]
Honor hardware screen rotation, v2

Requesting re-review to ensure nits were picked.
Comment 6 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-05-23 17:42:37 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/b9179adbbdb1
Comment 7 Ed Morley [:emorley] 2012-05-24 09:27:12 PDT
https://hg.mozilla.org/mozilla-central/rev/b9179adbbdb1

Note You need to log in before you can comment on or make changes to this bug.