[Webkit-unassigned] [Bug 90269] [GTK] Add a new and reusable enchant-based spellchecker in WebCore

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=90269





--- Comment #2 from Martin Robinson <mrobinson at webkit.org>  2012-06-29 09:01:47 PST ---
(From update of attachment 150155)
View in context: https://bugs.webkit.org/attachment.cgi?id=150155&action=review

Looks good, though I think we should make the interface more closely match WebCore style.

> Source/WebCore/platform/text/gtk/TextCheckerEnchant.cpp:57
> +void TextCheckerEnchant::ignoreWord(const char* word)
> +{
> +    for (Vector<EnchantDict*>::const_iterator iter = m_enchantDicts.begin(); iter != m_enchantDicts.end(); ++iter)
> +        enchant_dict_add_to_session(*iter, word, -1);
> +}
> +
> +void TextCheckerEnchant::learnWord(const char* word)
> +{

These should probably take Strings, which encapsulate encoding information better.

> Source/WebCore/platform/text/gtk/TextCheckerEnchant.cpp:62
> +void TextCheckerEnchant::checkSpellingOfString(const char* string, int* misspellingLocation, int* misspellingLength)

It think it makes sense to rename this to be more precise. checkSpellingOfString could be getLocationAndLengthOfFirstMisspellilng. This aligns with the WebKit style of using the word "get" for functions and methods that have output arguments.

> Source/WebCore/platform/text/gtk/TextCheckerEnchant.cpp:86
> +            while (attrs.get()[end].is_word_end < 1)
> +                end++;

You're missing the fix that I recently made to the WebKit1 enchant code to handle contractions.

> Source/WebCore/platform/text/gtk/TextCheckerEnchant.cpp:114
> +char** TextCheckerEnchant::getGuessesForWord(const char* word)

Since this is a WebCore interface, a Vector<String> makes more sense here.

> Source/WebCore/platform/text/gtk/TextCheckerEnchant.cpp:125
> +        if (numberOfSuggestions > 0) {

This looks like it should be an early continue.

> Source/WebCore/platform/text/gtk/TextCheckerEnchant.cpp:127
> +            if (numberOfSuggestions > 10)
> +                numberOfSuggestions = 10;

10 should probably be a constant like:
static const int maximumNumberOfSuggestions = 10;

> Source/WebCore/platform/text/gtk/TextCheckerEnchant.cpp:141
> +void TextCheckerEnchant::updateSpellCheckingLanguages(const char* languages)

It makes sense for this to take a String instead of a const char*.

> Source/WebCore/platform/text/gtk/TextCheckerEnchant.cpp:146
> +        char** langs = g_strsplit(languages, ",", -1);

You could use String.split here and avoid having to free the list of languages.

> Source/WebCore/platform/text/gtk/TextCheckerEnchant.cpp:179
> +#endif /* ENABLE(SPELLCHECK) */

Nit: You should use a C++ comment here.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.