Last Comment Bug 720794 - Screen Orientation API: implement reading and event
: Screen Orientation API: implement reading and event
: dev-doc-complete
Product: Core
Classification: Components
Component: DOM: Device Interfaces (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla14
Assigned To: Mounir Lamouri (:mounir)
: Andrew Overholt [:overholt]
Depends on: 720795 720799 725951 798857
Blocks: 673922
  Show dependency treegraph
Reported: 2012-01-24 12:23 PST by Mounir Lamouri (:mounir)
Modified: 2013-08-15 13:52 PDT (History)
9 users (show)
mounir: in‑testsuite?
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Part 1 - HAL (19.21 KB, patch)
2012-01-24 12:24 PST, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review
Part 2 - DOM (12.70 KB, patch)
2012-01-24 12:25 PST, Mounir Lamouri (:mounir)
jst: review+
jonas: superreview+
mounir: checkin+
Details | Diff | Splinter Review
Part 1 - HAL (15.43 KB, patch)
2012-02-03 05:23 PST, Mounir Lamouri (:mounir)
cjones.bugs: review+
mounir: checkin+
Details | Diff | Splinter Review

Description Mounir Lamouri (:mounir) 2012-01-24 12:23:34 PST
This bug is for screen orientation api implementation that doesn't involve locking the screen. This last part isn't finalized yet but reading the orientation value and getting informed of a change seems to had a global consensus in dev-webapi.
Comment 1 Mounir Lamouri (:mounir) 2012-01-24 12:24:37 PST
Created attachment 591216 [details] [diff] [review]
Part 1 - HAL
Comment 2 Mounir Lamouri (:mounir) 2012-01-24 12:25:03 PST
Created attachment 591217 [details] [diff] [review]
Part 2 - DOM
Comment 3 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-01-24 22:57:48 PST
Comment on attachment 591216 [details] [diff] [review]
Part 1 - HAL

Fyi, we're building up widget support for screen /rotation/ in bug
714416.  I think the constants involved for /orientation/ need to be
different, so having this extra interface seems right.  Hal seems fine
for it.

>diff --git a/hal/android/AndroidHal.cpp b/hal/android/AndroidHal.cpp

>+GetCurrentScreenOrientation(dom::ScreenOrientation* aScreenOrientation)
>+  *aScreenOrientation = dom::eScreenOrientation_LandscapePrimary;

This and the other no-op implementations aren't correct.  You need to
grab the primary nsIScreen and return landscape/portrait-primary
depending the screen width/height.  Content will be able to detect if
we tell it the wrong thing, so we need to get it right.

I would also strongly recommend factoring this out into a
hal/fallback/ScreenOrientation.cpp file and using it for all the
fallback impls, rather than maintaining one per backend.

Looks ok other than two issues above.
Comment 4 Mounir Lamouri (:mounir) 2012-02-03 05:23:45 PST
Created attachment 594140 [details] [diff] [review]
Part 1 - HAL

Should fix the two points you mentioned.
Comment 5 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-02-03 18:16:04 PST
Comment on attachment 594140 [details] [diff] [review]
Part 1 - HAL

This looks good.

It feels a little weird proxying just the screen orientation when we need the screen dimensions for existing DOM and CSS interfaces, and can derive the orientation from the dimensions, but we can certainly extend this when we need to.
Comment 6 Jonas Sicking (:sicking) No longer reading bugmail consistently 2012-02-08 19:09:00 PST
Comment on attachment 591217 [details] [diff] [review]
Part 2 - DOM

Review of attachment 591217 [details] [diff] [review]:


::: dom/base/nsScreen.cpp
@@ +373,5 @@
> +               mOrientation != eScreenOrientation_Landscape,
> +               "Invalid orientation value passed to notify method!");
> +
> +  if (mOrientation != previousOrientation) {
> +    // TODO: use an helper method, see bug 720768.

Yes, dispatching events needs to be way simpler :(
Comment 7 Ed Morley [:emorley] 2012-02-09 04:09:46 PST
Backed out of inbound for build failures on all platforms:
Comment 8 Ed Morley [:emorley] 2012-02-09 08:46:25 PST
Relanded, but the push had to be backed out again for bustage:

Please can this/the rest of the push be run on try before landing a third time :-)
Comment 10 Mark Finkle (:mfinkle) (use needinfo?) 2012-03-17 19:01:25 PDT
I backed this out along with all the other patches in the initial push. Something in the push regressed Ts on Android by 20% and I don't know which patch is to blame:

Regression Ts increase 20.6% on Android 2.2 Mozilla-Inbound
    Previous: avg 2653.656 stddev 87.856 of 30 runs up to revision 1239bd0779a6
    New     : avg 3201.178 stddev 110.202 of 5 runs since revision 0d61a0d8dba4
    Change  : +547.522 (20.6% / z=6.232)
    Graph   :
Comment 12 Jean-Yves Perrier [:teoli] 2013-08-15 13:52:34 PDT
This has been documented, some time ago ;-)

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