550396 - cache nsAccessible children for XUL tree accessibles

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 1

[alexander :surkov (:asurkov)]

Attachment: patch2

Comment 2

[[e-mail]]

Comment on attachment 430523 [details] [diff] [review]
patch2

>+  nsISupports *supports = static_cast<nsIAccessNode*>(aAccessNode);
[It's not obvious from reading the patch why you added this.]

>-  GetTreeItemAccessible(row, aChild);
>+  NS_IF_ADDREF(*aChild = GetTreeItemAccessible(row));
>+
>   if (aDeepestChild && *aChild) {
>     // Look for accessible cell for the found item accessible.
>     nsRefPtr<nsXULTreeItemAccessibleBase> treeitemAcc =
>       nsAccUtils::QueryObject<nsXULTreeItemAccessibleBase>(*aChild);
> 
>-    nsCOMPtr<nsIAccessible> cellAccessible;
>-    treeitemAcc->GetCellAccessible(column, getter_AddRefs(cellAccessible));
>-    if (cellAccessible)
>-      cellAccessible.swap(*aChild);
>+    nsAccessible *cellAccessible = treeitemAcc->GetCellAccessible(column);
>+    NS_IF_ADDREF(*aChild = cellAccessible);
You don't null-check cellAccessible any more, is this intentional?
Note: either way, you leak the old value of *aChild!

>-  *aAccessNode = new nsXULTreeItemAccessible(mDOMNode, mWeakShell, this,
>-                                             mTree, mTreeView, aRow);
>-  NS_IF_ADDREF(*aAccessNode);
>+  nsRefPtr<nsAccessible> accessible =
>+    new nsXULTreeItemAccessible(mDOMNode, mWeakShell, this, mTree, mTreeView,
>+                                aRow);
>+
>+  return accessible.forget();
[This isn't the only way of doing this, and I'm not sure which is best:
 1. nsRefPtr and .forget(), as above
 2. NS_IF_ADDREF (on a local variable, rather than *aAccessNode)
 3. Change the return value to nsAccessible*]

Comment 3

[alexander :surkov (:asurkov)]

(In reply to comment #2)
> (From update of attachment 430523 [details] [diff] [review])
> >+  nsISupports *supports = static_cast<nsIAccessNode*>(aAccessNode);
> [It's not obvious from reading the patch why you added this.]

aAccessNode can be nsAccessible which isn't casted to nsISupports unambiguously.

> You don't null-check cellAccessible any more, is this intentional?
> Note: either way, you leak the old value of *aChild!

thanks for the catch.

> [This isn't the only way of doing this, and I'm not sure which is best:
>  1. nsRefPtr and .forget(), as above
>  2. NS_IF_ADDREF (on a local variable, rather than *aAccessNode)
>  3. Change the return value to nsAccessible*]

I don't have preferences between 1 and 2. I can go with you like more. I'm not sure I like 3 because nothing says the memory is allocated by this method and should be freed by caller.

Comment 4

[alexander :surkov (:asurkov)]

Attachment: patch3

Comment 5

[[e-mail]]

Comment on attachment 431286 [details] [diff] [review]
patch3

This way is fine, thanks.

Comment 6

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 431286 [details] [diff] [review]
patch3

r=me with nits:

>+++ b/accessible/src/xul/nsXULTreeAccessible.h
>@@ -99,17 +99,17 @@ public:
> 
>   /**
>    * Return tree item accessible at the givem row. If accessible doesn't exist
>    * in the cache then create and cache it.
>    *
>    * @param aRow         [in] the given row index
>    * @param aAccessible  [out] tree item accessible

No longer an @param ;)

>-  virtual void CreateTreeItemAccessible(PRInt32 aRowIndex,
>-                                        nsAccessNode** aAccessNode);
>+  virtual already_AddRefed<nsAccessible>
>+    New_TreeItemAccessible(PRInt32 aRowIndex);

This name is a bit strange. Is there precedent for this kind of naming in gecko?

Comment 7

[alexander :surkov (:asurkov)]

(In reply to comment #6)

> >+    New_TreeItemAccessible(PRInt32 aRowIndex);
> 
> This name is a bit strange. Is there precedent for this kind of naming in
> gecko?

Not 100% match. There are NEW_ macros.

Here I wanted emphasize this method makes "new class()". Therefore I think it's ok to go with Neil's 3d suggestion regardless of my previous comment.

Comment 8

[alexander :surkov (:asurkov)]

(In reply to comment #7)
> (In reply to comment #6)
> 
> > >+    New_TreeItemAccessible(PRInt32 aRowIndex);
> > 
> > This name is a bit strange. Is there precedent for this kind of naming in
> > gecko?
> 
> Not 100% match. There are NEW_ macros.
> 
> Here I wanted emphasize this method makes "new class()". Therefore I think it's
> ok to go with Neil's 3d suggestion regardless of my previous comment.

Or if you'd like we could go with Create_ prefix like we do in nsAccessibilityService.

Comment 9

[alexander :surkov (:asurkov)]

landed on 1.9.3 - http://hg.mozilla.org/mozilla-central/rev/5ba932292ee1 (I changed New_ to Create).

Comment 10

[David Bolter [:davidb] (NeedInfo me for attention)]

Thanks.