Closed Bug 210782 Opened 21 years ago Closed 21 years ago

Split accessibility interfaces into publc and private interfaces

Categories

(Core :: Disability Access APIs, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: aaronlev, Assigned: aaronlev)

Details

(Keywords: access)

Attachments

(1 file, 4 obsolete files)

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.
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.
Attachment #126566 - Flags: review?(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
=> Aaron and cc me
Assignee: kyle.yuan → aaronl
Attachment #126566 - Attachment is obsolete: true
Attachment #126733 - Flags: review?(kyle.yuan)
Comment on attachment 126733 [details] [diff] [review]
Contains new files missing from last patch. Otherwise it's the same.

r=kyle
Attachment #126733 - Flags: review?(kyle.yuan) → review+
Attachment #126733 - Flags: superreview?(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.
Attachment #126733 - Flags: superreview?(dougt) → superreview?(brendan)
Comment on attachment 126851 [details] [diff] [review]
After chatting with dougt, simplified creation of services nsIAccessibleRetrieval and nsIAccessibilityService

Carrying r=
Attachment #126851 - Flags: superreview?(jst)
Attachment #126851 - Flags: review+
Comment on attachment 126733 [details] [diff] [review]
Contains new files missing from last patch. Otherwise it's the same.

Switching request to other patch
Attachment #126733 - Flags: superreview?(brendan)
Attachment #126851 - Flags: superreview?(jst) → superreview?(brendan)
Attachment #126851 - Flags: superreview?(brendan) → superreview?(bryner)
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?
Attachment #126851 - Attachment is obsolete: true
Comment on attachment 126988 [details] [diff] [review]
New patch fixing jst's comments

Carrying r=.
Seeking sr=jst
Attachment #126988 - Flags: superreview?(jst)
Attachment #126988 - Flags: review+
Attachment #126851 - Flags: superreview?(bryner)
Attachment #126566 - Flags: review?(kyle.yuan)
Whiteboard: Looking for jst to sr= new patch resulting from his feedback
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
Attachment #126988 - Flags: superreview?(jst) → superreview+
Attached patch Final patch. (obsolete) — Splinter Review
Attachment #127332 - Attachment is obsolete: true
checked in
Status: NEW → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
Whiteboard: Looking for jst to sr= new patch resulting from his feedback
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: