582863 - Spell checker shouldn't copy the text node contents unnecessarily

Status: RESOLVED FIXED

(Core :: Spelling checker, defect)

Description

[(no longer active)]

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.

Comment 1

[(no longer active)]

Attachment: Patch (v1)

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

> mSoftText.AppendASCII(textFragment->Get1b() + firstOffsetInNode, len);

This is not correct. Get1b does not necessarily return ASCII. Why don't you use nsTextFragment::AppendTo here?

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 4

[(no longer active)]

Attachment: 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?

Comment 5

[(no longer active)]

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

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

How about just moving them into the header file so they can be inlined?

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

The definitions of AppendTo I mean

Comment 8

[(no longer active)]

Attachment: Patch (v3)

Moved AppendTo to the header and started using it.

Comment 9

[(no longer active)]

Attachment: 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).

Comment 10

[(no longer active)]

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

Comment 11

[Boris Zbarsky [:bzbarsky]]

Or disable that test..

Comment 12

[(no longer active)]

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

Comment 13

[(no longer active)]

roc: ping?

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Sure, disable it. We're not shipping TSF enabled at the moment.

Comment 15

[(no longer active)]

TestWinTSF test disabled:

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

Patch landed again:

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