Bug 111713 – [WK2][EFL] Text Checker's settings refactor

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit EFL

[Grzegorz Czajkowski]

This was pick up while irc discussion.
In TextCheckerEfl.cpp it's no need to call client methods which in fact, call ewk settings of text checker.

This bug additionally:
 - gets rid of C'ism in text checker's settings,
 - uses TextCheckerState as the main store for text checker's settings.

The text checker API to support others spell checker engines will be refactored in the separate bug.

[Grzegorz Czajkowski]

Created attachment 192435 [details]
proposed patch

[Chris Dumez]

Comment on attachment 192435 [details]
proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=192435&action=review

> Source/WebKit2/UIProcess/API/efl/ewk_settings.cpp:198
>          WKTextCheckerContinuousSpellCheckingEnabledStateChanged(enable);

Would it make sense to call this from TextCheckerSettings::setContinuousSpellCheckingEnabled() instead? It looks a bit weird to set the setting twice here.

> Source/WebKit2/UIProcess/efl/TextCheckerSettings.cpp:37
> +TextCheckerSettings* TextCheckerSettings::shared()

We could also return a reference as it cannot be NULL. I don't really mind either way.

> Source/WebKit2/UIProcess/efl/TextCheckerSettings.cpp:39
> +    static TextCheckerSettings* textCheckerSettings = adoptRef(new TextCheckerSettings).leakRef();

Adopt then leak? Looks like assigning "new TextCheckerSettings" directly would do the same thing, right?

> Source/WebKit2/UIProcess/efl/TextCheckerSettings.cpp:45
> +    , m_languagesUpdateTimer(this, &TextCheckerSettings::languagesUpdateTimerFired)

TextCheckerSettings:: seems useless

> Source/WebKit2/UIProcess/efl/TextCheckerSettings.cpp:46
> +    , m_spellCheckingSettingChangeTimer(this, &TextCheckerSettings::spellCheckingSettingChangeTimerFired)

Ditto.

> Source/WebKit2/UIProcess/efl/TextCheckerSettings.cpp:67
> +void TextCheckerSettings::languagesUpdateTimerFired(WebCore::Timer<TextCheckerSettings>*)

WebCore:: is probably not needed here.

> Source/WebKit2/UIProcess/efl/TextCheckerSettings.h:40
> +class TextCheckerSettings : public RefCounted<TextCheckerSettings> {

Does this really need to be ref counted? It is a singleton.

> Source/WebKit2/UIProcess/efl/TextCheckerSettings.h:44
> +    const WebKit::TextCheckerState& state() const { return m_state; }

WebKit:: is not needed here.

> Source/WebKit2/UIProcess/efl/TextCheckerSettings.h:48
> +    void continuousSpellCheckingChangeCallbackSet(Ewk_Settings_Continuous_Spell_Checking_Change_Cb callback);

I find it weird to use Yoda naming style in non-public API.

> Source/WebKit2/UIProcess/efl/TextCheckerSettings.h:49
> +    void continuousSpellCheckingChangeCallbackCall();

Ditto + Should we add "Async" in the name to clarify that the callback is not called synchronously?

> Source/WebKit2/UIProcess/efl/TextCheckerSettings.h:57
> +    WebKit::TextCheckerState m_state;

WebKit:: is not needed here.

[Grzegorz Czajkowski]

Created attachment 192892 [details]
apply Christophe's suggestions

[Grzegorz Czajkowski]

(In reply to comment #2)
> (From update of attachment 192435 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=192435&action=review
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_settings.cpp:198
> >          WKTextCheckerContinuousSpellCheckingEnabledStateChanged(enable);
> 
> Would it make sense to call this from TextCheckerSettings::setContinuousSpellCheckingEnabled() instead? It looks a bit weird to set the setting twice here.

Good catch. Thanks. I prefer using WK2's API as it calls the client method responsible for setting change (in fact TextCheckerSettings::setContinuousSpellCheckingEnabled()) and it also sends the message with updated TextCheckerState to the WebProcess.

> 
> > Source/WebKit2/UIProcess/efl/TextCheckerSettings.cpp:37
> > +TextCheckerSettings* TextCheckerSettings::shared()
> 
> We could also return a reference as it cannot be NULL. I don't really mind either way.

Agree. Changed to reference to simplify the TextCheckerSettings code (Remove adpotRef(...).leakRef(), RefCounted).

> 
> > Source/WebKit2/UIProcess/efl/TextCheckerSettings.cpp:39
> > +    static TextCheckerSettings* textCheckerSettings = adoptRef(new TextCheckerSettings).leakRef();
> 
> Adopt then leak? Looks like assigning "new TextCheckerSettings" directly would do the same thing, right?

Right.

> 
> > Source/WebKit2/UIProcess/efl/TextCheckerSettings.cpp:45
> > +    , m_languagesUpdateTimer(this, &TextCheckerSettings::languagesUpdateTimerFired)
> 
> TextCheckerSettings:: seems useless

Without it I got the following error message:
ISO C++ forbids taking the address of an unqualified or parenthesized non-static member function to form a pointer to member function.  Say ‘&WebKit::TextCheckerSettings::languagesUpdateTimerFired’ [-fpermissive]

> 
> > Source/WebKit2/UIProcess/efl/TextCheckerSettings.cpp:67
> > +void TextCheckerSettings::languagesUpdateTimerFired(WebCore::Timer<TextCheckerSettings>*)
> 
> WebCore:: is probably not needed here.

Removed.

> 
> > Source/WebKit2/UIProcess/efl/TextCheckerSettings.h:40
> > +class TextCheckerSettings : public RefCounted<TextCheckerSettings> {
> 
> Does this really need to be ref counted? It is a singleton.

Removed.

> 
> > Source/WebKit2/UIProcess/efl/TextCheckerSettings.h:44
> > +    const WebKit::TextCheckerState& state() const { return m_state; }
> 
> WebKit:: is not needed here.

Removed.

> 
> > Source/WebKit2/UIProcess/efl/TextCheckerSettings.h:48
> > +    void continuousSpellCheckingChangeCallbackSet(Ewk_Settings_Continuous_Spell_Checking_Change_Cb callback);
> 
> I find it weird to use Yoda naming style in non-public API.

Added a new typedef - ContinuousSpellCheckingChangeCallback. Is is Ok?

> 
> > Source/WebKit2/UIProcess/efl/TextCheckerSettings.h:49
> > +    void continuousSpellCheckingChangeCallbackCall();
> 
> Ditto + Should we add "Async" in the name to clarify that the callback is not called synchronously?

Added 'Async'.

> 
> > Source/WebKit2/UIProcess/efl/TextCheckerSettings.h:57
> > +    WebKit::TextCheckerState m_state;
> 
> WebKit:: is not needed here.

Removed.

Thanks for your review!

[Chris Dumez]

Comment on attachment 192892 [details]
apply Christophe's suggestions

View in context: https://bugs.webkit.org/attachment.cgi?id=192892&action=review

> Source/WebKit2/UIProcess/efl/TextCheckerSettings.h:49
> +    void continuousSpellCheckingChangeCallbackSet(ContinuousSpellCheckingChangeCallback);

Yoda style? How about setContinuousSpellCheckingChangeCallback()?

> Source/WebKit2/UIProcess/efl/TextCheckerSettings.h:50
> +    void continuousSpellCheckingChangeCallbackAsyncCall();

Ditto. How about callContinuousSpellCheckingChangeCallbackAsync() ?

[Mikhail Pozdnyakov]

Comment on attachment 192892 [details]
apply Christophe's suggestions

View in context: https://bugs.webkit.org/attachment.cgi?id=192892&action=review

> Source/WebKit2/UIProcess/efl/TextCheckerSettings.cpp:48
> +    m_state.isContinuousSpellCheckingEnabled = false;

why not to initialize m_state within initialization list as well?

> Source/WebKit2/UIProcess/efl/TextCheckerSettings.cpp:52
> +TextCheckerSettings::~TextCheckerSettings()

do we need empty destructor? can it be defined in header?

> Source/WebKit2/UIProcess/efl/TextCheckerSettings.h:47
> +    void setSpellCheckingLanguages(const Vector<String>& = Vector<String>());

nit: I would keep here the variable name..

[Kenneth Rohde Christiansen]

Comment on attachment 192892 [details]
apply Christophe's suggestions

View in context: https://bugs.webkit.org/attachment.cgi?id=192892&action=review

> Source/WebKit2/ChangeLog:13
> +        In 'TextCheckerEfl.cpp', it's no need to call the client's methods

there is not need to call

> Source/WebKit2/ChangeLog:19
> +        * PlatformEfl.cmake:
> +        Add new 'TextCheckerSettings.cpp' file to the build.

Simplify this: Add new files to the build.

> Source/WebKit2/ChangeLog:26
> +        * UIProcess/API/efl/ewk_settings.cpp:
> +        (ewk_settings_continuous_spell_checking_change_cb_set):
> +        (ewk_settings_continuous_spell_checking_enabled_get):
> +        (ewk_settings_spell_checking_languages_set):
> +        Use the 'TextCheckerSettings' class instead of internal
> +        ewk structure.

Use TextCheckerSettings instead of port-specific structure.

> Source/WebKit2/ChangeLog:36
> +        (Ewk_Text_Checker::initialize):
> +        Remove 'isContinuousSpellCheckingEnabled' and 'setContinuousSpellCheckingEnabled'
> +        callbacks from the WKTextChecker's client. They are not needed any longer.

Maybe explain why they are no longer needed

> Source/WebKit2/ChangeLog:66
> +        (WebKit::TextCheckerSettings::TextCheckerSettings):
> +        Set the default values of spell/grammar checking to false.
> +        Set the client callback to 0.

I find these comments a bit too detailed... the main changes get lost in details!

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker.cpp:280
> +    /* The callback should be called if the context menu "Check Spelling While Typing" option
> +       was toggled by the user.
> +    FIXME:
> +    1) Invoke the context menu on the input field.
> +    2) Choose the sub menu "Spelling and Grammar" option (not implemented for WK2).
> +    3) Toggle "Check Spelling While Typing" option.
> +    4) Check whether the callback is called. */
> +

So you are replacing the tests with FIXME's - why cant this be tested?

> Source/WebKit2/UIProcess/efl/TextCheckerSettings.cpp:32
> +
> +#include "ewk_text_checker_private.h"
> +

I would like to avoid this

> Source/WebKit2/UIProcess/efl/TextCheckerSettings.cpp:63
> +
> +    // Set the default language if user didn't specify any.
> +    if (enabled && !Ewk_Text_Checker::hasDictionary())
> +        setSpellCheckingLanguages();
> +}

Really bad that these implementations depends on the EWK public API. That is a layering violation

> Source/WebKit2/UIProcess/efl/TextCheckerSettings.h:38
> +
> +typedef Ewk_Settings_Continuous_Spell_Checking_Change_Cb ContinuousSpellCheckingChangeCallback;
> +

:(

[Grzegorz Czajkowski]

Comment on attachment 192892 [details]
apply Christophe's suggestions

Clearing the review flag according to Kenneth's review.

Thanks guys for the review. I'll apply all mentioned suggestion in the next patch.

[Grzegorz Czajkowski]

Created attachment 194546 [details]
do not use Ewk public API in the text checker implementation

[Grzegorz Czajkowski]

Comment on attachment 192892 [details]
apply Christophe's suggestions

View in context: https://bugs.webkit.org/attachment.cgi?id=192892&action=review

>> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker.cpp:280
>> +
> 
> So you are replacing the tests with FIXME's - why cant this be tested?

To test it I needed to apply Michal Pakula's patches for context menu. The spelling options are available through "Spelling and Grammar' sub menu which is not implemented for WK2.

>> Source/WebKit2/UIProcess/efl/TextCheckerSettings.cpp:48
>> +    m_state.isContinuousSpellCheckingEnabled = false;
> 
> why not to initialize m_state within initialization list as well?

I agree with you. To initialize those struct memebers within initialization list we will have to provide an idiomatic constructor to TextCheckerState. I am wondering if WK2 reviewers could approve that change as it's simple container only. Do you mind if I leave it as it is?

[Kenneth Rohde Christiansen]

Comment on attachment 194546 [details]
do not use Ewk public API in the text checker implementation

View in context: https://bugs.webkit.org/attachment.cgi?id=194546&action=review

> Source/WebKit2/UIProcess/API/efl/ewk_text_checker.cpp:202
> +#if ENABLE(SPELLCHECK)
> +    return TextCheckerSettings::shared().state().isContinuousSpellCheckingEnabled;
> +#else

Don't we have any C API for this?

> Source/WebKit2/UIProcess/API/efl/ewk_text_checker.cpp:220
> +    const Vector<String>& languages = TextCheckerSettings::shared().availableSpellCheckingLanguages();

C API?

> Source/WebKit2/UIProcess/API/efl/ewk_text_checker.cpp:221
> +    size_t numberOfLanuages = languages.size();

spelling issue!

> Source/WebKit2/UIProcess/API/efl/ewk_text_checker.cpp:235
> +    TextCheckerSettings::shared().setSpellCheckingLanguages(newLanguages);

C API?

> Source/WebKit2/UIProcess/API/efl/ewk_text_checker.cpp:245
> +    Vector<String> languages = TextCheckerSettings::shared().loadedSpellCheckingLanguages();

C API?

> Source/WebKit2/UIProcess/efl/TextCheckerEngine.cpp:35
> +TextCheckerEngine& TextCheckerEngine::shared()

instance() ?

[Mikhail Pozdnyakov]

Comment on attachment 194546 [details]
do not use Ewk public API in the text checker implementation

View in context: https://bugs.webkit.org/attachment.cgi?id=194546&action=review

> Source/WebKit2/UIProcess/efl/TextCheckerSettings.cpp:41
> +    return textCheckerSettings;

Does TextCheckerSettings make any sense without TextCheckerEngine?
I find it strange to have two singletons? Why do we have them?

> Source/WebKit2/UIProcess/efl/TextCheckerSettings.cpp:48
> +    m_state.isContinuousSpellCheckingEnabled = false;

why not in init list of the class?

> Source/WebKit2/UIProcess/efl/TextCheckerSettings.cpp:79
> +    return TextCheckerEngine::shared().loadedSpellCheckingLanguages();

?? why wrap method of one class with method from another class?

[Grzegorz Czajkowski]

Comment on attachment 194546 [details]
do not use Ewk public API in the text checker implementation

View in context: https://bugs.webkit.org/attachment.cgi?id=194546&action=review

>> Source/WebKit2/UIProcess/API/efl/ewk_text_checker.cpp:202
>> +#else
> 
> Don't we have any C API for this?

Actually yes, but it's not a typical getter. There's a callback function exposed through WKTextCheckerClient.
Frankly speaking, I don't see much sense (apart from setContinuousSpellCheckingEnabled [1]) of exposing settings via callback functions as it has been implemented for WK2 C API.

If you guys prefer calling this callback here, we will have to expose WKTextCheckerClient somehow. Maybe this is a good point to get rid of Ewk_Text_Checker::initialize from (ewk_text_checker_private.h)?
Following this we should be aware of further dependency as our text checker allows to override the default spellchecker engine via APIs. So at lest we will have to ensure access to Ewk client callbacks for WKTextCheckerClient (at the moment those are being stored in ewk_text_checker.cpp)

[1] This can be changed via context menu, user should be notified about the value change.

>> Source/WebKit2/UIProcess/API/efl/ewk_text_checker.cpp:220
>> +    const Vector<String>& languages = TextCheckerSettings::shared().availableSpellCheckingLanguages();
> 
> C API?

WKTextCheckerClient doesn't expose any APIs related with languages/dictionaries manipulations. Do you think of adding a new version of API to extend current WKTextCheckerClient or rather of adding a new C API specific for EFL.

>> Source/WebKit2/UIProcess/API/efl/ewk_text_checker.cpp:221
>> +    size_t numberOfLanuages = languages.size();
> 
> spelling issue!

Good catch! Thanks

>> Source/WebKit2/UIProcess/efl/TextCheckerEngine.cpp:35
>> +TextCheckerEngine& TextCheckerEngine::shared()
> 
> instance() ?

Ok.

>> Source/WebKit2/UIProcess/efl/TextCheckerSettings.cpp:41
>> +    return textCheckerSettings;
> 
> Does TextCheckerSettings make any sense without TextCheckerEngine?
> I find it strange to have two singletons? Why do we have them?

Yeah, it doesn't make much sense. I was trying to explain the reason of introducing TextCheckerEngine in ChangeLog (the easy way to replace Enchant in the future?). See more details in ChangeLog. 
How about seTextCheckerEngine as a member of TextCheckerSetting class. We could call it in ewk_text_checker.cpp (when TextCheckerEnchant is created) and avoid defining TextCheckerEngine. But then we will have enchant related code in ewk.

>> Source/WebKit2/UIProcess/efl/TextCheckerSettings.cpp:48
>> +    m_state.isContinuousSpellCheckingEnabled = false;
> 
> why not in init list of the class?

See comment 10.

>> Source/WebKit2/UIProcess/efl/TextCheckerSettings.cpp:79
>> +    return TextCheckerEngine::shared().loadedSpellCheckingLanguages();
> 
> ?? why wrap method of one class with method from another class?

Right. To manipulate languages stuff, we need to have access to spell checker engine. Two reason why they are exposed through TextCheckerSetting and forbidden for TextCheckerEngine are.
 - setSpellCheckingLanguages method loads the languages on timer to do not block UI (maybe we could do that in TextCheckeEngine?)
 - with consistency, other languages related methods are called through settings.

[Grzegorz Czajkowski]

Comment on attachment 194546 [details]
do not use Ewk public API in the text checker implementation

Clearing the review flags due to Kenneth and Mikhail review. I will simplify the code and apply the reviewers suggestions.

[Grzegorz Czajkowski]

Created attachment 196091 [details]
simplify the patch

[Mikhail Pozdnyakov]

Comment on attachment 196091 [details]
simplify the patch

View in context: https://bugs.webkit.org/attachment.cgi?id=196091&action=review

> Source/WebKit2/UIProcess/API/efl/ewk_text_checker.cpp:260
> +        listOflanguages = eina_list_append(listOflanguages, eina_stringshare_add(languages[i].utf8().data()));

think creation eina list from Vector<String> could be a separate aux function to avoid code repeating.

> Source/WebKit2/UIProcess/efl/TextCheckerEfl.cpp:60
> +    initializeState();

why do we need initializeState() function? is it re-used anywhere?

> Source/WebKit2/UIProcess/efl/TextCheckerEngine.cpp:47
> +void TextCheckerEngine::checkSpellingOfString(const String& text, int& misspellingLocation, int& misspellingLength)

I wish we had WK2 api for all this, otherwise why do we need all those wrapper methods? could m_textCheckerEngine be just exported?

[Grzegorz Czajkowski]

Created attachment 197731 [details]
use WK2 C API

 - Allows to call WKTextCheckerClient callbacks instead of internal WebTextChecker and TextChecker::state().
 - Detach text checker initialization from context.

See WebKit2's ChangeLog for more details.

[Mikhail Pozdnyakov]

Comment on attachment 197731 [details]
use WK2 C API

View in context: https://bugs.webkit.org/attachment.cgi?id=197731&action=review

> Source/WebKit2/UIProcess/efl/TextCheckerClientEfl.cpp:51
> +    memset(&m_wkTextCheckerClient, 0, sizeof(m_wkTextCheckerClient));

sizeof(m_wkTextCheckerClient)?

> Source/WebKit2/UIProcess/efl/TextCheckerClientEfl.h:33
> +#include "WKTextChecker.h"

<WebKit2/ ..>

> Source/WebKit2/UIProcess/efl/TextCheckerClientEfl.h:41
> +    WKTextCheckerClient wkClient() { return m_wkTextCheckerClient; }

const WKTextCheckerClient& wkClient() const { return m_wkTextCheckerClient; } to avoid copying, and do we really need it?

> Source/WebKit2/UIProcess/efl/TextCheckerEfl.cpp:79
> +    TextCheckerClientEfl::instance().wkClient().setContinuousSpellCheckingEnabled(

you can access client directly

> Source/WebKit2/UIProcess/efl/TextCheckerEfl.cpp:100
> +        TextCheckerClientEfl::instance().ensureSpellCheckingLanguage();

that's not good to access TextCheckerClientEfl from here, cannot ensureSpellCheckingLanguage functionality be implemented right here inside TextChecker?

> Source/WebKit2/UIProcess/efl/TextCheckerEfl.cpp:269
> +    TextCheckerClientEfl::instance().wkClient().learnWord(spellDocumentTag,

why TextCheckerClientEfl from here? 

See:
class WebTextChecker : public TypedAPIObject<APIObject::TypeTextChecker> {
...
    WebTextCheckerClient& client() { return m_client; }
}

> Source/WebKit2/UIProcess/efl/TextCheckerEfl.cpp:270
> +        WKStringCreateWithUTF8CString(word.utf8().data()),

adopt

> Source/WebKit2/UIProcess/efl/TextCheckerEfl.cpp:281
> +    TextCheckerClientEfl::instance().wkClient().ignoreWord(spellDocumentTag,

ditto.

> Source/WebKit2/UIProcess/efl/TextCheckerEfl.cpp:282
> +        WKStringCreateWithUTF8CString(word.utf8().data()),

adopt

[Grzegorz Czajkowski]

Created attachment 198270 [details]
apply Mikhail's comments

[Mikhail Pozdnyakov]

Comment on attachment 198270 [details]
apply Mikhail's comments

View in context: https://bugs.webkit.org/attachment.cgi?id=198270&action=review

> Source/WebKit2/UIProcess/API/efl/ewk_text_checker.cpp:56
> +    return clientCallbacks;

we have already a singleton right? could callbacks be members of TextCheckerClientEfl?

> Source/WebKit2/UIProcess/efl/TextCheckerClientEfl.h:41
> +    const WKTextCheckerClient& wkClient() { return m_wkTextCheckerClient; }

I see only one usage of this method: inside ewk_text_checker_continuous_spell_checking_enabled_get. If that suffices I would substitute
this method with "bool continuousSpellCheckingEnabled() const", and seems we do not need even m_wkTextCheckerClient class member.

> Source/WebKit2/UIProcess/efl/TextCheckerClientEfl.h:63
> +    static bool isContinuousSpellCheckingEnabled(const void*);

those could be free static functions inside cpp file (they are just dispatchers right), TextCheckerClientEfl instance could be passed as client info.
TextCheckerClientEfl could contain ewk callbacks (see my comment above).

[Grzegorz Czajkowski]

Comment on attachment 197731 [details]
use WK2 C API

View in context: https://bugs.webkit.org/attachment.cgi?id=197731&action=review

>> Source/WebKit2/UIProcess/efl/TextCheckerEfl.cpp:100
>> +        TextCheckerClientEfl::instance().ensureSpellCheckingLanguage();
> 
> that's not good to access TextCheckerClientEfl from here, cannot ensureSpellCheckingLanguage functionality be implemented right here inside TextChecker?

ensureSpellCheckingLanguages needs enchant library which is a member of TextCheckerClientEfl. If we had access to spell checking engine we'd implement it in TextChecker.

[Grzegorz Czajkowski]

Created attachment 198502 [details]
apply Mikhail's suggestions (comment #20)

[Grzegorz Czajkowski]

(In reply to comment #20)
> (From update of attachment 198270 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=198270&action=review
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_text_checker.cpp:56
> > +    return clientCallbacks;
> 
> we have already a singleton right? could callbacks be members of TextCheckerClientEfl?

Good idea - moved.

> 
> > Source/WebKit2/UIProcess/efl/TextCheckerClientEfl.h:41
> > +    const WKTextCheckerClient& wkClient() { return m_wkTextCheckerClient; }
> 
> I see only one usage of this method: inside ewk_text_checker_continuous_spell_checking_enabled_get. If that suffices I would substitute
> this method with "bool continuousSpellCheckingEnabled() const", and seems we do not need even m_wkTextCheckerClient class member.

Fine by me.
Added "bool isContinuousSpellCheckingEnabled() const" which calls WK2 callback.
Removed m_wkTextCheckerClient from TextCheckerClientEfl.

> 
> > Source/WebKit2/UIProcess/efl/TextCheckerClientEfl.h:63
> > +    static bool isContinuousSpellCheckingEnabled(const void*);
> 
> those could be free static functions inside cpp file (they are just dispatchers right), TextCheckerClientEfl instance could be passed as client info.
> TextCheckerClientEfl could contain ewk callbacks (see my comment above).

Could be, that was my first intention. If we move those callbacks out of the class we won't have access to the private methods/members of TextCheckerClientEfl. It affect other changes - we need access to m_textCheckerEnchant, timers etc. Is it ok if we leave them as class members?

[Grzegorz Czajkowski]

Created attachment 198691 [details]
pass 'this' as clientInfo to the callbacks

[Mikhail Pozdnyakov]

Comment on attachment 198691 [details]
pass 'this' as clientInfo to the callbacks 

View in context: https://bugs.webkit.org/attachment.cgi?id=198691&action=review

now looks better :)

> Source/WebKit2/UIProcess/efl/TextCheckerClientEfl.cpp:170
> +    WKMutableArrayRef suggestionsForWord = WKMutableArrayCreate();

when is it deleted?

[Grzegorz Czajkowski]

(In reply to comment #25)
> (From update of attachment 198691 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=198691&action=review
> 
> now looks better :)
> 
> > Source/WebKit2/UIProcess/efl/TextCheckerClientEfl.cpp:170
> > +    WKMutableArrayRef suggestionsForWord = WKMutableArrayCreate();
> 
> when is it deleted?

I suppose when we return from WebTextCheckerClient::guessesForWord()
See https://trac.webkit.org/browser/trunk/Source/WebKit2/UIProcess/WebTextCheckerClient.cpp#L153

[Mikhail Pozdnyakov]

(In reply to comment #26)
> (In reply to comment #25)
> > (From update of attachment 198691 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=198691&action=review
> > 
> > now looks better :)
> > 
> > > Source/WebKit2/UIProcess/efl/TextCheckerClientEfl.cpp:170
> > > +    WKMutableArrayRef suggestionsForWord = WKMutableArrayCreate();
> > 
> > when is it deleted?
> 
> I suppose when we return from WebTextCheckerClient::guessesForWord()
> See https://trac.webkit.org/browser/trunk/Source/WebKit2/UIProcess/WebTextCheckerClient.cpp#L153

OK, LGTM than.

[Andreas Kling]

Comment on attachment 198691 [details]
pass 'this' as clientInfo to the callbacks 

r=me based on comments from Mikhail.

[WebKit Commit Bot]

Comment on attachment 198691 [details]
pass 'this' as clientInfo to the callbacks 

Clearing flags on attachment: 198691

Committed r148670: <http://trac.webkit.org/changeset/148670>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.