Bug 119036 – [WK2][GTK][EFL] Unify TextChecker implementations of Gtk and Efl into one

• Status: RESOLVED
• Resolution: WONTFIX
• Component: WebKit2

[Kwang Yul Seo]

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.

[Kwang Yul Seo]

Created attachment 207378 [details]
Patch

[WebKit Commit Bot]

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

[Grzegorz Czajkowski]

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.

[Kwang Yul Seo]

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

[Kwang Yul Seo]

Created attachment 207419 [details]
Patch

[Grzegorz Czajkowski]

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.

[Kwang Yul Seo]

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.

[Kwang Yul Seo]

Created attachment 207436 [details]
Patch

[Grzegorz Czajkowski]

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.

[Kwang Yul Seo]

Created attachment 207441 [details]
Patch

[Kwang Yul Seo]

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

[Grzegorz Czajkowski]

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

[Grzegorz Czajkowski]

Comment on attachment 207441 [details]
Patch

LGTM!

[Grzegorz Czajkowski]

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

[Chris Dumez]

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

[Chris Dumez]

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

[Gyuyoung Kim]

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

[Grzegorz Czajkowski]

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

[Kwang Yul Seo]

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

[Michael Catanzaro]

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.

[Martin Robinson]

This patch is likely obsolete after https://bugs.webkit.org/show_bug.cgi?id=144648.

[Michael Catanzaro]

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.

[Michael Catanzaro]

EFL port has been removed.