Bug 51587 – [GTK] Remove mandatory Enchant dependency

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[ManDay]

Behaviour: Webkit-GTK unconditionally depends on enchant spellchecking.
Expected behaviour: Neither webkit nor webkit-GTK should unconditionally depend on a spellchecker.

[Martin Robinson]

ManDay: Can you explain a bit more why you need this? Xan & Carlos: Thoughts on this one?

[ManDay]

I feel that imposing a spellchecker on a user is unasked for. Checking spelling is a distinct component that might be optionally looked for, but certainly not necessary. Many people, me being one of them, would like to forgo a spellchecker and remain a minimalistic setup. Imagine if bash unconditionally shipped with a spell-checker (for filenames). It might be a contrived example, yet I'd find it equally inappropriate. Thanks for your understanding.

[Martin Robinson]

(In reply to comment #2)
> I feel that imposing a spellchecker on a user is unasked for. Checking spelling is a distinct component that might be optionally looked for, but certainly not necessary. Many people, me being one of them, would like to forgo a spellchecker and remain a minimalistic setup. Imagine if bash unconditionally shipped with a spell-checker (for filenames). It might be a contrived example, yet I'd find it equally inappropriate. Thanks for your understanding.

Keep in mind that it's up the application to decide whether or not to enable the spell checker. If you simply want to turn the spell checker off, I recommend filing a bug against the application embedding WebKitGTK+.

On the other hand, if there is an issue with the dependency itself, then perhaps we shouldn't close this bug.

[ManDay]

It is my very point that the spell checker is an *optional* component. Thus my words, it should not be an enforced dependency.
A minimalistic setup, which I mentioned, for most people does not mean "install everything and then disable it" but rather "install only what is necessecary". I'm aware of the counter-argument that disk space is cheap these days and one should not care about what is installed, but I oppose to it, because it may only serve as an argument against an argument and eventually does not justify the actual point (the dependency).

[ManDay]

I'd like to add something to prevent any possible misunderstanding: I'm experiencing this issue on Gentoo-Linux and file it here because there is no USE-Flag (Gentoo install option) that would remove the dependency. I therefore assume that there is no configure-Option or even the possibility to forgo enchant. If this impression of mine is wrong and there has always been an adequate configure option to compile without enchant please take my apologies and consider this issue closed. I would then file a bug at Gentoo.

[Ryuan Choi]

I have WebKitGTK+ without enchant because our linux platform doesn't support enchant library yet.

If possible, can we have an option to remove enchant dependency when compiling?

[Ryuan Choi]

Created attachment 79706 [details]
Patch

[Ryuan Choi]

(In reply to comment #7)
> Created an attachment (id=79706) [details]
> Patch

This is modified patch which we did.

[Martin Robinson]

Comment on attachment 79706 [details]
Patch

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

Looks good, but I have a few style nits.

> ChangeLog:9
> +        Add enable-spell-checker option which decide whether to use spell
> +        checker. If disabled, WebKit/GTK+ will not use enchant library.

Should be "which decides" and WebKitGTK+.

> Source/WebKit/gtk/WebCoreSupport/EditorClientGtk.cpp:32
> +#if ENABLE(SPELL_CHECKER)
>  #include <enchant.h>
> +#endif

Please split this out of the main block of includes now that it has an #ifdef around it.

> Source/WebKit/gtk/webkit/webkitwebsettings.cpp:39
> +#if ENABLE(SPELL_CHECKER)
>  #include <enchant.h>
> +#endif

Ditto.

> Source/WebKit/gtk/webkit/webkitwebsettings.cpp:1267
> +        g_value_set_boolean(value, false);

You should use FALSE here explicitly.

[Ryuan Choi]

Created attachment 79725 [details]
Patch

[Ryuan Choi]

(In reply to comment #10)
> Created an attachment (id=79725) [details]
> Patch

Thank you for your reviewing.

I applied what you mentioned.

[Alejandro G. Castro]

I've commented with Martin that maybe we could try to avoid of the ifdefs adding a dummy spell checker, and separating the enchant code in a file and the dummy in other file. That way we could make the compile option --enable-enchant instead of removing the spell checking option. If it is disabled we would use the dummy implementation.

Spell checking is a webkit feature so I think it would be better that solution because it would be more consistent to always have the spell checking option that you can enable or disable, even if that is dummy. And we would avoid the enchant library compilation if we want. Does this make sense to you?

[Ryuan Choi]

(In reply to comment #12)
> I've commented with Martin that maybe we could try to avoid of the ifdefs adding a dummy spell checker, and separating the enchant code in a file and the dummy in other file. That way we could make the compile option --enable-enchant instead of removing the spell checking option. If it is disabled we would use the dummy implementation.
> 
> Spell checking is a webkit feature so I think it would be better that solution because it would be more consistent to always have the spell checking option that you can enable or disable, even if that is dummy. And we would avoid the enchant library compilation if we want. Does this make sense to you?

Yes, It looks better.
If then, how about SpellCheckerEnchant.cpp and SpellCheckerDummy.cpp?
I'll move some of functions in EditorClient into each files.

[Martin Robinson]

(In reply to comment #13)
> If then, how about SpellCheckerEnchant.cpp and SpellCheckerDummy.cpp?
> I'll move some of functions in EditorClient into each files.

Sounds like a good plan, but why not just split out the implementations of the methods of EditorClient into three files:

EditorClient.cpp: for code that is shared
EditorClientEnchant.cpp: for the enchant versions of the spell check methods EditorClientNoSpellChecking.cpp:  (or something similar) for the empty implementation

Perhaps then webkitwebsettings.cpp could just call static methods of the EditorClient to update the spelling dictionary.

[Martin Robinson]

It looks like someone else has started splitting this functionality off into a separate client altogether here: https://bugs.webkit.org/show_bug.cgi?id=53213 If this lands it will simplify your patch greatly.

[Martin Robinson]

Comment on attachment 79725 [details]
Patch

r- for now, because we hope to implement this differently.

[Ryuan Choi]

Created attachment 82579 [details]
Patch

[Martin Robinson]

Comment on attachment 82579 [details]
Patch

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

Great stuff. This is much cleaner than the old approach.

> configure.ac:296
> +AC_MSG_CHECKING([whether to enable enchant support])
> +AC_ARG_ENABLE([enchant],
> +  [AS_HELP_STRING([--enable-enchant],[enable support for enchant])],
> +  [],[enable_enchant="yes"])
> +AC_MSG_RESULT([$enable_enchant])
> +
> +if test "$enable_enchant" = "yes"; then
> +PKG_CHECK_MODULES(ENCHANT, enchant >= $ENCHANT_REQUIRED_VERSION, [], [enable_enchant="no"])

Instead of --enable-enchant, I think this should be called --enable-spellcheck since we have no alternative at the moment.

> Source/WebKit/gtk/WebCoreSupport/TextCheckerClientEnchant.cpp:77
> +    PangoLanguage* language = pango_language_get_default();
> +    PangoLogAttr* attrs = g_new(PangoLogAttr, utflen+1);

You could use a GOwnPtr for these.

> Source/WebKit/gtk/webkit/webkitwebsettings.cpp:1054
> +#if ENABLE(ENCHANT)
> +        g_slist_foreach(priv->enchant_dicts, WebKit::TextCheckerClientEnchant::freeSpellCheckingLanguage, 0);
>          g_slist_free(priv->enchant_dicts);
> -        priv->enchant_dicts = spellDictionaries;
> +        priv->enchant_dicts = WebKit::TextCheckerClientEnchant::updateSpellCheckingLanguage(priv->spell_checking_languages);
> +#endif

Instead of having the WebKitWebView continue to hold the enchant_dicts property, it is should be a member of TextCheckerEnchant. Since different WebViews may have different enchant_dicts, the WebView should communicate with the instance of the TextChecker associated with it.

[Ryuan Choi]

Created attachment 82745 [details]
Patch

[Ryuan Choi]

(In reply to comment #18)
> (From update of attachment 82579 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=82579&action=review
> 
> Great stuff. This is much cleaner than the old approach.
> 
> > configure.ac:296
> > +AC_MSG_CHECKING([whether to enable enchant support])
> > +AC_ARG_ENABLE([enchant],
> > +  [AS_HELP_STRING([--enable-enchant],[enable support for enchant])],
> > +  [],[enable_enchant="yes"])
> > +AC_MSG_RESULT([$enable_enchant])
> > +
> > +if test "$enable_enchant" = "yes"; then
> > +PKG_CHECK_MODULES(ENCHANT, enchant >= $ENCHANT_REQUIRED_VERSION, [], [enable_enchant="no"])
> 
> Instead of --enable-enchant, I think this should be called --enable-spellcheck since we have no alternative at the moment.
> 
Done.

> > Source/WebKit/gtk/WebCoreSupport/TextCheckerClientEnchant.cpp:77
> > +    PangoLanguage* language = pango_language_get_default();
> > +    PangoLogAttr* attrs = g_new(PangoLogAttr, utflen+1);
> 
> You could use a GOwnPtr for these.
> 
Done.

> > Source/WebKit/gtk/webkit/webkitwebsettings.cpp:1054
> > +#if ENABLE(ENCHANT)
> > +        g_slist_foreach(priv->enchant_dicts, WebKit::TextCheckerClientEnchant::freeSpellCheckingLanguage, 0);
> >          g_slist_free(priv->enchant_dicts);
> > -        priv->enchant_dicts = spellDictionaries;
> > +        priv->enchant_dicts = WebKit::TextCheckerClientEnchant::updateSpellCheckingLanguage(priv->spell_checking_languages);
> > +#endif
> 
> Instead of having the WebKitWebView continue to hold the enchant_dicts property, it is should be a member of TextCheckerEnchant. Since different WebViews may have different enchant_dicts, the WebView should communicate with the instance of the TextChecker associated with it.

Right, It looks quite better.
Please check that I did as you want.

[WebKit Commit Bot]

Comment on attachment 82745 [details]
Patch

Clearing flags on attachment: 82745

Committed r79130: <http://trac.webkit.org/changeset/79130>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Alejandro G. Castro]

The commit broke a lot of spellchecking tests:

http://build.webkit.org/results/GTK%20Linux%2064-bit%20Debug/r79193%20(19546)/results.html

Did they work for you?

[Ryuan Choi]

(In reply to comment #23)
> The commit broke a lot of spellchecking tests:
> 
> http://build.webkit.org/results/GTK%20Linux%2064-bit%20Debug/r79193%20(19546)/results.html
> 
> Did they work for you?

Oops, I will check any mistake.

[Alejandro G. Castro]

(In reply to comment #24)
> (In reply to comment #23)
> > The commit broke a lot of spellchecking tests:
> > 
> > http://build.webkit.org/results/GTK%20Linux%2064-bit%20Debug/r79193%20(19546)/results.html
> > 
> > Did they work for you?
> 
> Oops, I will check any mistake.

I found the issue, apparently the list of dictionaries is not correctly initialized. After fixing that I found there is also a crasher becase we are trying to free the default pango language. I'm creating a bug and uploading a patch and add you to the CC.

I hope this helps.

[Alejandro G. Castro]

(In reply to comment #25)
>
> [...]
>
> I found the issue, apparently the list of dictionaries is not correctly initialized. After fixing that I found there is also a crasher becase we are trying to free the default pango language. I'm creating a bug and uploading a patch and add you to the CC.
> 

Added bug 54860