Closed
Bug 613919
Opened 14 years ago
Closed 14 years ago
Make characterCount fast
Categories
(Core :: Disability Access APIs, defect)
Core
Disability Access APIs
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)
(deleted),
patch
|
davidb
:
approval2.0+
|
Details | Diff | Splinter Review |
No description provided.
Attachment #492271 -
Flags: review?(fherrera)
Attachment #492271 -
Flags: review?(bolterbugz)
Assignee | ||
Comment 1•14 years ago
|
||
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 2•14 years ago
|
||
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+
Assignee | ||
Comment 3•14 years ago
|
||
(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.
Assignee | ||
Comment 4•14 years ago
|
||
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.
Assignee | ||
Comment 5•14 years ago
|
||
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
Comment 6•14 years ago
|
||
I believe you but please attach actually test cases ;)
Comment 7•14 years ago
|
||
(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•14 years ago
|
||
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.
Assignee | ||
Comment 9•14 years ago
|
||
(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•14 years ago
|
||
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+
Assignee | ||
Comment 11•14 years ago
|
||
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.
Assignee | ||
Comment 12•14 years ago
|
||
Attachment #492285 -
Attachment is obsolete: true
Assignee | ||
Updated•14 years ago
|
blocking2.0: --- → ?
Updated•14 years ago
|
blocking2.0: ? → final+
Comment 13•14 years ago
|
||
Why didn't this land?
Assignee | ||
Comment 14•14 years ago
|
||
Assignee | ||
Comment 15•14 years ago
|
||
(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.
Updated•14 years ago
|
Attachment #492344 -
Flags: approval2.0+
Updated•14 years ago
|
blocking2.0: final+ → -
Comment 16•14 years ago
|
||
Note: I wouldn't want this to land too late in the cycle.
Assignee | ||
Comment 17•14 years ago
|
||
(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.
Assignee | ||
Comment 18•14 years ago
|
||
landed on 2.0 - http://hg.mozilla.org/mozilla-central/rev/12e99b3942fe
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.
Description
•