646406 - AccTreeWalker::NextChild should return raw pointer

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

Attachment: patch

Accessibility serivce's GetOrCreateAccessible binds accessible to the document, so it doesn't make sense to deal with smart pointers.

Comment 1

[Trevor Saunders (:tbsaunde)]

Comment on attachment 522965 [details] [diff] [review]
patch


>@@ -927,20 +923,17 @@ nsAccessibilityService::GetOrCreateAcces
>   }
> 
>   if (weakFrame.GetFrame()->GetContent() != content) {
>     // Not the main content for this frame. This happens because <area>
>     // elements return the image frame as their primary frame. The main content
>     // for the image frame is the image content. If the frame is not an image
>     // frame or the node is not an area element then null is returned.
>     // This setup will change when bug 135040 is fixed.
>-    nsAccessible* areaAcc = GetAreaAccessible(weakFrame.GetFrame(),
>-                                              aNode, aWeakShell);
>-    NS_IF_ADDREF(areaAcc);
>-    return areaAcc;
>+    return GetAreaAccessible(weakFrame.GetFrame(), aNode, aWeakShell);
>   }
> 
>   nsDocAccessible* docAcc =
>     GetAccService()->GetDocAccessible(aNode->GetOwnerDoc());
>   if (!docAcc) {
>     NS_NOTREACHED("No document for accessible being created!");

That phrasing really threw me off at first, it  seems like "tried to get  accessible for node with no document" or something like that might be better.

>     return nsnull;
>   }

> nsXULColorPickerAccessible::CacheChildren()
> {
>   nsAccTreeWalker walker(mWeakShell, mContent, PR_TRUE);
> 
>-  nsRefPtr<nsAccessible> child;
>-  while ((child = walker.GetNextChild())) {
>+  nsAccessible* child = nsnull;
>+  while ((child = walker.NextChild())) {
>     PRUint32 role = child->Role();
> 
>     // Get an accessbile for menupopup or panel elements.

accessible?

>     if (role == nsIAccessibleRole::ROLE_ALERT) {
>       AppendChild(child);
>       return;
>     }

>     if (role == nsIAccessibleRole::ROLE_MENUPOPUP) {
>       // Get an accessbile for menupopup or panel elements.

accessible?

>-      menupopupAccessible.swap(child);
>+      menupopup = child;
> 
>     } else if (isMenuButton && role == nsIAccessibleRole::ROLE_PUSHBUTTON) {
>       // Button type="menu-button" contains a real button. Get an accessible
>       // for it. Ignore dropmarker button what is placed as a last child.

s/what/which/?

>-      buttonAccessible.swap(child);
>+      button = child;
>       break;

r=me

Comment 2

[alexander :surkov (:asurkov)]

landed with comments addressed on cedar http://hg.mozilla.org/projects/cedar/rev/6fa8fbd811fa

Comment 3

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/6fa8fbd811fa