570710 - avoid extra traversal of children array during text event creation

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

GetText() in nsAccTextChangeEvent constructor makes us to traverse children array for second time.

Comment 1

[alexander :surkov (:asurkov)]

Attachment: patch

try-server build is expected

Comment 2

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

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

> nsAccTextChangeEvent::
>   nsAccTextChangeEvent(nsIAccessible *aAccessible,
>-                       PRInt32 aStart, PRUint32 aLength, PRBool aIsInserted,
>+                       PRInt32 aStart, PRUint32 aLength,
>+                       nsAString& aModifiedText, PRBool aIsInserted,
>                        PRBool aIsAsynch, EIsFromUserInput aIsFromUserInput) :
>   nsAccEvent(aIsInserted ? nsIAccessibleEvent::EVENT_TEXT_INSERTED : nsIAccessibleEvent::EVENT_TEXT_REMOVED,
>              aAccessible, aIsAsynch, aIsFromUserInput, eAllowDupes),
>-  mStart(aStart), mLength(aLength), mIsInserted(aIsInserted)
>+  mStart(aStart), mLength(aLength), mIsInserted(aIsInserted),
>+  mModifiedText(aModifiedText)
> {
>-#ifdef XP_WIN
>-  nsCOMPtr<nsIAccessibleText> textAccessible = do_QueryInterface(aAccessible);
>-  NS_ASSERTION(textAccessible, "Should not be firing test change event for non-text accessible!!!");
>-  if (textAccessible) {
>-    textAccessible->GetText(aStart, aStart + aLength, mModifiedText);
>-  }
>-#endif
> }
>

I don't understand this change. It makes Linux and Mac less performant right?

Comment 3

[Marco Zehe (:MarcoZ)]

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

r=me with one nit:
>+    // Expose imaginary embedded object character if the accessible hans't

"has no" instead of the mistyped "hasn't".

Thanks for also preparing the tests for the other bug! r=me stands unless try-server build reveals errors.

Comment 4

[alexander :surkov (:asurkov)]

(In reply to comment #2)
> I don't understand this change. It makes Linux and Mac less performant right?

Absolutely. But it must be worth sacrifice.

Comment 5

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

(In reply to comment #0)
> GetText() in nsAccTextChangeEvent constructor makes us to traverse children
> array for second time.

I'm not familiar with how this happens. Can you explain?

Comment 6

[alexander :surkov (:asurkov)]

http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-7b292e6c3dc5/tryserver-win32/

Comment 7

[alexander :surkov (:asurkov)]

(In reply to comment #5)
> (In reply to comment #0)
> > GetText() in nsAccTextChangeEvent constructor makes us to traverse children
> > array for second time.
> 
> I'm not familiar with how this happens. Can you explain?

GetText() finds children by offsets iterating through array and get text from them.

Comment 8

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

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

r=me thanks!

> nsresult
> nsAccessible::AppendTextTo(nsAString& aText, PRUint32 aStartOffset, PRUint32 aLength)
> {
>+  // Return text representation of not text accessible within hypertext

Please change "not text" to "non-text".

>+++ b/accessible/src/html/nsHyperTextAccessible.cpp
>@@ -426,16 +426,17 @@ nsHyperTextAccessible::GetPosAndText(PRI
>       // Embedded object, append marker
>       // XXX Append \n for <br>'s
>       if (startOffset >= 1) {
>         -- startOffset;
>       }
>       else {
>         if (endOffset > 0) {
>           if (aText) {
>+            // XXX: should use nsIAccessible::AppendTextTo.

Bug filed?

(The testing design worries me a bit in terms of making this a bit harder to debug oranges etc. but ok)

I tested trunk with and without your patch. There was of course no impact without a11y, and with a11y, as expected, your changes cut the processing time in half. Nice.
http://people.mozilla.com/~dbolter/perf/textchangeevents.html

Comment 9

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

Average numbers
without patch: 10 113
with patch:    10  54

Comment 10

[alexander :surkov (:asurkov)]

(In reply to comment #8)
> >+            // XXX: should use nsIAccessible::AppendTextTo.
> 
> Bug filed?

We have couple bugs about text problems, all of them should end in rethinking GetPosAndText and relatives. This should be a part of it.

Comment 11

[alexander :surkov (:asurkov)]

landed on 1.9.3 - http://hg.mozilla.org/mozilla-central/rev/c8f859ac1b6c