Unify TextCheckerEfl.cpp and TextCheckerGtk.cpp into TextChecker.cpp because two implementations are mostly the same. This remove code duplication and Gtk port can reuse the Efl implementation of asynchronous spell checking and unified text checker.
Created attachment 207378 [details] Patch
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
Comment on attachment 207378 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=207378&action=review > Source/WebKit2/ChangeLog:27 > + * UIProcess/TextChecker.cpp: Renamed from Source/WebKit2/UIProcess/efl/TextCheckerEfl.cpp. I am wondering whether TextChecker.cpp is a proper name. We'll have TextChecker.cpp TextCheckerQt.cpp and TextCheckerMac.mm if the commit lands. How about DefaultTextChecker.cpp which will be used for EFL and GTK? In addition, we could consider to get rid of TextCheckerQt.cpp in a favour of DefaultTextChecker.cpp as its implementation uses the client's one. If the client is not defined, DefaultTextChecker.cpp should behave as the TextCheckerQt.cpp. I'd better ask Qt guys about that. > Source/WebKit2/UIProcess/TextChecker.cpp:35 > +#if ENABLE(SPELLCHECK) Would be nice if we could omit usage of the macro in the whole file. Mario Sanchez Prada has proposed it, maybe it's a good chance to get rid of it here? See https://bugs.webkit.org/show_bug.cgi?id=95718#c2 for more details. > Source/WebKit2/UIProcess/TextChecker.cpp:66 > +#if ENABLE(SPELLCHECK) > + return WebTextChecker::shared()->client().continuousSpellCheckingAllowed(); > +#else > + return false; > +#endif IMHO it's better to implement the methods once they are used (when the client implements it). As far as I know neither EFL or GTK implement it. I'd leave it as not implemented. > Source/WebKit2/UIProcess/TextChecker.cpp:84 > +void TextChecker::setGrammarCheckingEnabled(bool isGrammarCheckingEnabled) Ditto. > Source/WebKit2/UIProcess/TextChecker.cpp:108 > +void TextChecker::grammarCheckingEnabledStateChanged(bool enabled) Ditto. > Source/WebKit2/UIProcess/TextChecker.cpp:221 > +void TextChecker::checkGrammarOfString(int64_t spellDocumentTag, const UChar* text, uint32_t length, Vector<WebCore::GrammarDetail>& grammarDetails, int32_t& badGrammarLocation, int32_t& badGrammarLength) Ditto. > Source/WebKit2/UIProcess/TextChecker.cpp:235 > +bool TextChecker::spellingUIIsShowing() Ditto. > Source/WebKit2/UIProcess/TextChecker.cpp:244 > +void TextChecker::toggleSpellingUIIsShowing() Ditto. > Source/WebKit2/UIProcess/TextChecker.cpp:251 > +void TextChecker::updateSpellingUIWithMisspelledWord(int64_t spellDocumentTag, const String& misspelledWord) Ditto. > Source/WebKit2/UIProcess/TextChecker.cpp:261 > +void TextChecker::updateSpellingUIWithGrammarString(int64_t spellDocumentTag, const String& badGrammarPhrase, const GrammarDetail& grammarDetail) Ditto.
(In reply to comment #3) > I am wondering whether TextChecker.cpp is a proper name. We'll have TextChecker.cpp TextCheckerQt.cpp and TextCheckerMac.mm if the commit lands. > How about DefaultTextChecker.cpp which will be used for EFL and GTK? I think TextCheckerDefault.cpp is a better name. Similar cases are Source/WebCore/platform/text/LocaleToScriptMappingDefault.cpp and Source/WTF/wtf/unicode/CollatorDefault.cpp. > In addition, we could consider to get rid of TextCheckerQt.cpp in a favour of DefaultTextChecker.cpp as its implementation uses the client's one. If the client is not defined, DefaultTextChecker.cpp should behave as the TextCheckerQt.cpp. I'd better ask Qt guys about that. That's a good idea. Let's do this in a separate bug. > > Source/WebKit2/UIProcess/TextChecker.cpp:35 > > +#if ENABLE(SPELLCHECK) > > Would be nice if we could omit usage of the macro in the whole file. Mario Sanchez Prada has proposed it, maybe it's a good chance to get rid of it here? See https://bugs.webkit.org/show_bug.cgi?id=95718#c2 for more details. I agree with Mario. I will get rid of ENABLE(SPELLCHECK) guards in the file and follow the Gtk style. > > > Source/WebKit2/UIProcess/TextChecker.cpp:66 > > +#if ENABLE(SPELLCHECK) > > + return WebTextChecker::shared()->client().continuousSpellCheckingAllowed(); > > +#else > > + return false; > > +#endif > > IMHO it's better to implement the methods once they are used (when the client implements it). As far as I know neither EFL or GTK implement it. I'd leave it as not implemented. It makes sense when there is only one port. Now we have at least two ports (Gtk and Efl) and each port has varying degree of implementations. For example, only the EFL port has implementations of uniqueSpellDocumentTagCallback and closeSpellDocumentWithTagCallback. I think it is better to implement all the methods so that later each port can implement some methods without touching TextCheckerDefault.cpp.
Created attachment 207419 [details] Patch
Comment on attachment 207419 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=207419&action=review Looks fine. Added some comments before formal review. > Source/WebKit2/UIProcess/TextCheckerDefault.cpp:73 > +void TextChecker::setGrammarCheckingEnabled(bool isContinuousSpellCheckingEnabled) You've probably thought of isGrammarCheckingEnabled param. > Source/WebKit2/UIProcess/TextCheckerDefault.cpp:75 > + if (state().isContinuousSpellCheckingEnabled == isContinuousSpellCheckingEnabled) if (state().isGrammarCheckingEnabled ... ) > Source/WebKit2/UIProcess/TextCheckerDefault.cpp:77 > + textCheckerState.isContinuousSpellCheckingEnabled = isContinuousSpellCheckingEnabled; textCheckerState.isGrammarCheckingEnabled = ... > Source/WebKit2/UIProcess/TextCheckerDefault.cpp:78 > + WebTextChecker::shared()->client().setContinuousSpellCheckingEnabled(isContinuousSpellCheckingEnabled); ... setGrammarCheckingEnabled(isGrammarCheckingEnabled); That's why I was against of adding methods that can not be used/tested :p Additionally, could we match the style of the method to TextChecker::setContinuousSpellCheckingEnabled? They will look similar then (only the setting will be different). > Source/WebKit2/UIProcess/TextCheckerDefault.cpp:94 > + WebTextChecker::shared()->client().setGrammarCheckingEnabled(isGrammarCheckingEnabled); I am not sure, but I guess we don't need to send the notification to the client in this case. We are doing it in the setGrammarCheckingEnabled which is called when user has changed an appropriate context menu option. Additionally, could we keep the style of the above method? > Source/WebKit2/UIProcess/TextCheckerDefault.cpp:168 > +#endif Would be nice to add a comment to which #if it refers to. > Source/WebKit2/UIProcess/TextCheckerDefault.cpp:236 > +#endif Ditto.
Comment on attachment 207419 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=207419&action=review >> Source/WebKit2/UIProcess/TextCheckerDefault.cpp:78 >> + WebTextChecker::shared()->client().setContinuousSpellCheckingEnabled(isContinuousSpellCheckingEnabled); > > ... setGrammarCheckingEnabled(isGrammarCheckingEnabled); > > That's why I was against of adding methods that can not be used/tested :p > > Additionally, could we match the style of the method to TextChecker::setContinuousSpellCheckingEnabled? They will look similar then (only the setting will be different). Oops. I will fix it. Now I see your point :) >> Source/WebKit2/UIProcess/TextCheckerDefault.cpp:94 >> + WebTextChecker::shared()->client().setGrammarCheckingEnabled(isGrammarCheckingEnabled); > > I am not sure, but I guess we don't need to send the notification to the client in this case. We are doing it in the setGrammarCheckingEnabled which is called when user has changed an appropriate context menu option. > > Additionally, could we keep the style of the above method? You are right. I will remove the notification.
Created attachment 207436 [details] Patch
Comment on attachment 207436 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=207436&action=review > Source/WebKit2/UIProcess/TextCheckerDefault.cpp:69 > + > + WebTextChecker::shared()->client().setContinuousSpellCheckingEnabled(isContinuousSpellCheckingEnabled); Nit: IMHO the comment about the setting change has been removed unnecessarily. It shows the difference between setContinuousSpellCheckingEnabled(bool) and continuousSpellCheckingEnabledStateChanged(bool) > Source/WebKit2/UIProcess/TextCheckerDefault.cpp:79 > + > + WebTextChecker::shared()->client().setGrammarCheckingEnabled(isGrammarCheckingEnabled); Nit: I'd add the comment here as well.
Created attachment 207441 [details] Patch
(In reply to comment #9) > > Source/WebKit2/UIProcess/TextCheckerDefault.cpp:69 > > + > > + WebTextChecker::shared()->client().setContinuousSpellCheckingEnabled(isContinuousSpellCheckingEnabled); > > Nit: IMHO the comment about the setting change has been removed unnecessarily. It shows the difference between setContinuousSpellCheckingEnabled(bool) and continuousSpellCheckingEnabledStateChanged(bool) Done. > > Source/WebKit2/UIProcess/TextCheckerDefault.cpp:79 > > + > > + WebTextChecker::shared()->client().setGrammarCheckingEnabled(isGrammarCheckingEnabled); > > Nit: I'd add the comment here as well. Done. Thanks for the review!
(In reply to comment #11) > (In reply to comment #9) > > > Source/WebKit2/UIProcess/TextCheckerDefault.cpp:69 > > > + > > > + WebTextChecker::shared()->client().setContinuousSpellCheckingEnabled(isContinuousSpellCheckingEnabled); > > > > Nit: IMHO the comment about the setting change has been removed unnecessarily. It shows the difference between setContinuousSpellCheckingEnabled(bool) and continuousSpellCheckingEnabledStateChanged(bool) > > Done. > > > > Source/WebKit2/UIProcess/TextCheckerDefault.cpp:79 > > > + > > > + WebTextChecker::shared()->client().setGrammarCheckingEnabled(isGrammarCheckingEnabled); > > > > Nit: I'd add the comment here as well. > > Done. > > Thanks for the review! You're welcome! Feel free to add me to CC in spellchecking bugs if you'd like to hear my opinions.
Comment on attachment 207441 [details] Patch LGTM!
Chris. Would you mind sharing TextCheckingEfl.cpp with GTK+? IMHO it's good direction to have sharable TextCheckerDefault.cpp as well as TextCheckerEnchant.cpp I'd like to know your views. Thanks
(In reply to comment #14) > Chris. > Would you mind sharing TextCheckingEfl.cpp with GTK+? IMHO it's good direction to have sharable TextCheckerDefault.cpp as well as TextCheckerEnchant.cpp > I'd like to know your views. Thanks No, I do not mind. I think it is nice to share as much code as possible with other ports. I like the patch but a WK2 owner will need to sign off on this.
Comment on attachment 207441 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=207441&action=review > Source/WebKit2/UIProcess/TextCheckerDefault.cpp:31 > +#if PLATFORM(EFL) || PLATFORM(GTK) I don't really think we need this #if. Ports that do not wish to use it can simply not add it to their build system, right? See for e.g. Source/WebCore/platform/text/LocaleToScriptMappingDefault.cpp
(In reply to comment #15) > (In reply to comment #14) > > Chris. > > Would you mind sharing TextCheckingEfl.cpp with GTK+? IMHO it's good direction to have sharable TextCheckerDefault.cpp as well as TextCheckerEnchant.cpp > > I'd like to know your views. Thanks > > No, I do not mind. I think it is nice to share as much code as possible with other ports. I like the patch but a WK2 owner will need to sign off on this. Yes, WebKit direction is to share code between ports as much as possible.
Hello Kwang, Are you planning to continue working on this? I was trying to informally review this patch as it uses EFL's implementation of async spellchecking and it just needs minor improvements. As I already mention. there are plans to get rid of sync implementation of spellchecking (in WebCore and ports as well) in a favour of async path. As far as I know WebKit-GTK+ (both WK1 and WK2) implements synchronous spellchecking only. Switching to async path of spellchecing without this patch will completely break this feature for WebKit-GTK+.
(In reply to comment #18) > Hello Kwang, > Are you planning to continue working on this? I was trying to informally review this patch as it uses EFL's implementation of async spellchecking and it just needs minor improvements. > > As I already mention. there are plans to get rid of sync implementation of spellchecking (in WebCore and ports as well) in a favour of async path. > > As far as I know WebKit-GTK+ (both WK1 and WK2) implements synchronous spellchecking only. Switching to async path of spellchecing without this patch will completely break this feature for WebKit-GTK+. Yes, I am working on this at WebKitGTK hackfest.
Comment on attachment 207441 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=207441&action=review Thanks for this nice cleanup. r=me if you can get an owner to rubber stamp this. Owners? > Source/WebKit2/UIProcess/API/gtk/WebKitTextChecker.h:40 > + bool isSpellCheckingEnabled(); This function should be const. >> Source/WebKit2/UIProcess/TextCheckerDefault.cpp:31 >> +#if PLATFORM(EFL) || PLATFORM(GTK) > > I don't really think we need this #if. Ports that do not wish to use it can simply not add it to their build system, right? > See for e.g. Source/WebCore/platform/text/LocaleToScriptMappingDefault.cpp Don't forget to remove this #if, as Chris requested.
This patch is likely obsolete after https://bugs.webkit.org/show_bug.cgi?id=144648.
Comment on attachment 207441 [details] Patch Certainly it will need rebased, but I think the basic idea is still sound, merging the two files together. Though EFL's file is no longer the same as ours, so I guess this will need some work to rebase. I will r- just because we will want to review again if this is rebased. Thanks for your patch, Kwang, and sorry for the delay in getting a review.
EFL port has been removed.