Lorien Hu (:lsocks) Reporter
	Description • 9 years ago

      No description provided.

	Trevor Saunders (:tbsaunde)
	Comment 2 • 9 years ago

Comment on attachment 8637229 [details] [diff] [review]
check proxy tree

>+void
>+DocAccessibleParent::CheckProxies()
>+{
>+  // Ensure that mAccessibles is correct.
>+  for (auto iter = mAccessibles.Iter(); !iter.Done(); iter.Next()) {
>+    ProxyAccessible* proxy = iter.Get()->mProxy;
>+
>+    // Check that this proxy's parent has it as a child.
>+    bool validParent = false;
>+    ProxyAccessible* parent = proxy->Parent();
>+    for (size_t i = 0; i < parent->ChildrenCount(); i++) {
>+      if (!mAccessibles.GetEntry(parent->ID()))
>+        MOZ_CRASH("found proxy unknown by mAccessibles as proxy's parent");

blank line please

and actually you should be able to hoist this out of the loop right?

>+      if (proxy == parent->ChildAt(i)) {
>+        validParent = true;
>+        break;
>+      }
>+    }
>+    if (!validParent)
>+      MOZ_CRASH("mAccessibles broken - proxy's parent does not have it as a child");
>+
>+    if (proxy->ChildrenCount() == 0)
>+      continue;
>+
>+    // Check that this proxy's children have it as its parent.
>+    bool validChildren = false;
>+    for (size_t i = 0; i < proxy->ChildrenCount(); i++) {
>+      ProxyAccessible* child = proxy->ChildAt(i);
>+      if (!mAccessibles.GetEntry(child->ID()))
>+        MOZ_CRASH("found proxy unknown by mAccessibles in proxy's children");

nit, blank line

>+      if (proxy == child->Parent()) {
>+        validChildren = true;
>+        break;

isn'this inverted? your checking that *1* child has parent as its parent, not that *all* of them do.

>+DocAccessibleParent::CheckProxyTree(ProxyAccessible* root)

aRoot

>+{
>+  size_t sum = 1;
>+  for (size_t i = 0; i < ChildrenCount(); i++) {

you mean root->ChildrenCount() right?

>+    ProxyAccessible* child = root->ChildAt(i);
>+    sum = sum + CheckProxyTree(child);

what does the var buy you?

>   void CheckDocTree() const;
>+  void CheckProxies();
>+  size_t CheckProxyTree(ProxyAccessible* root);

const for both

	Trevor Saunders (:tbsaunde)
	Comment 3 • 9 years ago

I think we could also add checks to RecvBindChildDoc() and maybe other Recv methods?

	Lorien Hu (:lsocks) Reporter
	Comment 7 • 9 years ago

Fixed an issue in the previous version where it was miscounting the proxies due to counting documents which aren't part of mAccessibles.

	David Bolter [:davidb] (NeedInfo me for attention)
	Comment 8 • 9 years ago

Trevor I see an old review request on the latest patch, still useful?

	Trevor Saunders (:tbsaunde)
	Comment 9 • 9 years ago

(In reply to David Bolter [:davidb] from comment #8)
> Trevor I see an old review request on the latest patch, still useful?

not really clear

	Lorien Hu (:lsocks) Reporter
	Comment 10 • 9 years ago

I think tbsaunde was fuzzing with this a few months ago, I'm not sure what the intention with it is re: tree integration.

	David Bolter [:davidb] (NeedInfo me for attention)
	Comment 11 • 9 years ago

Aha! OK thanks. (I get nag emails while the review request remains but that's ok)

	David Bolter [:davidb] (NeedInfo me for attention)
	Comment 12 • 8 years ago

Comment on attachment 8637353 [details] [diff] [review]
check proxy tree

Lorien, Trevor's thinking we can land this if you put it behind #ifdebug ...