Closed
Bug 720794
Opened 12 years ago
Closed 12 years ago
Screen Orientation API: implement reading and event
Categories
(Core :: DOM: Device Interfaces, defect)
Core
DOM: Device Interfaces
Tracking
()
RESOLVED
FIXED
mozilla14
People
(Reporter: mounir, Assigned: mounir)
References
Details
(Keywords: dev-doc-complete)
Attachments
(2 files, 1 obsolete file)
12.70 KB,
patch
|
jst
:
review+
sicking
:
superreview+
mounir
:
checkin+
|
Details | Diff | Splinter Review |
15.43 KB,
patch
|
cjones
:
review+
mounir
:
checkin+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•12 years ago
|
||
Attachment #591216 -
Flags: review?(jones.chris.g)
Assignee | ||
Comment 2•12 years ago
|
||
Attachment #591217 -
Flags: superreview?(jonas)
Attachment #591217 -
Flags: review?(jst)
Updated•12 years ago
|
Attachment #591217 -
Flags: review?(jst) → review+
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 >+void >+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.
Attachment #591216 -
Flags: review?(jones.chris.g)
Assignee | ||
Comment 4•12 years ago
|
||
Should fix the two points you mentioned.
Attachment #591216 -
Attachment is obsolete: true
Attachment #594140 -
Flags: review?(jones.chris.g)
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.
Attachment #594140 -
Flags: review?(jones.chris.g) → review+
Comment on attachment 591217 [details] [diff] [review] Part 2 - DOM Review of attachment 591217 [details] [diff] [review]: ----------------------------------------------------------------- sr=me ::: 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 :(
Attachment #591217 -
Flags: superreview?(jonas) → superreview+
Assignee | ||
Updated•12 years ago
|
Attachment #594140 -
Flags: checkin+
Assignee | ||
Updated•12 years ago
|
Attachment #591217 -
Flags: checkin+
Assignee | ||
Updated•12 years ago
|
Flags: in-testsuite?
Target Milestone: --- → mozilla13
Comment 7•12 years ago
|
||
Backed out of inbound for build failures on all platforms: https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=4a763183482f https://hg.mozilla.org/integration/mozilla-inbound/rev/c4c7eaaeec91
Target Milestone: mozilla13 → ---
Comment 8•12 years ago
|
||
Relanded, but the push had to be backed out again for bustage: https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=1ca8d5a931ac https://hg.mozilla.org/integration/mozilla-inbound/rev/33ffd55f2dfe Please can this/the rest of the push be run on try before landing a third time :-)
Updated•12 years ago
|
Keywords: dev-doc-needed
Comment 9•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/330b210f503d https://hg.mozilla.org/mozilla-central/rev/2421d39e0ab3
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla14
Comment 10•12 years ago
|
||
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: https://hg.mozilla.org/mozilla-central/rev/e94edfdb1f5b 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 : http://mzl.la/zD3EWy
Updated•12 years ago
|
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 11•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/cf2f1e899ee8 https://hg.mozilla.org/mozilla-central/rev/de61f2eaaea4
Status: REOPENED → RESOLVED
Closed: 12 years ago → 12 years ago
Resolution: --- → FIXED
Comment 12•11 years ago
|
||
This has been documented, some time ago ;-)
Keywords: dev-doc-needed → dev-doc-complete
You need to log in
before you can comment on or make changes to this bug.
Description
•