Closed Bug 846204 Opened 9 years ago Closed 9 years ago

unregister DocManager from DOM event listeners

Categories

(Core :: Disability Access APIs, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla22

People

(Reporter: surkov, Assigned: surkov)

References

Details

(Keywords: access)

Attachments

(1 file)

Attached patch patchSplinter Review
spun off bug 694254 (also see bug 694254 comment #64).

1) DocManager is not cycle collected
2) Document and document accessible may have different life cycles (it doesn't make sense to keep listeners if we don't use them)
Attachment #719352 - Flags: review?(trev.saunders)
Blocks: 694254
Comment on attachment 719352 [details] [diff] [review]
patch

>+DocManager::RemoveListeners(nsIDocument* aDocument)
>+{
>+  nsPIDOMWindow* window = aDocument->GetWindow();
>+  nsIDOMEventTarget* target = window->GetChromeEventHandler();
>+  nsEventListenerManager* elm = target->GetListenerManager(true);

does it really make sense to force creation of the listener manager?  if we just created it how can it be holding a ref to us?

btw would it make more sense to do this in DocAccessible::Shutdown() so you can just grab the elm off the pres shell you already have?
Attachment #719352 - Flags: review?(trev.saunders) → review+
(In reply to Trevor Saunders (:tbsaunde) from comment #1)
> Comment on attachment 719352 [details] [diff] [review]
> patch
> 
> >+DocManager::RemoveListeners(nsIDocument* aDocument)
> >+{
> >+  nsPIDOMWindow* window = aDocument->GetWindow();
> >+  nsIDOMEventTarget* target = window->GetChromeEventHandler();
> >+  nsEventListenerManager* elm = target->GetListenerManager(true);
> 
> does it really make sense to force creation of the listener manager?  if we
> just created it how can it be holding a ref to us?

makes sense, just copy/paste

> btw would it make more sense to do this in DocAccessible::Shutdown() so you
> can just grab the elm off the pres shell you already have?

I'd like to keep removal where they were added. If DocManager adds listeners then it should take care to remove them. To stay symmetrical.
Assignee: nobody → surkov.alexander
Status: NEW → ASSIGNED
Is this ready to land?
(In reply to Ed Morley [:edmorley UTC+0] from comment #3)
> Is this ready to land?

yes, it is
https://hg.mozilla.org/integration/mozilla-inbound/rev/d253208c90bc

*fingers crossed* - thanks for the patch, Alexander!
https://hg.mozilla.org/mozilla-central/rev/d253208c90bc
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
Depends on: 849496
Depends on: 881504
You need to log in before you can comment on or make changes to this bug.