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*]

(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 on attachment 431286 [details] [diff] [review] patch3 This way is fine, thanks.

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?

(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.

(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.

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

Thanks.