538633 - nsAccessible tree navigation methods should deal with nsAccessible pointers

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

nsAccessible tree navigation methods should work with nsAccessible pointers, otherwise it's necessary to query nsAccessible from returned nsIAccessible object in order to use nsAccessible tree navigation methods again.

Comment 1

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

Sounds good. Got a WIP?

Comment 2

[alexander :surkov (:asurkov)]

(In reply to comment #1)
> Sounds good. Got a WIP?

Yes. I hope to put the patch today.

Comment 3

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 4

[alexander :surkov (:asurkov)]

try server build is here http://build.mozilla.org/tryserver-builds/[e-mail]-try-a26ad599b96c

Comment 5

[[e-mail]]

Comment on attachment 420793 [details] [diff] [review]
patch

+  nsRefPtr<nsAccessible> rootAcc =
>+    nsAccUtils::QueryObject<nsAccessible>(aRootAccessible);
I've possibly asked in a previous review and since forgotten, but why not use nsAccUtils::QueryAccessible(aRootAccessible); ? In particular repeating the type name is ugly. I think you can work around it by using a class and some helper methods. At least, it keeps all the ugliness in one place:
template<class T>
class nsQueryObject
{
public:
  nsQueryObject(T* aRawPtr) : mRawPtr(aRawPtr) {}
  template<class U>
  operator already_AddRefed<U>() {
    U* object = nsnull;
    CallQueryInterface(mRawPtr, &object);
    return object;
  }
private:
  T* mRawPtr;
}
template<class T>
nsQueryObject<T> do_QueryObject(T* aRawPtr)
{
  return nsQueryObject<T>(aRawPtr);
}
template<class T>
nsQueryObject<T> do_QueryObject(nsCOMPtr<T> aCOMPtr)
{
  return nsQueryObject<T>(aCOMPtr);
}
template<class T>
nsQueryObject<T> do_QueryObject(nsRefPtr<T> aRefPtr)
{
  return nsQueryObject<T>(aRefPtr);
}
Then you can write things like
nsCOMPtr<nsIAccessibleHyperText> hyperTextParent(do_QueryObject(GetParent()));
or
nsRefPtr<nsAccessible> rootAcc(do_QueryObject(aRootAccessible));
[I wonder whether we should have this in xpcom/glue somewhere?]
[Note that if you do make this change, I'd want to sr again, so you'd probably prefer to do it in a separate bug.]

>+      nsRefPtr<nsAccessible> tmp = mChildren.ElementAt(0);
>+      mChildren.ElementAt(0) = child;
>+      mChildren.ElementAt(idx) = tmp;
My eyes, the goggles do nothing! Would you mind using [] instead of ElementAt? mChildren[0] = child; just looks right in a way ElementAt doesn't.

>   aText += nsDependentSubstring(mBulletText, aStartOffset, aLength);
[You're not supposed to create an nsDependentSubstring explicitly. Just call the Substring function to create one for you.]

>-  if (parent) {
>+  NS_ENSURE_TRUE(parent,);
Was this intentional? (NS_ENSURE_TRUE warns.)

>+  nsAccessible* table = mParent->GetParent();
>+  CallQueryInterface(table, aTable);
CallQueryInterface(mParent->GetParent(), aTable);

>   return IsDefunct() ? nsnull : mParent.get();
[Why .get()?]

Comment 6

[Ginn Chen]

(In reply to comment #5)
> >   return IsDefunct() ? nsnull : mParent.get();
> [Why .get()?]

It is required by some compilers, e.g. Sun Studio 12.
The error message is like this,
Error: Ambiguous "?:" expression, second operand of type "nsCOMPtr<nsIDOMNode>" and third operand of type "int" can be converted to one another.

However, Sun Studio 12u1 doesn't generate a warning or an error for this.
It deals 0 for a special case, like other compilers.

Comment 7

[alexander :surkov (:asurkov)]

(In reply to comment #5)

> [I wonder whether we should have this in xpcom/glue somewhere?]
> [Note that if you do make this change, I'd want to sr again, so you'd probably
> prefer to do it in a separate bug.]

that's worth idea, I'll file a bug to xpcom/glue and the patch.

Comment 8

[[e-mail]]

(In reply to comment #7)
> (In reply to comment #5)
> > [I wonder whether we should have this in xpcom/glue somewhere?]
> > [Note that if you do make this change, I'd want to sr again, so you'd probably
> > prefer to do it in a separate bug.]
By the way, they were supposed to be two separate comments, and probably would have made more sense in reverse order, i.e. I wasn't asking to sr xpcom/glue.

Comment 9

[alexander :surkov (:asurkov)]

(In reply to comment #8)
> (In reply to comment #7)
> > (In reply to comment #5)
> > > [I wonder whether we should have this in xpcom/glue somewhere?]
> > > [Note that if you do make this change, I'd want to sr again, so you'd probably
> > > prefer to do it in a separate bug.]
> By the way, they were supposed to be two separate comments, and probably would
> have made more sense in reverse order, i.e. I wasn't asking to sr xpcom/glue.

Sure.

Comment 10

[Marco Zehe (:MarcoZ)]

Comment on attachment 420793 [details] [diff] [review]
patch

r=me for functionality tests. I manually tested the try-server build, among other things I threw Boris' testcase from bug 522847 at it, which ran about 30% faster than with a regular nightly of Minefie.d Very nice!

Comment 11

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

Comment on attachment 420793 [details] [diff] [review]
patch

r=me dude, with Neil's comments addressed. I have no additional comments, you even remembered to rev NS_ACCESSIBLE_IMPL_CID :)

Comment 12

[alexander :surkov (:asurkov)]

(In reply to comment #5)

> >+      nsRefPtr<nsAccessible> tmp = mChildren.ElementAt(0);
> >+      mChildren.ElementAt(0) = child;
> >+      mChildren.ElementAt(idx) = tmp;
> My eyes, the goggles do nothing! Would you mind using [] instead of ElementAt?
> mChildren[0] = child; just looks right in a way ElementAt doesn't.

I agree.

> 
> >   aText += nsDependentSubstring(mBulletText, aStartOffset, aLength);
> [You're not supposed to create an nsDependentSubstring explicitly. Just call
> the Substring function to create one for you.]

I'll file the bug since I didn't change this code and it isnt' seemed related.

> 
> >-  if (parent) {
> >+  NS_ENSURE_TRUE(parent,);
> Was this intentional? (NS_ENSURE_TRUE warns.)

Yes, no parent is an error. I'd like to see warning so I have a chance to catch this some time.

Comment 13

[alexander :surkov (:asurkov)]

landed on 1.9.3 - http://hg.mozilla.org/mozilla-central/rev/8551655e7bac

Comment 14

[alexander :surkov (:asurkov)]

I filed bug 538964 for do_QueryObject.

Comment 15

[alexander :surkov (:asurkov)]

pushed mac bustage - http://hg.mozilla.org/mozilla-central/rev/502ed2576efc

Comment 16

[alexander :surkov (:asurkov)]

Attachment: substring patch

Neil, did you mean this, right?

Comment 17

[alexander :surkov (:asurkov)]

(In reply to comment #16)
> Created an attachment (id=421045) [details]
> substring patch
> 

landed - http://hg.mozilla.org/mozilla-central/rev/6ca5aac8eabf