Last Comment Bug 660153 - comb next/prev accessible methods
: comb next/prev accessible methods
Status: RESOLVED FIXED
: access
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla7
Assigned To: alexander :surkov
:
Mentors:
Depends on:
Blocks: cleana11y
  Show dependency treegraph
 
Reported: 2011-05-26 22:39 PDT by alexander :surkov
Modified: 2011-06-06 19:31 PDT (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (14.56 KB, patch)
2011-05-26 22:39 PDT, alexander :surkov
no flags Details | Diff | Splinter Review
patch2 (14.66 KB, patch)
2011-05-26 23:02 PDT, alexander :surkov
tbsaunde+mozbugs: review+
Details | Diff | Splinter Review

Description alexander :surkov 2011-05-26 22:39:28 PDT
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 2011-05-26 22:39:59 PDT
Created attachment 535557 [details] [diff] [review]
patch
Comment 2 alexander :surkov 2011-05-26 23:02:39 PDT
Created attachment 535559 [details] [diff] [review]
patch2

correct patch
Comment 3 Trevor Saunders (:tbsaunde) 2011-06-03 20:09:42 PDT
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 2011-06-06 00:13:51 PDT
(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 2011-06-06 18:07:32 PDT
Comment on attachment 535559 [details] [diff] [review]
patch2

rereview request
Comment 6 Trevor Saunders (:tbsaunde) 2011-06-06 18:35:23 PDT
> 
> > 
> > > 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 2011-06-06 19:21:44 PDT
(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 2011-06-06 19:31:43 PDT
landed - http://hg.mozilla.org/mozilla-central/rev/88a8177cf358

Note You need to log in before you can comment on or make changes to this bug.