Comment on attachment 547702 updated patch Review of attachment 547702: -----------------------------------------------------------------
In general I like this patch. I'm minusing because of the GetLanguage part, but the rest generally looks good. About testing strategies, you can load dictionaries from a directory using the loadDictionariesFromDir API (see <http://mxr.mozilla.org /mozilla- central/source/extensions/spellcheck/hunspell/tests/unit/test_hunspell.js#212> for an example). But I would be fine with tests written without loading custom dictionaries, if we use the same code for this and the :lang CSS selector (see below). Once we do that, we can get more unit tests for the :lang selector (this is our only test for that now: <http://mxr.mozilla.org/mozilla-central/source/layout/reftests/unicode /unicode-lang.html?force=1>) and then we can trust that editor is going to use the same code to retrieve the language name, which reduces the need for loading additional dictionaries in our tests. But we should still get tests to make sure that the stuff in other languages is not spell-checked by the editor. You can use the existing spellcheck tests in <http://mxr.mozilla.org/mozilla- central/source/layout/reftests/editor> as a sample for that. Once we have this level of testing, we can just load some simple dictionaries for some fictional languages (such as testing, testing-AB, testing-AC) to make sure that they're not going to affect other tests in the system, and use them to test the fallback language stuff. For dynamic changes, let's see if bz has any ideas how to handle them efficiently, but I think that we definitely want to handle them, because it's the only way that a web page can signal us to change the spell checking language. But this can definitely be done in a follow-up bug. Does this make sense? ::: editor/composer/src/nsEditorSpellCheck.cpp @@ +251,4 @@ > } > } > > + // If we have not set editor, and editor has not a @lang, we try to get a Nit: "If the editable element doesn't have a lang attribute, ..." @@ +283,5 @@ > NS_IMETHODIMP > +nsEditorSpellCheck::GetLanguage(nsIEditor* aEditor, nsAString &aResult) > +{ > + nsCOMPtr<nsIDOMElement> rootElement; > + nsresult rv = aEditor->GetRootElement(getter_AddRefs(rootElement)); This only work correctly for the plaintext editor. For HTML editor, you need to call GetActiveEditingHost() (you should probably cast to nsHTMLEditor manually :( ), and use the @lang attribute on that element if it exists, and walk up the parent chain if it doesn't. @@ +289,5 @@ > + > + nsCOMPtr<nsIContent> rootContent = do_QueryInterface(rootElement); > + NS_ENSURE_TRUE(rootContent, NS_ERROR_FAILURE); > + > + nsCOMPtr<nsIContent> parent = rootContent->GetParent(); So, we have some code here <http://mxr.mozilla.org/mozilla- central/source/layout/style/nsCSSRuleProcessor.cpp#1613> (also see the GetLang function in that file) which implements the same thing for the :lang CSS selector. I think we should consolidate all this into an nsIContent method named GetLanguage or something, and then we can simply call this function in both places. -- You received this bug notification because you are a member of Ubuntu Bugs, which is subscribed to Ubuntu. https://bugs.launchpad.net/bugs/303269 Title: Automatically select language for spell check based on user input To manage notifications about this bug go to: https://bugs.launchpad.net/firefox/+bug/303269/+subscriptions -- ubuntu-bugs mailing list ubuntu-bugs@lists.ubuntu.com https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
