208803 - Need XP way for destroyed docs to be removed from accessibility cache

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Aaron Leventhal]

Right now we destroy doc accessibles in the accessibility cache in
nsWindow::~nsWindow().

However, there is a case where the accessibility code is not going through MSAA
(for example an in-process xpcom accessibility client), so nsWindow will not
know about doc accessibles which need to be destroyed.

Comment 1

[Aaron Leventhal]

Attachment: Uses nsIObserverService to broadcast destruction of presshells

Comment 2

[Kyle Yuan]

Comment on attachment 125235 [details] [diff] [review]
Uses nsIObserverService to broadcast destruction of presshells

r=kyle

Comment 3

[Boris Zbarsky [:bzbarsky]]

Hmm... If a document has multiple presshells, wouldn't this destroy something
multiple times?  Or destroy it prematurely if one presshell goes away before the
other one does (eg printing)?  Or do we only put the 0th presshell into the
hashtable?

In any case, that diff is missing the nsIPresShell changes, no?

Comment 4

[Alec Flett]

Comment on attachment 125235 [details] [diff] [review]
Uses nsIObserverService to broadcast destruction of presshells

wow, I'd really rather avoid using nsIObserverService for this. why can't you
register directly with the presshells themselves, and have THEM broadcast the
shutdown?
I don't even see NS_PRESSHELL_DESTROY_TOPIC defined in this patch?

Comment 5

[Aaron Leventhal]

Alec, what is bad about using nsIObserverService to do this? Would you rather 
have me add methods to nsIPresShell to do what the observer service already 
does? Or is there something that pres shell already does that you want me to 
use?

I`ll submit a new patch with the missing file when I get back to the office 
Monday.

Comment 6

[Aaron Leventhal]

Attachment: Patch includes definition of NS_PRESSHELL_DESTROY_TOPIC. Still seeking clarity on why/how not to use observer service for this patch.

Comment 7

[Aaron Leventhal]

Comment on attachment 125706 [details] [diff] [review]
Patch includes definition of NS_PRESSHELL_DESTROY_TOPIC. Still seeking clarity on why/how not to use observer service for this patch.

Carrying Kyle's r=
Seeking more feedback from alecf.

Comment 8

[Aaron Leventhal]

Comment on attachment 125706 [details] [diff] [review]
Patch includes definition of NS_PRESSHELL_DESTROY_TOPIC. Still seeking clarity on why/how not to use observer service for this patch.

Alec, please ignore the line
#include "nsIDOM3Node.h"
in nsPresShell.cpp. That's part of a different patch that was accidentally
included.

Comment 9

[Aaron Leventhal]

Turns out I need this notification for typeaheadfind as well.

Comment 10

[Alec Flett]

the reason is that nsIObserverService is designed to notify about global events,
when you don't know who the consumer (or often the notifier, either) will be -
presshell stuff is very layout-specific, so layout should be the one doing the
notifying. 

Think of it this way... a specific pres shell will be notifying this global
service, which will have to look up global observers, and call a specific
observer.. a sort of 1:N:1 relationship. However, the semantics of what you're
trying to implement require a 1:1 relationship between the notifier and the
observer. you shouldn't involve a third party (the nsIObserverService)

Comment 11

[Alec Flett]

ah! After seeing the code in bug 209354, I see  why you were tempted to do this.
If you'll notice, nobody actually uses those reflow topic notifications.. in
fact that code should more than likely be deleted.

Comment 12

[Aaron Leventhal]

Alec, for bug 209354, I think since they want to get it into the 1.4 branch at
the last moment, wouldn't it be safer to use an existing notification system
than to add new infrastructure?

Comment 13

[Aaron Leventhal]

Alec, please let me know if there's good sample code for me to follow to impl
things without observer service.

Doesn't the 1:N:1 relationship stay essentially 1:1? Wouldn't the observer
service use a hash table to see if there are any listeners for a given message?

Comment 14

[Alec Flett]

if you want a simple fix, then add a single observer method to nsIPresShell,
like SetShutdownObserver(nsIObserver*)

Comment 15

[Boris Zbarsky [:bzbarsky]]

Also note that there is no reason we can't do it one way on the branch and a
better way on trunk....

Comment 16

[Alec Flett]

so is there a reason that the approach used in bug 209354 can't be used here?

Comment 17

[Aaron Leventhal]

Alec, that's my thinking too.

I tried it once before and ran into a problem - I'm not 100% sure what it was.
Perhaps it was that unload is not fired for chrome documents. I'll look and see.

Comment 18

[Aaron Leventhal]

Attachment: Uses onunload instead of new nsIObserver topic

Comment 19

[Aaron Leventhal]

Comment on attachment 126069 [details] [diff] [review]
Uses onunload instead of new nsIObserver topic

Seeking reviews for better way which uses DOM events

Comment 20

[Kyle Yuan]

Comment on attachment 126069 [details] [diff] [review]
Uses onunload instead of new nsIObserver topic

yes, this is better than hacking on nsPresShell.

r=kyle

Comment 21

[Aaron Leventhal]

Comment on attachment 125706 [details] [diff] [review]
Patch includes definition of NS_PRESSHELL_DESTROY_TOPIC. Still seeking clarity on why/how not to use observer service for this patch.

Removing sr= request from old patch.

=> Still looking for sr= on new patch that uses DOM unload event

Comment 22

[Alec Flett]

Comment on attachment 126069 [details] [diff] [review]
Uses onunload instead of new nsIObserver topic

sr=alecf

Comment 23

[Aaron Leventhal]

checked in

Comment 24

[Kyle Yuan]

Comment on attachment 126069 [details] [diff] [review]
Uses onunload instead of new nsIObserver topic

For making the accessibility feature work better and more stable in 1.4.x, we
need to get this patch in. This fix has been in trunk for a long period. It
won't affect other modules.

Comment 25

[Asa Dotzler [:asa]]

Comment on attachment 126069 [details] [diff] [review]
Uses onunload instead of new nsIObserver topic

This is not going to make 1.4.1.  Please re-request aproval after 1.4.1 ships
if you'd like to get this in for 1.4.2. 
Kyle, can you all take this into your tree locally until after 1.4.1 ships
(real soon now) and then work with us to get it landed first thing for 1.4.2?