461923 - remove nsIAccessibleTreeCache and nsPIAccessibleTreeItem

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Comment 1

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 2

[alexander :surkov (:asurkov)]

Attachment: patch2

Comment 3

[alexander :surkov (:asurkov)]

Attachment: patch3

one more patch

Comment 4

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

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

Looking good, some questions:

>+already_AddRefed<nsXULTreeAccessible>
>+nsAccUtils::QueryAccessibleTree(nsIAccessible *aAccessible)

>+already_AddRefed<nsXULTreeitemAccessible>
>+nsAccUtils::QueryAccessibleTreeitem(nsIAccessNode *aAccessNode)

>+class nsXULTreeAccessible;
>+class nsXULTreeitemAccessible;

I wonder, should we #ifdef MOZ_XUL this stuff (here and elsewhere for XUL related impl)?

>-  nsIAccessible *accessible = nsnull;
>-  accService->GetAccessibleFor(mDOMNode, &accessible);
>+  nsCOMPtr<nsIAccessible> accessible;
>+  accService->GetAccessibleFor(mDOMNode, getter_AddRefs(accessible));

Why this change?

>       if (treeIndex >= 0) {
>-        nsCOMPtr<nsIAccessibleTreeCache> treeCache(do_QueryInterface(accessible));
>-        NS_IF_RELEASE(accessible);
>-        nsCOMPtr<nsIAccessible> treeItemAccessible;
>-        if (!treeCache ||
>-            NS_FAILED(treeCache->GetCachedTreeitemAccessible(
>-                      treeIndex,
>-                      nsnull,
>-                      getter_AddRefs(treeItemAccessible))) ||
>-                      !treeItemAccessible) {
>-          return nsnull;
>+        nsRefPtr<nsXULTreeAccessible> treeCache =
>+          nsAccUtils::QueryAccessibleTree(accessible);
>+        if (treeCache) {
>+          treeCache->GetCachedTreeitemAccessible(treeIndex, nsnull,
>+                                                 getter_AddRefs(accessible));
>         }
>-        NS_IF_ADDREF(accessible = treeItemAccessible);
>       }
>     }
>   }
> #endif
> 
>-  return accessible;
>+  return accessible.forget();
> }

You ref count looks okay here but I'm not 100% sure.

>+PRBool
>+nsXULTreeAccessible::IsDefunct()
>+{
>+  if (nsXULSelectableAccessible::IsDefunct())
>+    return PR_TRUE;
>+
>+  return !mTree || !mTreeView;
>+}

could do:
return nsXULSelectableAccessible::IsDefunct() || !mTree || !mTreeView;

>+void
> nsXULTreeAccessible::GetCachedTreeitemAccessible(PRInt32 aRow,
>                                                  nsITreeColumn* aColumn,
>-                                                 nsIAccessible** aAccessible)
>+                                                 nsIAccessible **aAccessible)

Probably not good to change the * placement style only for one argument. I'd go with whatever fits the for the rest of the source file.

>   nsCOMPtr<nsIAccessNode> accessNode;
>-  GetCacheEntry(*mAccessNodeCache, (void*)(aRow * kMaxTreeColumns + columnIndex), getter_AddRefs(accessNode));
>-  if (!accessNode)
>-  {
>-    nsXULTreeitemAccessibleWrap* treeItemAcc =
>+  GetCacheEntry(*mAccessNodeCache,
>+                (void*)(aRow * kMaxTreeColumns + columnIndex),
>+                getter_AddRefs(accessNode));

I'm fine with reformatting the code to look better as long as we don't lose valuable hg blame info.

>-    PutCacheEntry(*mAccessNodeCache, (void*)(aRow * kMaxTreeColumns + columnIndex), accessNode);
>+    PutCacheEntry(*mAccessNodeCache,
>+                  (void*)(aRow * kMaxTreeColumns + columnIndex), accessNode);

Ditto here.

>+ * Accessbile class for columns element of XUL tree.

nit: Accessible

Comment 5

[alexander :surkov (:asurkov)]

(In reply to comment #4)
> (From update of attachment 385397 [details] [diff] [review])
> Looking good, some questions:
> 
> >+already_AddRefed<nsXULTreeAccessible>
> >+nsAccUtils::QueryAccessibleTree(nsIAccessible *aAccessible)
> 
> >+already_AddRefed<nsXULTreeitemAccessible>
> >+nsAccUtils::QueryAccessibleTreeitem(nsIAccessNode *aAccessNode)
> 
> >+class nsXULTreeAccessible;
> >+class nsXULTreeitemAccessible;
> 
> I wonder, should we #ifdef MOZ_XUL this stuff (here and elsewhere for XUL
> related impl)?

right, thanks for the catch.

> >-  nsIAccessible *accessible = nsnull;
> >-  accService->GetAccessibleFor(mDOMNode, &accessible);
> >+  nsCOMPtr<nsIAccessible> accessible;
> >+  accService->GetAccessibleFor(mDOMNode, getter_AddRefs(accessible));
> 
> Why this change?

right, not necessary.

> I'm fine with reformatting the code to look better as long as we don't lose
> valuable hg blame info.

ok, let's not change this.

Comment 6

[alexander :surkov (:asurkov)]

Attachment: patch4

Comment 7

[Marco Zehe (:MarcoZ)]

Comment on attachment 385541 [details] [diff] [review]
patch4

This looks great and also works for me! r=me

Comment 8

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

Comment on attachment 385541 [details] [diff] [review]
patch4

Thanks, r=me. Note we are removing an interface here (nsPIAccessibleTreeItm); I wonder if the bug summary needs changing?

Comment 9

[alexander :surkov (:asurkov)]

(In reply to comment #8)
> (From update of attachment 385541 [details] [diff] [review])
> Thanks, r=me. Note we are removing an interface here (nsPIAccessibleTreeItm); I
> wonder if the bug summary needs changing?

if you like

Comment 10

[alexander :surkov (:asurkov)]

landed on 1.9.2 - http://hg.mozilla.org/mozilla-central/rev/e97a9e879fc0

Comment 11

[alexander :surkov (:asurkov)]

These interface were priviate and iirc they weren't documented on devmo. Is there anything here to document? (I keep in mind "dev-doc-needed" keyword).

Comment 12

[Eric Shepherd [:sheppy]]

OK, that's what I needed to know. I've removed the doc needed keyword.