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

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 1

[Marco Zehe (:MarcoZ)]

Can you start a try-server build for this?

Comment 2

[alexander :surkov (:asurkov)]

(In reply to comment #1)
> Can you start a try-server build for this?

yeah, it's running already :)

Comment 3

[alexander :surkov (:asurkov)]

try server build (it's not finished entirely yet) http://build.mozilla.org/tryserver-builds/[e-mail]-try-20399f8d8129

Comment 4

[alexander :surkov (:asurkov)]

Attachment: patch2

fix linux fail

Comment 5

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

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)

Comment 6

[alexander :surkov (:asurkov)]

(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 ;).

Comment 7

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

Comment on attachment 445684 [details] [diff] [review]
patch2

xforms change looks ok :)

Comment 8

[alexander :surkov (:asurkov)]

(In reply to comment #7)
> (From update of attachment 445684 [details] [diff] [review])
> xforms change looks ok :)

thank you! :)

Comment 9

[[e-mail]]

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?

Comment 10

[alexander :surkov (:asurkov)]

(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.

Comment 11

[alexander :surkov (:asurkov)]

(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 12

[[e-mail]]

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!

Comment 13

[alexander :surkov (:asurkov)]

(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);

Comment 14

[[e-mail]]

(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>&).]

Comment 15

[alexander :surkov (:asurkov)]

Is it ok?

nsRefPtr<nsAccessible> accessible(do_QueryInterface(aAccessible));
if (!accessible)
  accessible = this;

Comment 16

[[e-mail]]

Comment on attachment 445684 [details] [diff] [review]
patch2

sr=me with the ?: fix.

Comment 17

[alexander :surkov (:asurkov)]

landed on 1.9.3 with Neil's comment - http://hg.mozilla.org/mozilla-central/rev/49ca6b8821f2