Bug 61787 – [GTK] Add WebKitSpellChecker interface and implementations

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Xan Lopez]

Exposes the spell checking functionality to the UAs and allows to change the used implementation at runtime.

[Xan Lopez]

Created attachment 95437 [details]
Patch

[Xan Lopez]

There will be a few style errors in the .c file, style-check bug IMHO.

[WebKit Review Bot]

Attachment 95437 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/gtk/ChangeLog', u'Source/Web..." exit_code: 1

Source/WebKit/gtk/webkit/webkitspellchecker.c:54:  Declaration has space between * and variable name in WebKitSpellCheckerInterface* iface  [whitespace/declaration] [3]
Source/WebKit/gtk/webkit/webkitspellchecker.c:81:  Declaration has space between * and variable name in WebKitSpellCheckerInterface* iface  [whitespace/declaration] [3]
Source/WebKit/gtk/webkit/webkitspellchecker.c:106:  Declaration has space between * and variable name in WebKitSpellCheckerInterface* iface  [whitespace/declaration] [3]
Source/WebKit/gtk/webkit/webkitspellchecker.c:126:  Declaration has space between * and variable name in char* webkit_spell_checker_get_autocorrect_suggestions_for_misspelled_word  [whitespace/declaration] [3]
Source/WebKit/gtk/webkit/webkitspellchecker.c:128:  Declaration has space between * and variable name in WebKitSpellCheckerInterface* iface  [whitespace/declaration] [3]
Source/WebKit/gtk/webkit/webkitspellchecker.c:152:  Declaration has space between * and variable name in WebKitSpellCheckerInterface* iface  [whitespace/declaration] [3]
Source/WebKit/gtk/webkit/webkitspellchecker.c:174:  Declaration has space between * and variable name in WebKitSpellCheckerInterface* iface  [whitespace/declaration] [3]
Total errors found: 7 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Martin Robinson]

Comment on attachment 95437 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=95437&action=review

>> Source/WebKit/gtk/webkit/webkitspellchecker.c:54
>> +    WebKitSpellCheckerInterface* iface;
> 
> Declaration has space between * and variable name in WebKitSpellCheckerInterface* iface  [whitespace/declaration] [3]

The code style in C file is actually to have the asterisk with the variable name. It would probably be better to make this file a C++ file now. My reasoning is that one day we may find out we need to use some WTF, WebCore or WebCoreSupport class here and then have to change the entire style of the file when we convert it to C++.

[Martin Robinson]

Comment on attachment 95437 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=95437&action=review

I like this implementation a lot. r- mostly because I think Source/WebKit/gtk/webkit/webkitspellchecker.c should be C++.

> Source/WebKit/gtk/WebCoreSupport/TextCheckerClientEnchant.cpp:3
>   *  Copyright (C) 2008 Nuanti Ltd.

I think you want to rename this file to TextCheckerClient since it no longer depends on Enchant.

> Source/WebKit/gtk/WebCoreSupport/TextCheckerClientEnchant.cpp:31
>  #include <enchant.h>

I think you can remove this include.

> Source/WebKit/gtk/WebCoreSupport/TextCheckerClientEnchant.cpp:67
>      // Currently, it computes an empty string.

This comment can be removed now.

> Source/WebKit/gtk/WebCoreSupport/TextCheckerClientEnchant.cpp:88
> +    char* suggestion;
> +    int i = 0;
>  
> -        if (numberOfSuggestions > 0)
> -            enchant_dict_free_suggestions(dict, suggestions);
> -    }
> -}
> +    guesses.clear();
>  
> -static void getAvailableDictionariesCallback(const char* const languageTag, const char* const, const char* const, const char* const, void* data)
> -{
> -    Vector<CString>* dicts = static_cast<Vector<CString>*>(data);
> +    for (suggestion = suggestions[i]; suggestion; i++)
> +        guesses.append(String::fromUTF8(suggestion));

I think it makes more sense to write this as:

for (int i = 0; suggestion[i]; i++)
    guesses.append(String::fromUTF8(suggestions[i]));

> Source/WebKit/gtk/webkit/webkitdefines.h:107
> +typedef struct _WebKitSpellChecker WebKitSpellChecker; /* dummy typedef */

I think you can remove the comment here.

> Source/WebKit/gtk/webkit/webkitglobals.cpp:250
> +static WebKitSpellChecker* spellChecker = 0;

Please make this a GRefPtr.

> Source/WebKit/gtk/webkit/webkitspellchecker.c:1
> +/*

I think we decided to change this file to C++ in the end? Let's call this WebKitTextChecker because it may one day do grammar checking!

> Source/WebKit/gtk/webkit/webkitspellchecker.c:27
> + * #WebKitSpellChecker provides APIs for the spell checking
> + * functionality used internally by WebKit to perform spell checking

spell checking -> text checking

> Source/WebKit/gtk/webkit/webkitspellchecker.c:35
> +static void webkit_spell_checker_default_init(WebKitSpellCheckerInterface* iface)

iface -> interface.

> Source/WebKit/gtk/webkit/webkitspellchecker.c:48
> + * Checks @string for misspellings using @checker, storing the
> + * location and length of the first misspelling in
> + * @misspelling_location and @misspelling_length respectively.

Might as well let this be two lines for consistency, since the lines above it are so long.

>>> Source/WebKit/gtk/webkit/webkitspellchecker.c:54
>>> +    WebKitSpellCheckerInterface* iface;
>> 
>> Declaration has space between * and variable name in WebKitSpellCheckerInterface* iface  [whitespace/declaration] [3]
> 
> The code style in C file is actually to have the asterisk with the variable name. It would probably be better to make this file a C++ file now. My reasoning is that one day we may find out we need to use some WTF, WebCore or WebCoreSupport class here and then have to change the entire style of the file when we convert it to C++.

iface -> interface. When this becomes C++, please define interface when you first call WEBKIT_SPELL_CHECKER_GET_IFACE.

> Source/WebKit/gtk/webkit/webkitspellchecker.c:75
> + * Returns: a newly allocated %NULL-terminated array of guessed
> + * corrections for a misspelled word @word. Free it with %g_strfreev
> + * when done with it.

I cannot remember if a transfer annotation is necesary here or not.

>> Source/WebKit/gtk/webkit/webkitspellchecker.c:81
>> +    WebKitSpellCheckerInterface* iface;
> 
> Declaration has space between * and variable name in WebKitSpellCheckerInterface* iface  [whitespace/declaration] [3]

Same comment as above here.

> Source/WebKit/gtk/webkit/webkitspellchecker.c:100
> + * Sets @languages as the list of languages to use by @checker. The
> + * accepted format is a list of comma (',') separated language codes
> + * of the form 'en_US', ie, language_VARIANT.

Nice doc here.

> Source/WebKit/gtk/webkit/webkitspellchecker.c:122
> + * Returns: the suggestion for the autocorrection of @word

Should mention that the word is newly allocated.

>> Source/WebKit/gtk/webkit/webkitspellchecker.c:128
>> +    WebKitSpellCheckerInterface* iface;
> 
> Declaration has space between * and variable name in WebKitSpellCheckerInterface* iface  [whitespace/declaration] [3]

Same comments as above.

> Source/WebKit/gtk/webkit/webkitspellchecker.c:146
> + * Instructs the @checker to add @word to its dictionary as a properly
> + * spelled word.

Here it is worth clarifying if the word is learned for the session or permanently. You might say something like, "The @checker learns the word permanently, possibly adding it to the spelling dictionary."

>> Source/WebKit/gtk/webkit/webkitspellchecker.c:152
>> +    WebKitSpellCheckerInterface* iface;
> 
> Declaration has space between * and variable name in WebKitSpellCheckerInterface* iface  [whitespace/declaration] [3]

Same comments as above.

> Source/WebKit/gtk/webkit/webkitspellchecker.c:168
> + * Instructs the @checker to ignore @word as a misspelling from now
> + * on.

Should probably describe what the difference is between ignored words and learned words and whether or not this change is permanent.

>> Source/WebKit/gtk/webkit/webkitspellchecker.c:174
>> +    WebKitSpellCheckerInterface* iface;
> 
> Declaration has space between * and variable name in WebKitSpellCheckerInterface* iface  [whitespace/declaration] [3]

Same comments as above.

> Source/WebKit/gtk/webkit/webkitspellcheckerenchant.cpp:29
> +#include "GOwnPtr.h"
> +#include "webkitspellchecker.h"
> +
> +#include <enchant.h>
> +#include <gtk/gtk.h>
> +#include <wtf/text/CString.h>

This should be one big block.

> Source/WebKit/gtk/webkit/webkitspellcheckerenchant.cpp:31
> +static EnchantBroker* broker = 0;

Instead of manually creating this wherever you need it, I think it would be better to make this a helper function like:

static EnchantBroker* getEnchantBroker()
{
    static EnchantBroker* broker;
    if (!broker)
     ...etc...
}

> Source/WebKit/gtk/webkit/webkitspellcheckerenchant.cpp:34
> +    GSList* m_enchantDicts;

I think we don't usually use m_foo for GObject private sections or plain structs.

> Source/WebKit/gtk/webkit/webkitspellcheckerenchant.cpp:40
> +                        G_IMPLEMENT_INTERFACE (WEBKIT_TYPE_SPELL_CHECKER,

Extra space here after G_IMPLEMENT_INTERFACE.

> Source/WebKit/gtk/webkit/webkitspellcheckerenchant.cpp:49
> +    EnchantDict* dict = static_cast<EnchantDict*>(data);
> +    enchant_broker_free_dict(broker, dict);

Both these lines are very simple, so I think they can be combined into one.

> Source/WebKit/gtk/webkit/webkitspellcheckerenchant.cpp:57
> +    g_slist_foreach(priv->m_enchantDicts, freeSpellCheckingLanguage, 0);
> +    g_slist_free(priv->m_enchantDicts);

If you follow the approach below, you'll need to call delete priv; here.

> Source/WebKit/gtk/webkit/webkitspellcheckerenchant.cpp:66
> +    g_type_class_add_private(klass, sizeof(WebKitSpellCheckerEnchantPrivate));

I think it would be better to consistently use the "new" approach that I've listed below for all new gobjects. Eventually I want to be able to safely use C++ objects everywhere. :)

> Source/WebKit/gtk/webkit/webkitspellcheckerenchant.cpp:71
> +    WebKitSpellCheckerEnchantPrivate* priv = G_TYPE_INSTANCE_GET_PRIVATE(checker, WEBKIT_TYPE_SPELL_CHECKER_ENCHANT, WebKitSpellCheckerEnchantPrivate);

Here you can just do: 

checker->priv = new WebKitSpellCheckerEnchantPrivate();

> Source/WebKit/gtk/webkit/webkitspellcheckerenchant.cpp:75
> +static void check_spelling_of_string(WebKitSpellChecker* checker, const char* string, int* misspellingLocation, int* misspellingLength)

This should be named checkSpellingOfString.

> Source/WebKit/gtk/webkit/webkitspellcheckerenchant.cpp:105
> +            // Set the iterator to be at the current word end, so we don't
> +            // check characters twice.

This can be one line.

> Source/WebKit/gtk/webkit/webkitspellcheckerenchant.cpp:130
> +static char** get_guesses_for_word(WebKitSpellChecker* checker, const char* word, const char* context)

This should be named getGuessesForWord.

> Source/WebKit/gtk/webkit/webkitspellcheckerenchant.cpp:165
> +    Vector<CString>* dicts = static_cast<Vector<CString>*>(data);
> +
> +    dicts->append(languageTag);

Extra newline here.

> Source/WebKit/gtk/webkit/webkitspellcheckerenchant.cpp:168
> +static void update_spell_checking_languages(WebKitSpellChecker* checker, const char* languages)

This should be named updateSpellCheckingLanguages.

> Source/WebKit/gtk/webkit/webkitspellcheckerenchant.cpp:170
> +    EnchantDict* dict;

dict is only used three times and right at the beginning of the block, so it should be defined in those locations instead of function wide.

> Source/WebKit/gtk/webkit/webkitspellcheckerenchant.cpp:193
> +            // No dictionaries selected, we get one from the list

This comment is missing a period.

> Source/WebKit/gtk/webkit/webkitspellcheckerenchant.cpp:209
> +    return NULL;

NULL -> 0.

> Source/WebKit/gtk/webkit/webkitspellcheckerenchant.cpp:212
> +static void learn_word(WebKitSpellChecker* checker, const char* word)

This should be named learnWord.

> Source/WebKit/gtk/webkit/webkitspellcheckerenchant.cpp:215
> +    WebKitSpellCheckerEnchantPrivate* priv = WEBKIT_SPELL_CHECKER_ENCHANT(checker)->priv;
> +    GSList* dicts = priv->m_enchantDicts;

this can just be GSList* dicts = WEBKIT_SPELL_CHECKER_ENCHANT(checker)->priv->enchantDicts.

> Source/WebKit/gtk/webkit/webkitspellcheckerenchant.cpp:221
> +    for (; dicts; dicts = dicts->next) {
> +        EnchantDict* dict = static_cast<EnchantDict*>(dicts->data);
> +
> +        enchant_dict_add_to_personal(dict, word, -1);
> +    }

It seems weird to add this word to all dictionaries. Is that the right approach?

> Source/WebKit/gtk/webkit/webkitspellcheckerenchant.cpp:233
> +    WebKitSpellCheckerEnchantPrivate* priv = WEBKIT_SPELL_CHECKER_ENCHANT(checker)->priv;
> +    GSList* dicts = priv->m_enchantDicts;
> +
> +    for (; dicts; dicts = dicts->next) {
> +        EnchantDict* dict = static_cast<EnchantDict*>(dicts->data);
> +
> +        enchant_dict_add_to_session(dict, word, -1);
> +    }

Same comments as above.

> Source/WebKit/gtk/webkit/webkitspellcheckernone.cpp:27
> +                        G_IMPLEMENT_INTERFACE (WEBKIT_TYPE_SPELL_CHECKER,

Extra space here after G_IMPLEMENT_INTERFACE.

[Xan Lopez]

Shouldn't grammar checking be handled by another interface in case we ever add support for it? Not sure I agree with naming this the very vague "TextChecker" when all it does is spell checking (and probably all it ever will do). If we ever add support for grammar checking we can just implement a new interface and have another (or the same?) object implement that, I think. They are different concepts, so IMHO it makes sense.

About the other non-style comments wrt the implementation of the checker, I'm just copying the existing implementation. If this has weird things or bugs (in fact I actually found a small one already that I silently fixed, but nevermind...) we should probably address them in other bugs to not make this more complicated than it should.

[Martin Robinson]

(In reply to comment #6)
> Shouldn't grammar checking be handled by another interface in case we ever add support for it? Not sure I agree with naming this the very vague "TextChecker" when all it does is spell checking (and probably all it ever will do). If we ever add support for grammar checking we can just implement a new interface and have another (or the same?) object implement that, I think. They are different concepts, so IMHO it makes sense.

I do not find TextChecker too vague and part of the reason I think we should stick with that is that it matches exactly the name that WebKit is using. This is also why I think that grammar checking should go into the same interface. I think that moving it to a separate interface would make the API much more complex (due to the amount of boilerplate needed to implement an interface) and the grammar checking one that you propose would only have 1 or pehraps 2 virtual methods.

> About the other non-style comments wrt the implementation of the checker, I'm just copying the existing implementation. If this has weird things or bugs (in fact I actually found a small one already that I silently fixed, but nevermind...) we should probably address them in other bugs to not make this more complicated than it should.

Do you mean where it's adding the word to all dictionaries? I agree that this can be fixed in another patch. I was merely curious about your opinion on that. :)

[Xan Lopez]

Mr. Noronha, we need your opinion on this. The two options atm are:

a) One TextChecker interface, to which we can possibly add grammar methods in the future.

b) One SpellChecker interface, no grammar methods. If we want grammar checking in the future we add a GrammarChecker interface.

Martin wants a), I I think b) makes more sense for now.

[Gustavo Noronha (kov)]

Mr Robinson, and Mr Lopez, my thoughts on the matter: I tend to prefer avoiding conflating too many responsibilities in a single object. I tried searching other grammar checking APIs for inspiration, but didn't find much. It does feel like a lot of boilerplate would need to be created just for a couple methods, but I think keeping it separate would be a better bet on making it future-proof. Just like we have APIs to add words to dictionaries right now (and that may need to become more complex a bit to allow specifying which dicts to add to, and whether it should be saved or for the session only) we might have more complex stuff for grammar checking down the road, so I guess I'll go with Xan on this one.

This was the closest to a 'grammar checking API' I could find, btw:

http://semantica-software.com/index.php?option=com_content&view=article&id=50&Itemid=58&lang=en

Mac seems to use an API very similar to the one used internally by WebKit, unsurprisingly, with a single 'SpellChecker' object featuring also  'checkGrammarOfString':

http://developer.apple.com/library/mac/#documentation/Cocoa/Reference/ApplicationKit/Classes/NSSpellChecker_Class/Reference/Reference.html

[Xan Lopez]

Created attachment 96483 [details]
Patch

[Martin Robinson]

Comment on attachment 96483 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=96483&action=review

Nice! Please add missing documentation and make the following changes before landing.

> Source/WebKit/gtk/webkit/webkitglobals.cpp:258
> +GObject* webkit_get_text_checker()

This needs documentation, right?

> Source/WebKit/gtk/webkit/webkitglobals.cpp:270
> +void webkit_set_text_checker(GObject* checker)

Ditto.

> Source/WebKit/gtk/webkit/webkitspellchecker.cpp:28
> + * in input or text areas. This can be used, for example, by browsers

input or text areas -> editable areas. I believe it includes things like editable divs.

> Source/WebKit/gtk/webkit/webkitspellchecker.cpp:59
> +    WebKitSpellCheckerInterface* interface;
> +
> +    g_return_if_fail(WEBKIT_IS_SPELL_CHECKER(checker));
> +    g_return_if_fail(string);
> +
> +    interface = WEBKIT_SPELL_CHECKER_GET_IFACE(checker);

Please declare WebKitSpellCheckerInterface when you first assign it.

> Source/WebKit/gtk/webkit/webkitspellchecker.cpp:74
> + * of guessed corrections for a misspelled word @word. Free it with

s/guessed/suggested would be better here, I believe.

> Source/WebKit/gtk/webkit/webkitspellchecker.cpp:86
> +    WebKitSpellCheckerInterface* interface;
> +
> +    g_return_val_if_fail(WEBKIT_IS_SPELL_CHECKER(checker), 0);
> +    g_return_val_if_fail(word, 0);
> +
> +    interface = WEBKIT_SPELL_CHECKER_GET_IFACE(checker);

Again, please declare WebKitSpellCheckerInterface when you first use it.

> Source/WebKit/gtk/webkit/webkitspellchecker.cpp:112
> +    WebKitSpellCheckerInterface* interface;
> +
> +    g_return_if_fail(WEBKIT_IS_SPELL_CHECKER(checker));
> +
> +    interface = WEBKIT_SPELL_CHECKER_GET_IFACE(checker);
> +    if (interface->update_spell_checking_languages)
> +        interface->update_spell_checking_languages(checker, languages);

Ditto.

> Source/WebKit/gtk/webkit/webkitspellchecker.cpp:134
> +    WebKitSpellCheckerInterface* interface;
> +
> +    g_return_val_if_fail(WEBKIT_IS_SPELL_CHECKER(checker), 0);
> +    g_return_val_if_fail(word, 0);
> +
> +    interface = WEBKIT_SPELL_CHECKER_GET_IFACE(checker);

Ditto.

> Source/WebKit/gtk/webkit/webkitspellchecker.cpp:159
> +    WebKitSpellCheckerInterface* interface;
> +
> +    g_return_if_fail(WEBKIT_IS_SPELL_CHECKER(checker));
> +    g_return_if_fail(word);
> +
> +    interface = WEBKIT_SPELL_CHECKER_GET_IFACE(checker);

Ditto.

> Source/WebKit/gtk/webkit/webkitspellchecker.cpp:181
> +    WebKitSpellCheckerInterface* interface;
> +
> +    g_return_if_fail(WEBKIT_IS_SPELL_CHECKER(checker));
> +    g_return_if_fail(word);
> +
> +    interface = WEBKIT_SPELL_CHECKER_GET_IFACE(checker);

Ditto.

> Source/WebKit/gtk/webkit/webkitspellchecker.h:46
> +WEBKIT_API GType webkit_spell_checker_get_type(void) G_GNUC_CONST;
> +
> +WEBKIT_API void    webkit_spell_checker_check_spelling_of_string(WebKitSpellChecker* checker, const char* string, int* misspelling_location, int* misspelling_length);

Crazy obsessive mode: Please line up webkit_spell_checker_get_type with webkit_spell_checker_check_spelling_of_string. :)

> Source/WebKit/gtk/webkit/webkitspellcheckerenchant.cpp:53
> +    EnchantDict* dict = static_cast<EnchantDict*>(data);
> +    enchant_broker_free_dict(broker, dict);

I think this should be one line.

> Source/WebKit/gtk/webkit/webkitwebview.cpp:3434
>      else if (name == g_intern_string("enable-webgl"))
> -        settings->setWebGLEnabled(g_value_get_boolean(&value));
> +        settings->setWebGLEnabled(g_value_get_boolean(&value)
> +);

This seems like an accident?

[Xan Lopez]

Comment on attachment 96483 [details]
Patch

Landed as requested in r88399.

[Xan Lopez]

Closing.