Closed Bug 461923 Opened 11 years ago Closed 11 years ago

remove nsIAccessibleTreeCache and nsPIAccessibleTreeItem

Categories

(Core :: Disability Access APIs, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: surkov, Assigned: surkov)

References

Details

Attachments

(1 file, 3 obsolete files)

No description provided.
Attached patch patch (obsolete) — Splinter Review
Assignee: nobody → surkov.alexander
Status: NEW → ASSIGNED
Attachment #385374 - Flags: review?(marco.zehe)
Attachment #385374 - Flags: review?(bolterbugz)
Attached patch patch2 (obsolete) — Splinter Review
Attachment #385374 - Attachment is obsolete: true
Attachment #385375 - Flags: review?(marco.zehe)
Attachment #385375 - Flags: review?(bolterbugz)
Attachment #385374 - Flags: review?(marco.zehe)
Attachment #385374 - Flags: review?(bolterbugz)
Attached patch patch3 (obsolete) — Splinter Review
one more patch
Attachment #385375 - Attachment is obsolete: true
Attachment #385397 - Flags: review?(marco.zehe)
Attachment #385397 - Flags: review?(bolterbugz)
Attachment #385375 - Flags: review?(marco.zehe)
Attachment #385375 - Flags: review?(bolterbugz)
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
(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.
Attached patch patch4Splinter Review
Attachment #385397 - Attachment is obsolete: true
Attachment #385541 - Flags: review?(marco.zehe)
Attachment #385541 - Flags: review?(bolterbugz)
Attachment #385397 - Flags: review?(marco.zehe)
Attachment #385397 - Flags: review?(bolterbugz)
Attachment #385541 - Flags: review?(marco.zehe) → review+
Comment on attachment 385541 [details] [diff] [review]
patch4

This looks great and also works for me! r=me
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?
Attachment #385541 - Flags: review?(bolterbugz) → review+
(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
Summary: remove nsIAccessibleTreeCache → remove nsIAccessibleTreeCache and nsPIAccessibleTreeItem
landed on 1.9.2 - http://hg.mozilla.org/mozilla-central/rev/e97a9e879fc0
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
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).
OK, that's what I needed to know. I've removed the doc needed keyword.
Keywords: dev-doc-needed
You need to log in before you can comment on or make changes to this bug.