cache position in set and set size

RESOLVED FIXED

Status

()

Core
Disability Access APIs
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: surkov, Assigned: surkov)

Tracking

(Blocks: 1 bug, {access})

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

25.23 KB, patch
davidb
: review+
MarcoZ
: review+
neil@parkwaycc.co.uk
: superreview+
Details | Diff | Splinter Review
Comment hidden (empty)
(Assignee)

Comment 1

8 years ago
try server build - http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/surkov.alexander@gmail.com-3e6ebe020507/
(Assignee)

Comment 2

8 years ago
Created attachment 454834 [details] [diff] [review]
patch
Assignee: nobody → surkov.alexander
Status: NEW → ASSIGNED
Attachment #454834 - Flags: superreview?(neil)
Attachment #454834 - Flags: review?(marco.zehe)
Attachment #454834 - Flags: review?(bolterbugz)
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 on attachment 454834 [details] [diff] [review]
patch

r=me with mentioned nits.
Attachment #454834 - Flags: review?(bolterbugz) → review+
(Assignee)

Comment 5

8 years ago
(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?

Updated

8 years ago
Attachment #454834 - Flags: superreview?(neil) → superreview+

Comment 6

8 years ago
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?
(Assignee)

Comment 7

8 years ago
(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 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.
Attachment #454834 - Flags: review?(marco.zehe) → review+
(Assignee)

Comment 9

8 years ago
landed on 2.0 (1.9.3) - http://hg.mozilla.org/mozilla-central/rev/7584f12e17e3
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
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.
(Assignee)

Updated

8 years ago
Depends on: 576835
(Assignee)

Comment 11

8 years ago
I filed bug 576835 to address the warning.
You need to log in before you can comment on or make changes to this bug.