[webkit-reviews] review granted: [Bug 61787] [GTK] Add WebKitSpellChecker interface and implementations : [Attachment 96483] Patch

[bugzilla-daemon at webkit.org]

Martin Robinson <mrobinson at webkit.org> has granted Xan Lopez
<xan.lopez at gmail.com>'s request for review:
Bug 61787: [GTK] Add WebKitSpellChecker interface and implementations
https://bugs.webkit.org/show_bug.cgi?id=61787

Attachment 96483: Patch
https://bugs.webkit.org/attachment.cgi?id=96483&action=review

------- Additional Comments from Martin Robinson <mrobinson at webkit.org>
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?