Bug 90268 – [WK2][GTK] Implement a new spell checker API for WebKit2GTK+

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Mario Sanchez Prada]

As in the case of WK1, we need to implement a spell checker API in WebKit2GTK+ so apps needing this (e.g. epiphany) can migrate to WK2.

The idea would be to provide a new API client for the TextChecker, and provide at least the same functionality present in WK1.

After talking to Martin Robinson and Carlos García, it is not clear at this stage whether it would be interesting or not to expose the spell checker out of WK2GTK+, thus allowing apps using it directly and/or replacing it with another specific spell checker, so in the first iteration of the patch I will be proposing something like what we have in WK1, while we make up our minds on this.

Wrapping up, the current proposal would be something like this:

  * Implement new API client for the TextChecker:
    UIProcess/API/gtk/WebKitTextCheckerClient.*

  * Implement and expose a new interface for the spell checker:
    UIProcess/API/gtk/WebKitSpellChecker.*

  * Implement but DON'T expose a specific implementation, based in enchant:
    UIProcess/API/gtk/WebKitSpellCheckerEnchant

Last, I've just observed that in WK1 the TextCheckerClient is designed to be a global object, so I'll be associating the TextCheckerClient to the WebKitWebContext, instead of doing it to the WebView.

Also, because of this global nature, instead of creating WebKitSettings for wrapping 'enable-spell-checking' and 'spell-checking-languages' properties, which would link those properties to the WebView as in WK1 (and we don't want that, since spell checking is a global feature in WK2), I'll be handling those properties in WebKitWebContext, providing getters and setters for then (so the use can enable/disable it and change the list of languates).

Anyway, this is a first proposal and what I'll be basing my first patch in.

[Mario Sanchez Prada]

(In reply to comment #0)
>[...] 
> After talking to Martin Robinson and Carlos García, it is not clear at this 
> stage whether it would be interesting or not to expose the spell checker out of 
> WK2GTK+, thus allowing apps using it directly and/or replacing it with another 
> specific spell checker, so in the first iteration of the patch I will be 
> proposing something like what we have in WK1, while we make up our minds on
> this.

I'm adding Daniel Drake to CC, because he maybe could provide some insight on this specific question.

I wonder if it could be interesting for OLPC guys to be able to replace the default enchant-based spellchecker with their own one, as they do for the case of the file chooser.

[Mario Sanchez Prada]

Created attachment 150157 [details]
Patch proposal (NO Unit tests yet)

Attaching a patch with the changes implementing the current proposal.

I'm not setting the r? flag because the patch is still incomplete (no unit tests, documentation not reviewed...), but still I'd like to share it here since I'm taking some time off since tomorrow (for 2 weeks) and I think it would be nice not to keep it for myself during all that time.

So, feel free to provide feedback and I will update the patch accordingly after my holidays.

Thanks

[Mario Sanchez Prada]

As usual, forgot to mark the dependency with other bug. In this case it's bug 90269, which will provide the new TextCheckerEnchant class in WebCore that the present patch here is using

[Daniel Drake]

Thanks for considering the OLPC use case. We historically haven't shipped spell checkers, so I think there are no special considerations here. As long as its easy to not ship the dictionaries (runtime-disabling the spell checker as a result), because our disk space is limited.

[WebKit Review Bot]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Gustavo Noronha (kov)]

Comment on attachment 150157 [details]
Patch proposal (NO Unit tests yet)

Attachment 150157 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/13273485

[Mario Sanchez Prada]

Created attachment 153519 [details]
Patch proposal + Unit tests + Documentation

New patch adding the missing unit tests and documentation bits. I have also validated it with epiphany so it must be pretty close to what we actually want to have (I hope). Thus, asking for review

[Martin Robinson]

Comment on attachment 153519 [details]
Patch proposal + Unit tests + Documentation

As I said earlier, I really don't think we need this API for WebKit2 since the context menu APIs are already rich enough to preserve spelling suggestions when making custom context menus. I think it's sufficient to enable a basic enchant spell checker and a way to change the language and turn it on and off.

[Mario Sanchez Prada]

(In reply to comment #8)
> (From update of attachment 153519 [details])
> As I said earlier, I really don't think we need this API for WebKit2 since the 
> context menu APIs are already rich enough to preserve spelling suggestions when 
> making custom context menus. I think it's sufficient to enable a basic enchant 
> spell checker and a way to change the language and turn it on and off.

Thanks for the feedback Martin.

So, if I'm getting you right, you basically mean:

 * Remove the "WebKitSpellChecker interface + WebKitSpellCheckerEnchant class"
   combo and have just one simple WebKitSpellChecker class relying in WebCore's
   TextCheckerEnchant new class (see bug 90269).

 * Do not expose this WebKitSpellChecker class in the API. Just keep it hidden
   as an internal one in API/gtk, to be used from WebKitWebContext.cpp and
   WebKitTextCheckerClient.cpp.

 * In WebKitWebContext.h, expose only the following methods (actually the only new API):
   - webkit_web_context_get_spell_checking_enabled
   - webkit_web_context_set_spell_checking_enabled
   - webkit_web_context_get_spell_checking_languages
   - webkit_web_context_set_spell_checking_languages):

 * Move webkit_web_context_get_spell_checker() declaration to
   WebKitWebContextPrivate.h, since we still need it to get the spell checker
   from WebKitTextCheckerClient.cpp.

Am I right?

If so, I will work on it asap next week, unless someone else steps in and opposes to it (maybe someone would prefer to still expose  WebKitSpellChecker as an interface and allow embedders to define their own).

[Martin Robinson]

I think it's a good first step. If we need the interface later, perhaps we can add it (precisely the way we did for WebKit1). Carlos, what are your thoughts?

[Mario Sanchez Prada]

Created attachment 154019 [details]
Patch proposal + Unit tests + Documentation

(In reply to comment #10)
> I think it's a good first step. If we need the interface later, perhaps we can add it (precisely the way we did for WebKit1). Carlos, what are your thoughts?

I'm attaching a new patch using this (quite simpler) approach. I basically added just two new files (WebKitTextChecker.[h|cpp]) providing a new internal C++ class that will serve both to provide an implementation for the TextCheckerClient (so calls from WebCore will work) and to have an object which WebKitWebContext can rely on to implement the newly added API.

If in the future we want to go back to the previous approach we can always do it, but now I see this other implementation I think it makes sense and probably good enough (bonus points for the patch being half big than the previous one!)

Asking for review now

[Gustavo Noronha (kov)]

Comment on attachment 154019 [details]
Patch proposal + Unit tests + Documentation

Attachment 154019 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/13329367

[Mario Sanchez Prada]

Created attachment 154076 [details]
Patch proposal + Unit tests + Documentation

There was an issue with the previous patch in WebKitTextChecker.cpp's guessesForWordCallback(), causing epiphany to crash when trying to create a contextual menu for a mispelled word.

This new patch fixes that.

[Martin Robinson]

Comment on attachment 154076 [details]
Patch proposal + Unit tests + Documentation

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
> [phone]  Mario Sanchez Prada  <[e-mail]>
> +
> +        [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.

[Mario Sanchez Prada]

Created attachment 154937 [details]
 Patch proposal + Unit tests + Documentation

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.

[Martin Robinson]

Comment on attachment 154937 [details]
 Patch proposal + Unit tests + Documentation

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

> Source/WebKit2/UIProcess/API/gtk/WebKitTextChecker.h:31
> +class WebKitTextChecker {
> +public:
> +    virtual ~WebKitTextChecker();

Oops. Please make this fast allocated before landing. Look around for WTF_MAKE_FAST_ALLOCATED.

[Mario Sanchez Prada]

(In reply to comment #16)
> (From update of attachment 154937 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=154937&action=review
> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitTextChecker.h:31
> > +class WebKitTextChecker {
> > +public:
> > +    virtual ~WebKitTextChecker();
> 
> Oops. Please make this fast allocated before landing. Look around for WTF_MAKE_FAST_ALLOCATED.

I don't see any usage of WTF_MAKE_FAST_ALLOCATED macro inside Source/WebKit2, but just in WebCore/, JavaScriptCore/, WTF/ and also WebKit/chromium.

Are you sure you want me to use this in this file? And if so, could you please elaborate a bit on the reasons?

Thanks for the review, btw

[Mario Sanchez Prada]

(In reply to comment #17)
> [...]
> > Oops. Please make this fast allocated before landing. Look around for WTF_MAKE_FAST_ALLOCATED.
> 
> I don't see any usage of WTF_MAKE_FAST_ALLOCATED macro inside Source/WebKit2, but just in WebCore/, JavaScriptCore/, WTF/ and also WebKit/chromium.
> 
> Are you sure you want me to use this in this file? And if so, could you please elaborate a bit on the reasons?

Ok. Martin explained to me why this is needed and so I'll add it when landing

[Mario Sanchez Prada]

Committed r123967: <http://trac.webkit.org/changeset/123967>

[Mario Sanchez Prada]

Reopening as per recent rollout (see bug 92656)

[Mario Sanchez Prada]

Created attachment 155505 [details]
Patch proposal + Unit tests + Documentation

After reviewing the issue with previous patch for bug 90269, I reviewed this one to make sure the same issue was not happening here, as it seemed to be the case.

Now uploading a new patch anyway updated against latest code in the repo.

[WebKit Review Bot]

Comment on attachment 154937 [details]
 Patch proposal + Unit tests + Documentation

Cleared Martin Robinson's review+ from obsolete attachment 154937 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Mario Sanchez Prada]

Committed r124741: <http://trac.webkit.org/changeset/124741>

[Mario Sanchez Prada]

Comment on attachment 155505 [details]
Patch proposal + Unit tests + Documentation

This patch was exactly the same one than before (reviewed by Martin) so I just re-commited it after making sure that the fix for bug 90269 was not failing anymore.

Hence, now clearing the r? flag...

[Carlos Garcia Campos]

Comment on attachment 155505 [details]
Patch proposal + Unit tests + Documentation

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

Sorry for reviewing this when it has already landed. Do you plan to extend the spellchecker API in the future? Because in that case maybe it makes sense to use a separate object (spellchecker manager, or something like that) instead of adding a lot of API related to the spellchecker to WebContext object (like we do for cookies, for example, where we have a cookie manager object)

> Source/WebKit2/UIProcess/API/gtk/WebKitTextChecker.cpp:147
> +void WebKitTextChecker::setSpellCheckingLanguages(const String& languages)
> +{
> +    if (m_spellCheckingLanguages == languages)
> +        return;
> +    m_spellCheckingLanguages = languages;

Here we could use a CString instead as received in the API in UTF8, so that we don't need to keep two copies.

> Source/WebKit2/UIProcess/API/gtk/WebKitTextChecker.cpp:150
> +    m_textChecker->updateSpellCheckingLanguages(languages);

And here we could convert it to String to pass it the WebCore

> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:80
> +    GOwnPtr<gchar> spellCheckingLanguages;

Using a CString in the text checker we woulnd't need this.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:435
> + * Get the current status of the spell checking feature.

Maybe Get whether spell checking feature is currently enabled? "Status" might be confusing, and webkit_web_context_set_spell_checking_enabled() docs says "Enable or disable the spell checking feature".

> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:482
> +    return context->priv->spellCheckingLanguages.get();

Here we could return the value cached in the spell checker. 

return context->priv->textChecker->getSpellCheckingLanguages().data();

> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:508
> +    context->priv->textChecker->setSpellCheckingLanguages(String(languages));

This should be String::fromUTF8, since all strings received in the public API must be UTF8

> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:509
> +    context->priv->spellCheckingLanguages.set(g_strdup(languages));

And this wouldn't be needed.

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebContext.cpp:236
> +    GRefPtr<WebKitWebContext> webContext(webkit_web_context_get_default());

Don't need to keep a ref, it's a global singleton

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebContext.cpp:240
> +    const gchar* currentLanguage(webkit_web_context_get_spell_checking_languages(webContext.get()));

This looks weird to me, I would use const gchar* currentLanguage = webkit_web_context_get_spell_checking_languages(webContext.get()); instead. And maybe also rename currentLanguage to currentLanguages.

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebContext.cpp:245
> +    webkit_web_context_set_spell_checking_languages(webContext.get(), 0);
> +    currentLanguage = webkit_web_context_get_spell_checking_languages(webContext.get());
> +    g_assert_cmpstr(currentLanguage, ==, 0);

This is confusing, the API docs says "In case %NULL is passed, the default value as returned by gtk_get_default_language() will be used." Does gtk_get_default_language() return NULL? In any case I think it would be better to check g_assert_cmpstr(currentLanguage, ==, gtk_get_default_language()); to make sure what the API docs says is true.

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebContext.cpp:254
> +    gboolean isSpellCheckingEnabled = webkit_web_context_get_spell_checking_enabled(webContext.get());
> +    g_assert(!isSpellCheckingEnabled);

Thsi could be just g_assert(!webkit_web_context_get_spell_checking_enabled(webContext.get()));

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebContext.cpp:257
> +    isSpellCheckingEnabled = webkit_web_context_get_spell_checking_enabled(webContext.get());
> +    g_assert(isSpellCheckingEnabled);

And here g_assert(webkit_web_context_get_spell_checking_enabled(webContext.get()));

[Mario Sanchez Prada]

(In reply to comment #25)
> (From update of attachment 155505 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=155505&action=review
> 
> Sorry for reviewing this when it has already landed.

No problem. I filed a new bug and attached a patch there addressing these issues you pointed out here.

So, thanks for reviewing it anyway

> Do you plan to extend the spellchecker API in the future?
> Because in that case maybe it makes sense to use a separate object (spellchecker manager,
> or something like that) instead of adding a lot of API related to the spellchecker to
> WebContext object (like we do for cookies, for example, where we have a cookie manager object)

My initial idea was to do something similar to what we did for WK1 (having a WebKitSpellChecker object exposed in the API, which would give us additional flexibility), but at some point Martin and others convinced me that having a simple API to just enable/disable the feature and set a list of languages would be enough.

Which is a long way of saying that "no, I don't plan to" :-)

[Martin Robinson]

*** Bug 68547 has been marked as a duplicate of this bug. ***