Closed Bug 566298 Opened 10 years ago Closed 10 years ago

don't use nsIAccessible::GetNextSibling() to iterate children

Categories

(Core :: Disability Access APIs, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: surkov, Assigned: surkov)

References

(Blocks 2 open bugs)

Details

(Keywords: access)

Attachments

(1 file, 1 obsolete file)

Attached patch patch (obsolete) — Splinter Review
No description provided.
Attachment #445669 - Flags: superreview?(neil)
Attachment #445669 - Flags: review?(bolterbugz)
Can you start a try-server build for this?
(In reply to comment #1)
> Can you start a try-server build for this?

yeah, it's running already :)
Attached patch patch2Splinter Review
fix linux fail
Attachment #445669 - Attachment is obsolete: true
Attachment #445684 - Flags: superreview?(neil)
Attachment #445684 - Flags: review?(bolterbugz)
Attachment #445669 - Flags: superreview?(neil)
Attachment #445669 - Flags: review?(bolterbugz)
Comment on attachment 445684 [details] [diff] [review]
patch2

Great! r=me. Minor nit:

>+++ b/accessible/src/base/nsDocAccessible.cpp
>@@ -1798,19 +1798,18 @@ void nsDocAccessible::RefreshNodes(nsIDO

>-      // use GetChildren() to fetch children at one time, instead of using
>-      // GetNextSibling(), because after we shutdown the first child,
>-      // mNextSibling will be set null.
>+      // use GetChildren() to fetch children at one time, because after shutdown
>+      // the child references are cleared.

Nit: I prefer "to fetch all children at once, ..."

(I didn't review the xforms change closely)
Attachment #445684 - Flags: review?(bolterbugz) → review+
(In reply to comment #5)

> Nit: I prefer "to fetch all children at once, ..."

sure (just copying from prev comment)
 
> (I didn't review the xforms change closely)

though it makes sense to review (even we don't contribute into xforms any more, but it's not nice to break them ;).
(In reply to comment #7)
> (From update of attachment 445684 [details] [diff] [review])
> xforms change looks ok :)

thank you! :)
Comment on attachment 445669 [details] [diff] [review]
patch

>+      if (sibling == this) {
I spotted this, but you quickly posted a new patch ;-)

>+    nsCOMPtr<nsIDOMNode> childNode(child->GetDOMNode());
This doesn't compile for me. Am I missing some other patch?
(In reply to comment #9)
> (From update of attachment 445669 [details] [diff] [review])
> >+      if (sibling == this) {
> I spotted this, but you quickly posted a new patch ;-)

:)

> >+    nsCOMPtr<nsIDOMNode> childNode(child->GetDOMNode());
> This doesn't compile for me. Am I missing some other patch?

Yes, unfortunately it's built upon bug 566260, bug 566291 and bug 566293. I'm waiting for tree to check in them. Sorry.
(In reply to comment #10)

> > >+    nsCOMPtr<nsIDOMNode> childNode(child->GetDOMNode());
> > This doesn't compile for me. Am I missing some other patch?
> 
> Yes, unfortunately it's built upon bug 566260, bug 566291 and bug 566293. I'm
> waiting for tree to check in them. Sorry.

they were landed
Comment on attachment 445684 [details] [diff] [review]
patch2

>+  nsRefPtr<nsAccessible> accessible =
>+    aAccessible ? do_QueryObject(aAccessible) : this;
I don't think this does what you think it does. In fact it doesn't do what I think it does either ;-) What you actually get is
  nsRefPtr<nsAccessible> accessible = aAccessible ?
      do_QueryObject(aAccessible) :
      do_QueryObject(static_cast<nsIAccessible*>(this));
I don't believe this is what you want!
(In reply to comment #12)

>   nsRefPtr<nsAccessible> accessible = aAccessible ?
>       do_QueryObject(aAccessible) :
>       do_QueryObject(static_cast<nsIAccessible*>(this));
> I don't believe this is what you want!

Yep, I don't need to query "this". Should I do?
nsRefPtr<nsAccessible> accessible = aAccessible ?
  do_QueryObject(aAccessible) :
  static_cast<nsAccessible*>(this);
(In reply to comment #13)
Should I do?
> nsRefPtr<nsAccessible> accessible = aAccessible ?
>   do_QueryObject(aAccessible) :
>   static_cast<nsAccessible*>(this);
That will make no difference at all. The problem is that using ? : forces the compiler to use the same assignment operator both ways. [In this case the one it uses is operator=(nsQueryObject<nsIAccessible>&).]
Is it ok?

nsRefPtr<nsAccessible> accessible(do_QueryInterface(aAccessible));
if (!accessible)
  accessible = this;
Comment on attachment 445684 [details] [diff] [review]
patch2

sr=me with the ?: fix.
Attachment #445684 - Flags: superreview?(neil) → superreview+
landed on 1.9.3 with Neil's comment - http://hg.mozilla.org/mozilla-central/rev/49ca6b8821f2
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.