Bug 51587

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

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.

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

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

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.

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?

Created attachment 79706 [details] Patch

(In reply to comment #7) > Created an attachment (id=79706) [details] > Patch This is modified patch which we did.

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.

Created attachment 79725 [details] Patch

(In reply to comment #10) > Created an attachment (id=79725) [details] > Patch Thank you for your reviewing. I applied what you mentioned.

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?

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

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

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 on attachment 79725 [details] Patch r- for now, because we hope to implement this differently.

Created attachment 82579 [details] Patch

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.

Created attachment 82745 [details] Patch

(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 on attachment 82745 [details] Patch Clearing flags on attachment: 82745 Committed r79130: <http://trac.webkit.org/changeset/79130>

All reviewed patches have been landed. Closing bug.

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?

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

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

(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