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)

All
Android
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 10

People

(Reporter: fabrice, Assigned: fabrice)

References

Details

Attachments

(2 files, 2 obsolete files)

Attached file HTML test page
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
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
Attached patch wip (obsolete) — Splinter Review
Seems to work, but I didn't test much.
Attachment #550929 - Flags: feedback?(doug.turner)
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.
Attachment #550929 - Flags: feedback?(doug.turner) → feedback+
> 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.
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?
Depends on: 676316
Attached patch cleaned up patch (obsolete) — Splinter Review
Addressed comments.
Tested with chrome and content orientation listeners.
Assignee: nobody → fabrice
Attachment #550929 - Attachment is obsolete: true
Attachment #551093 - Flags: review?(doug.turner)
Attachment #551093 - Flags: review?(doug.turner) → review+
Can't we use the existing DisableDeviceMotionUpdates instead of duplicating its contents?
Comment on attachment 551093 [details] [diff] [review]
cleaned up patch

what jdm said.
Attachment #551093 - Flags: review+ → review-
fabrice - i see what you need to do.  make RemoveOrientationEventListener() directly call DisableDeviceMotionUpdates();

Do that, and I think we are good here!
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)
Attachment #564697 - Flags: review?(doug.turner) → review+
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.