Closed
Bug 676595
Opened 13 years ago
Closed 13 years ago
Android orientation manager is never shut down
Categories
(Firefox for Android Graveyard :: Bookmarks, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 10
People
(Reporter: fabrice, Assigned: fabrice)
References
Details
Attachments
(2 files, 2 obsolete files)
687 bytes,
text/html
|
Details | |
2.21 KB,
patch
|
dougt
:
review+
|
Details | Diff | Splinter Review |
The Android device orientation is started when adding an event listener, but is never shutdown when we remove the listener. removeWindowListener90 is never called in nsDeviceMotion and we see events in logcat: I/GeckoEvent( 4838): SensorEvent type = 3 AK8975 Orientation sensor -126.0 43.0 -7.0
Assignee | ||
Comment 1•13 years ago
|
||
tracking what happens when we do removeEventListener(), we end up at http://mxr.mozilla.org/mozilla-central/source/content/events/src/nsEventListenerManager.cpp#491 If I understand correctly, there's no code here that links back to nsDeviceMotion to remove the listener
Assignee | ||
Comment 2•13 years ago
|
||
Seems to work, but I didn't test much.
Attachment #550929 -
Flags: feedback?(doug.turner)
Comment 3•13 years ago
|
||
Comment on attachment 550929 [details] [diff] [review] wip 1) bug 676316 covers the nsDeviceMotion::RemoveWindowListener error. No need to include it here. jdm will push very soon. 2) remove printfs 3) what is the magic aType == 4900 about? Otherwise good. Thanks for tracking down.
Updated•13 years ago
|
Attachment #550929 -
Flags: feedback?(doug.turner) → feedback+
Assignee | ||
Comment 4•13 years ago
|
||
> 1) bug 676316 covers the nsDeviceMotion::RemoveWindowListener error. No > need to include it here. jdm will push very soon. ok > 3) what is the magic aType == 4900 about? It's beacuse I had no time to find the constant used for the deviceorientation type.
Comment 5•13 years ago
|
||
also.. it is sorta odd that the nsEventListenerManager needs to know about this event specifically. isn't there some place in the nsGlobalWindow that we can move the |RemoveWindowListener| call?
Assignee | ||
Comment 6•13 years ago
|
||
Addressed comments. Tested with chrome and content orientation listeners.
Assignee: nobody → fabrice
Attachment #550929 -
Attachment is obsolete: true
Attachment #551093 -
Flags: review?(doug.turner)
Updated•13 years ago
|
Attachment #551093 -
Flags: review?(doug.turner) → review+
Comment 7•13 years ago
|
||
Can't we use the existing DisableDeviceMotionUpdates instead of duplicating its contents?
Comment 8•13 years ago
|
||
Comment on attachment 551093 [details] [diff] [review] cleaned up patch what jdm said.
Attachment #551093 -
Flags: review+ → review-
Comment 9•13 years ago
|
||
fabrice - i see what you need to do. make RemoveOrientationEventListener() directly call DisableDeviceMotionUpdates(); Do that, and I think we are good here!
Assignee | ||
Comment 10•13 years ago
|
||
Followed comment #9. I tested with content (see test page) and chrome (the camera capture picker) listeners, and that works well.
Attachment #551093 -
Attachment is obsolete: true
Attachment #564697 -
Flags: review?(doug.turner)
Updated•13 years ago
|
Attachment #564697 -
Flags: review?(doug.turner) → review+
Assignee | ||
Comment 11•13 years ago
|
||
pushed: https://hg.mozilla.org/integration/mozilla-inbound/rev/312306f72fa3
Comment 12•13 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/312306f72fa3
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 10
You need to log in
before you can comment on or make changes to this bug.
Description
•