199060 - Accessibility reorg, move widget accessibility code into mozilla/accessible, merge class hierarchies

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Aaron Leventhal]

This reorg is designed to improve the accessibility architecture.

Goals:
- Reduce the number of ifdef's by adding toolkit-specific directories
accessible/src/msaa, accessible/src/atk, accessible/src/mac
- Get ready to add hash table based cache, which will keep us very fast and fix
bug 193802
- Remove crashes the occur on exit of the program
- Remove leaks
- Move even more code out of widget into accessible -- it really doesn't need to
be loaded unless accessibility is

The new system is very logical. Classes with the word "Wrap" appended to the
name will exist in the toolkit directory, and contain code and interfaces
specific to that toolkit. They alway inherit from the class without the word
Wrap appended.

nsCacheNode
nsCacheNodeWrap: public nsCacheNode
nsAccessible: public nsCacheNodeWrap
nsAccessibleWrap: public nsAccessible
nsDocAccessible: public nsAccessibleWrap
nsDocAccessibleWrap: public nsDocAccessible
nsRootAccessible: public nsDocAccessibleWrap
nsRootAccessibleWrap: public nsRootAccessible

Comment 1

[Aaron Leventhal]

Attachment: Big patch. Doesn't impl cache or events yet.

Kyle, this is not ready for review, because of problems it has in accessible
explorer, which I'll debug soon.

However, it's ready to look at to see what I'm doing.

Comment 2

[Aaron Leventhal]

Kyle, I renamed the class nsHTMLIFrameRootAccessible to nsDocAccessible, but
didn't rename the files nsHTMLIFrameRootAccessible.* to nsDocAccessible.* yet.

Comment 3

[Kyle Yuan]

The patch looks awesome! After a quick look, I have some comments(questions):

1) are you still going to combine accessibility library into widget?
2) some code in nsRootAccessibleWrap::ShutdownAll() are exactly same, can we 
move them into nsRootAccessible::ShutdownAll()?
3) it would be better to rename GetNSAccessibleFor to GetXPAccessibleFor;
4) some files missed a newline at end.

Bolian, could you take a look to evaluate how many work we should do for 
porting mai code into mozilla/accessible?

Comment 4

[Aaron Leventhal]

Attachment: New patch containing Kyle's suggestions

I also moved Shutdown() into nsRootAccessible. Then I realized I could get of
gInstanceCount which is used to decide when to call release gLastFocusedNode. I
just moved that into ShutdownAll()

Comment 5

[Kyle Yuan]

Comment on attachment 118388 [details] [diff] [review]
New patch containing Kyle's suggestions

review comment:

1) should we rename the nsHTMLIFrameRootAccessible.cpp/.h to
nsHTMLIFrameAccessible and move nsDocAccessible out there
(accessible/src/base/nsDocAccessible.cpp/.h is a good choice)?

2) in accessible/src/msaa/nsAccessibleWrap.cpp
+nsAccessibleWrap::nsAccessibleWrap(nsIDOMNode* aNode, nsIWeakReference
*aShell): 
+  nsAccessible(aNode, aShell)
+{
+#ifdef DEBUG_LEAKS
+  printf("Accessibles=%d\n", ++gAccessibles);
+#endif
+}
these code should go up to nsAccessible, as well as the destructor.

3) in accessible/src/mac/nsRootAccessibleWrap.cpp
+nsRootAccessibleWrap::nsRootAccessibleWrap(nsIDOMNode *aDOMNode,
nsIWeakReference *aShell): 
+  nsRootAccessible(aDOMNode, aShell)
+{
+  ++gActiveRootAccessibles;
+}
you are increasing gActiveRootAccessibles twice here. (just in atk/ & mac/)
same issue in the nsRootAccessibleWrap.h (seems you forgot to remove the
definition of gActiveRootAccessibles)

4) in accessible/src/Makefile.in
-DIRS = \
+ifdef MOZ_ACCESSIBILITY_ATK
+DIRS += atk
+else
+ifeq ($(MOZ_WIDGET_TOOLKIT),windows)
+DIRS += msaa
+else
+ifeq ($(MOZ_WIDGET_TOOLKIT),mac)
+DIRS += mac
+endif
+endif
+endif
+
+DIRS += \
   base \
   html \
   $(null)
I'd like to put the platform-specific code *after* the xp code in the compiling
order. But I think this won't break the build because we are using 2-pass build
steps.

Comment 6

[Aaron Leventhal]

Attachment: Ready for review. Will impl the actual cache and fix event handling in another bug.

This contains Kyle's latest ideas for improvements, and fixes bugs that we had
with MSAA SDK's Accessible Explorer.

Comment 7

[Kyle Yuan]

Comment on attachment 118488 [details] [diff] [review]
Ready for review. Will impl the actual cache and fix event handling in another bug.

review comments:

1) accessible/public/Makefile.in
+#ifndef MOZ_ACCESSIBILITY_ATK
+#ifeq ($(MOZ_WIDGET_TOOLKIT),windows)
+DIRS = msaa
+#endif
+#endif
please remove the "#" that's a comment sign in Makefile. And I think it's not
necessary to have ifndef MOZ_ACCESSIBILITY_ATK here

2) accessible/src/Makefile.in
+ifdef MOZ_ACCESSIBILITY_ATK
+DIRS += atk
+else
+ifeq ($(MOZ_WIDGET_TOOLKIT),windows)
+DIRS += msaa
+else
|ifdef MOZ_ACCESSIBILITY_ATK| should be changed to |ifeq
($(MOZ_WIDGET_TOOLKIT),gtk2)|, since ATK is totally integrated with gtk2.
And these lines should be put before the xp part. Sorry, that was my mistake :(

3) accessible/src/mac/nsAccessibleWrap.cpp
+nsAccessibleWrap::nsAccessibleWrap(nsIDOMNode* aNode, nsIWeakReference
*aShell): 
+  nsAccessible(aNode, aShell)
+{
+#ifdef DEBUG_LEAKS
+  printf("Accessibles=%d\n", ++gAccessibles);
+#endif
+}
+
+//-----------------------------------------------------
+// destruction
+//-----------------------------------------------------
+nsAccessibleWrap::~nsAccessibleWrap()
+{
+#ifdef DEBUG_LEAKS
+  printf("Accessibles=%d\n", --gAccessibles);
+#endif
+}
you forgot to remove those code

r=kyle with those changes. no new patch needed.

BTW, I've successfully built it on Linux with a little hacking on the Makefile
that was mentioned in point 1) & 2).

Comment 8

[Aaron Leventhal]

Attachment: Kyle's nits. Also removed extra #include's

Comment 9

[Aaron Leventhal]

Attachment: changes.diff -- only the changes that need to be reviewed, no moved/deleted files

Comment 10

[Aaron Leventhal]

Attachment: Summary of what's been changed

Comment 11

[Aaron Leventhal]

Attachment: Most of alecf's changes -- see below

This is a replacement for the short version of the diff. It can't be applied.

x Changed CreateIFrameAccessible to CreateOuterDocAccessible
x No longer need nsAccessible::gInstanceCount 
x Implemented nsAccessible::Shutdown
x Got rid of extra printfs
x Newer license
x Made sure nsCacheNode::~nsCacheNode doesn't call Shutdown if it's already
happened
x Changed nsICacheNode and nsCacheNode.* to nsIAccessNode, nsAccessNode
x Got rid of nsCOMPtr use in arguments to helper methods
x Fixed licenses on new files to use latest updated versions, (c) 2003

Alecf also wants nsIAccessibility::Shutdown() call to be moved to when the
xpcom shutdown signal is sent. I suggest we file a separate bug for that.

Comment 12

[Aaron Leventhal]

Comment on attachment 118608 [details] [diff] [review]
Most of alecf's changes -- see below 

Carrying Kyle's r=

Comment 13

[Alec Flett]

Comment on attachment 118608 [details] [diff] [review]
Most of alecf's changes -- see below 

sr=alecf

Comment 14

[Aaron Leventhal]

Comment on attachment 118608 [details] [diff] [review]
Most of alecf's changes -- see below 

This is a big change, but it only affects accessibility, which is not loaded in
the normal case. Seeking approval so we can get it in ASAP. The Beijing team
and I both need this in so we can continue our work.

Comment 15

[Aaron Leventhal]

Comment on attachment 118608 [details] [diff] [review]
Most of alecf's changes -- see below 

Sorry, wrong flag set.

See above comment as to why I think this should get a=. (Basically it's not
going to be loaded 99.99% of the time)

Comment 16

[Asa Dotzler [:asa]]

Comment on attachment 118608 [details] [diff] [review]
Most of alecf's changes -- see below 

please land first thing in beta. thanks.

Comment 17

[Aaron Leventhal]

fixed

Comment 18

[Firefox Bug Husbandry Bot]

https://bugzilla.mozilla.org/show_bug.cgi?id=1527381