151040 - Add access to Mac OS X global spelling checkers

Status: VERIFIED FIXED

(Camino Graveyard :: OS Integration, enhancement, P2)

Description

[Albert Kammerer]

Please consider adding access to Mac OS X global spelling checker for all text
blocks. I spend so much time filling out forms and such on the web (like this
one for instance!) that it would really be helpfull to have access to a spelling
checker. See OmniWeb for a decent implementation of this feature.

Comment 1

[saari (gone)]

good idea

Comment 2

[Greg K.]

Related to/duplicate of bug 86886?

Comment 3

[Kathleen :Brade]

-->brade

Comment 4

[Peter Farago]

Related to/Dependant on bug <a href="http://bugzilla.mozilla.org/show_bug.cgi?id=148098">
148098</a>?

Comment 5

[Steve Klingsporn]

*** Bug 175645 has been marked as a duplicate of this bug. ***

Comment 6

[Wincent Colaiuta]

This would be a nice feature, but I wouldn't prioritise it over fixing bug
148098... we don't need form input to get any slower on old machines. :-)

Comment 7

[Steve Smart]

If we implement a native text widget (bug 148098, we get spell-checking  (as
well as services and things listed in a dozen other bugs) for free. It is
probably also possible to hack in the spell checker while still using gecko
text, but this is still unlikely to create any major performance hit.   It _is_
going to be important to decide at some point whether the difficulties
implementing features to fix all the problems (both with features and with
performance) in gecko text inputoutweigh the work it would take to use a native
widget; this bug is definitely closely realated to 148098, even though it
doesn't necessarily have to depend on it.

Comment 8

[Gabriel Bender]

I think that the use native widgets would slow down scrolling and page
rendering. Omniweb, for example, is switching from native widgets because of
speed issues (or so I heard a couple months ago).

Comment 9

[Calum Robinson]

I've looked into this a little and the biggest problem seems to be a way to draw
the red dots under the misspelt words. I don't know if there is any way to do
this within a text field in Gecko. 

Comment 10

[Bill McGonigle (not currently reading bugmail; please contact directly)]

How about just drawing the text in red, italics, perhaps?  Most text entry
widgets don't support styled text anyhow.

Comment 11

[Kathleen :Brade]

drawing the red dots isn't a problem (iirc); I ran into problems with the OSX
spellchecker not showing its dialog.

Comment 12

[Kathleen :Brade]

Attachment: work in progress

This patch is where I left off (includes debugging etc); may be missing some
nib/resource changes.

Comment 13

[S Woodside]

brade what does it do?

Comment 14

[Kathleen :Brade]

kin: see patch attached (it's VERY rough)

Simon Woodside: the patch is a first attempt to invoke the OSX spell checker for
textareas / input fields.  More needs to be done before it can land.

Comment 15

[Jasper]

Anny news, would be nice for 1.0 release?

Comment 16

[Kathleen :Brade]

Attachment: diffs from the tree where I had started work on spellcheck

This is probably about the same as the previous patch with some other things
thrown in too.	I am about to lose this computer so posting this patch in case
someone else wants to pick it up and make it work.

Comment 17

[Steve Jacobs (irc: encro)]

If Useful: Apple recently posted some sample code to the reference library.

Sample Code: SpellingChecker-CocoaCarbon
http://developer.apple.com/samplecode/SpellingChecker-CocoaCarbon/SpellingChecker-CocoaCarbon.html

Comment 18

[Steve Jacobs (irc: encro)]

If Useful: Apple recently posted some sample code to the reference library.

Sample Code: SpellingChecker-CocoaCarbon
http://developer.apple.com/samplecode/SpellingChecker-CocoaCarbon/SpellingChecker-CocoaCarbon.html

Comment 19

[Julian Pellico]

Ludo, would you mind if I looked at this?

Comment 20

[Ludovic Hirlimann [:Usul]]

(In reply to comment #19)
> Ludo, would you mind if I looked at this?

Not at all

Comment 21

[Julian Pellico]

I'm taking it....for better or worse =)

Comment 22

[Simon Fraser [no longer active]]

Assuming we use the gecko inline editor work, depends on bug 58612.

I think we'll also have to implement nsIEditorSpellCheck to wrap the cocoa spell
checker.

Comment 23

[Julian Pellico]

Attachment: beta patch

Hey all,

This is a beta patch. I just wanted to invite anyone watching to give their
thoughts on it.

Things NOT implemented are:
1) spellcheck as you type. It just beeps - I know.
2) underlining or coloring misspelled words. This kinda goes with #1 - doesn't
matter too much by itself.

Note on the patch: the way I currently implement the Ignore feature is to have
all text fields on a page share an ignore list. Any fields in another window or
tab have a separate ignore list. The ignore list is thrown away whenever the
location in a tab changes.

Comment 24

[Julian Pellico]

Attachment: beta nibs

Comment 25

[Jasper]

Julian I was quickly looking at the nib you attached and I was wondering. In OS
X text area contextmenu the menu looks as follows:

Guesses Section
------
Ignore Spelling
Learn Spelling
------

Everything beneath these options are in your nib file I was just wondering if
you also had these opptions as they are pretty necesarry.

Comment 26

[Jasper]

Attachment: html page that shows css would be ideal to do the dots if possible

...

Comment 27

[Julian Pellico]

Attachment: Mozilla underlining misspelled words

I found a way that Mozilla uses to mark things as misspelled. This shows what
it looks like. It's a solid underline but I think it looks fine.

Comment 28

[Julian Pellico]

I'd love to pick this up again. But, 58612 hasn'gt been touched for awhile.
Maybe I should apologize on bug 58612 for my rude comment, and then gently ask
them to pick it up again :)

Comment 29

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Can we get someone with some clout :-) to "lean on" bug 58612 so that it gets
fixed in the Gecko 1.9 cycle so that we can actually get spell-checking by the
time Camino 1.1 rolls around off the trunk again...?

Comment 30

[Yaw Anokwa]

For all that is sacred in this world, can someone...anyone, please work on this?
The masses are awaiting spelcheking in Camino.

Comment 31

[Samuel Sidler (old account; do not CC)]

(In reply to comment #30)
> For all that is sacred in this world, can someone...anyone, please work on this?
> The masses are awaiting spelcheking in Camino.

This bug is dependent on a core bug. Until that core bug is fixed, there won't
be spell checking in Camino. If you want this to be fixed, go vote for bug 58612.

Comment 32

[François Maurit]

(In reply to comment #31)
> (In reply to comment #30)
> > For all that is sacred in this world, can someone...anyone, please work on this?
> > The masses are awaiting spelcheking in Camino.
> 
> This bug is dependent on a core bug. Until that core bug is fixed, there won't
> be spell checking in Camino. If you want this to be fixed, go vote for bug 58612.

I would expect Firefox spellchecker to depend on bug 58612, as it is crossplatform. But why should 
Camino's ?
Bug 58612 sounds like a complete implementation. (With discussion over how to render the erroneous 
words and what event should trigger spell checking.)
For Camino, we should aim at getting as close as possible to how Safari works, that is rely on Mac OS X 
spell checker. (Same dictionary, same dialog box for spell check settings, same rendering of erroneous 
words, etc.) (As was initially requested for that bug.)
This is certainly possible to reuse in a web browser if Safari team got it right. Isn't it the whole idea 
behind Camino: leverage Mac OS X functionalities to better fit into the mac user experience ?

Comment 33

[Julian Pellico]

I wuold be working on this wholeheartedly, as I think it is possible to do it
right without requiring the Core bug, but I am extremely distracted since at
this moment I do not have a place to live starting Sep 23.

Comment 34

[Samuel Sidler (old account; do not CC)]

Because this needs changes to core which won't get fixed until Gecko 1.9 (see bug 58612), targeting for Camino 2.0.

Comment 35

[Mike Pinkerton (not reading bugmail)]

here's what we need to do now that the spellcheck code has landed on the trunk (from brettw):

1. Enable the spellcheck extension and make sure it gets distributed.
Also, be sure you have the dictionaries you want. They go in
components/myspell.

2. Set layout.textarea.spellcheckDefault = true. This will make
textareas get spellchecking by default.

3. Check that this works. There will be no UI, but you should see the
red underlining if everything is set up properly.

4. Add the context menu stuff. See my patch for browser.js on the bug.
You can call my shared JS file for most of the tasks. You may want to
wait on this part for a few weeks: my shared JS UI file might get
converted into a component or something else.

Comment 36

[Julian Pellico]

(In reply to comment #35)
> here's what we need to do now that the spellcheck code has landed on the trunk
> (from brettw):
> 
> 1. Enable the spellcheck extension and make sure it gets distributed.
> Also, be sure you have the dictionaries you want. They go in
> components/myspell.

Shouldn't we use the OS X dictionaries? Any idea how we could do that?

Comment 37

[Simon Fraser [no longer active]]

Yes, we should. Is there a dictionary API that we can implement?

Comment 38

[Bill McGonigle (not currently reading bugmail; please contact directly)]

(In reply to comment #37)
> Yes, we should. Is there a dictionary API that we can implement?

Assuming this needs to be done in Core(?), this looks like it might be the right API:
http://developer.apple.com/documentation/Carbon/Reference/Dictionary_Manager/index.html

Comment 39

[Julian Pellico]

Comment on attachment 172534 [details] [diff] [review]
beta patch

rumor has it someone's working on this for real, again ;)

Comment 40

[Julian Pellico]

Attachment: MainMenu.nib

Comment 41

[Julian Pellico]

Attachment: changes to core

Comment 42

[Julian Pellico]

Attachment: changes to camino v1

I tried to add some new files into the patch using /dev/null, I hope it works with 'patch' =)
---------
Changes in order to build:
Add to mozconfig: --enable-extensions=default,spellcheck

To the pbxproj:
Add src/embedding/CHEditorSpellCheck*
In the Targets tab, Add spellcheck.xpt to Copy Files (64) (I think it's the first Copy Files)
In the Targets tab, Add libspellchecker.dylib to the next Copy Files
Add to HEADER_SEARCH_PATHS: ../dist/include/editor ../dist/include/txtsvc

My changes introduced some build error which I had to work around, this is what it looks like:
/mozilla-head/mozilla/camino/src/embedding/CHBrowserView.mm: In function `unsigned int -[CHBrowserView draggingEntered:](CHBrowserView*, objc_selector*, objc_object*)':
/mozilla-head/mozilla/camino/src/embedding/CHBrowserView.mm:1528: warning: invalid conversion from `int' to `OpaqueDragRef*'

Comment 43

[Julian Pellico]

Comment on attachment 208498 [details] [diff] [review]
changes to core

Hi Boris, I noticed you reviewed the core patch for  bug 302050. Would you mind taking a look at this one?

Comment 44

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 208498 [details] [diff] [review]
changes to core

For future reference, this would be a _lot_ easier to review if you use better diff options.  Something like diff -up8 or so.  -p puts in the function names, -8 gives more context.

>Index: extensions/spellcheck/src/mozEnglishWordUtils.cpp
>Index: extensions/spellcheck/src/mozInlineSpellChecker.h

I can't review this part; please get someone familiar with spell-checker to do so.

>Index: layout/forms/nsTextControlFrame.cpp
> nsTextControlFrame::Destroy(nsPresContext* aPresContext)
> {
>+    nsContentUtils::UnregisterPrefCallback(PREF_DEFAULT_SPELLCHECK, 
>+                                           nsTextControlFrame::RealTimeSpellCallback, this);

This is mis-indented.

>@@ -1705,7 +1707,12 @@
>+  // we working on Camino decided to allow inline spellcheck for single line fields
>+#ifdef MOZ_MACBROWSER

That's fine but NOT with ifdefs.  Add a pref to control it, and set it in your config or something.

>nsTextControlFrame::RealTimeSpellCallback(const char* pref, void* context)
>+{

This should assert that |pref| is what you expect.  Also, aPref, aContext for the args.

>+    nsTextControlFrame* frame = NS_REINTERPRET_CAST(nsTextControlFrame*, context);

NS_STATIC_CAST, please.

>+    if (frame)

Under what conditions would |frame| be null?  If it can't be, assert instead of checking.

>+    return NS_OK;

This function doesn't return an nsresult.  It returns an int.  I suggest returning 0.

>+
>+

Why so much space?  One blank line between functions should be about right.

>Index: layout/forms/nsTextControlFrame.h
>+  static NS_HIDDEN_(int) PR_CALLBACK RealTimeSpellCallback(const char* pref, void* closure);

I'm not sure NS_HIDDEN is a good idea here.  I'd take it out.

Comment 45

[Julian Pellico]

apologies

Comment 46

[Julian Pellico]

Attachment: changes to core v2

I guess I'll contact bryner about the spellcheck part

Comment 47

[Boris Zbarsky [:bzbarsky]]

I suggest contacting a spellcheck owner/peer (like mscott) for the spellcheck part, actually.

Comment 48

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 209190 [details] [diff] [review]
changes to core v2

>Index: layout/forms/nsTextControlFrame.cpp
> #define PREF_DEFAULT_SPELLCHECK "layout.textarea.spellcheckDefault"
>+#define PREF_SPELLCHECK_SINGLE_LINE "browser.spellcheck.singleLine"

Wouldn't it make more sense to call the new pref "layout.textInput.spellcheckDefault"?  Compare with the "layout.textarea.spellcheckDefault" pref?  And perhaps change things such that it's possible to spell-check single-line inputs even if you're not spell-checking textareas?

That is, depending on whether this is a single or multi-line control register for different prefs and so forth?

Comment 49

[Julian Pellico]

(In reply to comment #48)
> (From update of attachment 209190 [details] [diff] [review] [edit])
> >Index: layout/forms/nsTextControlFrame.cpp
> > #define PREF_DEFAULT_SPELLCHECK "layout.textarea.spellcheckDefault"
> >+#define PREF_SPELLCHECK_SINGLE_LINE "browser.spellcheck.singleLine"
> 
> Wouldn't it make more sense to call the new pref
> "layout.textInput.spellcheckDefault"?  Compare with the
> "layout.textarea.spellcheckDefault" pref?  And perhaps change things such that
> it's possible to spell-check single-line inputs even if you're not
> spell-checking textareas?
That's exactly the point: the pref doesn't single handedly turn on or off spellchecking for text inputs; it turns it on or off *given that* spellchecking is on. It treats the already existing pref as a master pref. I saw no use in making it possible to enable realtime spell on JUST single-line inputs.

> 
> That is, depending on whether this is a single or multi-line control register
> for different prefs and so forth?
> 
I'm sorry; what?

Comment 50

[Boris Zbarsky [:bzbarsky]]

> That's exactly the point: the pref doesn't single handedly turn on or off
> spellchecking for text inputs; it turns it on or off *given that* spellchecking
> is on.

Right.  This is the part I don't understand.  Why?  That is, why apply a pref that controls spellchecking for textareas to text inputs and then have _another_ pref to decide whether it applies?  Why not just apply one pref to textareas and another to text inputs?

As things stand, to make your patch correct you would need to register an observer for both prefs.  And then you still have the conceptual weirdness of a textarea pref affecting the behavior of text inputs...

Comment 51

[Brett Wilson]

For what its worth, I originally was planning on having one pref with values 0 (off) 1 (on for textareas) 2 (on for everything). But then we moved the pref to be called *.textarea.* which makes this make less sense.

There's no reason I can think of why you would want to have spellchecking on by default for single-line, but off by default for multi-line. But having two prefs control three states as Julian did seems a little strange to me. Would it make people happier to rename it to "layout.spellcheckDefault" and have three possible values?

Comment 52

[Julian Pellico]

Comment on attachment 209190 [details] [diff] [review]
changes to core v2

Hi,
Would you want to lend a hand and  review the changes to extensions/spellcheck that are in this patch?

Comment 53

[Julian Pellico]

Attachment: changes to camino v2

includes small fixes

Comment 54

[Boris Zbarsky [:bzbarsky]]

Yeah, making it a tri-state would also work for me.  We didn't ship that pref in Firefox 1.5, did we?

Comment 55

[Scott MacGregor]

Comment on attachment 209190 [details] [diff] [review]
changes to core v2

Is this include really necessary? 

+#include "mozISpellI18NUtil.h"

I don't see where you've added any new code to the spellchecker that would depend on that header file. 

I'm not comfortable reviewing the layout changes. I'll leave that to bz and others.

Comment 56

[Julian Pellico]

(In reply to comment #55)
> (From update of attachment 209190 [details] [diff] [review] [edit])
> Is this include really necessary? 
> 
> +#include "mozISpellI18NUtil.h"
> 
> I don't see where you've added any new code to the spellchecker that would
> depend on that header file. 

The containing header file, mozInlineSpellChecker.h, has build errors when I build on my Mac, related to the use of mozISpellI18NUtil. Probably there is a difference in the include tree between platforms.

Comment 57

[Julian Pellico]

Attachment: changes to core v3

Hi Boris, I hope this is the final core patch

Comment 58

[Julian Pellico]

Some more info about building in Camino, I see I added some additional header search paths:

../editor/composer/src
../extensions/spellcheck/idl/_xpidlgen
../extensions/spellcheck/src

I guess these should be added to dist like the other items. I'll leave that for the sr to consider that...

Comment 59

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 209805 [details] [diff] [review]
changes to core v3

>Index: layout/forms/nsTextControlFrame.cpp
>@@ -130,13 +130,13 @@
>-#define PREF_DEFAULT_SPELLCHECK "layout.textarea.spellcheckDefault"
>+#define PREF_DEFAULT_SPELLCHECK "layout.spellcheckDefault"

You need to change places that use the old pref value (eg firefox.js).  I don't see changes to those in this patch.  Note that you need to change not only the name but the value in that file.

>@@ -1702,20 +1704,38 @@ nsTextControlFrame::SyncRealTimeSpell()

>+  // check the pref to see what the default should be, default is 0: never spellcheck
>+  PRInt32 spellcheckLevel = nsContentUtils::GetIntPref(PREF_DEFAULT_SPELLCHECK, 0);
>+  if (!readOnly) {

Why not get the pref once we have !readonly?  Seems like it would be more efficient in that case, and identical in the other cases.

>+        enable = true;

PR_TRUE.

r=bzbarsky with those three issues fixed.  Thanks!

Comment 60

[Julian Pellico]

Attachment: changes to core v3.1 (checked in)

Hi Brian, wanna sr this? I think you're the man

Comment 61

[Julian Pellico]

Attachment: changes to camino v3

be sure to use both v3 patches

Comment 62

[Mike Pinkerton (not reading bugmail)]

--- src/browser/GoMenu.mm	29 Aug 2005 17:55:04 -0000	1.18
+++ src/browser/GoMenu.mm	30 Jan 2006 02:24:13 -0000
@@ -58,12 +58,13 @@
 
 #import "HistoryItem.h"
 #import "HistoryDataSource.h"
 
 #import "GoMenu.h"
 
+#include <Carbon/Carbon.h>

is this necessary?

+// Spellchecking
+- (IBAction)chCheckSpelling:(id)sender;
+- (IBAction)chShowGuessPanel:(id)sender;
+- (IBAction)chToggleContinuousSpellChecking:(id)sender;

why the |ch| prefix? 

+#define CH_ENSURE_SUCCESS(res) \
+  if (!NS_SUCCEEDED(res)) return;

isn't this already a gecko macro? why create our own?

+  else {
+      [self closeSpellDocument];
+      mSpellDocumentTag = [NSSpellChecker uniqueSpellDocumentTag];
+  }

nit. fix indents. 2 space.

+    nsCOMPtr<nsIDOMWindow> domWindow = getter_AddRefs([self getContentWindow]);
+    nsCOMPtr<nsPIDOMWindow> privateWindow = do_QueryInterface(domWindow);
+    if (!privateWindow)
+        return NULL;

again fix indents. several times in this routine.

+    nsCOMPtr<nsITextControlElement> textElem = do_QueryInterface(focusedItem);
+    nsITextControlElement *returnElem =  textElem;
+    return returnElem;

this is generally a bad pattern, we should avoid returning un-addref'd interfaces from functions. 

+    // NOTE |GetCurrentDoc| doesn't AddRef?!
+    nsIDocument* doc = content->GetCurrentDoc();
+    if (!doc)
+        return;
+    nsIPresShell* presShell = doc->GetShellAt(0);
+    if (!presShell)
+        return;

are you sure about these not addRef'ing?

+    if (spellChecker == nsnull) {

use |if (!spellChecker) {| instead

+    CHEditorSpellCheck* edSpellChecker = NS_REINTERPRET_CAST(CHEditorSpellCheck*, spellChecker);

that seems really skanky. why can we make this assumption? Why not add another platform-specific interface?

+// Imitate the spellcheck actions for Cocoa text fields.
+// "chFoo" actions avoid collision with the real Cocoa non-ch actions, we add ch
+// so that Cocoa text fields such as the location bar don't respond to the spellcheck items

this comment should be in the header file.

+// The only difference between "Check Spelling" and "SpellingÉ" is that

use "..." not the ellipsis char, so it's readable across platforms.

+    nsCOMPtr<nsITextControlElement> activeElement = [self focusedTextControlElement];

why is this wrapped in a nsCOMPtr if you didn't addref before the return? be consistent.

+    if (selStart != mSavedSelStart || selEnd != mSavedSelEnd) {
+        NSBeep();
+        NSLog(@"Selection changed");
+        return;
+    }

who's going to see this log message? Why bother printing it?

+    int newState = [rtSpell state] == NSOnState ? 2 : 0;

aren't there constants for these states?

+- (int)spellDocumentTag
+{
+    return mSpellDocumentTag;
+}
+

does this really need to be part of the public api? should it be a private category?

--- src/history/HistoryOutlineViewDelegate.mm	15 Jan 2006 21:03:32 -0000	1.10
+++ src/history/HistoryOutlineViewDelegate.mm	30 Jan 2006 02:24:15 -0000
@@ -47,12 +47,14 @@
 #import "BrowserWindowController.h"
 #import "BookmarkViewController.h"
 #import "MainController.h"
 
 #import "HistoryOutlineViewDelegate.h"
 
+#include <Carbon/Carbon.h>
+

again, not necessary

+class CHEditorSpellCheck: public nsIEditorSpellCheck {

open brace on a line by itself please

+    nsCOMPtr<nsITextServicesFilter> mTxtSrvFilter;  // weak
+    nsCOMPtr<nsITextControlFrame> mCurTextControl;  // weak
+    nsCOMPtr<nsIDOMNode> mDocRoot;                  // weak
+    nsCOMPtr<nsIEditor> mEditor;                    // weak

how can these be weak, we own a strong ref to them with the nsCOMPtr!

+    static CHEditorSpellCheck* mSharedSpellCheck;   // never dies after creation

prefix statics with |s|, eg |sSharedSpellCheck|

+CHEditorSpellCheck::CHEditorSpellCheck() 
+: mCurGuesses(nil),
+mCurGuessIndex(-1),
+mSpellDocTag(-1)
+{
+}

isn't there an init call taht needs to be made here to init the refcount?

+NS_IMETHODIMP
+CHEditorSpellCheck::CanSpellCheck(PRBool* canSpellCheck)
+{
+    *canSpellCheck = PR_TRUE;
+    return NS_OK;
+}

you should validate that |canSpellCheck| isn't null first.

+    PRInt32 begin,end;
+    PRBool isMisspelled = PR_FALSE;
+    PRBool docEOF = PR_FALSE;
+    PRBool wrapped = PR_FALSE;

|begin| and |end| should be initialized here to something reasonable.

Also, shouldn't |CHEditorSpellCheck| be a standalone component that firefox can use? We don't want to ship the dictionary for mac FF either.

Comment 63

[Julian Pellico]

Hi mike...

(In reply to comment #62)
>  #import "GoMenu.h"
> 
> +#include <Carbon/Carbon.h>
> 
> is this necessary?
Yes. All the added Carbon includes are necessary to allow us to remove it from PreferenceManager.h, which I need to include for CHBrowserView. Carbon.h pulls in a symbol collision for DragRef. I only need to include PrefMgr for the new define I added. Maybe that could go in its own file.

> +#define CH_ENSURE_SUCCESS(res) \
> +  if (!NS_SUCCEEDED(res)) return;
> 
> isn't this already a gecko macro? why create our own?
Which is it? This question open to everyone =)

> +    nsCOMPtr<nsITextControlElement> textElem = do_QueryInterface(focusedItem);
> +    nsITextControlElement *returnElem =  textElem;
> +    return returnElem;
> 
> this is generally a bad pattern, we should avoid returning un-addref'd
> interfaces from functions. 
I learned it from some other BrowserView code, and the core code in the next comment. Nonetheless,  I'll change it by the next patch unless you revoke the comment

> +    // NOTE |GetCurrentDoc| doesn't AddRef?!
> +    nsIDocument* doc = content->GetCurrentDoc();
> +    if (!doc)
> +        return;
> +    nsIPresShell* presShell = doc->GetShellAt(0);
> +    if (!presShell)
> +        return;
> 
> are you sure about these not addRef'ing?
Yup, I followed all the core code that calls these.

> +    CHEditorSpellCheck* edSpellChecker =
> NS_REINTERPRET_CAST(CHEditorSpellCheck*, spellChecker);
> 
> that seems really skanky. why can we make this assumption? Why not add another
> platform-specific interface?
It's safe because we know that CHEditorSpellCheck is the implementor  of nsIEditorSpellCheck according to AppComponents. I'm not sure about platform-specific interfaces - feel free to tell me more offline (well, online, but not here).

> +    nsCOMPtr<nsITextControlElement> activeElement = [self
> focusedTextControlElement];
> 
> why is this wrapped in a nsCOMPtr if you didn't addref before the return? be
> consistent.
I need to hop the interfaces with QI. I suppose that's a good reason to make |focusedTextControlElement| return by ref? =)

> 
> +    int newState = [rtSpell state] == NSOnState ? 2 : 0;
> 
> aren't there constants for these states?
Yeah, in nsTextControlFrame.h (I created them). If we include that, we can use the constants. That file includes a *slew* of other files, though.

> +- (int)spellDocumentTag
> +{
> +    return mSpellDocumentTag;
> +}
> +
> 
> does this really need to be part of the public api? should it be a private
> category?
Except that, when doing realtime spellcheck, the CHEditorSpellCheck gets invoked without the BrowserView. But, the BrowserView owns the spellcheck tag. I couldn't come up with any other way of syncing the BrowserView to the CHEdSpellCheck.

> +    nsCOMPtr<nsITextServicesFilter> mTxtSrvFilter;  // weak
> +    nsCOMPtr<nsITextControlFrame> mCurTextControl;  // weak
> +    nsCOMPtr<nsIDOMNode> mDocRoot;                  // weak
> +    nsCOMPtr<nsIEditor> mEditor;                    // weak
> 
> how can these be weak, we own a strong ref to them with the nsCOMPtr!
I'm kinda asking if they should be weak. Should I hold on to them?

> +CHEditorSpellCheck::CHEditorSpellCheck() 
> +: mCurGuesses(nil),
> +mCurGuessIndex(-1),
> +mSpellDocTag(-1)
> +{
> +}
> 
> isn't there an init call taht needs to be made here to init the refcount?
In our constructor? Wouldn't that be in one of the base classes? Do you mean the NS_IMPL_ISUPPORTS stuff? I changed the file to use that instead of the separate macros.

> +    PRInt32 begin,end;
> +    PRBool isMisspelled = PR_FALSE;
> +    PRBool docEOF = PR_FALSE;
> +    PRBool wrapped = PR_FALSE;
> 
> |begin| and |end| should be initialized here to something reasonable.
Really? The core code doesn't. I'm all in favor of precedence...

> Also, shouldn't |CHEditorSpellCheck| be a standalone component that firefox can
> use? We don't want to ship the dictionary for mac FF either.
Tell me more about standalone components.

Comment 64

[Julian Pellico]

What do we do to get someone to land the core patch?

Comment 65

[Ludovic Hirlimann [:Usul]]

(In reply to comment #64)
> What do we do to get someone to land the core patch?
> 

If it has r and sr then ask someone to land it for you.

Comment 66

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 210068 [details] [diff] [review]
changes to core v3.1 (checked in)

I checked this in on trunk.

For future reference, it'd be good to put core patches in core bugs...

Comment 67

[Brett Wilson]

Comment on attachment 210068 [details] [diff] [review]
changes to core v3.1 (checked in)

We need this on the branch. bz: Are you the right person to ask for this stuff?

Also, the changes to the spellchecker seemed to be for Camino. Does this affect Firefox on the Mac, or are we still planning on using myspell there too?

Comment 68

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 210068 [details] [diff] [review]
changes to core v3.1 (checked in)

Looks ok, assuming bug 302050 is going on branch

Comment 69

[Julian Pellico]

> Also, the changes to the spellchecker seemed to be for Camino. Does this affect
> Firefox on the Mac, or are we still planning on using myspell there too?
> 

Didn't the patch for bug 302050 include mozEnglishWordUtils? I don't know if that code is live or dead, but it's possible that Firefox is using that code for spellcheck. My change to that file would fix an issue that I assume would occur for any build that has realtime spell enabled, although I only know for sure that it was an issue in Camino. Also, if Firefox gets a UI for the realtime pref, which I don't think it has currently, then the changes to nsTextControlFrame will ensure that the pref is updated live.

Comment 70

[Julian Pellico]

Attachment: changes to camino v3.1

whom should I flag for review?

Comment 71

[Julian Pellico]

patch notes: I needed to leave the change for the Carbon/Carbon.h includes because I needed to include PrefMgr.h for CHBrowserView after all. Everything else I said I'd do in comment 63 I did.

Comment 72

[Simon Fraser [no longer active]]

Comment on attachment 211692 [details] [diff] [review]
changes to camino v3.1

> Index: src/application/MainController.mm
> ===================================================================

> +  // sync realtime spellcheck setting
> +  // default is zero - off. 2 is on.
> +  int enableRealTimeSpellCheck = [prefsManager getIntPref:kRealTimeSpellPref withSuccess:&success];
> +  if (!success || !enableRealTimeSpellCheck) {
> +    [mRealTimeSpellMenuItem setState:NSOffState];
> +  }
> +  else {
> +    [mRealTimeSpellMenuItem setState:NSOnState];
> +  }

Does the menu this item is in have auto-updating turned off?
Otherwise this won't stick.

> Index: src/browser/GoMenu.mm
> ===================================================================
> RCS file: /cvsroot/mozilla/camino/src/browser/GoMenu.mm,v
> retrieving revision 1.18
> diff -u -6 -p -r1.18 GoMenu.mm
> --- src/browser/GoMenu.mm	29 Aug 2005 17:55:04 -0000	1.18
> +++ src/browser/GoMenu.mm	13 Feb 2006 06:44:52 -0000
> @@ -58,12 +58,13 @@
>  
>  #import "HistoryItem.h"
>  #import "HistoryDataSource.h"
>  
>  #import "GoMenu.h"
>  
> +#include <Carbon/Carbon.h>

Put framework includes at the top, please.

> Index: src/embedding/CHBrowserView.h
> ===================================================================

> +// Spellchecking
> +// Imitate the spellcheck actions for Cocoa text fields.
> +// "chFoo" actions avoid collision with the real Cocoa non-ch actions, we add ch
> +// so that Cocoa text fields such as the location bar don't respond to the spellcheck items
> +- (IBAction)chCheckSpelling:(id)sender;
> +- (IBAction)chShowGuessPanel:(id)sender;
> +- (IBAction)chToggleContinuousSpellChecking:(id)sender;
> +
> +- (void)checkSpelling:(id)sender;
> +- (void)changeSpelling:(id)sender;
> +- (void)ignoreSpelling:(id)sender;
> +- (void)closeSpellDocument;
> +- (int)spellDocumentTag;

Spell checking adds a lot of code to CHBrowserView. Perhaps you could
move it into a separate file which contains a category of CHBrowserView.

It also adds some bad dependencies. Perhaps we should hide it all behind
a protocol, and move the implementation up into non-embedding Camino code.

> Index: src/embedding/CHBrowserView.mm
> ===================================================================

> +// Spellcheck integration
> +#include "nsITextControlElement.h"
> +#include "nsIFocusController.h"
> +#include "nsIContent.h"
> +#include "nsIFrame.h"
> +#include "nsITextControlFrame.h"
> +#include "nsIInlineSpellChecker.h"
> +#include "nsIEditor.h"
> +#include "nsIEditorSpellCheck.h"
> +#include "nsISelection.h"
> +#include "nsIPlaintextEditor.h"

This strengthens my conviction. CHBrowserView is supposed to be a "model"
gecko embedder, and most of these headers are not frozen and should not
be used by embedders. It would be nice to move this all out.

>  // security
>  #include "nsISecureBrowserUI.h"
>  #include "nsISSLStatusProvider.h"
>  #include "nsISSLStatus.h"
>  #include "nsIX509Cert.h"
>  
>  #import "CHBrowserView.h"
> +#import "PreferenceManager.h"

You're adding a dependency here of lower-level code (in embedding)
on higher-level code (in Camino). No can do.

> +- (void)spellcheckCore;

What does this mean? The method name is confusing

> +#pragma mark -
> +// Cocoa spellcheck integration
> +
> +// Get the current text control frame that has the focus, or NULL if none.
> +- (already_AddRefed<nsITextControlElement>)getFocusedTextControlElement
> +{
> +  nsCOMPtr<nsIDOMWindow> domWindow = getter_AddRefs([self getContentWindow]);
> +  nsCOMPtr<nsPIDOMWindow> privateWindow = do_QueryInterface(domWindow);
> +  if (!privateWindow)
> +    return nsnull;

Does this work in framesets?

> +// Search for the misspelled word starting at the selection, wrapping if necessary
> +- (void)spellcheckCore
> +{
> +  nsresult rv;

Move this down to where you first use it.

> +  nsCOMPtr<nsITextControlElement> activeElement = [self getFocusedTextControlElement];
> +  nsCOMPtr<nsIContent> content = do_QueryInterface(activeElement);
> +  if (!content)
> +    return;
> +  // NOTE |GetCurrentDoc| doesn't AddRef?!

Right. Raw pointer return values are not addreffed.

> +  // manual spell check should use the inline spell checker's spell checker if possible.
> +  // otherwise we use our own editor spellcheck, which is shared between all 
> +  // CHBrowserViews, and re-inited if the editor is different, i.e.
> +  // a different text field is active.

So when do we fail to get the inline spell checker's spell checker? Your comment
should say why/when this happens.

> +  CHEditorSpellCheck* edSpellChecker = NS_REINTERPRET_CAST(CHEditorSpellCheck*, spellChecker);
> +  if (edSpellChecker == NULL) {
> +    NSLog(@"Unable to get CHEditorSpellCheck");
> +    return;
> +  }

This is pretty evil. Normally if you need to do something like this, you create a
private interface (nsPIFoo) and QI the object to it. How can you be sure that there
isn't some other component in core which is creating spell checker objects?

Note that going to about:plugins can get core objects instantiated that
we normally override in AppComponents. See bug 285211.

> +  PRUnichar* prWord;
> +  rv = spellChecker->GetNextMisspelledWord(&prWord);
> +  NS_ENSURE_TRUE(rv,);

prWord is leaked. (Sucks that the they don't use nsAString).

> +  // now that new range is highlighted, get the (slightly incorrect) flat range
> +  // to cache for our own purpose
> +  rv = textFrame->GetSelectionRange(&mSavedSelStart, &mSavedSelEnd);
> +  // not much ado if that failed

Explain to the lay reader lwhen these offsets will be wrong.

> +// Correct the spelling of a misspelled word already spellchecked
> +- (void)changeSpelling:(id)sender
> +{
> +  nsresult rv;
> +  
> +  nsCOMPtr<nsITextControlElement> activeElement = [self getFocusedTextControlElement];
> +  if (!activeElement) {
> +    NSLog(@"Focus lost");
> +    return;
> +  }

Remove NSLog in final patch.

> +  // Let's make sure that the user hasn't changed the selection since they checked the spelling
> +  // for the misspelled word, otherwise the spellchecker just inserts the text willy-nilly
> +  // this just compares the selected range at the time the spelling was checked, to
> +  // the time the ignore was issued; doesn't check that the identity of the focused
> +  // text frame is the same.
> +  int selStart, selEnd;
> +  textFrame->GetSelectionRange(&selStart, &selEnd);
> +  if (selStart != mSavedSelStart || selEnd != mSavedSelEnd) {
> +    NSBeep();

Beep?

> +  nsCOMPtr<nsIEditor> editor;
> +  rv = textFrame->GetEditor(getter_AddRefs(editor));
> +  NS_ENSURE_TRUE(rv,);
> +  
> +  nsAutoString newWord;
> +  NSString* value = [[sender selectedCell] stringValue];
> +  [value assignTo_nsAString:newWord];
> +  nsCOMPtr<nsIDOMNode> node;
> +  PRInt32 offset;
> +  // get a node and offset for normal selection (which is the word to change)
> +  nsCOMPtr<nsISelection> selection;
> +  rv = editor->GetSelection(getter_AddRefs(selection));
> +  NS_ENSURE_TRUE(rv,);
> +  rv = selection->GetFocusNode(getter_AddRefs(node));
> +  NS_ENSURE_TRUE(rv,);
> +  rv = selection->GetFocusOffset(&offset);
> +  NS_ENSURE_TRUE(rv,);
> +  
> +  nsCOMPtr<nsIInlineSpellChecker> inlineSpChecker;
> +  rv = editor->GetInlineSpellChecker(PR_TRUE, getter_AddRefs(inlineSpChecker));    
> +  NS_ENSURE_TRUE(rv,);
> +  
> +  // if realtime spellcheck is on, we pass it to the inline checker
> +  // otherwise, it won't work, we have to do it ourselves
> +  PRBool enabled;
> +  rv = inlineSpChecker->GetEnableRealTimeSpell(&enabled);
> +  if (enabled) {
> +    nsAutoString word;
> +    [[[sender selectedCell] stringValue] assignTo_nsAString:word];

I think [sender selectedCell] needs some explanation.

> +    // pass a point in the highlighted word to the inline checker
> +    rv = inlineSpChecker->ReplaceWord(node, offset, newWord);
> +  }
> +  else {

In this clause, 'offset' is not used, so maybe move the above code into
here.

> +    // taken from nsTextServicesDocument::InsertText
> +    rv = editor->BeginTransaction();
> +    if (NS_FAILED(rv))
> +      return;
> +    nsCOMPtr<nsIPlaintextEditor> textEditor = do_QueryInterface(editor, &rv);
> +    // word is already the selection
> +    if (NS_SUCCEEDED(rv))
> +      rv = textEditor->InsertText(newWord);
> +    editor->EndTransaction();
> +  }
> +}
> +
> +- (IBAction)chToggleContinuousSpellChecking:(id)sender
> +{
> +  NSMenuItem* rtSpell = (NSMenuItem*)sender;
> +  [rtSpell setState:([rtSpell state] == NSOnState ? NSOffState : NSOnState)];
> +  int newState = [rtSpell state] == NSOnState ? 2 : 0;
> +  [[PreferenceManager sharedInstance] setPref:kRealTimeSpellPref toInt:newState];

So we're saving the pref separately from Cocoa. Should we, and does the right
thing happen when native text fields (e.g. the url bar) have focus?

> +// NSIgnoreMisspelledWords
> +- (void)ignoreSpelling:(id)sender
> +{
> +  nsresult rv;
> +  
> +  // inline spellchecker will call CHEditorSpellCheck::IgnoreWordAllOccurrences
> +  // which will ignore the word with NSSpellChecker
> +  // inline checker will update the highlighting for this text frame,but
> +  // what about other text frames that have the word now ignored?

Did you file a bug?

> +  nsCOMPtr<nsITextControlElement> activeElement = [self getFocusedTextControlElement];
> +  if (!activeElement) {
> +    NSLog(@"Focus changed");
> +    return;

Remove NSLog.

> +  }
> +  nsCOMPtr<nsIContent> content = do_QueryInterface(activeElement);
> +  // NOTE |GetCurrentDoc| doesn't AddRef?!
> +  nsCOMPtr<nsIDocument> doc = content->GetCurrentDoc();
> +  nsIPresShell* presShell = doc->GetShellAt(0);
> +  if (!presShell)
> +    return;
> +  
> +  nsIFrame* frame = presShell->GetPrimaryFrameFor(content);
> +  nsCOMPtr<nsITextControlFrame> textFrame = do_QueryInterface(frame);
> +  if (!textFrame)
> +    return;
> +  
> +  nsCOMPtr<nsIEditor> editor;
> +  rv = textFrame->GetEditor(getter_AddRefs(editor));
> +  NS_ENSURE_TRUE(rv,);
> +  
> +  nsCOMPtr<nsIInlineSpellChecker> inlineSpChecker;
> +  rv = editor->GetInlineSpellChecker(PR_TRUE, getter_AddRefs(inlineSpChecker));
> +  NS_ENSURE_TRUE(rv,);

This hunk of code appears quite a bit. Factor it?


> Index: src/history/HistoryOutlineViewDelegate.mm
> ===================================================================
> RCS file: /cvsroot/mozilla/camino/src/history/HistoryOutlineViewDelegate.mm,v
> retrieving revision 1.10
> diff -u -6 -p -r1.10 HistoryOutlineViewDelegate.mm
> --- src/history/HistoryOutlineViewDelegate.mm	15 Jan 2006 21:03:32 -0000	1.10
> +++ src/history/HistoryOutlineViewDelegate.mm	13 Feb 2006 06:44:54 -0000
> @@ -47,12 +47,14 @@
>  #import "BrowserWindowController.h"
>  #import "BookmarkViewController.h"
>  #import "MainController.h"
>  
>  #import "HistoryOutlineViewDelegate.h"
>  
> +#include <Carbon/Carbon.h>
> +

Put framework includes at the top, please.

> +++ src/embedding/CHEditorSpellCheck.h	2006-02-12 19:54:13.000000000 -0800

> +/*
> + * Class CHEditorSpellCheck: a replacement for the class
> + * nsEditorSpellCheck, loaded into the spellcheck module at runtime.
> + */
> +class CHEditorSpellCheck: public nsIEditorSpellCheck 
> +{
> +public:
> +  CHEditorSpellCheck();
> +  virtual ~CHEditorSpellCheck();
> +  
> +  NS_DECL_ISUPPORTS
> +  NS_DECL_NSIEDITORSPELLCHECK
> +    
> +  // get a shared spell checker that can be used (and recycled) during manual spellcheck
> +  static nsresult GetSharedSpellChecker(nsIEditorSpellCheck** aSpellCheck);
> +  void SetSpellDocumentTag(int spellDocTag);

Why a setter and no getter?

> --- /dev/null	2006-02-12 22:53:27.000000000 -0800
> +++ src/embedding/CHEditorSpellCheck.mm	2006-02-04 01:57:23.000000000 -0800

> +// static
> +nsresult CHEditorSpellCheck::GetSharedSpellChecker(nsIEditorSpellCheck** aSpellCheck)
> +{
> +  if (!sSharedSpellCheck) {
> +    sSharedSpellCheck = new CHEditorSpellCheck;
> +  }
> +  *aSpellCheck = sSharedSpellCheck;
> +  return *aSpellCheck ? NS_OK : NS_ERROR_NULL_POINTER;
> +}

Someone needs to hold a reference to this object, otherwise it might
get deleted.

> +NS_IMETHODIMP
> +CHEditorSpellCheck::InitSpellChecker(nsIEditor* editor, PRBool enableSelectionChecking)
> +{
> +  nsresult rv;
> +  
> +  // we try to reuse the same EditorSpellCheck if the user is only using manual check
> +  if (editor == mEditor) {
> +    NSLog(@"-[CHEditorSpellCheck InitSpellChecker]: editor the same, not reiniting");
> +    return NS_OK;
> +  }

Remove NSLog

> +// spell check with wrapping
> +NS_IMETHODIMP
> +CHEditorSpellCheck::GetNextMisspelledWord(PRUnichar** nextMisspelledWord)
> +{
> +  // It would be nice to get the flat string of the text frame and pass the
> +  // whole thing to NSSpellChecker. However,
> +  // we wouldn't know the block index and offset of the misspelled word in
> +  // the TSD.
> +  nsresult rv = EnsureConverter();
> +  NS_ENSURE_SUCCESS(rv, rv);
> +  if(!mWordUtil)
> +    return NS_ERROR_NULL_POINTER;
> +  
> +  // Here we build a range that has the entire editor contents and reinit
> +  // the TSD with it
> +  nsCOMPtr<nsIDOMRange> docRange = 
> +    do_CreateInstance("@mozilla.org/content/range;1", &rv);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +  rv = docRange->SelectNodeContents(mDocRoot);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +  rv = mTsDoc->SetExtent(docRange);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +  
> +  PRUint32 selOffset;
> +  rv = SetupDoc(&selOffset);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +  
> +  PRInt32 begin,end;
> +  PRBool isMisspelled = PR_FALSE;
> +  PRBool docEOF = PR_FALSE;
> +  PRBool wrapped = PR_FALSE;
> +  nsCOMPtr<nsIDOMRange> wrapExtent;
> +  // get a range of the current cursor position which we might use for wrapping
> +  rv = mTsDoc->GetDOMRangeFor(selOffset, 0, getter_AddRefs(wrapExtent));
> +  NS_ENSURE_SUCCESS(rv, rv);
> +  
> +  while (!isMisspelled && (!wrapped || !docEOF))
> +  {
> +    nsString str;
> +    rv = mTsDoc->GetCurrentTextBlock(&str);
> +    
> +    if (NS_FAILED(rv))
> +      return rv;

Remove blank line before, add one after.

> +    do {
> +      rv = mWordUtil->FindNextWord(str.get(),str.Length(),selOffset,&begin,&end);

Spaces after commas please.

> +      if(NS_SUCCEEDED(rv)&&(begin != -1)){

Space after if.

> +        const nsAString &currWord = Substring(str, begin, end - begin);
> +        rv = CheckWordInternal(currWord, &isMisspelled, YES);
> +        NS_ENSURE_SUCCESS(rv, rv);
> +        if(isMisspelled){
> +          *nextMisspelledWord = ToNewUnicode(currWord);
> +          rv = mTsDoc->SetSelection(begin, end-begin);
> +          rv = mTsDoc->ScrollSelectionIntoView();
> +          break;
> +        }
> +      }
> +      if (end != -1)
> +        selOffset = end;
> +    } while (end != -1);
> +    
> +    // wrapping stuff here: we've reached the end, gotta wrap
> +    // lazily set the start of the wrap range and set the TSD extent
> +    if (NS_SUCCEEDED(mTsDoc->IsDone(&docEOF)) && docEOF && !wrapped) {
> +      rv = wrapExtent->SetStart(mDocRoot, 0);
> +      NS_ENSURE_SUCCESS(rv, rv);
> +      rv = mTsDoc->SetExtent(wrapExtent);
> +      NS_ENSURE_SUCCESS(rv, rv);
> +      rv = mTsDoc->FirstBlock();
> +      NS_ENSURE_SUCCESS(rv, rv);
> +      wrapped = PR_TRUE;
> +      docEOF = PR_FALSE;
> +    }
> +    else
> +      mTsDoc->NextBlock();
> +    
> +    selOffset=0;
> +  }
> +  
> +  if (!isMisspelled)
> +    *nextMisspelledWord = ToNewUnicode(NS_LITERAL_STRING(""));

Can't just return nsnull?

> +  return rv;
> +}
> +
> +NS_IMETHODIMP
> +CHEditorSpellCheck::GetSuggestedWord(PRUnichar** suggestedWord)
> +{
> +  nsAutoString suggestion;
> +  if (mCurGuessIndex < [mCurGuesses count]) {
> +    [[mCurGuesses objectAtIndex:mCurGuessIndex++] assignTo_nsAString:suggestion];
> +  }

What if mCurGuessIndex is still -1 ?

> +  *suggestedWord = ToNewUnicode(suggestion);
> +  return NS_OK;
> +}

> +NS_IMETHODIMP
> +CHEditorSpellCheck::CheckWordInternal(const nsAString& word, PRBool* isMisspelled,
> +                                      BOOL computeSuggestions)
> +{
> +  // This is the case at the start of inline spellcheck
> +  if (mSpellDocTag == -1) {
> +    nsCOMPtr<nsIDOMDocument> domDoc;
> +    nsresult rv = mEditor->GetDocument(getter_AddRefs(domDoc));
> +    nsCOMPtr<nsIDocument> doc = do_QueryInterface(domDoc);
> +    nsCOMPtr<nsISupports> container = doc->GetContainer();
> +    nsCOMPtr<nsPIDOMWindow> window = do_GetInterface(container);
> +    
> +    CHBrowserView* browserView = [CHBrowserView browserViewFromDOMWindow:window];
> +    if (!browserView) {
> +      NSLog(@"Failed to get current browser view");
> +      return NS_ERROR_FAILURE;
> +    }
> +    mSpellDocTag = [browserView spellDocumentTag];
> +  }

This is pretty weird, that we reach over to get a CHBrowserView, just
to getthe spell check doc tag. Is there a better way?

> +  NSSpellChecker* spChecker = [NSSpellChecker sharedSpellChecker];
> +  NSString* cocoaWord = [NSString stringWith_nsAString:word];
> +  NSRange spRange = [spChecker checkSpellingOfString:cocoaWord 
> +                                          startingAt:0
> +                                            language:nil
> +                                                wrap:NO
> +                              inSpellDocumentWithTag:mSpellDocTag
> +                                           wordCount:NULL];
> +  
> +  *isMisspelled = (spRange.length != 0);
> +  
> +  if (computeSuggestions) {
> +    [mCurGuesses release];
> +    mCurGuesses = nil;
> +    mCurGuessIndex = -1;
> +    if (*isMisspelled) {
> +      mCurGuesses = [[spChecker guessesForWord:cocoaWord] retain];
> +      mCurGuessIndex = 0;
> +    }
> +  }
> +  
> +  return NS_OK;
> +}
> +
> +NS_IMETHODIMP
> +CHEditorSpellCheck::CheckCurrentWordNoSuggest(const PRUnichar* prWord,
> +                                              PRBool* isMisspelled)
> +{
> +  nsresult rv = CheckWordInternal(nsDependentString(prWord), isMisspelled, NO);
> +  return rv;
> +}
> +
> +NS_IMETHODIMP
> +CHEditorSpellCheck::ReplaceWord(const PRUnichar* misspelledWord, 
> +                                const PRUnichar* replaceWord, 
> +                                PRBool allOccurrences)
> +{
> +  return NS_ERROR_NOT_IMPLEMENTED;
> +}
> +
> +// this method IS used

And the others aren't?

> +NS_IMETHODIMP
> +CHEditorSpellCheck::IgnoreWordAllOccurrences(const PRUnichar* prWord)
> +{
> +  if (mSpellDocTag < 0)
> +    NSLog(@"Whoa, IgnoreWordAllOccurrences: spell tag not set");
> +  NSString* word = [NSString stringWith_nsAString:nsDependentString(prWord)];
> +  [[NSSpellChecker sharedSpellChecker] ignoreWord:word inSpellDocumentWithTag:mSpellDocTag];
> +  return NS_OK;
> +}
> +
> +NS_IMETHODIMP
> +CHEditorSpellCheck::GetPersonalDictionary()
> +{
> +  return NS_ERROR_NOT_IMPLEMENTED;
> +}
> +
> +NS_IMETHODIMP
> +CHEditorSpellCheck::GetPersonalDictionaryWord(PRUnichar **word)
> +{
> +  return NS_ERROR_NOT_IMPLEMENTED;
> +}
> +
> +NS_IMETHODIMP
> +CHEditorSpellCheck::AddWordToDictionary(const PRUnichar *word)
> +{
> +  return NS_ERROR_NOT_IMPLEMENTED;
> +}
> +
> +NS_IMETHODIMP
> +CHEditorSpellCheck::RemoveWordFromDictionary(const PRUnichar *word)
> +{
> +  return NS_ERROR_NOT_IMPLEMENTED;
> +}
> +
> +NS_IMETHODIMP
> +CHEditorSpellCheck::GetDictionaryList(PRUnichar ***dictionaryList, PRUint32 *count)
> +{
> +  return NS_ERROR_NOT_IMPLEMENTED;
> +}
> +
> +NS_IMETHODIMP
> +CHEditorSpellCheck::GetCurrentDictionary(PRUnichar** dictionary)
> +{
> +  return NS_ERROR_NOT_IMPLEMENTED;
> +}
> +
> +NS_IMETHODIMP
> +CHEditorSpellCheck::SetCurrentDictionary(const PRUnichar* dictionary) 
> +{
> +  return NS_ERROR_NOT_IMPLEMENTED;
> +}
> +
> +NS_IMETHODIMP
> +CHEditorSpellCheck::UninitSpellChecker() 
> +{
> +  return NS_OK;
> +}
> +
> +NS_IMETHODIMP
> +CHEditorSpellCheck::SetFilter(nsITextServicesFilter* filter)
> +{
> +  mTxtSrvFilter = filter;
> +  return NS_OK;
> +}
> +
> +NS_IMETHODIMP
> +CHEditorSpellCheck::SetupDoc(PRUint32 *outBlockOffset)
> +{
> +  nsresult  rv;
> +  
> +  nsITextServicesDocument::TSDBlockSelectionStatus blockStatus;
> +  PRInt32 selOffset;
> +  PRInt32 selLength;
> +  
> +  rv = mTsDoc->LastSelectedBlock(&blockStatus, &selOffset, &selLength);
> +  if (NS_SUCCEEDED(rv) && (blockStatus != nsITextServicesDocument::eBlockNotFound))
> +  {
> +    switch (blockStatus)
> +    {
> +      case nsITextServicesDocument::eBlockOutside:  // No TB in S, but found one before/after S.
> +      case nsITextServicesDocument::eBlockPartial:  // S begins or ends in TB but extends outside of TB.
> +                                                    // the TS doc points to the block we want.
> +        *outBlockOffset = selOffset + selLength;
> +        break;
> +        
> +      case nsITextServicesDocument::eBlockInside:  // S extends beyond the start and end of TB.
> +                                                   // we want the block after this one.
> +        rv = mTsDoc->NextBlock();
> +        *outBlockOffset = 0;
> +        break;
> +        
> +      case nsITextServicesDocument::eBlockContains: // TB contains entire S.
> +        *outBlockOffset = selOffset + selLength;
> +        break;
> +        
> +      case nsITextServicesDocument::eBlockNotFound: // There is no text block (TB) in or before the selection (S).
> +      default:
> +        NS_NOTREACHED("Shouldn't ever get this status");
> +    }
> +  }
> +  else  //failed to get last sel block. Just start at beginning
> +  {
> +    rv = mTsDoc->FirstBlock();
> +    *outBlockOffset = 0;
> +  }
> +  
> +  return rv;
> +}
> +
> +NS_IMETHODIMP
> +CHEditorSpellCheck::EnsureConverter()
> +{
> +  nsresult res = NS_OK;
> +  if (!mWordUtil)
> +  {
> +    nsCOMPtr<mozISpellI18NManager> manager(do_GetService("@mozilla.org/spellchecker/i18nmanager;1", &res));
> +    if (manager && NS_SUCCEEDED(res))
> +    {
> +      nsXPIDLString language;
> +      res = manager->GetUtil(language.get(), getter_AddRefs(mWordUtil));
> +    }
> +  }
> +  return res;
> +}


General comments:

I find it a little weird that both CHBrowserView and CHEditorSpellCheck are talking to the Cocoa spell checker.
Can we push all the interaction with NSSpellChecker into one class?

I'm concerned about adding all that code to CHBrowserView, especially since it sucks in so many non-embedding
headers. And, we have to avoid including higher-level headers in lower-level files.

I'd also be interested to hear about how the inline spell check "feels" compared to Cocoa's. Does it behave exactly
the same way, or does it feel odd and clunky?

Comment 73

[Mike Pinkerton (not reading bugmail)]

I was going to work on refactoring this so that it could be used by firefox as well and get it landed. Brett said the core stuff should be on the 18branch by the end of this week.

I'll start with the patch julian posted and clean that up and report back.

Comment 74

[Mike Pinkerton (not reading bugmail)]

julian, what was the reasoning behind replacing nsEditorSpellCheck rather than reimplementing just the code behind nsISpellChecker? Is it because you needed all that additional contextual info? 

I'm just worried about copying all that code, it's more to maintain and get out of sync.

Comment 75

[Julian Pellico]

(In reply to comment #74)
> julian, what was the reasoning behind replacing nsEditorSpellCheck rather than
> reimplementing just the code behind nsISpellChecker? Is it because you needed
> all that additional contextual info? 
> 
> I'm just worried about copying all that code, it's more to maintain and get out
> of sync.
> 

Hoo boy, let's see. I think I was primarily concerned about all the dictionary stuff in the nsEditorSpellCheck. The CanSpellCheck method requires at least one dictionary, and I was kind of confused about how to handle it. However, I can't think of anything really compelling right now. I looked at the code and I'm not sure what contextual info I gained. I'll let you know if I think of anything else.

Comment 76

[Reinout van Schouwen]

The latest comment on bug 302050 seems to indicate that the core patches from this bug should be going in to gecko 1.8.1. Is this the case?

Comment 77

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Blocker as dep for blocker, and general goodness.

Comment 78

[Mike Pinkerton (not reading bugmail)]

i have a re-implementation of the mozISpellCheckingEngine on top of NSSpellChecker. Much smaller patch, I'll include it a bit later once I get some of the context menu stuff hooked up for camino to make sure that part works as well.

Comment 79

[Mike Pinkerton (not reading bugmail)]

brett: why does the mySpell spellchecker's makefile specifically disable static linkage? Any ideas? I can make it initially just a dylib, but at some point we'd like to make it static for Camino. Is it a license issue with mySpell, which the OSX impl wouldn't have, or is it for some other reason (like ease of disabling)?

Comment 80

[Brett Wilson]

I don't know why it's static. The license should be OK. All the spellchecking stuff is kind of separate, like an extension. I suspect it's that way just for disabling. I don't have any feeling of how it should be.

Comment 81

[Julian Pellico]

Mike, sweet. I'm eager to take a gander at that patch. =)

Comment 82

[Mike Pinkerton (not reading bugmail)]

Attachment: replace mySpell engine

Here's the gecko part of the patch that replaces the mySpell engine with one based on NSSpellChecker. I'll attach the camino part separately once I get some of the dylib v. static issues dealt with.

Comment 83

[Mike Pinkerton (not reading bugmail)]

Attachment: Camino portion, sans nib and project changes (trunk)

here's the chunk for Camino that builds the suggestion context menu. Nib and project changes will come later, they're not too complicated. Note that this will only build on the trunk, there are api changes needed for the branch, but they're small.

Comment 84

[Julian Pellico]

Be sure you get the spelling actions into Edit->Spelling. I had code in the BroswserView to do this (didn't see any actions in the patch...) Make sure that the spellcheck wraps when you check manually; this didn't happen by default with the nsEditorSpellCheck stuff, I had to take care of that myself.

Comment 85

[Håkan Waara]

Comment on attachment 220399 [details] [diff] [review]
replace mySpell engine

* Ideally, you'd add the NSString category somewhere global and fix bug 334670. (We need mac string utilites in the tree.) 

Otherwise, please note in that bug that this category will need to be removed later.

* There's some weird tab/space inconsistencies in the patch... :-)

* Is the personal dictionary in OS X "global"? Will we need to hook up gecko's mozIPersonalDictionary to that later?

Other than those things, r=me fwiw.

Comment 86

[Håkan Waara]

Comment on attachment 220401 [details] [diff] [review]
Camino portion, sans nib and project changes (trunk)

>@@ -3362,9 +3392,20 @@
> 
>   const int kFrameRelatedItemsTag = 100;
>   const int kFrameInapplicableItemsTag = 101;
>   const int kSelectionRelatedItemsTag = 102;
>+  const int kSpellingRelatedItemsTag = 103;
>   
>+  if (showSpellingItems)
>+    showSpellingItems = [self prepareSpellingSuggestionMenu:result tag:kSpellingRelatedItemsTag];
>+
>+  if (!showSpellingItems) {
>+      // word spelled correctly or not applicable, remove all traces of spelling items
>+      NSMenuItem* selectionItem;
>+      while ((selectionItem = [result itemWithTag:kSpellingRelatedItemsTag]) != nil)
>+        [result removeItem:selectionItem];
>+    }
>+

How about:

if (showSpellingItems)
  ...
else {
  ...
}

Also, some weird whitespace inconsistencies in this patch too. 2 space vs 4 space indenting, and some tabs.

>+- (BOOL)prepareSpellingSuggestionMenu:(NSMenu*)inMenu tag:(int)inTag
>+{
>+  #define ENSURE_TRUE(x) if (!x) return NO;

I'm not sure I like the temporary-#define stuff in this method and replaceMispelledWord:.

For this one, perhaps you can define it globally (with the bool return), and for the #define in replaceMispelledWord:, define ENSURE_EXISTS() globally?

...

>+  nsCOMPtr<nsISelection> selection;
>+  editor->GetSelection(getter_AddRefs(selection));
>+  ENSURE_TRUE(selection);
>+  PRInt32 anchorOffset;
>+  selection->GetAnchorOffset(&anchorOffset);
>+  nsCOMPtr<nsIDOMNode> anchorNode;
>+  selection->GetAnchorNode(getter_AddRefs(anchorNode));

This chunk (+ the fetching of the spellchecker) is also done in replaceMispelledWord:, so I think it deserves a GeckoUtils.h helper.

>+  nsCOMPtr<nsIDOMRange> mispelledRange;
>+  inlineChecker->GetMispelledWord(anchorNode, (long)anchorOffset, getter_AddRefs(mispelledRange));

Wanna move this down a bit, where the if (mispelledRange) check is?

>+  nsCOMPtr<nsIEditorSpellCheck> spellCheck;
>+  inlineChecker->GetSpellChecker(getter_AddRefs(spellCheck));
>+  ENSURE_TRUE(spellCheck);

* Is the spellchecker something that is per <editor>? If we're fetching a service, we could cache it.

r=me

Comment 87

[Mike Pinkerton (not reading bugmail)]

How about:

if (showSpellingItems)
  ...
else {
  ...
}

Re-read the code, the point is that |showSpellingItems| can change and we need to re-test it. It's not just 1 check that can do with a single if-then-else.

Comment 88

[Håkan Waara]

(In reply to comment #87)
> How about:
> 
> if (showSpellingItems)
>   ...
> else {
>   ...
> }
> 
> Re-read the code, the point is that |showSpellingItems| can change and we need
> to re-test it. It's not just 1 check that can do with a single if-then-else.
> 

Ah, now I see that, thanks.

Comment 89

[Mark Mentovai]

Comment on attachment 220399 [details] [diff] [review]
replace mySpell engine

>Index: osxspell/src/Makefile.in
>+MOZILLA_INTERNAL_API = 1

Do you need that?

>Index: osxspell/src/mozOSXSpell.mm
>+// SetPersonalDictionary
>+//
>+// Hold onto the personal dictionare we're given.

Typo

>+NS_IMETHODIMP mozOSXSpell::Check(const PRUnichar *aWord, PRBool *aResult)
>+{
>+  NS_ENSURE_ARG_POINTER(aWord);
>+  NS_ENSURE_ARG_POINTER(aResult);
>+	*aResult = PR_FALSE;

Detab

>Index: osxspell/src/mozOSXSpellFactory.cpp
>+ * The Initial Developer of the Original Code is Mike Pinkerton.

We usually put Google Inc. there.

>+ * Contributor(s): Mike Pinkerton <[e-mail]>

with your name on a new line?  I don't care, I think you know how I feel about licensing...

Comment 90

[[e-mail]]

*** Bug 339259 has been marked as a duplicate of this bug. ***

Comment 91

[Brian Ryner (not reading)]

Comment on attachment 220399 [details] [diff] [review]
replace mySpell engine

>+CPPSRCS = \
>+		  mozOSXSpellFactory.cpp \
>+			$(NULL)
>+			
>+CMMSRCS = \
>+			mozOSXSpell.mm \
>+			$(NULL)

I don't suppose it would be possible to just combine these files into one .mm file?

>--- /dev/null	1 Jan 1970 00:00:00 -0000
>+++ osxspell/src/mozOSXSpell.h	1 May 2006 16:18:07 -0000
>+class mozOSXSpell : public mozISpellCheckingEngine
>+{
>+public:
>+  NS_DECL_ISUPPORTS
>+  NS_DECL_MOZISPELLCHECKINGENGINE
>+
>+  mozOSXSpell();
>+  virtual ~mozOSXSpell();
>+
>+protected:

Do you intend to subclass this?  If not, make the destructor private and nonvirtual, and change protected: to private:.

>+ 
>+	// NSSpellChecker provides the ability to add words to the local dictionary,
>+	// but it's much easier to let the rest of Gecko handle that via the personal
>+	// dictionary given to us and just be ignorant about new words.

Tabs.

>--- /dev/null	1 Jan 1970 00:00:00 -0000
>+++ osxspell/src/mozOSXSpellFactory.cpp	1 May 2006 16:18:07 -0000
>+static nsModuleComponentInfo components[] = {
>+  { NULL, MOZ_OSXSPELL_CID, MOZ_OSXSPELL_CONTRACTID, mozOSXSpellConstructor }

It wouldn't hurt to have a name here instead of NULL, just for ease of debugging.

Looks good otherwise.

Comment 92

[Mike Pinkerton (not reading bugmail)]

Attachment: fixes all issues raised during review, landed on trunk

Comment 93

[Mike Pinkerton (not reading bugmail)]

trunk patch checked in there, should be available for firefox, i'll work on adding it to the camino build in the next step. I also need branch approval for that patch, as it seems to compile fine there.

Comment 94

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 224315 [details] [diff] [review]
fixes all issues raised during review, landed on trunk

requesting branch approval

Comment 95

[Mike Pinkerton (not reading bugmail)]

landed on the branch (only for camino since it can't work with carbon firefox), set the pref on both trunk and branch to enable spellcheck for camino.

i still have to land the patch that hooks up the context menu for suggestions, but that doesn't require project changes and can be done easily (i only have one mac that has XCode2.0 on it).

Comment 96

[philippe (part-time)]

Could this have caused bug 341980 (Camino) and bug 341317 (Firefox/core) ?
(tested with trunk builds only)

Comment 97

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

This doesn't appear to actually *work* on the branch (at least, no little red dots are drawn), or at least not on 10.3.9.  Checked a clean profile and the latest branch nightly....

Comment 98

[Mike Pinkerton (not reading bugmail)]

i can't see how it could have caused that regression, these patches do no drawing of their own. 

you're right, the branch doesn't work. we're missing spellcheck.xpt which i guess i biffed in the static target. I don't have access to an XCode2.0 machine this week in mtnView, so it will have to wait a week, unless mento can get to it.

Comment 99

[Mark Mentovai]

Attachment: Enable osxspell on 1.8 branch (checked in)

The project file is fine, we've got spellchecker.xpt in both targets.  The problem is that osxspell is in use instead of myspell, because the Makefile change wasn't made on the 1.8[.1] branch.  Because myspell isn't finding its dictionaries (they were correctly not added to the project file), spell checking is disabled.  This patch turns on osxspell when Cocoa widgets are in use, as on the trunk.

Comment 100

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 226615 [details] [diff] [review]
Enable osxspell on 1.8 branch (checked in)

wtf. i wonder how this got missed. thanks for catching it. 

r/sr=pink

Comment 101

[Mark Mentovai]

Comment on attachment 226615 [details] [diff] [review]
Enable osxspell on 1.8 branch (checked in)

Checked in on MOZILLA_1_8_BRANCH before 1.8.1b1.

Comment 102

[Matthew Hutton]

In the latest nightly (26-06-06) Camino underlines spellings when they are incorrect (which is excellent btw.) however it doesn't seem to be possible to bring up a list of suggested spelling corrections as in other programs.

Comment 103

[Shawn Medero]

(In reply to comment #102)
> it doesn't seem to be possible to bring up a list of suggested spelling corrections as in other programs.

Mike said in comment #95 that he _did not_ check in support for the contextual menu items, etc. etc. etc.

Comment 104

[Brandon Sharitt]

Is it just me, or does the spell checker(or at least the red underliner) not work in rich text boxes, such as those in GMail, Wordpress, and many forums? Those are generally the ones I need spell checking the most, and the ones that specifically don't work right in Safari(no rich text options).

Comment 105

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #104)
> Is it just me, or does the spell checker(or at least the red underliner) not
> work in rich text boxes, such as those in GMail, Wordpress, and many forums?

That's bug 333038.

Comment 106

[Mike Pinkerton (not reading bugmail)]

Attachment: Camino portion (no nib changes) landed on trunk

Comment 107

[Mike Pinkerton (not reading bugmail)]

landed the camino part on the trunk, still doing branch builds to get that ready (on my dog-slow pbg4)

Comment 108

[Mike Pinkerton (not reading bugmail)]

context menu suggestions landed on branch as well.

enjoy.

Comment 109

[Chris Lawson (gone)]

Verified with latest trunk nightly.

Comment 110

[Patrick]

I'm a bit confused now if this is a Camino-only bug or if morphed into a core bug, affecting Firefox and Tb as well?

Anyway, my current OS X install doesn't provide a Norwegian spell checker. For a while i could add the necessary spell.xpi files to add that functionality. 

However, since we hooked in to the the spell checkers provided by OS X, i can't seem to use the spell check functionality by the xpi files anymore. They got installed successfully but i'm not able to activate them. 

Am i missing something, or should i file a spin-off bug?

Comment 111

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #110)
> I'm a bit confused now if this is a Camino-only bug or if morphed into a core
> bug, affecting Firefox and Tb as well?

It's a Camino bug that happens to have required a "bit" of Core work.

 
> Anyway, my current OS X install doesn't provide a Norwegian spell checker. ... 
> Am i missing something, or should i file a spin-off bug?

No, we only use the Mac OS X dictionaries in Camino.  You should check out cocoAspell <http://cocoaspell.leuski.net/>, which apparently plugs in to the Mac OS X spelling system and offers a wider selection of languages, including Norwegian.  (For Hebrew speakers, there's Hebrew Spelling Service <http://www.mitzpettel.com/software/hspell.php> that does the same thing and is tweaked for Hebrew.)

Comment 112

[Mike Pinkerton (not reading bugmail)]

Firefox *trunk* uses the OS X spellchecker as well, and would have this problem.

Comment 113

[Patrick]

(In reply to comment #112)
> Firefox *trunk* uses the OS X spellchecker as well, and would have this
> problem.
> 

I've filed bug 345859 for this.

Comment 114

[Mark Mentovai]

In case anyone goes looking for trouble in fox-trunk, note that carbonfox uses myspell, while cocoafox uses osxspell.  The trunk builds have not been switched to cocoafox yet.  All nightly foxes and birds with spell checking are still using myspell.

Sincerely,
The zookeeper