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

[bugzilla-daemon at webkit.org]

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

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

------- Additional Comments from Mario Sanchez Prada <msanchez at igalia.com>
Thanks for the review Martin. Attaching a new patch trying to address all the
issues you pointed out.

See some comments below...

(In reply to comment #14)
> (From update of attachment 154076 [details])
> [...]
> Looks like you have a double ChangeLog here.

Indeed. Fixed.

> > Source/WebKit2/GNUmakefile.am:146
> > +# Spell checker
> 
> I think you can omit this comment.

Fixed.

> > Source/WebKit2/GNUmakefile.am:150
> > +	$(ENCHANT_CFLAGS)
> 
> Do you also need to add ENCHANT_LIBS somewhere?

It was already in the LIBADD part, but I think it makes more sense if we keep
it in the same place we are adding ENCHANT_CFLAGS to the compile flags, in case
spellchecker support is enabled.

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

> 
> The asterisk is in the wrong place here.
[...]
> Ditto. It seems like all the clientInfo arguments have this issue.

All fixed.

> > Source/WebKit2/UIProcess/API/gtk/WebKitTextChecker.cpp:120
> > +	 m_textChecker->checkSpellingOfString(string, misspellingLocation,
misspellingLength);
> 
> Should you exit early if m_spellCheckingEnabled is false?

Not needed. This is a private class to implement TextCheckerClient's API in the
UIProcess, and it will be used from the WebProcess only (not from outside WK).
In this sense WebCore won't ever ask for checking the spelling of a string, or
to guess words for a mispelled one, if a previous call to
isContinousSpellCheckingEnabled() has returned false.

Exactly the same approach (not early returning) is how things are currently
implemented in WK1, btw.

So, I would leave it as it is now.

> > 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.

Fixed.

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:436
> > +gboolean webkit_web_context_get_spell_checking_enabled(WebKitWebContext
*context)
> 
> Asterisk issue here as well.

Fixed.

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

Fixed.

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

Fixed.

> > 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


Fixed.

> > 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.


Fixed.

Btw, if you have some time to review patch for bug 90269 that would be awesome
too, as it's blocking this one because of the enchant-based text checker, which
we want in WebCore not to repeat code in WK1 and WK2.