Profiling Google Docs we discovered that we're initing the spell checker for Google Docs, even though Google use their own spell checker. This causes a sync IPC call to SendSetDictionary, which can take between 20~50ms on my high end machine. For example, this profile, on the test case for (bug 1326349): https://clptr.io/2jGYcyj Potentially this can happen every time we switch the dictionary language, which I'm told an happen multiple times at runtime. We can prevent us loading the dict by checking whether the focus is spellcheck=false in EditorBase::OnFocus(). http://searchfox.org/mozilla-central/rev/a712d69adb9b2588f88aff678216b2be94d3719c/editor/libeditor/EditorBase.cpp#5148

Comment on attachment 8827304 [details] Bug 1330912 - Don't init spell checker dictionary on spellcheck=false content. https://reviewboard.mozilla.org/r/105018/#review105856 ::: editor/libeditor/EditorBase.cpp:108 (Diff revision 1) > #include "nsStyleStructFwd.h" // for nsIFrame::StyleUIReset, etc. > #include "nsTextNode.h" // for nsTextNode > #include "nsThreadUtils.h" // for nsRunnable > #include "nsTransactionManager.h" // for nsTransactionManager > #include "prtime.h" // for PR_Now > +#include "nsGenericHTMLElement.h" Please keep "A-Z" order of the included header files. And add |// for nsGenericHTMLElement to its tail. ::: editor/libeditor/EditorBase.cpp:5148 (Diff revision 1) > > void > EditorBase::OnFocus(nsIDOMEventTarget* aFocusEventTarget) > { > InitializeSelection(aFocusEventTarget); > + nsCOMPtr<nsIContent> node(do_QueryInterface(aFocusEventTarget)); nit: use nsCOMPtr<nsIContent> node = do_QueryInterface(...); style, see the coding rule: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#CC_practices And also I like "content" rather than "node" because it's a smart pointer for nsIContent, not nsINode. ::: editor/libeditor/EditorBase.cpp:5150 (Diff revision 1) > + (node->IsHTMLElement() && > + !static_cast<nsGenericHTMLElement*>(node.get())->Spellcheck())) { I think that this is wrong. When it's in design mode, aFocusEventTarget is probably the document node. I guess that you need to use GetEditorRoot()? But if you need <input> or <textarea> when it's a plaintext editor, you need to use GetExposedRoot()? Although, the latter isn't implemented in HTMLEditor.

Comment on attachment 8827304 [details] Bug 1330912 - Don't init spell checker dictionary on spellcheck=false content. https://reviewboard.mozilla.org/r/105018/#review111372 ::: editor/libeditor/EditorBase.cpp:5150 (Diff revision 1) > + (node->IsHTMLElement() && > + !static_cast<nsGenericHTMLElement*>(node.get())->Spellcheck())) { You're right. I tested 3 cases; contentEditable, designMode, and textArea. The only thing that works in all three is GetExposedRoot(). EditorBase implements GetExposedRoot(), and so since HTMLEditor extends from that, do we not need to worry about HTMLEditor not directly implementing GetExposedRoot() itself?

Comment on attachment 8827304 [details] Bug 1330912 - Don't init spell checker dictionary on spellcheck=false content. https://reviewboard.mozilla.org/r/105018/#review105856 > I think that this is wrong. When it's in design mode, aFocusEventTarget is probably the document node. > > I guess that you need to use GetEditorRoot()? But if you need <input> or <textarea> when it's a plaintext editor, you need to use GetExposedRoot()? Although, the latter isn't implemented in HTMLEditor. You're right. I tested 3 cases; contentEditable, designMode, and textArea. The only thing that works in all three is GetExposedRoot(). EditorBase implements GetExposedRoot(), and so since HTMLEditor extends from that, do we not need to worry about HTMLEditor not directly implementing GetExposedRoot() itself?

(why does changing reviewer clear review flag...)

Comment on attachment 8827304 [details] Bug 1330912 - Don't init spell checker dictionary on spellcheck=false content. https://reviewboard.mozilla.org/r/105018/#review111426

Chris, did you mean to land this patch? :-)

There are test failures on my try push, and I've not gotten around to debugging them yet.

The failing Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2acf014d27cd The failures are caused by nsGenericHTMLElement::SpellCheck() not assuming that the lack of the "spellcheck" attribute on the root implies that spell checking is not disabled. I'm not sure if nsGenericHTMLElement::SpellCheck() should or shouldn't assume that. Irrespective of that, if I change my patch to use GetExposedRoot() in the PlainTextEditor case and GetEditorRoot() otherwise, as Masayuki suggested was necessary, things work as expected. So my previous claim that GetExposedRoot() works in all cases was false; Masayuki correctly pointed out that in some cases we want to use GetExposedRoot() and other we want to use GetEditorRoot(). Try push with fixed patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4e8dba747d6023d3e6c3867347f848e2069c176d

Comment on attachment 8827304 [details] Bug 1330912 - Don't init spell checker dictionary on spellcheck=false content. Masayuki: Can you please re-review this patch here. MozReview seems to have lost the request when I re-uploaded this patch after fixing the mochitests. Thanks.

Comment on attachment 8827304 [details] Bug 1330912 - Don't init spell checker dictionary on spellcheck=false content. https://reviewboard.mozilla.org/r/105018/#review120220 ::: editor/libeditor/EditorBase.cpp:5192 (Diff revision 3) > void > EditorBase::OnFocus(nsIDOMEventTarget* aFocusEventTarget) > { > InitializeSelection(aFocusEventTarget); > + nsCOMPtr<nsIContent> content = do_QueryInterface(aFocusEventTarget); > + Element* root = IsPlaintextEditor() ? GetExposedRoot() : GetEditorRoot(); You could misunderstand about IsPlaintextEditor(). Even if it's a contenteditable editor, this may return true if addon sets plaintext editor flag. If we'd implement contenteditable="plaintext-only", this could be true. I think that GetExposedRoot() should be a virtual method and overridden by HTMLEditor. Then, it should return GetEditorRoot() from it. On the other hand, in designMode, what this should behave? In designMode, HTMLEditor::GetEditorRoot() returns <body> element (if there is). So, specifying spellcheck="false" to <body> should work even in designMode? But <html spellcheck="false"> should be ignored?

:cpearce, do you have a time to fix this after masayuki's review? If no, I will take this.

Feel free to take it.

Previous fix won't consider the following case. So GetEditorRoot isn't better for this situation. data:text/html,<div contenteditable=true spellcheck=false>ABC<br><div spellcheck=true>DEF<br></div></div>

(In reply to Makoto Kato [:m_kato] from comment #16) > Previous fix won't consider the following case. So GetEditorRoot isn't > better for this situation. > > data:text/html,<div contenteditable=true spellcheck=false>ABC<br><div > spellcheck=true>DEF<br></div></div> hmm, indeed, the spec allows such complicated behavior... https://html.spec.whatwg.org/multipage/interaction.html#spelling-and-grammar-checking Is it possible to control spellchecker working in specific nodes? Even if it's possible, it sounds not good for performance. What other browsers do? # Anyway, it's worthwhile not to load spellchecker dictionaries when moving focus to an editor which active editing host disables the spellchecker. If user wants to do it, should be able to do it forcibly from context menu.

(In reply to Masayuki Nakano [:masayuki] from comment #17) > Is it possible to control spellchecker working in specific nodes? Even if > it's possible, it sounds not good for performance. It will causes performance issue. When moving caret, we might have to load dictionary... > What other browsers do? At latest, Chrome seems to use browser's setting, not lang attribute. Also, mixed spellcheck case is same behavior as Firefox. I would not like to traverse all nodes into contenteditable due to performance, so we should change SetDictionary to async if possible. If <input> or <textarea>, we might be able to check SpellCheck() easily.

(but after adding spellcheck attribute by DOM, we might have to load it on focus even if <input> with spellcheck=false)

(In reply to Makoto Kato [:m_kato] from comment #18) > (In reply to Masayuki Nakano [:masayuki] from comment #17) > > What other browsers do? > > At latest, Chrome seems to use browser's setting, not lang attribute. Also, > mixed spellcheck case is same behavior as Firefox. > > I would not like to traverse all nodes into contenteditable due to > performance, so we should change SetDictionary to async if possible. If > <input> or <textarea>, we might be able to check SpellCheck() easily. Sound like that we should file an issue to WHATWG. I think that the attribute of any elements in active editing host should be ignored for performance.

Comment on attachment 8856399 [details] Bug 1330912 - Part 2. Use async version on UpdateCurrentDictionary. https://reviewboard.mozilla.org/r/128334/#review130850 ::: editor/composer/nsEditorSpellCheck.cpp:753 (Diff revision 2) > // dictionary from the list. This must work, if it doesn't, there is > // no point trying another one. > - break; > + return; > } > } > - return rv; > + return; nit: unnecessary return. ::: editor/composer/nsEditorSpellCheck.cpp:867 (Diff revision 2) > +nsEditorSpellCheck::SetFallbackDictionary(DictionaryFetcher* aFetcher) > +{ > + MOZ_ASSERT(mUpdateDictionaryRunning); > + > + AutoTArray<nsString, 6> tryDictList; > + > + // We obtain a list of available dictionaries. > + nsTArray<nsString> dictList; Cannot use AutoTArray here? (If so, it's okay.) ::: editor/composer/nsEditorSpellCheck.cpp:892 (Diff revision 2) > nsAutoString preferredDict; > preferredDict = Preferences::GetLocalizedString("spellchecker.dictionary"); Although, out of scope of this bug, if this method is called a lot, this might bad for performance. This might cache the value or observe the change of the value, but I'm not sure if it's possible due to localized string pref... ::: editor/composer/nsEditorSpellCheck.cpp:1013 (Diff revision 2) > + mSpellChecker->SetCurrentDictionaryFromList(tryDictList)->Then( > + AbstractThread::MainThread(), > + __func__, > + [self, fetcher]() { > - // If an error was thrown while setting the dictionary, just > + // If an error was thrown while setting the dictionary, just > - // fail silently so that the spellchecker dialog is allowed to come > + // fail silently so that the spellchecker dialog is allowed to come > - // up. The user can manually reset the language to their choice on > + // up. The user can manually reset the language to their choice on > - // the dialog if it is wrong. > + // the dialog if it is wrong. > - > - DeleteSuggestedWordList(); > - > - return NS_OK; > + self->DeleteSuggestedWordList(); > + self->EndUpdateDictionary(); > + fetcher->mCallback->EditorSpellCheckDone(); > + }); Although, I'm not sure, at reject, needs to delete the suggested words. And anyway, don't we need to call EndUpdateDictionary in such case?

Comment on attachment 8856400 [details] Bug 1330912 - Part 3. Don't update dictionary during onfocus when spellcheck is unnecessary. https://reviewboard.mozilla.org/r/128336/#review130864 ::: editor/libeditor/EditorBase.h:1086 (Diff revision 2) > // True while the instance is handling an edit action. > bool mIsInEditAction; > // Whether caret is hidden forcibly. > bool mHidingCaret; > + // Whether spellchecker dictionary is initialized after focused. > + bool mUpdatedSpellCheckerDictionary; Hmm, I like |mUpdateSpellCheckerDictionary| (reverted the meaning) or |mSpellCheckerDictionaryUpdated| better. ::: editor/libeditor/EditorBase.cpp:1352 (Diff revision 2) > if (mInlineSpellChecker) { > + if (!mUpdatedSpellCheckerDictionary && enable) { > + mInlineSpellChecker->UpdateCurrentDictionary(); > + mUpdatedSpellCheckerDictionary = true; > + } > + nit: remove the spaces of this line.

Comment on attachment 8856401 [details] Bug 1330912 - Part 4. Update some tests to wait updating dictionary. https://reviewboard.mozilla.org/r/128338/#review130866

Comment on attachment 8856398 [details] Bug 1330912 - Part 1. Add async version of SetCurrentDictionary using list. https://reviewboard.mozilla.org/r/128332/#review131196 Nice, thank you! Will we still be using the sync messages after these patches land? If no, can you please file a follow-up bug to remove them? Thanks!

Comment on attachment 8856399 [details] Bug 1330912 - Part 2. Use async version on UpdateCurrentDictionary. https://reviewboard.mozilla.org/r/128334/#review131246 ::: editor/composer/nsEditorSpellCheck.cpp:1013 (Diff revision 2) > + mSpellChecker->SetCurrentDictionaryFromList(tryDictList)->Then( > + AbstractThread::MainThread(), > + __func__, > + [self, fetcher]() { > - // If an error was thrown while setting the dictionary, just > + // If an error was thrown while setting the dictionary, just > - // fail silently so that the spellchecker dialog is allowed to come > + // fail silently so that the spellchecker dialog is allowed to come > - // up. The user can manually reset the language to their choice on > + // up. The user can manually reset the language to their choice on > - // the dialog if it is wrong. > + // the dialog if it is wrong. > - > - DeleteSuggestedWordList(); > - > - return NS_OK; > + self->DeleteSuggestedWordList(); > + self->EndUpdateDictionary(); > + fetcher->mCallback->EditorSpellCheckDone(); > + }); This uses ResoveRejectMethodType, so this lamda is be called by both case.

Comment on attachment 8856399 [details] Bug 1330912 - Part 2. Use async version on UpdateCurrentDictionary. https://reviewboard.mozilla.org/r/128334/#review131256 ::: editor/composer/nsEditorSpellCheck.cpp:1013 (Diff revision 2) > + mSpellChecker->SetCurrentDictionaryFromList(tryDictList)->Then( > + AbstractThread::MainThread(), > + __func__, > + [self, fetcher]() { > - // If an error was thrown while setting the dictionary, just > + // If an error was thrown while setting the dictionary, just > - // fail silently so that the spellchecker dialog is allowed to come > + // fail silently so that the spellchecker dialog is allowed to come > - // up. The user can manually reset the language to their choice on > + // up. The user can manually reset the language to their choice on > - // the dialog if it is wrong. > + // the dialog if it is wrong. > - > - DeleteSuggestedWordList(); > - > - return NS_OK; > + self->DeleteSuggestedWordList(); > + self->EndUpdateDictionary(); > + fetcher->mCallback->EditorSpellCheckDone(); > + }); Hmm, okay, but it's very bad thing that we cannot know the fact from the caller...

(In reply to :Ehsan Akhgari (super long backlog, slow to respond) from comment #32) > Comment on attachment 8856398 [details] > Bug 1330912 - Part 1. Add async version of SetCurrentDictionary using list. > > https://reviewboard.mozilla.org/r/128332/#review131196 > > Nice, thank you! > > Will we still be using the sync messages after these patches land? If no, > can you please file a follow-up bug to remove them? Thanks! A lot of our tests use sync version. And, context menu for selecting dictionary still uses it.

Pushed by m_kato@ga2.so-net.ne.jp: https://hg.mozilla.org/integration/autoland/rev/3facdd7bcabd Part 1. Add async version of SetCurrentDictionary using list. r=Ehsan https://hg.mozilla.org/integration/autoland/rev/d813879551cb Part 2. Use async version on UpdateCurrentDictionary. r=masayuki https://hg.mozilla.org/integration/autoland/rev/ebdf1fcdd758 Part 3. Don't update dictionary during onfocus when spellcheck is unnecessary. r=masayuki https://hg.mozilla.org/integration/autoland/rev/665564221f54 Part 4. Update some tests to wait updating dictionary. r=masayuki

https://hg.mozilla.org/mozilla-central/rev/3facdd7bcabd https://hg.mozilla.org/mozilla-central/rev/d813879551cb https://hg.mozilla.org/mozilla-central/rev/ebdf1fcdd758 https://hg.mozilla.org/mozilla-central/rev/665564221f54