615940 - Using Device Orientation events starts an immortal rapid timer

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Alon Zakai (:azakai)]

Visiting the test case attachment in bug 485943 will start device orientation events being fired. They will continue to fire at a fast rate (>10Hz I think) even after the page is browsed away from or even the tab closed.

Comment 1

[Alon Zakai (:azakai)]

Attachment: patch

Patch ties into the suspend/resume capabilities in nsGlobalWindow (for the bfcache), to suspend and resume accelerometer updates.

Comment 2

[Doug Turner (:dougt)]

would it be easier to to just add and remove the window listener instead of having to manage mAccelerationUpdatesEnabled?

Comment 3

[Alon Zakai (:azakai)]

Not sure what you mean - to use the existence of the window listener as an indicator of whether we currently have acceleration updates enabled? In other words, to expose the list of window listeners, and check if the current window is present there?

Comment 4

[Doug Turner (:dougt)]

add a hasListener() method to nsIAccelerometer, then use that to store the state you are keeping now.

Comment 5

[Alon Zakai (:azakai)]

Attachment: patch v2

Rewrote with those changes to the idl.

Will we still be able to have this for 4.0, with those interface changes?

Comment 6

[Alon Zakai (:azakai)]

Attachment: better patch

This is what I should have done in the last patch.

Comment 7

[Doug Turner (:dougt)]

Comment on attachment 496051 [details] [diff] [review]
better patch

I'd rather not have a macro.  please remove ACCELEROMETER_ACTION.

Can you set mHasAcceleration in DisableAccelerationUpdates / EnableAccelerationUpdates instead of outside of it?


+  // Enable updates for the accelerometer.
+  void EnableAccelerationUpdates();
+
+  // Disables updates for the accelerometer.
+  void DisableAccelerationUpdates();
+

private

I think we passed the point where we can make IDL changes on interfaces.

How is this:

can you make the implementation of AddWindowListener / RemoveWindowListener check the parameter before doing anything?  This would allow you to have the IDL consistency and get what you want.

Comment 8

[Alon Zakai (:azakai)]

Attachment: better patch v2

Update with requested changes, except for

> Can you set mHasAcceleration in
> DisableAccelerationUpdates /
> EnableAccelerationUpdates instead of
> outside of it?

Enable/Disable is called frequently, as the page goes into and out of the bfcache. mHasAcceleration is set once for the page, when it is marked as having a need for acceleration updates. If it doesn't, then Enable/Disable will do nothing. So mHasAcceleration has to be set outside of Enable/Disable.

Comment 9

[Doug Turner (:dougt)]

http://hg.mozilla.org/mozilla-central/rev/968bfd08e337

Comment 10

[Anna (Waverley)]

Can someone verify this please? Or give me some hints on how to verify it?

Comment 11

[Alon Zakai (:azakai)]

I don't think there is a reasonable way to test this manually.