575576 - cache position in set and set size

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Comment 1

[alexander :surkov (:asurkov)]

try server build - http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-3e6ebe020507/

Comment 2

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 3

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

Comment on attachment 454834 [details] [diff] [review]
patch

Are we sure this is going to give us a performance win for mutating tables?

>+  nsAccessible* GetConceptualParent() const { return mParent; }

Interesting name.

>+  static AccGroupInfo* CreateGroupInfo(nsAccessible* aAccessible)

>+        role != nsIAccessibleRole::ROLE_ROW &&
>+        role != nsIAccessibleRole::ROLE_GRID_CELL)

Nit: I'd be tempted to check grid cell and row first just because we know there can be so many of them, and it might be nice to hit them and fail the check early.

Comment 4

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

Comment on attachment 454834 [details] [diff] [review]
patch

r=me with mentioned nits.

Comment 5

[alexander :surkov (:asurkov)]

(In reply to comment #3)
> (From update of attachment 454834 [details] [diff] [review])
> Are we sure this is going to give us a performance win for mutating tables?

no. it's not about mutation, it's about group position getting.

> >+  nsAccessible* GetConceptualParent() const { return mParent; }
> 
> Interesting name.

ideas?

Comment 6

[[e-mail]]

Comment on attachment 454834 [details] [diff] [review]
patch

>+  // Previous sibling of parent group is a tree item, this is the
>+  // conceptual tree item parent.
>+  if (parentPrevSiblingRole == nsIAccessibleRole::ROLE_OUTLINEITEM)
>+    mParent = parentPrevSibling;
>+}
So mParent can be null if all of these checks fail?

Comment 7

[alexander :surkov (:asurkov)]

(In reply to comment #6)

> So mParent can be null if all of these checks fail?

Yes. It's used for node_child_of relation which exposes specific kind of parent which might require heavy computations.

Comment 8

[Marco Zehe (:MarcoZ)]

Comment on attachment 454834 [details] [diff] [review]
patch

r=me for the functionality. This improves the table mutation test run with NVDA from an average of 106000/106000 MS to 35400/35350 MS.

Comment 9

[alexander :surkov (:asurkov)]

landed on 2.0 (1.9.3) - http://hg.mozilla.org/mozilla-central/rev/7584f12e17e3

Comment 10

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

This patch caused the following build warning:

../../../dist/include/nsAutoPtr.h: In member function ‘void nsAutoPtr<T>::assign(T*) [with T = AccGroupInfo]’:
../../../dist/include/nsAutoPtr.h:134:   instantiated from ‘nsAutoPtr<T>& nsAutoPtr<T>::operator=(T*) [with T = AccGroupInfo]’
../../../dist/include/nsAccessible.h:332:   instantiated from here
../../../dist/include/nsAutoPtr.h:71: warning: possible problem detected in invocation of delete operator:
../../../dist/include/nsAutoPtr.h:69: warning: ‘oldPtr’ has incomplete type
../../../dist/include/nsAccessible.h:55: warning: forward declaration of ‘struct AccGroupInfo’
../../../dist/include/nsAutoPtr.h:71: note: neither the destructor nor the class-specific operator delete will be called, even if they are declared when the class is defined.

because AccGroupInfo.h isn't included by nsAccessible.h.

Comment 11

[alexander :surkov (:asurkov)]

I filed bug 576835 to address the warning.