The default bug view has changed. See this FAQ.

comb next/prev accessible methods

RESOLVED FIXED in mozilla7

Status

()

Core
Disability Access APIs
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: surkov, Assigned: surkov)

Tracking

(Blocks: 1 bug, {access})

unspecified
mozilla7
access
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

14.66 KB, patch
tbsaunde
: review+
Details | Diff | Splinter Review
(Assignee)

Description

6 years ago
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
(Assignee)

Comment 1

6 years ago
Created attachment 535557 [details] [diff] [review]
patch
Attachment #535557 - Flags: review?(trev.saunders)
(Assignee)

Comment 2

6 years ago
Created attachment 535559 [details] [diff] [review]
patch2

correct patch
Attachment #535557 - Attachment is obsolete: true
Attachment #535557 - Flags: review?(trev.saunders)
Attachment #535559 - Flags: review?(trev.saunders)
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.
Attachment #535559 - Flags: review?(trev.saunders)
(Assignee)

Comment 4

6 years ago
(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.
(Assignee)

Comment 5

6 years ago
Comment on attachment 535559 [details] [diff] [review]
patch2

rereview request
Attachment #535559 - Flags: review?
> 
> > 
> > > 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
Attachment #535559 - Flags: review? → review+
(Assignee)

Comment 7

6 years ago
(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.
(Assignee)

Comment 8

6 years ago
landed - http://hg.mozilla.org/mozilla-central/rev/88a8177cf358
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla7
You need to log in before you can comment on or make changes to this bug.