210782 - Split accessibility interfaces into publc and private interfaces

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Aaron Leventhal]

In intend to eventually freeze the following accessibility interfaces, for use
in internal accessibility clients.
nsIAccessNode
nsIAccessible
nsIAccessibleDocument

Toward that end, I am moving methods internal to the implementation of
accessibility to nsPIAccessNode, nsPIAccessible and nsPIAccessibleDocument.

In addition, I am creating a singleton called nsIAccessibleRetrieval which will
have the nsIAccessibilityService methods that need to be public.

Comment 1

[Aaron Leventhal]

Attachment: Separates the interface, creates new singleton nsIAccessibleRetrieval

Note that nsPIAccessNode was already created in a prior patch.

Also note that nsIAccessibilityService inherits from the nsIAccessibleRetrieval
interfaces. This way it retains all of it's old methods.

When the service manager asks NS_NewAccessibleRetrieval to create and
nsIAccessibleRetrieval, we in turn ask the service manager for an
nsIAccessibilityService because they're both from the same object. This
prevents the creation of 2 nsAccessibiltiyService objects.

Comment 2

[Kyle Yuan]

Comment on attachment 126566 [details] [diff] [review]
Separates the interface, creates new singleton nsIAccessibleRetrieval

Aaron, you missed 3 new idl files in this patch: nsIAccessibleRetrieval.idl,
nsPIAccessible.idl, nsPIAccessibleDocument.idl

Comment 3

[Kyle Yuan]

=> Aaron and cc me

Comment 4

[Aaron Leventhal]

Attachment: Contains new files missing from last patch. Otherwise it's the same.

Comment 5

[Kyle Yuan]

Comment on attachment 126733 [details] [diff] [review]
Contains new files missing from last patch. Otherwise it's the same.

r=kyle

Comment 6

[Doug Turner (:dougt)]

Comment on attachment 126733 [details] [diff] [review]
Contains new files missing from last patch. Otherwise it's the same.

i am not a sr.

Comment 7

[Aaron Leventhal]

Attachment: After chatting with dougt, simplified creation of services nsIAccessibleRetrieval and nsIAccessibilityService

Comment 8

[Aaron Leventhal]

Comment on attachment 126851 [details] [diff] [review]
After chatting with dougt, simplified creation of services nsIAccessibleRetrieval and nsIAccessibilityService

Carrying r=

Comment 9

[Aaron Leventhal]

Comment on attachment 126733 [details] [diff] [review]
Contains new files missing from last patch. Otherwise it's the same.

Switching request to other patch

Comment 10

[Johnny Stenback (:jst)]

Comment on attachment 126851 [details] [diff] [review]
After chatting with dougt, simplified creation of services nsIAccessibleRetrieval and nsIAccessibilityService

- In NS_GetAccessibilityService():

 {
+  nsAccessibilityService *gAccessibilityService = nsnull;
+

The name of this variable suggests that this would be a global variable, though
it isn't. What's the intent here? Did you mean to make this a singleton service
and cache it in this variable? If so, it needs to really be global, or static,
and you need to do the proper cleanup too...

+  *aResult = gAccessibilityService;
+  if (*aResult) {
+    NS_ADDREF(*aResult);
+    return NS_OK;
+  }
+  return NS_ERROR_OUT_OF_MEMORY;

How about making the end of the function the common case and the error case the
early return here?

Comment 11

[Aaron Leventhal]

Attachment: New patch fixing jst's comments

Comment 12

[Aaron Leventhal]

Comment on attachment 126988 [details] [diff] [review]
New patch fixing jst's comments

Carrying r=.
Seeking sr=jst

Comment 13

[Johnny Stenback (:jst)]

Comment on attachment 126988 [details] [diff] [review]
New patch fixing jst's comments

Found a nit, and a minor problem:

- In nsAccessibilityService::GetAccessibilityService():

+  if (!nsAccessibilityService::gAccessibilityService) {
+    nsAccessibilityService::gAccessibilityService = new
nsAccessibilityService();
+  }
+  *aResult = nsAccessibilityService::gAccessibilityService;

Since this code is in the class nsAccessibilityService, there's no need to
qualify the gAccessibilityService variable with the class name.

+  if (!*aResult) {
+    return NS_ERROR_OUT_OF_MEMORY;
+  }

Move this check inside the other if (!gAccessibilityService) (but check
!gAccessibilityService there, not !*aResult, of course) to avoid checking this
twice when gAccessibilityService is set.

sr=jst

Comment 14

[Aaron Leventhal]

Attachment: Final patch.

Comment 15

[Aaron Leventhal]

checked in