575052 - cache text offsets within hypertext accessible

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]-9a5301486a41/

Comment 2

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 3

[alexander :surkov (:asurkov)]

I've updated perf tracking table - https://wiki.mozilla.org/Accessibility/EventCoalescing#Performance_improvements_tracking_table (tested on try server build I pointed above).

Comment 4

[Marco Zehe (:MarcoZ)]

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

r=me for the tests and the functionality part. Great job!

Comment 5

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

(In reply to comment #3)
> I've updated perf tracking table -
> https://wiki.mozilla.org/Accessibility/EventCoalescing#Performance_improvements_tracking_table
> (tested on try server build I pointed above).

Very nice.

Comment 6

[[e-mail]]

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

>+
>+  } else {
Nit: strange place for a blank line

>+  PRInt32 offset = mOffsets.SafeElementAt(index - 1, -1);
>+  if (offset != -1) {
>+    if (aInvalidateAfter) {
>+      PRInt32 count = mOffsets.Length() - index;
>+      if (count > 0)
>+        mOffsets.RemoveElementsAt(index, count);
>+    }
>+    return offset;
>+  }
Rather than using SafeElementAt, I'd prefer checking count >= 0 like this:
PRInt32 count = mOffsets.Length() - index;
if (count >= 0) {
  if (count > 0 && aInvalidateAfter)
    mOffsets.RemoveElementsAt(index, count);
  return mOffsets[index - 1];
}
This also makes things minusculely easier for the optimiser ;-)

>+  PRUint32 childCount = GetChildCount();
>+  while (mOffsets.Length() < childCount && mOffsets.Length() < index) {
index is always less than mChildren.Length() [or childCount], right? So you shouldn't have to compare against childCount.

Comment 7

[alexander :surkov (:asurkov)]

(In reply to comment #6)
> (From update of attachment 454345 [details] [diff] [review])
> >+
> >+  } else {
> Nit: strange place for a blank line

the empty line is intended to separate visually end of if block from else block, it seems to me it's a bit more readable, I would safe this if you don't mind.

> Rather than using SafeElementAt, I'd prefer checking count >= 0 like this:
> PRInt32 count = mOffsets.Length() - index;
> if (count >= 0) {
>   if (count > 0 && aInvalidateAfter)
>     mOffsets.RemoveElementsAt(index, count);
>   return mOffsets[index - 1];
> }
> This also makes things minusculely easier for the optimiser ;-)
> 
> >+  PRUint32 childCount = GetChildCount();
> >+  while (mOffsets.Length() < childCount && mOffsets.Length() < index) {
> index is always less than mChildren.Length() [or childCount], right? So you
> shouldn't have to compare against childCount.

I like this, thank you.

Comment 8

[alexander :surkov (:asurkov)]

Attachment: patch2

Neil's comments are fixed

Comment 9

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

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

r=me.

>+nsHyperTextAccessible::GetChildOffset(nsAccessible* aChild,
>+                                      PRBool 

>+  EnsureChildren();

Good.

We should probably add more tests (later).

Comment 10

[alexander :surkov (:asurkov)]

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