613919 - Make characterCount fast

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 1

[alexander :surkov (:asurkov)]

Attachment: patch2

fix linux bustage

Comment 2

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

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

r=me with question and nits:

I notice sometimes you check defunctness after:
nsRefPtr<nsHyperTextAccessible> textAcc(do_QueryObject(this));

But sometimes you don't. What is the criteria?

>+++ b/accessible/tests/mochitest/text.js
>+/**
>+ * Rest characterCount for the given array of accessibles.
>+ */

"Test" right? :)

> 
>       SimpleTest.finish();
>     }
> 
>     SimpleTest.waitForExplicitFinish();
>     addA11yLoadEvent(doTest);
>   </script>
> </head>
>diff --git a/accessible/tests/mochitest/text/test_singleline.html b/accessible/tests/mochitest/text/test_singleline.html
>--- a/accessible/tests/mochitest/text/test_singleline.html
>+++ b/accessible/tests/mochitest/text/test_singleline.html

>+      testCharacterCount(15,
>+                         "input", kOk,
>+                         "div", kOk,
>+                         "textarea", kTodo);

(You could add a short comment about why textarea is kTodo, here and in test_whitespaces)

Comment 3

[alexander :surkov (:asurkov)]

(In reply to comment #2)
> Comment on attachment 492285 [details] [diff] [review]
> patch2
> 
> r=me with question and nits:
> 
> I notice sometimes you check defunctness after:
> nsRefPtr<nsHyperTextAccessible> textAcc(do_QueryObject(this));
> 
> But sometimes you don't. What is the criteria?

if XPCOM method is called then I don't since it should take care of this. If this is internal method where we don't have this check usually then I do.

> "Test" right? :)

thx

> (You could add a short comment about why textarea is kTodo, here and in
> test_whitespaces)

perhaps that's normal behavior for textarea, Fernando said textarea tends to append \n in the end so perhaps we get it here. I hope Fernando will comment this.

Comment 4

[alexander :surkov (:asurkov)]

a11yperf_fx4 "613919 characterCount" testcase: 74 sec, after 44. If I don't get characterCount twice then results are the same what's expected. So once we cached offsets, we got 30 seconds vs "0" now.

Comment 5

[alexander :surkov (:asurkov)]

a11yperf_fx4 "573719: caretOffset" testcase 79ms against 106ms. If there's no initial caching of text offsets, then results are the same. That makes sense for real web pages, since when you move the caret then you shouldn't recalculate all things.

marking blocking bug 573719 based on testing.

Comment 6

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

I believe you but please attach actually test cases ;)

Comment 7

[Fernando Herrera]

(In reply to comment #3)
> > (You could add a short comment about why textarea is kTodo, here and in
> > test_whitespaces)
> 
> perhaps that's normal behavior for textarea, Fernando said textarea tends to
> append \n in the end so perhaps we get it here. I hope Fernando will comment
> this.


yeah, we can split that into:

testCharacterCount(15,
                   "input", kOk,
                   "div", kOk):
testCharacterCount(16,
                   "textarea", kOk);

Comment 8

[Fernando Herrera]

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


> 
>+/**
>+ * Rest characterCount for the given array of accessibles.
>+ */
>+function testCharacterCount(aCount)
>+{
>+  for (var i = 1; i < arguments.length; i += 2) {
>+    var textacc = getAccessible(arguments[i], [nsIAccessibleText]);
>+    var isFunc = arguments[i + 1] == kOk ? is : todo_is;
>+    isFunc(textacc.characterCount, aCount,
>+           "Wrong character count for " + prettyName(arguments[i]));
>+  }
>+}

Please add here function documentation saying what arguments are expected for @param ... like in testText*Offset functions.

Comment 9

[alexander :surkov (:asurkov)]

(In reply to comment #8)

> Please add here function documentation saying what arguments are expected for
> @param ... like in testText*Offset functions.

sometimes you need to be a reviewer to notice this :) thanks.

Comment 10

[Fernando Herrera]

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

I haven't tested the GET_NSIACCESSIBLETEXT changes, but it looks ok.

Comment 11

[alexander :surkov (:asurkov)]

hm, on some machines it makes some todo tests pass - http://stage.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-aa632b22abf3/tryserver-win32/tryserver_win7_test-mochitest-other-build183.txt.gz

perhaps I should clean up a mess with text accessible update in bug 613058 and bug 498015 before this one.

Comment 12

[alexander :surkov (:asurkov)]

Attachment: patch3 [for landing]

Comment 13

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

Why didn't this land?

Comment 14

[alexander :surkov (:asurkov)]

(In reply to comment #13)
> Why didn't this land?

cause of comment 11.

Comment 15

[alexander :surkov (:asurkov)]

(In reply to comment #14)
> (In reply to comment #13)
> > Why didn't this land?
> 
> cause of comment 11.

I'll land it after bug 498015.

Comment 16

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

Note: I wouldn't want this to land too late in the cycle.

Comment 17

[alexander :surkov (:asurkov)]

(In reply to comment #16)
> Note: I wouldn't want this to land too late in the cycle.

That's great perf improvement. But we may report wrong character count if we have bad accessible tree. Note, mochitest are stable now when this patch is applied.

Comment 18

[alexander :surkov (:asurkov)]

landed on 2.0 - http://hg.mozilla.org/mozilla-central/rev/12e99b3942fe