Bug 95718

Created attachment 161983 [details] proposed patch

Hrm... I think this is not actually a bug and thus there's no need to add these macros in UIProcess/gtk/TextCheckerGtk.cpp since that file is at a very low level in the UIProcess, that should be (and it is, I think) independent of this kind of configuration options. After all, the implementation of TextChecker's methods will rely on the API client (WKTextCheckerClient, in this case) provided for the specific platform, which should only be set if the SPELLCHECK feature is enabled, being this the correct place IMHO to use the macro. For the sake of clarity, this is how we do it in WebKit2GTK: 1. In WebKitWebContext.cpp, we create an instance of our internal class (not exposed through the WK2GTK API) WebKitTextChecker: static gpointer createDefaultWebContext(gpointer) { [...] #if ENABLE(SPELLCHECK) webContext->priv->textChecker = WebKitTextChecker::create(); #endif return webContext.get(); } 2. In the implementation of WebKitTextChecker::create(), we provide an implementation for the WKTextCheckerClient API client: WebKitTextChecker::WebKitTextChecker() : m_textChecker(WebCore::TextCheckerEnchant::create()) , m_spellCheckingEnabled(false) { WKTextCheckerClient wkTextCheckerClient = { kWKTextCheckerClientCurrentVersion, this, // clientInfo 0, // continuousSpellCheckingAllowed continuousSpellCheckingEnabledCallback, setContinuousSpellCheckingEnabledCallback, [...] }; [...] } 3. Finally, also in WebKitTextChecker::create(), we set that one as the API client to be used from UIProcess/WebTextChecker.cpp, and also from UIProcess/gtk/TextCheckerGtk.cpp: WebKitTextChecker::WebKitTextChecker() : m_textChecker(WebCore::TextCheckerEnchant::create()) , m_spellCheckingEnabled(false) { [...] WKTextCheckerSetClient(&wkTextCheckerClient); } So, if ENABLE(SPELLCHECK) macro returned false, we wouldn't have done any of this in the first place and so, all the calls in UIProcess/gtk/TextCheckerGtk.cpp would just do nothing, as they would finally execute methods of UIProcess/WebTextCheckerClient.cpp, like this one: void WebTextCheckerClient::checkSpellingOfString(uint64_t tag, const String& text, int32_t& misspellingLocation, int32_t& misspellingLength) { if (!m_client.checkSpellingOfString) return; m_client.checkSpellingOfString(tag, toAPI(text.impl()), &misspellingLocation, &misspellingLength, m_client.clientInfo); } Does all this rant make sense? Am I missing anything?

(In reply to comment #2) > Hrm... I think this is not actually a bug and thus there's no need to add these macros in UIProcess/gtk/TextCheckerGtk.cpp since that file is at a very low level in the UIProcess, that should be (and it is, I think) independent of this kind of configuration options. > > After all, the implementation of TextChecker's methods will rely on the API client (WKTextCheckerClient, in this case) provided for the specific platform, which should only be set if the SPELLCHECK feature is enabled, being this the correct place IMHO to use the macro. First of all thanks for very good review and nice explanations of WebKit2GTK design of spellchecking feature! I've found this issue while implementing TextCheckerEfl.cpp for WebKit2EFL (under review here https://bugs.webkit.org/show_bug.cgi?id=91854) which I can say, looks very similar to TextCheckerGtk and TextCheckerMac:) > > For the sake of clarity, this is how we do it in WebKit2GTK: > > 1. In WebKitWebContext.cpp, we create an instance of our internal class (not > exposed through the WK2GTK API) WebKitTextChecker: > > static gpointer createDefaultWebContext(gpointer) > { > [...] > #if ENABLE(SPELLCHECK) > webContext->priv->textChecker = WebKitTextChecker::create(); > #endif > return webContext.get(); > } > > 2. In the implementation of WebKitTextChecker::create(), we provide an implementation for the WKTextCheckerClient API client: > > WebKitTextChecker::WebKitTextChecker() > : m_textChecker(WebCore::TextCheckerEnchant::create()) > , m_spellCheckingEnabled(false) > { > WKTextCheckerClient wkTextCheckerClient = { > kWKTextCheckerClientCurrentVersion, > this, // clientInfo > 0, // continuousSpellCheckingAllowed > continuousSpellCheckingEnabledCallback, > setContinuousSpellCheckingEnabledCallback, > [...] > }; > [...] > } > > 3. Finally, also in WebKitTextChecker::create(), we set that one as the > API client to be used from UIProcess/WebTextChecker.cpp, and also from > UIProcess/gtk/TextCheckerGtk.cpp: > > WebKitTextChecker::WebKitTextChecker() > : m_textChecker(WebCore::TextCheckerEnchant::create()) > , m_spellCheckingEnabled(false) > { > [...] > WKTextCheckerSetClient(&wkTextCheckerClient); > } > > So, if ENABLE(SPELLCHECK) macro returned false, we wouldn't have done any of this in the first place and so, all the calls in UIProcess/gtk/TextCheckerGtk.cpp would just do nothing, as they would finally execute methods of UIProcess/WebTextCheckerClient.cpp, like this one: > > > void WebTextCheckerClient::checkSpellingOfString(uint64_t tag, const String& text, int32_t& misspellingLocation, int32_t& misspellingLength) > { > if (!m_client.checkSpellingOfString) > return; > > m_client.checkSpellingOfString(tag, toAPI(text.impl()), &misspellingLocation, &misspellingLength, m_client.clientInfo); > } > > Does all this rant make sense? Am I missing anything? I agree with you that TextChecker's methods just call client's methods defined by WKTextChecker's callbacks. If they are not set, it's safe to call them even SPELLCHECK macro is disabled (as you pointed out WebTextCheckerClient first checkc whether the client exists). But wouldn't be better to add the SPELLCHECK macro for below reasons: - to just do not compile that part of the code, especially if we have possibility to implemented port specific feature and already have macro? - to increase source readability, if someone looks at the code it's not clear that the part of code works OK only if SPELLCHECK is ON, - to consistency with files that implement spellchecking (it looks like TextCheckerGtk makes an exception) If you don't like this change just feel free and close this bug :)

(In reply to comment #3) > [...] > First of all thanks for very good review and nice explanations of > WebKit2GTK design of spellchecking feature! You're welcome. Glad to see you find it useful. > [...] > I agree with you that TextChecker's methods just call client's methods > defined by WKTextChecker's callbacks. If they are not set, it's safe to > call them even SPELLCHECK macro is disabled (as you pointed out > WebTextCheckerClient first checkc whether the client exists). Yes, and I think that's precisely one of the nice features of this kind of implementations (not only talking about the TextChecker, but more in general): to allow having the possibility of writing new platform-specific components in for specific features and "plugging" them into the generic implementation parts already provided in UIProcess. > But wouldn't be better to add the SPELLCHECK macro for below reasons: > - to just do not compile that part of the code, especially if we have > possibility to implemented port specific feature and already have macro? I think you could achieve a similar effect by adding the right conditions in the makefile, so you just compile the files you need depending on the SPELLCHECK feature being enabled or not. Actually, I think I did that in a preliminary version of my patch for GTK, but then I decided not to do it at some point, because I thought it was clearer. > - to increase source readability, if someone looks at the code it's not > clear that the part of code works OK only if SPELLCHECK is ON, I can't deny this. Of course having the SPELLCHECK macro used in TextCheckerGtk.cpp will make it clearer for readers looking at that file. In the other hand, I also think that a reader really into knowing how spell checking works in a platform will at some point (probably even sooner) end up looking at UIProcess/API/<platform> and there (s)he will find the SPELLCHECK macro anyway. Not an strong opinion though. Just trying to share my thoughts > - to consistency with files that implement spellchecking (it looks > like TextCheckerGtk makes an exception) Hrm.. I'm not sure. If I grep for SPELLCHECK in WebKit2/ folder: ChangeLog:4569: * GNUmakefile.am: Add flags to handle the SPELLCHECK feature. ChangeLog:5606: * GNUmakefile.am: Add flags to handle the SPELLCHECK feature. GNUmakefile.am:188:if ENABLE_SPELLCHECK GNUmakefile.am:190: -DENABLE_SPELLCHECK=1 \ UIProcess/API/gtk/WebKitTextChecker.h:23:#if ENABLE(SPELLCHECK) UIProcess/API/gtk/WebKitTextChecker.h:59:#endif // ENABLE(SPELLCHECK) UIProcess/API/gtk/WebKitTextChecker.cpp:29:#if ENABLE(SPELLCHECK) UIProcess/API/gtk/WebKitTextChecker.cpp:162:#endif // ENABLE(SPELLCHECK) UIProcess/API/gtk/WebKitWebContext.cpp:104:#if ENABLE(SPELLCHECK) UIProcess/API/gtk/WebKitWebContext.cpp:162:#if ENABLE(SPELLCHECK) UIProcess/API/gtk/WebKitWebContext.cpp:470:#if ENABLE(SPELLCHECK) UIProcess/API/gtk/WebKitWebContext.cpp:488:#if ENABLE(SPELLCHECK) UIProcess/API/gtk/WebKitWebContext.cpp:510:#if ENABLE(SPELLCHECK) UIProcess/API/gtk/WebKitWebContext.cpp:539:#if ENABLE(SPELLCHECK) ... I don't see any usage of that macro out of UIProcess/API/gtk, which is right IMHO because it happens at the platform API level. However this change your proposing happens at the level of WebKit2 UIProcess's implementation level (UIProcess), which should be as much agnostic of this kind of things as possible, I think. > If you don't like this change just feel free and close this bug :) Oh, no! I won't do it :-) I'm just sharing my thoughts on this in order to help, not that I'm 100% sure I'm completely right or something. Actually, I could very well be perfectly wrong :-) I would appreciate if someone else with experience in WebKit2GTK could drop some comments on this topic too. Martin? Carlos? ;-)

(In reply to comment #4) > (In reply to comment #3) > > But wouldn't be better to add the SPELLCHECK macro for below reasons: > > - to just do not compile that part of the code, especially if we have > > possibility to implemented port specific feature and already have macro? > > I think you could achieve a similar effect by adding the right conditions in the makefile, so you just compile the files you need depending on the SPELLCHECK feature being enabled or not. > > Actually, I think I did that in a preliminary version of my patch for GTK, but then I decided not to do it at some point, because I thought it was clearer. As I understand TextChecker class takes responsibility of public interface for WebKit2 and its implementation is placed in port's specific files for example, {port}/TextChecker{Port}.cpp. In my opinion adding the right conditions (as I assume disabling TextCheckerGtk.cpp if SPELLCHECK is OGG) to the makefile probably won't work - undefined reference may occur while linking process . Please correct me if I am wrong. > > > - to increase source readability, if someone looks at the code it's not > > clear that the part of code works OK only if SPELLCHECK is ON, > > I can't deny this. Of course having the SPELLCHECK macro used in TextCheckerGtk.cpp will make it clearer for readers looking at that file. > > In the other hand, I also think that a reader really into knowing how spell checking works in a platform will at some point (probably even sooner) end up looking at UIProcess/API/<platform> and there (s)he will find the SPELLCHECK macro anyway. > > Not an strong opinion though. Just trying to share my thoughts There are a lot of files/classes in WebKi2 for spelling for example, TextChecker, WebTextChecker, WebKitTextChecker, WKTextChecker. I think it's a little annoying to browse them to finally find SPELLCHECK macro. > > > - to consistency with files that implement spellchecking (it looks > > like TextCheckerGtk makes an exception) > > Hrm.. I'm not sure. If I grep for SPELLCHECK in WebKit2/ folder: > > ChangeLog:4569: * GNUmakefile.am: Add flags to handle the SPELLCHECK feature. > ChangeLog:5606: * GNUmakefile.am: Add flags to handle the SPELLCHECK feature. > GNUmakefile.am:188:if ENABLE_SPELLCHECK > GNUmakefile.am:190: -DENABLE_SPELLCHECK=1 \ > UIProcess/API/gtk/WebKitTextChecker.h:23:#if ENABLE(SPELLCHECK) > UIProcess/API/gtk/WebKitTextChecker.h:59:#endif // ENABLE(SPELLCHECK) > UIProcess/API/gtk/WebKitTextChecker.cpp:29:#if ENABLE(SPELLCHECK) > UIProcess/API/gtk/WebKitTextChecker.cpp:162:#endif // ENABLE(SPELLCHECK) > UIProcess/API/gtk/WebKitWebContext.cpp:104:#if ENABLE(SPELLCHECK) > UIProcess/API/gtk/WebKitWebContext.cpp:162:#if ENABLE(SPELLCHECK) > UIProcess/API/gtk/WebKitWebContext.cpp:470:#if ENABLE(SPELLCHECK) > UIProcess/API/gtk/WebKitWebContext.cpp:488:#if ENABLE(SPELLCHECK) > UIProcess/API/gtk/WebKitWebContext.cpp:510:#if ENABLE(SPELLCHECK) > UIProcess/API/gtk/WebKitWebContext.cpp:539:#if ENABLE(SPELLCHECK) > > > ... I don't see any usage of that macro out of UIProcess/API/gtk, which is right IMHO because it happens at the platform API level. GNUmakefile.am disables at the compile time TextCheckerEnchant,cpp and WebKitTextChecker.cpp files that are placed outside UIProcess/API/gtk and as I mentioned above in my opinion we can not just that in case of TextCheckerGtk.cpp Anyway I can find SPELLCHECK macro in TextCheckerEnchant{h.cpp} and WebKitTextChecker{h.cpp} but my sources may be out of date. > > However this change your proposing happens at the level of WebKit2 UIProcess's implementation level (UIProcess), which should be as much agnostic of this kind of things as possible, I think. > > > If you don't like this change just feel free and close this bug :) > > Oh, no! I won't do it :-) > > I'm just sharing my thoughts on this in order to help, not that I'm 100% sure I'm completely right or something. Actually, I could very well be perfectly wrong :-) > > I would appreciate if someone else with experience in WebKit2GTK could drop some comments on this topic too. Martin? Carlos? ;-) Ok. Anyway thanks for sharing your knowledge about spellchecking it's really precious for me as we are contributing spellchecking for WebKit2EFL.

(In reply to comment #5) >[...] > As I understand TextChecker class takes responsibility of public interface > for WebKit2 and its implementation is placed in port's specific files for > example, {port}/TextChecker{Port}.cpp. Well, actually TextChecker and WebTextChecker classes are at one level (the lowest level in WK2's UI process), then we have WKTextChecker at a higher level (the cross-platform C API) and finally we have WebKitTextChecker in the highest level (the platform-specific API). It's something like this, from lower to higher levels in WK2's UI Process: TextChecker / WebTextChecker <---> WKTextChecker <---> WebKitTextChecker ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ ^^^^^^^^^^^^^ ^^^^^^^^^^^^^^^^^ Implementation layer C API layer <platform> API layer It's messy, I know (at least it was messy for me first time I got into it), but the point is that IMHO the ENABLE(SPELLCHECK) guards should happen only at the level of the <platform> API layer, which were you most likely will start working from when adding a new implementation for a different platform. > In my opinion adding the right conditions (as I assume disabling > TextCheckerGtk.cpp if SPELLCHECK is OGG) to the makefile probably won't > work - undefined reference may occur while linking process . > Please correct me if I am wrong. Yes, it won't simply work by just using some ifs in the makefiles, you probably would need to make more changes (even in the source code). My point was that I would go that path only if I was really concerned about not compiling those generic hooks in the "implementation layer". But I personally think it doesn't pays off and so that's why I finally didn't mess with that either for GTK. > [...] > There are a lot of files/classes in WebKi2 for spelling for example, > TextChecker, WebTextChecker, WebKitTextChecker, WKTextChecker. > I think it's a little annoying to browse them to finally find > SPELLCHECK macro. I agree there are a lot of similar related files in WK2 for this, and I also think that's a little bit annoying because it's a bit confusing, at least the first time you face them (at least it was for me). But, in the other hand, if you think of them with the "diagram" I wrote above in mind, it's very clear that each of them respond to a very specific purpose in their very specific levels of abstraction, and also that the entry point for a platform is always the <platform> API layer (thus, "WebKitTextChecker"), which is where I would expect to find the macro in the first place. > > > - to consistency with files that implement spellchecking (it looks > > > like TextCheckerGtk makes an exception) > > > > Hrm.. I'm not sure. If I grep for SPELLCHECK in WebKit2/ folder: > > [...] > > ... I don't see any usage of that macro out of UIProcess/API/gtk, > > which is right IMHO because it happens at the platform API level. > > GNUmakefile.am disables at the compile time TextCheckerEnchant,cpp and > WebKitTextChecker.cpp files that are placed outside UIProcess/API/gtk > and as I mentioned above in my opinion we can not just that in case of > TextCheckerGtk.cpp For TextCheckerEnchant it makes sense not to even compile it when SPELLCHECK is not enabled, since for that to happen you need to have libenchant's development package installed on your system. So, if you don't have it there's nothing to do there. But TextCheckerGtk.cpp is not at a so low level as TextCheckerEnchant.cpp, since the former is in WebKit2/UIProcess, which means as much generic and cross-platform code as possible, and the later is in WebCore/platform, which is obviously platform-dependant code in the lowest (almost!) level of the WebKit project: WebCore. So, I think both files are not comparable in this regard. > Anyway I can find SPELLCHECK macro in TextCheckerEnchant{h.cpp} and > WebKitTextChecker{h.cpp} but my sources may be out of date. ...which, as I said, it's perfectly fine, since both files are located at platform-specific levels: TextCheckerEnchant{h.cpp}: WebCore/platform WebKitTextChecker{h.cpp}: WebKit2/UIProcess/API/gtk > > I would appreciate if someone else with experience in WebKit2GTK could drop some comments on this topic too. Martin? Carlos? ;-) > > Ok. Anyway thanks for sharing your knowledge about spellchecking it's > really precious for me as we are contributing spellchecking for WebKit2EFL. Glad to be helpful. Let's see what others think, though. As I said, I must be wrong after all :-)

I just realized my previous comment needs one clarification at least... (In reply to comment #6) > [...] > But TextCheckerGtk.cpp is not at a so low level as TextCheckerEnchant.cpp, > since the former is in WebKit2/UIProcess, which means as much generic and > cross-platform code as possible, and the later is in WebCore/platform, > which is obviously platform-dependant code in the lowest (almost!) level > of the WebKit project: WebCore. > > So, I think both files are not comparable in this regard. I know that WebKit2/UIProcess/gtk/TextCheckerGtk.cpp is a GTK specific file, so that probably makes my comment a bit confusing since I'm later saying this: > > Anyway I can find SPELLCHECK macro in TextCheckerEnchant{h.cpp} and > > WebKitTextChecker{h.cpp} but my sources may be out of date. > >...which, as I said, it's perfectly fine, since both files are located > at platform-specific levels: > > TextCheckerEnchant{h.cpp}: WebCore/platform > WebKitTextChecker{h.cpp}: WebKit2/UIProcess/API/gtk When I say that I find the ENABLE(SPELLCHECK) in those cases because they are "located at platform-specific levels", the important concept here from my point of view is "level". - WebKitTextChecker{h.cpp} is in WebKit2/UIProcess/API/gtk, which is the closest level to applications using WebKit2GTK. - TextCheckerEnchant{h.cpp} is in WebCore/platform/enchant, which is the closest level to the spell-checking technologie behind used (enchant). However, even if WebKit2/UIProcess/gtk/TextCheckerGtk.cpp is obviously a GTK specific file, it is not at the same kind of level than those other two files, since it's at WebKit2's UIProcess lowest level: what they call the cross-platform (well... almost!) implementation layer". Hope now it's clearer

Thanks again for kind explanation and a big portion very useful information for us. As a conclusion we can say that it's no good way using SPELLCHECK macro in TextChecker because it's some kind of violation of "implementation layer" mainly placed in TextChecker and WebTextChecker. This is fine for me especially if TextChecker's implementation in TextCheckerGtk is very generic and universal that there is no any issues when SPELLCHECK macro is OFF. If you decide that this macro is not necessary in TextCheckerGtk.cpp please close this bug, then I am going to remove that one from TextCheckerEfl.cpp (https://bugs.webkit.org/show_bug.cgi?id=91854)

(In reply to comment #8) > Thanks again for kind explanation and a big portion very useful > information for us. Glad to help. > As a conclusion we can say that it's no good way using SPELLCHECK macro in > TextChecker because it's some kind of violation of "implementation layer" > mainly placed in TextChecker and WebTextChecker. This is fine for me especially > if TextChecker's implementation in TextCheckerGtk is very generic and universal > that there is no any issues when SPELLCHECK macro is OFF. That would be my conclusion too, yes. > If you decide that this macro is not necessary in TextCheckerGtk.cpp please > close this bug, then I am going to remove that one from TextCheckerEfl.cpp > (https://bugs.webkit.org/show_bug.cgi?id=91854) Sounds good to me, but I first would prefer someone else to double check this, just to be at least 99% sure :-)

Comment on attachment 161983 [details] proposed patch Clearing review flag on patches from before 2014. If this patch is still relevant, please reset the r? flag.