Closed Bug 538633 Opened 10 years ago Closed 10 years ago

nsAccessible tree navigation methods should deal with nsAccessible pointers

Categories

(Core :: Disability Access APIs, defect)

x86
All
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: surkov, Assigned: surkov)

References

(Blocks 3 open bugs)

Details

(Keywords: access)

Attachments

(2 files)

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.
Blocks: 342045
Sounds good. Got a WIP?
(In reply to comment #1)
> Sounds good. Got a WIP?

Yes. I hope to put the patch today.
Attached patch patchSplinter Review
Assignee: nobody → surkov.alexander
Status: NEW → ASSIGNED
Attachment #420793 - Flags: superreview?(neil)
Attachment #420793 - Flags: review?(marco.zehe)
Attachment #420793 - Flags: review?(bolterbugz)
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()?]
Attachment #420793 - Flags: superreview?(neil) → superreview+
(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.
(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.
(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.
(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 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!
Attachment #420793 - Flags: review?(marco.zehe) → review+
Blocks: 522847
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 :)
Attachment #420793 - Flags: review?(bolterbugz) → review+
(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.
landed on 1.9.3 - http://hg.mozilla.org/mozilla-central/rev/8551655e7bac
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
I filed bug 538964 for do_QueryObject.
Attached patch substring patchSplinter Review
Neil, did you mean this, right?
Attachment #421045 - Flags: review?(neil)
Attachment #421045 - Flags: review?(neil) → review+
No longer blocks: 342045
Depends on: 342045
(In reply to comment #16)
> Created an attachment (id=421045) [details]
> substring patch
> 

landed - http://hg.mozilla.org/mozilla-central/rev/6ca5aac8eabf
Blocks: treea11y
You need to log in before you can comment on or make changes to this bug.