168999 - Split spellchecker implementation out of nsIEditorShell

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[Akkana Peck]

nsIEditorShell implements lots of spell checking commands.  They need to be
split off into a separate interface as part of editorshell removal.

Comment 1

[Akkana Peck]

Attachment: Do it

This works, and should be ready for review.  Kathy, if you could check the mac
build file, I'd be most grateful.  (The changes are all in the moz tree and
shouldn't need a commercial build to test whether it builds correctly, though
testing the functionality does require spellchecker libraries.)

Comment 2

[Akkana Peck]

Attachment: Do it and don't spew

Whoops, my tree still had some printfs associated with debugging some mail
issues (thanks, Kathy).  Here's the same patch without printfs.

Comment 3

[Kathleen :Brade]

Comment on attachment 102596 [details] [diff] [review]
Do it and don't spew

r=brade (and it builds on Mac)

Comment 4

[kinmoz]

Comment on attachment 102596 [details] [diff] [review]
Do it and don't spew

sr=[e-mail]

==== Do we still need the TextServices related headers?

 #include "nsTextServicesCID.h"
 #include "nsITextServicesDocument.h"
-#include "nsISpellChecker.h"


==== Can we line up the comments?

 #include "nsEditorShell.h"		      // for the CID
 #include "nsEditingSession.h"	     // for the CID
 #include "nsComposerController.h"		// for the CID
+#include "nsEditorSpellCheck.h"     // for the CID


==== Fix the indentation:

	    nsEditingSession.cpp		\
	    nsComposerCommandsUpdater.cpp	\
	   nsEditorService.cpp		  \
+	   nsEditorSpellCheck.cpp		  \
	    $(NULL)


==== Fix the indentation of the last line:


+  rv = nsComponentManager::CreateInstance(NS_SPELLCHECKER_CONTRACTID,
+					   nsnull,
+					   NS_GET_IID(nsISpellChecker),
+					(void
**)getter_AddRefs(mSpellChecker));

Comment 5

[Akkana Peck]

Attachment: Fix the indentation problems Kin noticed

Most of these indentation problems were due to existing tabs in the lines I
moved.	I've converted those tabs to spaces.

The indentation of the last line was intentional, to keep the line under 80
columns.  Kin said it was okay to keep that.

Comment 6

[Akkana Peck]

Comment on attachment 102617 [details] [diff] [review]
Fix the indentation problems Kin noticed

Transferring r= and sr= from previous patch.

I tested this with the mozdev spellchecker, and basic spellchecking works
(cool!).  Recheck Document crashes in nsAVLTree code, but I think that's
because the patch referenced on spellchecker.mozdev.org is out of sync with the
current source in mozdev.org CVS, and not anything to do with this patch.

Comment 7

[Aleksey Nogin]

I've build with spellchecker and this patch on Red Hat 8.0, everything works
fine, even recheck (I believe the crash is related to some state not being
initialized in a local profile, which is not the case for me).

Comment 8

[Asa Dotzler [:asa]]

Comment on attachment 102617 [details] [diff] [review]
Fix the indentation problems Kin noticed

a=asa for checkin to 1.2beta (on behalf of drivers)

Comment 9

[Akkana Peck]

Fix is in.

Comment 10

[Kathleen :Brade]

v

Comment 11

[Aleksey Nogin]

Actually, this may have added a nasty problem (at least with mozdev spellchecker). 

Reproducible: 100% :-(
1) I compose an email (with typos)
2) Click "Send" -> a spellchecker pops up hioghlighting the first typo
3) Click "Stop"

Expected: Spellchecker window goes away, the composition window is "unblocked"

Actual:

a) First, you get a JavaScript exception:
Error: uncaught exception: [Exception... "Component returned failure code:
0x80004002 (NS_NOINTERFACE) [nsIEditorSpellCheck.UninitSpellChecker]"  nsresult:
"0x80004002 (NS_NOINTERFACE)"  location: "JS frame ::
chrome://editor/content/EdSpellCheck.js :: CancelSpellCheck :: line 494"  data: no]

b) Second, nothing else happens, but from now on most of the buttons will
generate JavaScript exceptions:
Error: uncaught exception: [Exception... "Component returned failure code:
0x80004002 (NS_NOINTERFACE) [nsIEditorSpellCheck.UninitSpellChecker]"  nsresult:
"0x80004002 (NS_NOINTERFACE)"  location: "JS frame ::
chrome://editor/content/EdSpellCheck.js :: onClose :: line 504"  data: no]
Error: uncaught exception: [Exception... "Component returned failure code:
0x80004002 (NS_NOINTERFACE) [nsIEditorSpellCheck.GetNextMisspelledWord]" 
nsresult: "0x80004002 (NS_NOINTERFACE)"  location: "JS frame ::
chrome://editor/content/EdSpellCheck.js :: NextWord :: line 240"  data: no]

c) Most importantly, once you click the "Stop" button, you are completely stuck
- since SpellChecker controls no longer work, there does not seem to be a way to
 get rid of the SpellChecker window and there does not seem to be a way to
unblock the composition window, so the composed message is now effectively lost
(taking a screenshot to simplify re-typing seems to be the best you can do,
although may be if I try quitting Mozilla, it will offer to save a Draft - have
not tried that just yet).

To summarize, a regression with a dataloss potential :-(

P.S. BuildID 2002101418 on RedHat 8.0

Comment 12

[Kathleen :Brade]

It sounds to me like you are missing this file in your build:
  http://lxr.mozilla.org/seamonkey/source/editor/composer/src/nsEditorSpellCheck.cpp

I don't see problems with a recent Netscape spellchecker in my build (and don't
have build instructions for mozdev's spellchecker).

The output indicates that something is wrong with gSpellChecker which should be
coming from nsEditorSpellCheck.cpp (which is the new "glue" layer instead of
being in nsEditorShell).

Comment 13

[David Einstein]

Ok, I had not tried that.  I cannot really see how that could be a spellchecker
bug, but I'll dig, and see where the problem lies.

Comment 14

[Akkana Peck]

I see this too.  nsEditorSpellCheck::UninitSpellChecker() is being called twice,
presumably once from CancelSpellCheck and once from onClose().  I'm not clear
why it worked before, since it looks like the old code didn't allow it being
called twice either ...  Checked with Kin, and we both thought that the right
thing to do was to uninit only from onClose (since Cancel also ends up closing
the dialog); will attach a patch that does that.

Comment 15

[Akkana Peck]

Attachment: Don't call uninit twice

Comment 16

[Charles Manske]

Comment on attachment 103077 [details] [diff] [review]
Don't call uninit twice

I don't think this is the right thing to do. When using spellchecker in
Composer, hitting Esc or "X" on Windows will call "CancelSpellChecking() and
not "onClose()"
I suggest this:
if (gSpellChecker)
{
  try {   gSpellChecker.UninitSpellChecker();
  } catch (e) {}
  gSpellChecker = null;
} 

in both methods.

Comment 17

[Akkana Peck]

Attachment: More elaborate fix, zero gSpellChecker after uninit in both places

Charley, Kathy, Kin and I hashed out various options on IRC and eventually came
up with this.  It seems like we should be able to dispense with the if
(gSpellChecker) part, but in practice, that results in an error when dismissing
the spellcheck window if it was called up from spell check on send in mail
compose.

This also includes a fix that gets rid of all the thread-safe asserts (thanks
to timeless for helping debug that).

Comment 18

[Kathleen :Brade]

Comment on attachment 103082 [details] [diff] [review]
More elaborate fix, zero gSpellChecker after uninit in both places

r=brade

Comment 19

[kinmoz]

Comment on attachment 103082 [details] [diff] [review]
More elaborate fix, zero gSpellChecker after uninit in both places

sr=[e-mail]

Comment 20

[Asa Dotzler [:asa]]

Comment on attachment 103082 [details] [diff] [review]
More elaborate fix, zero gSpellChecker after uninit in both places

a=asa for checkin to 1.2 (on behalf of drivers)

Comment 21

[Akkana Peck]

Fixed.