avoid extra traversal of children array during text event creation

RESOLVED FIXED

Status

()

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

People

(Reporter: surkov, Assigned: surkov)

Tracking

({access})

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment)

(Assignee)

Description

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

Comment 1

8 years ago
Created attachment 449859 [details] [diff] [review]
patch

try-server build is expected
Attachment #449859 - Flags: review?(marco.zehe)
Attachment #449859 - Flags: review?(bolterbugz)
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

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

Comment 4

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

Updated

8 years ago
Summary: avoid extra array traversal during text event creation → avoid extra traversal of children array during text event creation
(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?
(Assignee)

Comment 7

8 years ago
(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 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
Attachment #449859 - Flags: review?(bolterbugz) → review+
Average numbers
without patch: 10 113
with patch:    10  54
(Assignee)

Comment 10

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

Comment 11

8 years ago
landed on 1.9.3 - http://hg.mozilla.org/mozilla-central/rev/c8f859ac1b6c
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
(Assignee)

Updated

7 years ago
Depends on: 631160
You need to log in before you can comment on or make changes to this bug.