Bug 51587 - [GTK] Remove mandatory Enchant dependency
Summary: [GTK] Remove mandatory Enchant dependency
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 420+
Hardware: All Linux
: P2 Major
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-12-24 02:06 PST by ManDay
Modified: 2011-02-21 02:05 PST (History)
6 users (show)

See Also:


Attachments
Patch (10.85 KB, patch)
2011-01-21 00:17 PST, Ryuan Choi
no flags Details | Formatted Diff | Diff
Patch (11.33 KB, patch)
2011-01-21 05:08 PST, Ryuan Choi
no flags Details | Formatted Diff | Diff
Patch (27.84 KB, patch)
2011-02-15 20:02 PST, Ryuan Choi
no flags Details | Formatted Diff | Diff
Patch (29.46 KB, patch)
2011-02-16 20:09 PST, Ryuan Choi
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description ManDay 2010-12-24 02:06:57 PST
Behaviour: Webkit-GTK unconditionally depends on enchant spellchecking.
Expected behaviour: Neither webkit nor webkit-GTK should unconditionally depend on a spellchecker.
Comment 1 Martin Robinson 2010-12-27 16:01:03 PST
ManDay: Can you explain a bit more why you need this? Xan & Carlos: Thoughts on this one?
Comment 2 ManDay 2010-12-28 02:54:41 PST
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.
Comment 3 Martin Robinson 2010-12-28 15:33:01 PST
(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.
Comment 4 ManDay 2010-12-29 01:35:09 PST
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).
Comment 5 ManDay 2010-12-29 01:48:47 PST
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.
Comment 6 Ryuan Choi 2011-01-20 16:51:05 PST
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?
Comment 7 Ryuan Choi 2011-01-21 00:17:48 PST
Created attachment 79706 [details]
Patch
Comment 8 Ryuan Choi 2011-01-21 00:19:03 PST
(In reply to comment #7)
> Created an attachment (id=79706) [details]
> Patch

This is modified patch which we did.
Comment 9 Martin Robinson 2011-01-21 01:06:12 PST
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.
Comment 10 Ryuan Choi 2011-01-21 05:08:14 PST
Created attachment 79725 [details]
Patch
Comment 11 Ryuan Choi 2011-01-21 05:13:44 PST
(In reply to comment #10)
> Created an attachment (id=79725) [details]
> Patch

Thank you for your reviewing.

I applied what you mentioned.
Comment 12 Alejandro G. Castro 2011-01-24 00:47:19 PST
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?
Comment 13 Ryuan Choi 2011-01-25 17:32:45 PST
(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.
Comment 14 Martin Robinson 2011-01-25 17:41:55 PST
(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.
Comment 15 Martin Robinson 2011-01-27 07:55:47 PST
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.
Comment 16 Martin Robinson 2011-01-27 07:56:18 PST
Comment on attachment 79725 [details]
Patch

r- for now, because we hope to implement this differently.
Comment 17 Ryuan Choi 2011-02-15 20:02:51 PST
Created attachment 82579 [details]
Patch
Comment 18 Martin Robinson 2011-02-16 08:45:51 PST
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.
Comment 19 Ryuan Choi 2011-02-16 20:09:51 PST
Created attachment 82745 [details]
Patch
Comment 20 Ryuan Choi 2011-02-16 20:27:04 PST
(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.
Comment 21 WebKit Commit Bot 2011-02-19 13:06:20 PST
Comment on attachment 82745 [details]
Patch

Clearing flags on attachment: 82745

Committed r79130: <http://trac.webkit.org/changeset/79130>
Comment 22 WebKit Commit Bot 2011-02-19 13:06:27 PST
All reviewed patches have been landed.  Closing bug.
Comment 23 Alejandro G. Castro 2011-02-21 01:06:55 PST
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?
Comment 24 Ryuan Choi 2011-02-21 01:19:29 PST
(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.
Comment 25 Alejandro G. Castro 2011-02-21 01:52:56 PST
(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.
Comment 26 Alejandro G. Castro 2011-02-21 02:05:14 PST
(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