Closed Bug 646406 Opened 9 years ago Closed 9 years ago

AccTreeWalker::NextChild should return raw pointer

Categories

(Core :: Disability Access APIs, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla5

People

(Reporter: surkov, Assigned: surkov)

References

(Blocks 1 open bug)

Details

(Keywords: access)

Attachments

(1 file)

Attached patch patchSplinter Review
Accessibility serivce's GetOrCreateAccessible binds accessible to the document, so it doesn't make sense to deal with smart pointers.
Attachment #522965 - Flags: review?(trev.saunders)
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
Attachment #522965 - Flags: review?(trev.saunders)
Attachment #522965 - Flags: review?
Attachment #522965 - Flags: review+
landed with comments addressed on cedar http://hg.mozilla.org/projects/cedar/rev/6fa8fbd811fa
Whiteboard: [fixed-in-cedar]
http://hg.mozilla.org/mozilla-central/rev/6fa8fbd811fa
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-cedar]
Target Milestone: --- → mozilla2.2
You need to log in before you can comment on or make changes to this bug.