[webkit-reviews] review denied: [Bug 90268] [WK2][GTK] Implement a new spell checker API for WebKit2GTK+ : [Attachment 154076] Patch proposal + Unit tests + Documentation

[bugzilla-daemon at webkit.org]

Martin Robinson <mrobinson at webkit.org> has denied Mario Sanchez Prada
<msanchez at igalia.com>'s request for review:
Bug 90268: [WK2][GTK] Implement a new spell checker API for WebKit2GTK+
https://bugs.webkit.org/show_bug.cgi?id=90268

Attachment 154076: Patch proposal + Unit tests + Documentation
https://bugs.webkit.org/attachment.cgi?id=154076&action=review

------- Additional Comments from Martin Robinson <mrobinson at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=154076&action=review


Looks good, but I think this could use a few minor tweaks.

> Source/WebKit2/ChangeLog:70
> +2012-07-20  Mario Sanchez Prada  <msanchez at igalia.com>
> +
> +	   [WK2][GTK] Implement a new spell checker API for WebKit2GTK+

Looks like you have a double ChangeLog here.

> Source/WebKit2/GNUmakefile.am:146
> +# Spell checker

I think you can omit this comment.

> Source/WebKit2/GNUmakefile.am:150
> +	$(ENCHANT_CFLAGS)

Do you also need to add ENCHANT_LIBS somewhere?

> Source/WebKit2/UIProcess/API/gtk/WebKitTextChecker.cpp:42
> +static bool continuousSpellCheckingEnabledCallback(const void *clientInfo)

The asterisk is in the wrong place here.

> Source/WebKit2/UIProcess/API/gtk/WebKitTextChecker.cpp:47
> +static void setContinuousSpellCheckingEnabledCallback(bool enabled, const
void *clientInfo)

Also here.

> Source/WebKit2/UIProcess/API/gtk/WebKitTextChecker.cpp:52
> +static void checkSpellingOfStringCallback(uint64_t tag, WKStringRef text,
int32_t* misspellingLocation, int32_t* misspellingLength, const void
*clientInfo)

Ditto.

> Source/WebKit2/UIProcess/API/gtk/WebKitTextChecker.cpp:57
> +static WKArrayRef guessesForWordCallback(uint64_t tag, WKStringRef word,
const void *clientInfo)

Ditto. It seems like all the clientInfo arguments have this issue.

> Source/WebKit2/UIProcess/API/gtk/WebKitTextChecker.cpp:120
> +    m_textChecker->checkSpellingOfString(string, misspellingLocation,
misspellingLength);

Should you exit early if m_spellCheckingEnabled is false?

> Source/WebKit2/UIProcess/API/gtk/WebKitTextChecker.h:30
> +class WebKitTextChecker : public RefCounted<WebKitTextChecker> {

It seems that RefCounted is a bit overkill here, since the checker will never
be shared. I think this should just be fast allocated and you could make the
member in web context be an OwnPtr.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:436
> +gboolean webkit_web_context_get_spell_checking_enabled(WebKitWebContext
*context)

Asterisk issue here as well.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:454
> +void webkit_web_context_set_spell_checking_enabled(WebKitWebContext
*context, gboolean enabled)

Ditto.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:467
> + * Get the the list of spell checking languages associated to

associated to -> associated with

> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:470
> + * in the format of every language in the list.

in the format of every language -> on the format of the languages in the list

> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:473
> + * The returned string should be freed with g_free() when no longer
> + * needed.

I think it makes sense to cache the language list and use transfer none here.