345112 - Make the inline spellchecker work incrementally

Status: RESOLVED FIXED

(Core :: Spelling checker, defect, P1)

Description

[Brett Wilson]

When the document is long, and text is inserted or the textarea is initialized with text, we should do the spellchecking in parts. We should work for X milliseconds and check as many words as possible in that time. Then we should save where we were and post a message. When we get the message, we will run for another X milliseconds. This process is repeated until all text is checked or we meet some sanity threshold.

It is possible for the DOM inside the textbox to change out from under us. If our current node is deleted, we can just start over. There are potentially other problems, but they are not very likely, and less important than the benefit we get here.

Comment 1

[Brett Wilson]

Attachment: Patch V1

This patch does asynchronous spellchecking. Here's how it works:

When we're checking, we check every N words the time, and if 50ms has elapsed, we stop and post a plevent with the data necessary to continue checking, including where we left off. When we get the event, we try to continue spellchecking where we left off for another 50ms and repeat.

It makes the turn-on-spellcheck performance excellent and I think is a great user experience.

It is possible for the DOM to change out from under us. For example, our remembered node could have been deleted from the DOM, or text could have been inserted or removed from that node so our offset isn't correct any more.

In this patch, if there is an error, I just stop checking and give up. roc and I talked earlier about restarting from the beginning, but I was concerned about this. It would be weird to clear all the words and then put them back. This is easy to change, I would be inclined to get this in ASAP because it needs lots of baking.

Other problems are if text is inserted in the node while we're away. This means we'll skip that many characters in the spellcheck. Because we always move to the beginning of the current word, you would have had to type more than one word in this time. The other possibility is deleting text, which will cause some things to be checked twice. An optimization means that we won't check for existing ranges in this case, potentially causing overlapping ranges.

One idea is to store the length of the node we stopped on. If it changed, start checking again from the beginning of the node. This should make these problems almost never happen.

I also changed a little bit how many words we will check at once. It used to be that when something happened (like loading a page with a lot of text in a textarea), we would just check until the maximum number of misspelled words is reached. This patch changes the number of words we'll mark wrong to be 3/4 the maximum. This means we'll stop earlier.

The use case of this is that when you load a large patch/Wikipedia article, you will run out of misspellings, but you also want to type. This leaves you 1/4 of the maximum to use for test you add, which is nice, because you really want to check the text you're writing.

This may have some funny effects, though, because we'll still check the text as your arrow moves over it. If you arrow through the area that was left unchecked, you'll get spellcheck markings, leaving people to think it's broken. Previously, the maximum # misspellings would be reached and you would never get any more markings, leaving people to think it's broken. I prefer the new behavior. It makes, for example, editing patches much better (where I'm adding text to the top).

Comment 2

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

+class mozInlineSpellStatus
+{
+public:

This needs more explanation. I actually don't know why you're not strongly referencing all the ranges here.

+      wordsSinceTimeCheck += 4; // misspelled words count extra toward out max

Make a proper constant please, we don't want magic numbers dancing around.

+    if (wordsSinceTimeCheck >= INLINESPELL_TIMEOUT_CHECK_FREQUENCY) {

so once you reach that threshold, we do a time check after every word?

+   printf("   resuming\n");

Remove

Comment 3

[Brian Ryner (not reading)]

Comment on attachment 230679 [details] [diff] [review]
Patch V1

>Index: mozInlineSpellChecker.h
>--- mozInlineSpellChecker.h	21 Jul 2006 16:32:39 -0000	1.2.4.4
>+++ mozInlineSpellChecker.h	26 Jul 2006 00:54:33 -0000
>+class mozInlineSpellStatus
>+{
>+public:
>+  mozInlineSpellStatus(mozInlineSpellChecker* aSpellChecker,
>+                       nsIDOMRange* aRange,
>+                       nsIDOMRange* aNoCheckRange,
>+                       nsIDOMRange* aCreatedRange);
>+
>+  nsRefPtr<mozInlineSpellChecker> mSpellChecker;
>+
>+  nsCOMPtr<nsIDOMRange> mRange;
>+  nsIDOMRange* mNoCheckRange;
>+  nsIDOMRange* mCreatedRange;

Can you just briefly mention who owns these pointers, since this object doesn't?

>+  // How many misspellings we can add at once. This is often less than the max
>+  // total number of misspellings. When you have a large textarea prepopulated
>+  // with text with many misspellings, we can hit this limit. By making it
>+  // lower than the total number of misspelled words, new text typed by the
>+  // user can also have spellchecking in it.
>+  PRInt32 mMaxNumMisspellingsToAddAtOnce;
> 

This doesn't seem to be used anywhere... ?



>Index: mozInlineSpellChecker.cpp
>+static NS_DEFINE_CID(kEventQueueService, NS_EVENTQUEUESERVICE_CID);

Is there a particular reason to use CID rather than contract id?


> // mozInlineSpellChecker::DoSpellCheck

>+    // see if we've run out of time, only check every N words for perf
>+    if (wordsSinceTimeCheck >= INLINESPELL_TIMEOUT_CHECK_FREQUENCY) {
>+      if (PR_Now() > beginTime + INLINESPELL_CHECK_TIMEOUT * 1000) {

This can be PR_USEC_PER_MSEC, which makes the units clearer.  It might also help to add unit suffixes onto your variable/constant names, up to you.

Plus what roc said.  Looks good otherwise.

Comment 4

[Brett Wilson]

Attachment: Patch V2

This patch addresses all the reviewers' comments above.

Comment 5

[Mike Beltzner [:beltzner, not reading bugmail]]

Brett, let's get this in on trunk ASAP and ask people to hammer away at it a little bit so by tomorrow's triage we can feel a little safer about taking it on branch.

Comment 6

[Brett Wilson]

Attachment: Trunk patch

Comment 7

[Brett Wilson]

Fixed on trunk, leaving open for branch checkin

Comment 8

[Brett Wilson]

Comment on attachment 231664 [details] [diff] [review]
Patch V2

The branch portion of this bug is included in the branch patch to bug 344560, so I'm clearing the request here.

Comment 9

[Brett Wilson]

The branch patch was incorporated into the patch for bug 344560 which was fixed.