cache nsAccessible children for XUL tree accessibles

RESOLVED FIXED

Status

()

Core
Disability Access APIs
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: surkov, Assigned: surkov)

Tracking

(Blocks: 2 bugs, {access})

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

8 years ago
Created attachment 430511 [details] [diff] [review]
patch
Attachment #430511 - Flags: superreview?(neil)
Attachment #430511 - Flags: review?(bolterbugz)
(Assignee)

Comment 1

8 years ago
Created attachment 430523 [details] [diff] [review]
patch2
Attachment #430511 - Attachment is obsolete: true
Attachment #430523 - Flags: superreview?(neil)
Attachment #430523 - Flags: review?(bolterbugz)
Attachment #430511 - Flags: superreview?(neil)
Attachment #430511 - Flags: review?(bolterbugz)

Comment 2

8 years ago
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*]
(Assignee)

Comment 3

8 years ago
(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.
(Assignee)

Comment 4

8 years ago
Created attachment 431286 [details] [diff] [review]
patch3
Attachment #430523 - Attachment is obsolete: true
Attachment #431286 - Flags: superreview?(neil)
Attachment #431286 - Flags: review?(bolterbugz)
Attachment #430523 - Flags: superreview?(neil)
Attachment #430523 - Flags: review?(bolterbugz)

Comment 5

8 years ago
Comment on attachment 431286 [details] [diff] [review]
patch3

This way is fine, thanks.
Attachment #431286 - Flags: superreview?(neil) → superreview+
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?
Attachment #431286 - Flags: review?(bolterbugz) → review+
(Assignee)

Comment 7

8 years ago
(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.
(Assignee)

Comment 8

8 years ago
(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.
(Assignee)

Comment 9

8 years ago
landed on 1.9.3 - http://hg.mozilla.org/mozilla-central/rev/5ba932292ee1 (I changed New_ to Create).
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Thanks.
You need to log in before you can comment on or make changes to this bug.