[webkit-reviews] review denied: [Bug 15616] [GTK] Add spell checking : [Attachment 29701] 0002-Add-spell-checking-languages-property.patch

[bugzilla-daemon at webkit.org]

Gustavo Noronha (kov) <gns at gnome.org> has denied 's request for review:
Bug 15616: [GTK] Add spell checking
https://bugs.webkit.org/show_bug.cgi?id=15616

Attachment 29701: 0002-Add-spell-checking-languages-property.patch
https://bugs.webkit.org/attachment.cgi?id=29701&action=review

------- Additional Comments from Gustavo Noronha (kov) <gns at gnome.org>
> @@ -49,6 +49,7 @@
>  #include "WindowFeatures.h"
>  
>  #include <glib.h>
> +#include <enchant.h>

I believe this include should go on the list above, between BackForwardList.h
and HistoryItem.h. We need a full style review for the gtk files, I think, but
let's try to at least keep the new code sane.

>      WEBKIT_API void
>      webkit_web_settings_add_extra_plugin_directory (WebKitWebView *web_view,
const gchar* directory);
> +
> +    GSList*
> +    webkit_web_settings_get_spell_languages (WebKitWebView *web_view);

Drop the space between the function name and the parens; the * is misplaced.

> +    /**
> +    * WebKitWebSettings:spell-checking-languages:
> +    *
> +    * The languages to be used for spell checking, separated by commas.

It would be good to say in what standard (such as ISO or an RFC) apps are
supposed to specify languages. A few examples should do good here, too.

>  static void webkit_web_settings_init(WebKitWebSettings* web_settings)
>  {
>      web_settings->priv = WEBKIT_WEB_SETTINGS_GET_PRIVATE(web_settings);
> +    web_settings->priv->spell_checking_languages_list = 0;
>  }

I don't think this initialization is necessary.

> @@ -398,6 +418,9 @@ static void webkit_web_settings_finalize(GObject* object)

>      g_free(priv->sans_serif_font_family);
>      g_free(priv->serif_font_family);
>      g_free(priv->user_stylesheet_uri);
> +    g_free(priv->spell_checking_languages);
> +
> +    g_slist_free(priv->spell_checking_languages_list);

Is it safe to free this without checking? I tend to think it is, but reading
the code was not enough to make me confident.

> +    case PROP_SPELL_CHECKING_LANGUAGES:
> +	   {
> +	   priv->spell_checking_languages =
g_strdup(g_value_get_string(value));
> +
> +	   SpellLanguage* lang;
> +	   GSList* spell_languages = 0;

I would prefer having no additional braces here, and have the variable
declarations outside the switch.

> +		   spell_languages = g_slist_prepend(spell_languages, lang);

Use append or reverse the list after adding all the languages; I think we
should honor the assumption that languages will be checked in the order they
are listed in the string.

> + * Since: 1.1.5

Forgot to mention; you'll want to update these to 1.1.6.


> +    WebKitWebSettings* settings;
> +    WebKitWebSettingsPrivate* priv;
> +    GSList *list;
> +
> +    settings = webkit_web_view_get_settings(web_view);
> +    priv = WEBKIT_WEB_SETTINGS_GET_PRIVATE (settings);
> +
> +    list = priv->spell_checking_languages_list;

Similar to the previous patch; initialization where declared.