Closed Bug 423395 Opened 12 years ago Closed 12 years ago

onEventSweepMouseMove handler stays around after drop

Categories

(Calendar :: Calendar Views, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: Fallen, Assigned: Fallen)

Details

(Keywords: perf)

Attachments

(1 file, 1 obsolete file)

Attached patch Fix - v1 (obsolete) — Splinter Review
Due to the wrong third argument of addEventListener (or removeEventListener), the mousemove and mouseup handlers stay around for each column the event has passed. This doubles the second time the event is dragged. It remained unnoticed, since the respective functions just return if there is no drag column.

This patch fixes the event handlers and asserts the drag column must exist for the event handlers, to better find errors in the future.

I'm unsure if the better useCapture parameter for add/removeEventListener is true of false. Maybe you know more about that, berend?
Attachment #309894 - Flags: review?(Berend.Cornelius)
Flags: wanted-calendar0.8+
Philipp, please don't set the +flag, that one is reserved for release drivers (Daniel, Clint, me).

Since we're in release lockdown now, only Daniel will be approving these requests. See http://groups.google.de/group/mozilla.dev.apps.calendar/msg/3b0493be1f9927aa
Flags: wanted-calendar0.8+ → wanted-calendar0.8?
Comment on attachment 309894 [details] [diff] [review]
Fix - v1

I'd like to stick to useCapture and change the removeListener instances to remove with useCapure instead.
Attachment #309894 - Flags: review?(Berend.Cornelius) → review-
This would fix leaking handlers and is supposed to be an easy/safe fix => wanted-0.8.
Flags: wanted-calendar0.8? → wanted-calendar0.8+
Attached patch Fix - v2Splinter Review
As discussed, this version fixes the other way around, changing the useCapture parameter of the removeEventListener calls.

I left out the ASSERT calls, to make sure no regressions show up.
Attachment #309894 - Attachment is obsolete: true
Attachment #309995 - Flags: review?(daniel.boelzle)
Comment on attachment 309995 [details] [diff] [review]
Fix - v2

r=dbo
Attachment #309995 - Flags: review?(daniel.boelzle) → review+
Checked in on HEAD and MOZILLA_1_8_BRANCH and SUNBIRD_0_8_BRANCH

-> FIXED
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → 0.8
Philipp, I couldn't see this checkin on bonsai. -> REOPENED
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Philipp on low bandwidth -- Checked in on HEAD and MOZILLA_1_8_BRANCH and SUNBIRD_0_8_BRANCH

-> FIXED
Status: REOPENED → RESOLVED
Closed: 12 years ago12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.