Closed
Bug 538633
Opened 15 years ago
Closed 15 years ago
nsAccessible tree navigation methods should deal with nsAccessible pointers
Categories
(Core :: Disability Access APIs, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: surkov, Assigned: surkov)
References
(Blocks 3 open bugs)
Details
(Keywords: access)
Attachments
(2 files)
(deleted),
patch
|
MarcoZ
:
review+
davidb
:
review+
neil
:
superreview+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
neil
:
review+
|
Details | Diff | Splinter Review |
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•15 years ago
|
||
Sounds good. Got a WIP?
Assignee | ||
Comment 2•15 years ago
|
||
(In reply to comment #1)
> Sounds good. Got a WIP?
Yes. I hope to put the patch today.
Assignee | ||
Comment 3•15 years ago
|
||
Assignee: nobody → surkov.alexander
Status: NEW → ASSIGNED
Attachment #420793 -
Flags: superreview?(neil)
Attachment #420793 -
Flags: review?(marco.zehe)
Attachment #420793 -
Flags: review?(bolterbugz)
Assignee | ||
Comment 4•15 years ago
|
||
try server build is here http://build.mozilla.org/tryserver-builds/surkov.alexander@gmail.com-try-a26ad599b96c
Comment 5•15 years ago
|
||
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.
Assignee | ||
Comment 7•15 years ago
|
||
(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•15 years ago
|
||
(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.
Assignee | ||
Comment 9•15 years ago
|
||
(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•15 years ago
|
||
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+
Comment 11•15 years ago
|
||
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+
Assignee | ||
Comment 12•15 years ago
|
||
(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.
Assignee | ||
Comment 13•15 years ago
|
||
landed on 1.9.3 - http://hg.mozilla.org/mozilla-central/rev/8551655e7bac
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 14•15 years ago
|
||
I filed bug 538964 for do_QueryObject.
Assignee | ||
Comment 15•15 years ago
|
||
pushed mac bustage - http://hg.mozilla.org/mozilla-central/rev/502ed2576efc
Assignee | ||
Comment 16•15 years ago
|
||
Neil, did you mean this, right?
Attachment #421045 -
Flags: review?(neil)
Updated•15 years ago
|
Attachment #421045 -
Flags: review?(neil) → review+
Assignee | ||
Updated•15 years ago
|
Assignee | ||
Comment 17•15 years ago
|
||
(In reply to comment #16)
> Created an attachment (id=421045) [details]
> substring patch
>
landed - http://hg.mozilla.org/mozilla-central/rev/6ca5aac8eabf
You need to log in
before you can comment on or make changes to this bug.
Description
•