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

try-server build is expected

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 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.

(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.

(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?

(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

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

(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.

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