Spell checker shouldn't copy the text node contents unnecessarily

RESOLVED FIXED in mozilla2.0b4

Status

()

Core
Spelling checker
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: Away for a while, Assigned: Away for a while)

Tracking

({perf})

Trunk
mozilla2.0b4
Points:
---

Firefox Tracking Flags

(blocking2.0 beta4+)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

7 years ago
Our spell checking code is pretty stupid!  (Well, that we all know!)

It just copies the text node around needlessly, which causes serious perf hits when editing textareas after bug 240933.  We can certainly do better than that, and I have a patch which does exactly that.
(Assignee)

Comment 1

7 years ago
Created attachment 461140 [details] [diff] [review]
Patch (v1)
Attachment #461140 - Flags: review?(roc)
(Assignee)

Updated

7 years ago
Summary: Spell checker shouldn't copy the text node unnecessarily → Spell checker shouldn't copy the text node contents unnecessarily
> mSoftText.AppendASCII(textFragment->Get1b() + firstOffsetInNode, len);

This is not correct. Get1b does not necessarily return ASCII. Why don't you use nsTextFragment::AppendTo here?
(ASCII only allows characters 0-127, but Get1b can contain characters 0-255.)

Seems like spellcheck (and editor!) need a general change from nsIDOMNode to nsIContent all over the place.
(Assignee)

Comment 4

7 years ago
Created attachment 461704 [details] [diff] [review]
Patch (v2)

(In reply to comment #2)
> > mSoftText.AppendASCII(textFragment->Get1b() + firstOffsetInNode, len);
> 
> This is not correct. Get1b does not necessarily return ASCII. Why don't you use
> nsTextFragment::AppendTo here?

It fails to link (because spell checker is not linked into libgklayout).  :(

But I switched to using AppendASCIItoUTF16 which seems to work (i.e. passes the reftests).  Is that good enough?
Attachment #461140 - Attachment is obsolete: true
Attachment #461704 - Flags: review?(roc)
Attachment #461140 - Flags: review?(roc)
(Assignee)

Comment 5

7 years ago
(In reply to comment #3)
> Seems like spellcheck (and editor!) need a general change from nsIDOMNode to
> nsIContent all over the place.

Yes!  But that is a huge project, which I've put off until after 2.0 for now... :/
How about just moving them into the header file so they can be inlined?
The definitions of AppendTo I mean
(Assignee)

Comment 8

7 years ago
Created attachment 462483 [details] [diff] [review]
Patch (v3)

Moved AppendTo to the header and started using it.
Attachment #461704 - Attachment is obsolete: true
Attachment #462483 - Flags: review?(roc)
Attachment #461704 - Flags: review?(roc)
Attachment #462483 - Flags: review?(roc) → review+
(Assignee)

Updated

7 years ago
Attachment #462483 - Flags: approval2.0?
(Assignee)

Comment 9

7 years ago
Created attachment 462925 [details] [diff] [review]
Patch (v4)

I actually made a mistake of appending the text to |str| instead of |mSoftText|.  This patch fixes that mistake (which was caught by unit tests on the try server).
Attachment #462483 - Attachment is obsolete: true
Attachment #462925 - Flags: approval2.0?
Attachment #462483 - Flags: approval2.0?

Updated

7 years ago
blocking2.0: --- → beta4+

Updated

7 years ago
Attachment #462925 - Flags: approval2.0?
(Assignee)

Comment 10

7 years ago
I landed the fix:

http://hg.mozilla.org/mozilla-central/rev/b64704446120

And then backed it out:

http://hg.mozilla.org/mozilla-central/rev/6941899e01f1

The problem (which the try server failed to catch) is that TestWinTSF.cpp #defines nsReadableUtils_h_, which prevents that header which declares AppendASCIItoUTF16 from being #included.  I'm testing the patch in bug 497812 to see if it fixes this problem.  If it doesn't, I think we need to go with the approach in attachment 461704 [details] [diff] [review].
Or disable that test..
(Assignee)

Comment 12

7 years ago
(In reply to comment #11)
> Or disable that test..

Given the way that test is written, I'd very much like to do that!  :-)

BTW, the patch in bug 497812 didn't help with this error, so I guess our choices are between disabling that test or taking attachment 461704 [details] [diff] [review].
(Assignee)

Comment 13

7 years ago
roc: ping?
Sure, disable it. We're not shipping TSF enabled at the moment.
(Assignee)

Comment 15

7 years ago
TestWinTSF test disabled:

http://hg.mozilla.org/mozilla-central/rev/12c4a1f5f22e

Patch landed again:

http://hg.mozilla.org/mozilla-central/rev/f8a0bd477fd3
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b4
You need to log in before you can comment on or make changes to this bug.