The spelling checker opens dictionary files in the content process, instead of using IPC to ask the parent process to do so. Additionally, it opens the directory containing those files to enumerate them.

Ally is working on this for e10s.

These are a few nits that I found in the code (including one heinous misspelling of the word "dictionary") as well as adding some MOZ_OVERRIDEs and virtuals that are nice indicators of what's going on.

This avoids creating mozHunspell instances in the child.

Comment on attachment 8483055 [details] [diff] [review] Fix a few nits. Review of attachment 8483055 [details] [diff] [review]: ----------------------------------------------------------------- The number of spelling mistakes in this code is quite ironic. ;-) ::: editor/composer/nsEditorSpellCheck.cpp @@ -699,5 @@ > } > NS_ENSURE_TRUE(rootContent, NS_ERROR_FAILURE); > > - DictionaryFetcher* fetcher = new DictionaryFetcher(this, aCallback, > - mDictionaryFetcherGroup); o_O I'm not sure how I did not catch this earlier. :( @@ +763,5 @@ > if (dictName.IsEmpty()) { > dictName.Assign(preferedDict); > } > > + nsresult rv = NS_OK; Why is this needed?!

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #6) > Why is this needed?! It technically isn't needed, but it's better to put declarations as close as their first-use as possible.

Comment on attachment 8483058 [details] [diff] [review] Avoid mozHunspell in the child altogether. Review of attachment 8483058 [details] [diff] [review]: ----------------------------------------------------------------- Can we somehow prevent the mozHunspell XPCOM component from being created in the child process altogether? ::: extensions/spellcheck/hunspell/src/mozHunspell.cpp @@ +611,5 @@ > { > mDynamicDirectories.RemoveObject(aDir); > LoadDictionaryList(); > + > + ContentParent::NotifyUpdatedDictionaries(); Please move this to LoadDictionaryList instead. ::: extensions/spellcheck/src/mozSpellChecker.cpp @@ +359,4 @@ > mozSpellChecker::GetCurrentDictionary(nsAString &aDictionary) > { > + if (XRE_GetProcessType() == GeckoProcessType_Content) { > + aDictionary = mCurrentDictionary; This won't work if someone calls mozISpellCheckingEngine::SetDictionary in the parent process. Can we just forward this to the parent so that we have one single source of truth for this?

Comment on attachment 8483059 [details] [diff] [review] Make selecting dictionaries work. Review of attachment 8483059 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/modules/InlineSpellChecker.jsm @@ +8,5 @@ > const MAX_UNDO_STACK_DEPTH = 1; > > +const Cc = Components.classes; > +const Ci = Components.interfaces; > +const Cu = Components.utils; Cc and Cu are unused in this file. @@ +167,2 @@ > spellchecker.GetDictionaryList(o1, o2); > + var list = Cu.waiveXrays(o1.value); Please get bholley to review this part. I don't think he'll like it. ;-)

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #8) > Can we just forward this to the parent so that we have one single source of > truth for this? I'd rather not do that mostly because we don't have to. We also call SetCurrentDictionary every time we focus a textarea or a window which sets things everywhere.

This addresses ehsan's comment 8 modulo comment 10.

In order to have the parent and child on the same page wrt which elements were editable or needed spellchecking menu items, I needed to refactor some common code out. I also attempt to avoid recalculating the same information twice.

This supercedes my previous "make selecting dictionaries work" patch. It avoids CPOWs as much as possible in the spellchecking contextmenu code. It turned out to be very difficult for me to interpose my "RemoteInlineSpellChecker" in the parent. To be honest, I was unable to actually try down how any of these objects are created. This patch attempts to shim the InlineSpellChecker object in the parent as cleanly as possible. I'm not thrilled about the mapping from spellInfo -> RemoteSpellChecker, but this seems to work. I had a bunch of trouble getting all of the states right (no dictionaries vs. turned off vs turned on). This does appear to work, though.

I still have to check with the Finnish add-on, but we can't use the default engine directly, this should Just Work (TM).

\o/

Comment on attachment 8486152 [details] [diff] [review] Don't use the default engine directly since we might not be using it. Review of attachment 8486152 [details] [diff] [review]: ----------------------------------------------------------------- Please test this with that Finnish spell checker.

Comment on attachment 8486153 [details] [diff] [review] de-rpc-ize PRemoteSpellcheckEngine Review of attachment 8486153 [details] [diff] [review]: ----------------------------------------------------------------- ::: extensions/spellcheck/hunspell/src/PRemoteSpellcheckEngine.ipdl @@ +5,5 @@ > include protocol PContent; > > namespace mozilla { > > +sync protocol PRemoteSpellcheckEngine { I don't know what these changes mean. Please ask someone else review this part. ::: extensions/spellcheck/hunspell/src/RemoteSpellCheckEngineParent.h @@ +24,1 @@ > bool* success) MOZ_OVERRIDE; Nit: indentation. @@ +29,2 @@ > bool* aIsMisspelled, > InfallibleTArray<nsString>* aSuggestions) Nit: indentation.

Moving old M2 P1 bugs to M3.

Comment on attachment 8486150 [details] [diff] [review] Refactor target-inspecting code from the context menu Review of attachment 8486150 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/modules/InlineSpellChecker.jsm @@ +363,5 @@ > + INPUT: 0x2, > + TEXTAREA: 0x4, > + > + TEXTINPUT: 0x8, > + KEYWORD: 0xA, Obviously, KEYWORD should be 0x10.

This isn't strictly necessary, but it avoids using CPOWs a few times during the computation.

I had to treat the "contenteditable" case completely separately to deal with password fields (which was one of the causes of the mochitest orange).

Comment on attachment 8498967 [details] [diff] [review] fix contenteditable Review of attachment 8498967 [details] [diff] [review]: ----------------------------------------------------------------- Can you explain in isEditable why sometimes it gets the EDITABLE flag and sometimes the CONTENTEDITABLE? ::: browser/base/content/nsContextMenu.js @@ +751,1 @@ > // If this.onEditableArea is false but editFlags is EDITABLE, then nit: update comment for CONTENTEDITABLE

Comment on attachment 8486151 [details] [diff] [review] Don't use CPOWs for the spellchecker in the context menu. Review of attachment 8486151 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/modules/InlineSpellCheckerContent.jsm @@ +6,5 @@ > +"use strict"; > + > +let {classes: Cc, interfaces: Ci, utils: Cu, results: Cr} = Components; > + > +Cu.import("resource://gre/modules/InlineSpellChecker.jsm"); übernit, but could you change it to: let { SpellCheckHelper } = Cu.import("resource://gre/modules/InlineSpellChecker.jsm"); It will make it more clear why this is being imported here in the content side, because in theory it doesn't make sense just by looking at the filename (which is supposed to be a module used in the parent)

https://hg.mozilla.org/integration/mozilla-inbound/rev/91ffa2ab03aa I squashed all of the commits into one since the intermediate steps were never tested independently.

This sounds like internal plumbing, no need to do manual QA on that, I think.