Closed Bug 208803 Opened 21 years ago Closed 21 years ago

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

Categories

(Core :: Disability Access APIs, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: aaronlev, Assigned: aaronlev)

References

Details

(Keywords: access, Whiteboard: in-process-accessibility)

Attachments

(1 file, 2 obsolete files)

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.
Keywords: access
OS: Windows 2000 → All
Hardware: PC → All
Attachment #125235 - Flags: review?(kyle.yuan)
Comment on attachment 125235 [details] [diff] [review]
Uses nsIObserverService to broadcast destruction of presshells

r=kyle
Attachment #125235 - Flags: review?(kyle.yuan) → review+
Attachment #125235 - Flags: superreview?(alecf)
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 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?
Attachment #125235 - Flags: superreview?(alecf) → superreview-
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 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.
Attachment #125706 - Flags: superreview?(alecf)
Attachment #125706 - Flags: review+
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.
Turns out I need this notification for typeaheadfind as well.
Blocks: 209354
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)

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.

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?
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?
if you want a simple fix, then add a single observer method to nsIPresShell,
like SetShutdownObserver(nsIObserver*)

Also note that there is no reason we can't do it one way on the branch and a
better way on trunk....
so is there a reason that the approach used in bug 209354 can't be used here?
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.
Attachment #125706 - Attachment is obsolete: true
Comment on attachment 126069 [details] [diff] [review]
Uses onunload instead of new nsIObserver topic

Seeking reviews for better way which uses DOM events
Attachment #126069 - Flags: superreview?
Attachment #126069 - Flags: review?
Attachment #126069 - Flags: superreview?(alecf)
Attachment #126069 - Flags: superreview?
Attachment #126069 - Flags: review?(kyle.yuan)
Attachment #126069 - Flags: review?
Comment on attachment 126069 [details] [diff] [review]
Uses onunload instead of new nsIObserver topic

yes, this is better than hacking on nsPresShell.

r=kyle
Attachment #126069 - Flags: review?(kyle.yuan) → review+
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
Attachment #125706 - Flags: superreview?(alecf)
Whiteboard: Seeking sr= for new approach using DOM unload event
Comment on attachment 126069 [details] [diff] [review]
Uses onunload instead of new nsIObserver topic

sr=alecf
Attachment #126069 - Flags: superreview?(alecf) → superreview+
checked in
Status: NEW → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
Whiteboard: Seeking sr= for new approach using DOM unload event → in-process-accessibility
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.
Attachment #126069 - Flags: approval1.4.x?
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?
Attachment #126069 - Flags: approval1.4.x? → approval1.4.x-
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: