Closed Bug 613919 Opened 14 years ago Closed 14 years ago

Make characterCount fast

Categories

(Core :: Disability Access APIs, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla2.0b11
Tracking Status
blocking2.0 --- -

People

(Reporter: surkov, Assigned: surkov)

References

Details

(Keywords: access, perf)

Attachments

(1 file, 2 obsolete files)

Attached patch patch (obsolete) (deleted) — Splinter Review
No description provided.
Attachment #492271 - Flags: review?(fherrera)
Attachment #492271 - Flags: review?(bolterbugz)
Attached patch patch2 (obsolete) (deleted) — Splinter Review
fix linux bustage
Attachment #492271 - Attachment is obsolete: true
Attachment #492285 - Flags: review?(fherrera)
Attachment #492285 - Flags: review?(bolterbugz)
Attachment #492271 - Flags: review?(fherrera)
Attachment #492271 - Flags: review?(bolterbugz)
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)
Attachment #492285 - Flags: review?(bolterbugz) → review+
(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.
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.
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.
Blocks: 573719
I believe you but please attach actually test cases ;)
(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 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.
(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 on attachment 492285 [details] [diff] [review] patch2 I haven't tested the GET_NSIACCESSIBLETEXT changes, but it looks ok.
Attachment #492285 - Flags: review?(fherrera) → review+
hm, on some machines it makes some todo tests pass - http://stage.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/surkov.alexander@gmail.com-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.
Attached patch patch3 [for landing] (deleted) — Splinter Review
Attachment #492285 - Attachment is obsolete: true
blocking2.0: --- → ?
blocking2.0: ? → final+
(In reply to comment #13) > Why didn't this land? cause of comment 11.
(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.
blocking2.0: final+ → -
Note: I wouldn't want this to land too late in the cycle.
(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.
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b11
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: