660153 - comb next/prev accessible methods

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

1) cached and not cached traverse methods are equivalent in terms no tree changes happens
2) make sibling methods const since no changes with accessible happens
3) move out IsDefunct() check from internal methods

Comment 1

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 2

[alexander :surkov (:asurkov)]

Attachment: patch2

correct patch

Comment 3

[Trevor Saunders (:tbsaunde)]

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


 NS_IMETHODIMP
> nsAccessible::GetNextSibling(nsIAccessible **aNextSibling) 
> {
>   NS_ENSURE_ARG_POINTER(aNextSibling);
>+  *aNextSibling = nsnull;
>+
>+  if (IsDefunct())
>+    return NS_ERROR_FAILURE;
> 
>   nsresult rv = NS_OK;
>   NS_IF_ADDREF(*aNextSibling = GetSiblingAtOffset(1, &rv));

we don't use NextSibling() here  because of the nsresult? if does js really need that? (I'm fine either way, but using Next / Prev Sibling() would be nice) 

> 
>   nsresult rv = NS_OK;
>   NS_IF_ADDREF(*aPreviousSibling = GetSiblingAtOffset(-1, &rv));

same

>+   */
>+  inline nsAccessible* NextSibling() const
>+    {  return GetSiblingAtOffset(1); }
>+  inline nsAccessible* PrevSibling() const
>+    { return GetSiblingAtOffset(-1); }

I think These methods will get non-cached accessibles where the methods they replace don't.  I'm not sure if that causes a problem or not, but I would be inclined to think not.

>   if (aError)
>     *aError = NS_OK; // fail peacefully
> 
>   return mParent->GetChildAt(GetIndexInParent() + aOffset);
> }

This means we could return null and *rv == NS_OK is that ok?

> nsXULTreeColumnsAccessible::GetSiblingAtOffset(PRInt32 aOffset,
>-                                               nsresult* aError)
>+                                               nsresult* aError) const
> {
>   if (aOffset < 0)
>     return nsXULColumnsAccessible::GetSiblingAtOffset(aOffset, aError);

uh, that if is really weird, is it correct or make sense somehow?

>       treeView->GetRowCount(&rowCount);
>       if (rowCount > 0 && aOffset <= rowCount) {
>-        nsRefPtr<nsXULTreeAccessible> treeAcc = do_QueryObject(mParent);
>+        nsRefPtr<nsXULTreeAccessible> treeAcc = do_QueryObject(GetParent());

why the change? and why do we need do_QueryObject() there?and do we need a ref pointer?

>-  nsRefPtr<nsXULTreeItemAccessibleBase> rowAcc = do_QueryObject(mParent);
>-
>+  nsRefPtr<nsXULTreeItemAccessibleBase> rowAcc = do_QueryObject(GetParent());

same

I think I'd like to see these comments addressed one way or another, but I'm not sure if we need another version of the patch, I supose that depends on what the answers are.

Comment 4

[alexander :surkov (:asurkov)]

(In reply to comment #3)
> Comment on attachment 535559 [details] [diff] [review] [review]
> patch2
> 
> 
>  NS_IMETHODIMP
> > nsAccessible::GetNextSibling(nsIAccessible **aNextSibling) 
> > {
> >   NS_ENSURE_ARG_POINTER(aNextSibling);
> >+  *aNextSibling = nsnull;
> >+
> >+  if (IsDefunct())
> >+    return NS_ERROR_FAILURE;
> > 
> >   nsresult rv = NS_OK;
> >   NS_IF_ADDREF(*aNextSibling = GetSiblingAtOffset(1, &rv));
> 
> we don't use NextSibling() here  because of the nsresult? if does js really
> need that? (I'm fine either way, but using Next / Prev Sibling() would be
> nice) 

> > 
> >   nsresult rv = NS_OK;
> >   NS_IF_ADDREF(*aPreviousSibling = GetSiblingAtOffset(-1, &rv));
> 
> same

You can use either version, on the one hand GetSiblingAtOffset doesn't hurt code readability here and on the another hand we should avoid trivial inline methods usage.

> >+   */
> >+  inline nsAccessible* NextSibling() const
> >+    {  return GetSiblingAtOffset(1); }
> >+  inline nsAccessible* PrevSibling() const
> >+    { return GetSiblingAtOffset(-1); }
> 
> I think These methods will get non-cached accessibles where the methods they
> replace don't.  I'm not sure if that causes a problem or not, but I would be
> inclined to think not.

term non-cached is no longer applied to accessibles because all accessibles are cached in means tree traversal can't trigger tree creation.

> 
> >   if (aError)
> >     *aError = NS_OK; // fail peacefully
> > 
> >   return mParent->GetChildAt(GetIndexInParent() + aOffset);
> > }
> 
> This means we could return null and *rv == NS_OK is that ok?

yes, error when something is really wrong (no sibling shouldn't set error state), keep in mind this method with nsresult is used for XPCOM GetNext/PrevSiblings().

> 
> > nsXULTreeColumnsAccessible::GetSiblingAtOffset(PRInt32 aOffset,
> >-                                               nsresult* aError)
> >+                                               nsresult* aError) const
> > {
> >   if (aOffset < 0)
> >     return nsXULColumnsAccessible::GetSiblingAtOffset(aOffset, aError);
> 
> uh, that if is really weird, is it correct or make sense somehow?

not weird, just implementation details, the XUL tree accessible mixes accessibles from DOM and from nsITreeView. Next sibling of columns accessible is accessible from nsITreeView, previous can be from DOM only.

> 
> >       treeView->GetRowCount(&rowCount);
> >       if (rowCount > 0 && aOffset <= rowCount) {
> >-        nsRefPtr<nsXULTreeAccessible> treeAcc = do_QueryObject(mParent);
> >+        nsRefPtr<nsXULTreeAccessible> treeAcc = do_QueryObject(GetParent());
> 
> why the change?

a trick, can't use mParent because the method is const.

> and why do we need do_QueryObject() there?

to cast nsAccessible to nsXULTreeAccessible, later it should be replaced on new casting approach, i.e. AsXULTree.

> and do we need a
> ref pointer?

QueryInterface always addrefs.

Comment 5

[alexander :surkov (:asurkov)]

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

rereview request

Comment 6

[Trevor Saunders (:tbsaunde)]

> 
> > 
> > > nsXULTreeColumnsAccessible::GetSiblingAtOffset(PRInt32 aOffset,
> > >-                                               nsresult* aError)
> > >+                                               nsresult* aError) const
> > > {
> > >   if (aOffset < 0)
> > >     return nsXULColumnsAccessible::GetSiblingAtOffset(aOffset, aError);
> > 
> > uh, that if is really weird, is it correct or make sense somehow?
> 
> not weird, just implementation details, the XUL tree accessible mixes
> accessibles from DOM and from nsITreeView. Next sibling of columns
> accessible is accessible from nsITreeView, previous can be from DOM only.

Ok, fine by me then, but I'd love a comment somwhere describing how these children are layed out.  If I understand right your saying the layout is DOM children | columns accessible | tree children (rows and such)

btw what is the coluns accessible representing?


> 
> > 
> > >       treeView->GetRowCount(&rowCount);
> > >       if (rowCount > 0 && aOffset <= rowCount) {
> > >-        nsRefPtr<nsXULTreeAccessible> treeAcc = do_QueryObject(mParent);
> > >+        nsRefPtr<nsXULTreeAccessible> treeAcc = do_QueryObject(GetParent());
> > 
> > why the change?
> 
> a trick, can't use mParent because the method is const.

hmm, so C++ doesn't let you use member variables at all? isn't your use there read only?

> 
> > and why do we need do_QueryObject() there?
> 
> to cast nsAccessible to nsXULTreeAccessible, later it should be replaced on
> new casting approach, i.e. AsXULTree.

ok

btw I assume you don't want to just take it as an invariant that the parent of this type is a xul tree accessible,  and use 
static_cast<nsXULTreeAccessible> or some other cast type?

> 
> > and do we need a
> > ref pointer?
> 
> QueryInterface always addrefs.

ok

Comment 7

[alexander :surkov (:asurkov)]

(In reply to comment #6)

> Ok, fine by me then, but I'd love a comment somwhere describing how these
> children are layed out.  If I understand right your saying the layout is DOM
> children | columns accessible | tree children (rows and such)

I think it should be documented somewhere.

> btw what is the coluns accessible representing?

columns container (list)

> hmm, so C++ doesn't let you use member variables at all? isn't your use
> there read only?

QueryInterface isn't const, so you can't use it on const pointer.

> btw I assume you don't want to just take it as an invariant that the parent
> of this type is a xul tree accessible,  and use 
> static_cast<nsXULTreeAccessible> or some other cast type?

that's one way but I prefer safe casting.

Comment 8

[alexander :surkov (:asurkov)]

landed - http://hg.mozilla.org/mozilla-central/rev/88a8177cf358