[WK2][EFL] Implementation of spellchecking feature.

[Grzegorz Czajkowski]

This implementation is based on Enchant library (mostly implemented in TextCheckerEnchant class).
The patch also provides API to overwrite the default WebKit spellchecker implementation.

There is one spellchecker object per application, it allows to check spelling in the editable areas.

WebKit spellchecker implementation doesn't ensure grammar checking.
Application is able to overwrite it (both grammar and spell checking) by defining its own implementation and setting appropriate callback functions.

File ewk_text_checker_settings.h defines API for manipulating spell checker feature:
 - enables/disables spell checking,
 - enables/disables grammar checking,
 - sets languages to use be default WebKit implementation of spellchecker feature with Enchant library support,
 - sets callbacks function to notify the client when the settings (spell and grammar checking) was changed by WebKit (not required). Changing of this setting at the WebKit level can be made as a result of modifying options in a Context Menu by a user.

I will submit the patches when I come back from holiday :)

[Grzegorz Czajkowski]

Created attachment 161908 [details]
proposed patch

This patch introduces spellchecking feature for WK2-EFL that is based on the Enchant library. The core spellchecking feature we share with WebKit-Gtk (contributed here https://bugs.webkit.org/show_bug.cgi?id=94320)

Additionally the patch provides API to overwrite the default WebKit spellchecker implementation as Enchant library doesn't ensure grammar checking. Application is able to overwrite it (both grammar and spell checking) by defining its own implementation and setting appropriate callback functions.
Some stubs which support grammar checking are delivered too.

This patch doesn't take care of layout/unit tests. They will be supported in the separated bugs.

[Gyuyoung Kim]

Comment on attachment 161908 [details]
proposed patch

Attachment 161908 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13745105

[Grzegorz Czajkowski]

Created attachment 161911 [details]
add missing FindEnchant.cmake file

[Gyuyoung Kim]

Comment on attachment 161911 [details]
add missing FindEnchant.cmake file

Attachment 161911 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13755017

[Grzegorz Czajkowski]

(In reply to comment #4)
> (From update of attachment 161911 [details])
> Attachment 161911 [details] did not pass efl-ews (efl):
> Output: http://queues.webkit.org/results/13755017

-- checking for module 'enchant'
--   package 'enchant' not found

libenchant-dev package should be installed. Gyuyoung, could you install this package? Thanks.

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 161911 [details]
add missing FindEnchant.cmake file

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

> Source/cmake/FindEnchant.cmake:16
> +#   This library is free software; you can redistribute it and/or
> +#   modify it under the terms of the GNU Library General Public
> +#   License as published by the Free Software Foundation; either
> +#   version 2 of the License, or (at your option) any later version.
> +#
> +#   This library is distributed in the hope that it will be useful,
> +#   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +#   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +#   Library General Public License for more details.
> +#
> +#   You should have received a copy of the GNU Library General Public License
> +#   along with this library; see the file COPYING.LIB.  If not, write to
> +#   the Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor,
> +#   Boston, MA 02110-1301, USA.

Would be good if you could add some documentation at the top (like the one in FindCairo.cmake). Most of our Find modules are BSD-licensed as well; are you sure LGPLv2 is preferred here?

> Source/cmake/FindEnchant.cmake:21
> +if( NOT WIN32 )
> +    find_package(PkgConfig)
> +    pkg_check_modules(Enchant enchant)
> +endif( NOT WIN32 )

The capitalization of the calls here is wrong, and using pkg-config on Windows if it's present should be harmless.

[Dominik Röttsches (drott)]

(In reply to comment #5)
> libenchant-dev package should be installed. Gyuyoung, could you install this package? Thanks.

FYI, installed the package on our buildbots - so it shouldn't break anything on those once it lands.

[Grzegorz Czajkowski]

(In reply to comment #7)
> (In reply to comment #5)
> > libenchant-dev package should be installed. Gyuyoung, could you install this package? Thanks.
> 
> FYI, installed the package on our buildbots - so it shouldn't break anything on those once it lands.

Thanks. I will submit a new patch as soon as I apply Kubo's suggestion.

[Grzegorz Czajkowski]

(In reply to comment #6)
> (From update of attachment 161911 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=161911&action=review
> 
> > Source/cmake/FindEnchant.cmake:16
> > +#   This library is free software; you can redistribute it and/or
> > +#   modify it under the terms of the GNU Library General Public
> > +#   License as published by the Free Software Foundation; either
> > +#   version 2 of the License, or (at your option) any later version.
> > +#
> > +#   This library is distributed in the hope that it will be useful,
> > +#   but WITHOUT ANY WARRANTY; without even the implied warranty of
> > +#   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> > +#   Library General Public License for more details.
> > +#
> > +#   You should have received a copy of the GNU Library General Public License
> > +#   along with this library; see the file COPYING.LIB.  If not, write to
> > +#   the Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor,
> > +#   Boston, MA 02110-1301, USA.
> 
> Would be good if you could add some documentation at the top (like the one in FindCairo.cmake). Most of our Find modules are BSD-licensed as well; are you sure LGPLv2 is preferred here?

Thanks for the hints. Added docs and replaced license to BSD.

> 
> > Source/cmake/FindEnchant.cmake:21
> > +if( NOT WIN32 )
> > +    find_package(PkgConfig)
> > +    pkg_check_modules(Enchant enchant)
> > +endif( NOT WIN32 )
> 
> The capitalization of the calls here is wrong, and using pkg-config on Windows if it's present should be harmless.

Ok. Just removed this block similarly to FindCairo.cmake and used better names.

[Grzegorz Czajkowski]

Created attachment 161930 [details]
refactoring of FindEnchant.cmake

[Gyuyoung Kim]

Comment on attachment 161930 [details]
refactoring of FindEnchant.cmake

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

> Source/WebKit2/PlatformEfl.cmake:129
> +    "${WEBCORE_DIR}/platform/text/enchant"

Wrong alphabetical order.

> Source/WebKit2/UIProcess/API/efl/WebKitTextChecker.cpp:2
> + * Copyright (C) 2012 Samsung Electronics

I was told BSD license is more useful. So, could you use BSD instead of LGPL ?

> Source/WebKit2/UIProcess/API/efl/WebKitTextChecker.cpp:53
> +OwnPtr<WebCore::TextCheckerEnchant> m_textCheckerEnchant = WebCore::TextCheckerEnchant::create();

Why this variable is not private member variable ?

> Source/WebKit2/UIProcess/API/efl/WebKitTextChecker.cpp:96
> +void WebKitTextChecker::checkSpellingOfString(uint64_t tag, WKStringRef text, int32_t* misspellingLocation, int32_t* misspellingLength, const void *)

s/const void */const void*/g

> Source/WebKit2/UIProcess/API/efl/WebKitTextChecker.cpp:133
> +    // FIXME: call client's function.

Generally, FIXME: is placed before notImplemented().

> Source/WebKit2/UIProcess/API/efl/WebKitTextChecker.h:36
> +

Unneeded line.

> Source/WebKit2/UIProcess/API/efl/ewk_text_checker.cpp:87
> +// EWK_TEXT_CHECKER_CALLBACK_SET(Ewk_Text_Checker_String_Grammar_Check, string_grammar_check)

I think we should not keep commented macro or function.

> Source/WebKit2/UIProcess/API/efl/ewk_text_checker.h:86
> +// EAPI void ewk_text_checker_string_grammar_check_cb_set(Ewk_Text_Checker_String_Grammar_Check cb);

I think we should not keep commented function declaration.

> Source/WebKit2/UIProcess/API/efl/ewk_text_checker.h:91
> +// EAPI void ewk_text_checker_ui_spelling_grammar_string_update_cb_set(Ewk_Text_Checker_UI_Spelling_Grammar_String_Update cb);

ditto.

> Source/WebKit2/UIProcess/API/efl/ewk_text_checker_private.h:4
> + * This library is free software; you can redistribute it and/or

ditto.

> Source/WebKit2/UIProcess/API/efl/ewk_text_checker_setting.cpp:33
> +    bool enableContinuousSpellChecking:1;

s/Checking:1/Checking : 1/g

> Source/WebKit2/UIProcess/API/efl/ewk_text_checker_setting.cpp:34
> +    bool enableGrammarChecking:1;

ditto.

> Source/WebKit2/UIProcess/API/efl/ewk_text_checker_setting.cpp:62
> +    // TODO: Consider to delegate calling of this method in WebProcess to do not delay/block UIProcess.

WebKit coding style guides to use FIXME: instead of TODO:

> Source/WebKit2/UIProcess/API/efl/ewk_text_checker_setting.h:4
> + * This library is free software; you can redistribute it and/or

ditto.

> Source/WebKit2/UIProcess/efl/TextCheckerEfl.cpp:81
> +        return;

A new line ?

[Grzegorz Czajkowski]

(In reply to comment #11)
> (From update of attachment 161930 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=161930&action=review
> 
> > Source/WebKit2/PlatformEfl.cmake:129
> > +    "${WEBCORE_DIR}/platform/text/enchant"
> 
> Wrong alphabetical order.

Thanks. Fixed.

> 
> > Source/WebKit2/UIProcess/API/efl/WebKitTextChecker.cpp:2
> > + * Copyright (C) 2012 Samsung Electronics
> 
> I was told BSD license is more useful. So, could you use BSD instead of LGPL ?

Ok, I didn't know that WebKit-EFL tends to use BSD license over LGPL. I just copied the license from ewk_view.cpp.
Anyway. All newly crated files (7) have BSD license from now :)

> 
> > Source/WebKit2/UIProcess/API/efl/WebKitTextChecker.cpp:53
> > +OwnPtr<WebCore::TextCheckerEnchant> m_textCheckerEnchant = WebCore::TextCheckerEnchant::create();
> 
> Why this variable is not private member variable ?

WebKitTextChecker class has got only static methods to easy initialize WKTextChecker's client (see ewk_text_checker.cpp). We can directly pass those callbacks without additional wrapper like WebKit-GTK does.
Additional advantage of this solution is to easy accessible methods from EnchantHelper subclass (mainly used in ewk_text_checker_setting.cpp) so we do not have keep WebKitTextChecker object anywhere.

According to above reasons I would like to avoid defining class with the constructor only to keep 'm_textCheckerEnchant' as private member.
Of course we can consider typical solution used in singletons like method 'initialize()' that will be responsible for initialization of 'm_textCheckerEnchant'. But does it make sense to initialize only one variable?

> 
> > Source/WebKit2/UIProcess/API/efl/WebKitTextChecker.cpp:96
> > +void WebKitTextChecker::checkSpellingOfString(uint64_t tag, WKStringRef text, int32_t* misspellingLocation, int32_t* misspellingLength, const void *)
> 
> s/const void */const void*/g

Fixed.

> 
> > Source/WebKit2/UIProcess/API/efl/WebKitTextChecker.cpp:133
> > +    // FIXME: call client's function.
> 
> Generally, FIXME: is placed before notImplemented().

Fixed in the whole file.

> 
> > Source/WebKit2/UIProcess/API/efl/WebKitTextChecker.h:36
> > +
> 
> Unneeded line.

Fixed.

> 
> > Source/WebKit2/UIProcess/API/efl/ewk_text_checker.cpp:87
> > +// EWK_TEXT_CHECKER_CALLBACK_SET(Ewk_Text_Checker_String_Grammar_Check, string_grammar_check)
> 
> I think we should not keep commented macro or function.
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_text_checker.h:86
> > +// EAPI void ewk_text_checker_string_grammar_check_cb_set(Ewk_Text_Checker_String_Grammar_Check cb);
> 
> I think we should not keep commented function declaration.

Those methods were commented out because they are connected with grammar checking feature which is not supported by Enchant. Removed.


> > Source/WebKit2/UIProcess/API/efl/ewk_text_checker_setting.cpp:33
> > +    bool enableContinuousSpellChecking:1;
> 
> s/Checking:1/Checking : 1/g
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_text_checker_setting.cpp:34
> > +    bool enableGrammarChecking:1;
> 
> ditto.

Fixed.

> 
> > Source/WebKit2/UIProcess/API/efl/ewk_text_checker_setting.cpp:62
> > +    // TODO: Consider to delegate calling of this method in WebProcess to do not delay/block UIProcess.
> 
> WebKit coding style guides to use FIXME: instead of TODO:

Fixed.


> > Source/WebKit2/UIProcess/efl/TextCheckerEfl.cpp:81
> > +        return;
> 
> A new line ?

Makes sense. Added

[Grzegorz Czajkowski]

Created attachment 161980 [details]
apply Gyuyoung's suggestions

[Gyuyoung Kim]

Comment on attachment 161980 [details]
apply Gyuyoung's suggestions

Attachment 161980 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13739440

[Gyuyoung Kim]

I install libenchant-dev to efl ews. Please submit patch again to test it.

[Grzegorz Czajkowski]

Created attachment 162222 [details]
no change, just to make efl ews happy

[Gyuyoung Kim]

Comment on attachment 162222 [details]
no change, just to make efl ews happy

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

Public API of WK2 is added with unit test. Please prepare unit test as well.

> Source/WebKit2/UIProcess/API/efl/ewk_text_checker_setting.h:67
> +EAPI void ewk_text_checker_setting_grammar_checking_changed_cb_set(Ewk_Text_Checker_Setting_Grammar_Checking_Changed cb);

It looks function name is ambiguous. Can't function name be reduced ?

> Source/WebKit2/UIProcess/API/efl/ewk_text_checker_setting.h:116
> + * If @languages is @c 0, the default language is used.

We decided to use NULL instead of 0 in description.

> Source/WebKit2/UIProcess/API/efl/ewk_text_checker_setting.h:123
> + *        of the form 'en_US', ie, language_VARIANT, may be @c 0.

ditto.

[WebKit Review Bot]

Comment on attachment 162222 [details]
no change, just to make efl ews happy

Attachment 162222 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13761569

New failing tests:
http/tests/cache/subresource-expiration-1.html
http/tests/cache/stopped-revalidation.html

[Grzegorz Czajkowski]

(In reply to comment #17)
> (From update of attachment 162222 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=162222&action=review
> 
> Public API of WK2 is added with unit test. Please prepare unit test as well.

See comment 1 please. I didn't want to add those to the main implementation to do not increase it. In my opinion this patch is enough huge. Anyway API has been approved yet.

> 
> > Source/WebKit2/UIProcess/API/efl/ewk_text_checker_setting.h:67
> > +EAPI void ewk_text_checker_setting_grammar_checking_changed_cb_set(Ewk_Text_Checker_Setting_Grammar_Checking_Changed cb);
> 
> It looks function name is ambiguous. Can't function name be reduced ?

Here I just mixed names from WKTextChecker and ewk names.
This function sets callback function to notify client when grammar checking setting has been changed. You can find more details in doxygen doc.
Anyway there are a lot of similar functions (see API from ewk_text_checker and ewk_text_checker_setting. Any suggestion?

> 
> > Source/WebKit2/UIProcess/API/efl/ewk_text_checker_setting.h:116
> > + * If @languages is @c 0, the default language is used.
> 
> We decided to use NULL instead of 0 in description.
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_text_checker_setting.h:123
> > + *        of the form 'en_US', ie, language_VARIANT, may be @c 0.
> 
> ditto.

Right. I will replace 0 -> NULL in next patch.

[Chris Dumez]

Comment on attachment 162222 [details]
no change, just to make efl ews happy

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

> Source/WebKit2/UIProcess/API/efl/WebKitTextChecker.cpp:157
> +        for (Vector<String>::const_iterator iter = guesses.begin(); iter != guesses.end(); ++iter) {

Coding style says to avoid iterators and use indexes instead for better readability.

> Source/WebKit2/UIProcess/API/efl/ewk_text_checker.h:50
> +typedef uint64_t (*Ewk_Text_Checker_Unique_Spell_Document_Tag_Get)(const Evas_Object *o);

Missing doc?

> Source/WebKit2/UIProcess/API/efl/ewk_text_checker.h:68
> + * @param cb a new callback to set or @c 0 to restore the default WebKit callback implementation

@c NULL

> Source/WebKit2/UIProcess/API/efl/ewk_text_checker.h:79
> + * @param cb a new callback to set or @c 0 to restore the default WebKit callback implementation

Ditto.

> Source/WebKit2/UIProcess/API/efl/ewk_text_checker.h:86
> + * @param cb a new callback to set or @c 0 to restore the default WebKit callback implementation

Ditto.

> Source/WebKit2/UIProcess/API/efl/ewk_text_checker.h:90
> +EAPI void ewk_text_checker_ui_spelling_status_get_cb_set(Ewk_Text_Checker_UI_Spelling_Status_Get cb);

Missing doc?

> Source/WebKit2/UIProcess/API/efl/ewk_text_checker.h:91
> +EAPI void ewk_text_checker_ui_toggle_spelling_status_get_cb_set(Ewk_Text_Checker_UI_Toggle_Spelling_Status_Get cb);

Ditto.

> Source/WebKit2/UIProcess/API/efl/ewk_text_checker.h:92
> +EAPI void ewk_text_checker_ui_spelling_misspelled_word_update_cb_set(Ewk_Text_Checker_UI_Spelling_Misspelled_Word_Update cb);

Ditto.

> Source/WebKit2/UIProcess/API/efl/ewk_text_checker.h:102
> + * @param cb a new callback to set or @c 0 to restore the default WebKit callback implementation

@c NULL

> Source/WebKit2/UIProcess/API/efl/ewk_text_checker.h:109
> + * @param cb a new callback to set or @c 0 to restore the default WebKit callback implementation

Ditto.

> Source/WebKit2/UIProcess/API/efl/ewk_text_checker.h:116
> + * @param cb a new callback to set or @c 0 to restore the default WebKit callback implementation

Ditto.

> Source/WebKit2/UIProcess/API/efl/ewk_text_checker_setting.cpp:76
> +    ewkTextCheckerSetting.spellCheckingLanguages.swap(languages);

What's the point of swapping here? Why can't we simply do:
ewkTextCheckerSetting.spellCheckingLanguages = newLanguages; ?

> Source/WebKit2/UIProcess/API/efl/ewk_text_checker_setting.cpp:151
> +    String::fromUTF8(languages).split(static_cast<UChar>(','), newLanguages);

Is the casting really needed?

> Source/WebKit2/UIProcess/API/efl/ewk_text_checker_setting.h:40
> +typedef void (*Ewk_Text_Checker_Setting_Continuous_Spell_Checking_Changed)(Eina_Bool enable);

Missing doc?

> Source/WebKit2/UIProcess/API/efl/ewk_text_checker_setting.h:41
> +typedef void (*Ewk_Text_Checker_Setting_Grammar_Checking_Changed)(Eina_Bool enable);

Ditto.

> Source/WebKit2/UIProcess/API/efl/ewk_text_checker_setting.h:52
> + * @param cb a new callback function to set or @c 0 to invalidate the previous one

@c NULL

> Source/WebKit2/UIProcess/API/efl/ewk_text_checker_setting.h:65
> + * @param cb a new callback function to set or @c 0 to invalidate the previous one

@c NULL

> Source/WebKit2/UIProcess/API/efl/ewk_text_checker_setting.h:72
> + * @return @c EINA_TRUE if continuous spell checking is enabled or @ EINA_FALSE if it's disabled

@c EINA_FALSE

> Source/WebKit2/UIProcess/API/efl/ewk_text_checker_setting.h:95
> + * @return @c EINA_TRUE if continuous spell checking is enabled or @ EINA_FALSE if it's disabled

@c EINA_FALSE

> Source/WebKit2/UIProcess/API/efl/ewk_text_checker_setting.h:113
> + * Sets @languages as the list of languages to use by default WebKit

You probably know better than me but just in case: Shouldn't it be @a languages?

[Gyuyoung Kim]

Comment on attachment 162222 [details]
no change, just to make efl ews happy

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

>> Here I just mixed names from WKTextChecker and ewk names.
>> This function sets callback function to notify client when grammar checking setting has been changed. You can find more details in doxygen doc.
>> Anyway there are a lot of similar functions (see API from ewk_text_checker and ewk_text_checker_setting. Any suggestion?

As you said, this function names are similar to existing API names. So, I couldn't fine better name yet. If there is no better suggestion, I agree to use this API names.

> Source/WebKit2/UIProcess/API/efl/ewk_text_checker_setting.cpp:73
> +void static _ewk_text_checker_setting_spell_checking_languages_set(const Vector<String>& newLanguages)

Is there any reason to use *void static* ?  AFAIK, *static void* is more general in WebKit for internal function. By the way, I wonder why this internal function only follows efl coding style for function name.

[Grzegorz Czajkowski]

Created attachment 162506 [details]
apply Christophe's and Gyuyoung suggestions

[Grzegorz Czajkowski]

(In reply to comment #20)
> (From update of attachment 162222 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=162222&action=review
> 
> > Source/WebKit2/UIProcess/API/efl/WebKitTextChecker.cpp:157
> > +        for (Vector<String>::const_iterator iter = guesses.begin(); iter != guesses.end(); ++iter) {
> 
> Coding style says to avoid iterators and use indexes instead for better readability.

Changed to indexes. Additionally size of vector is cached :)

> 
> > Source/WebKit2/UIProcess/API/efl/ewk_text_checker.h:50
> > +typedef uint64_t (*Ewk_Text_Checker_Unique_Spell_Document_Tag_Get)(const Evas_Object *o);
> 
> Missing doc?
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_text_checker.h:68
> > + * @param cb a new callback to set or @c 0 to restore the default WebKit callback implementation
> 
> @c NULL
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_text_checker.h:79
> > + * @param cb a new callback to set or @c 0 to restore the default WebKit callback implementation
> 
> Ditto.
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_text_checker.h:86
> > + * @param cb a new callback to set or @c 0 to restore the default WebKit callback implementation
> 
> Ditto.
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_text_checker.h:90
> > +EAPI void ewk_text_checker_ui_spelling_status_get_cb_set(Ewk_Text_Checker_UI_Spelling_Status_Get cb);
> 
> Missing doc?
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_text_checker.h:91
> > +EAPI void ewk_text_checker_ui_toggle_spelling_status_get_cb_set(Ewk_Text_Checker_UI_Toggle_Spelling_Status_Get cb);
> 
> Ditto.
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_text_checker.h:92
> > +EAPI void ewk_text_checker_ui_spelling_misspelled_word_update_cb_set(Ewk_Text_Checker_UI_Spelling_Misspelled_Word_Update cb);
> 
> Ditto.
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_text_checker.h:102
> > + * @param cb a new callback to set or @c 0 to restore the default WebKit callback implementation
> 
> @c NULL
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_text_checker.h:109
> > + * @param cb a new callback to set or @c 0 to restore the default WebKit callback implementation
> 
> Ditto.
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_text_checker.h:116
> > + * @param cb a new callback to set or @c 0 to restore the default WebKit callback implementation
> 
> Ditto.

I added the missing docs, all 0 were replaced to NULL.

> 
> > Source/WebKit2/UIProcess/API/efl/ewk_text_checker_setting.cpp:76
> > +    ewkTextCheckerSetting.spellCheckingLanguages.swap(languages);
> 
> What's the point of swapping here? Why can't we simply do:
> ewkTextCheckerSetting.spellCheckingLanguages = newLanguages; ?

Removed unnecessary assignment. 

> 
> > Source/WebKit2/UIProcess/API/efl/ewk_text_checker_setting.cpp:151
> > +    String::fromUTF8(languages).split(static_cast<UChar>(','), newLanguages);
> 
> Is the casting really needed?

I tested this one - no needed. Removed. Thanks :)

> 
> > Source/WebKit2/UIProcess/API/efl/ewk_text_checker_setting.h:40
> > +typedef void (*Ewk_Text_Checker_Setting_Continuous_Spell_Checking_Changed)(Eina_Bool enable);
> 
> Missing doc?
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_text_checker_setting.h:41
> > +typedef void (*Ewk_Text_Checker_Setting_Grammar_Checking_Changed)(Eina_Bool enable);
> 
> Ditto.
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_text_checker_setting.h:52
> > + * @param cb a new callback function to set or @c 0 to invalidate the previous one
> 
> @c NULL
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_text_checker_setting.h:65
> > + * @param cb a new callback function to set or @c 0 to invalidate the previous one
> 
> @c NULL
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_text_checker_setting.h:72
> > + * @return @c EINA_TRUE if continuous spell checking is enabled or @ EINA_FALSE if it's disabled
> 
> @c EINA_FALSE
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_text_checker_setting.h:95
> > + * @return @c EINA_TRUE if continuous spell checking is enabled or @ EINA_FALSE if it's disabled
> 
> @c EINA_FALSE

Fixed/Added.

> 
> > Source/WebKit2/UIProcess/API/efl/ewk_text_checker_setting.h:113
> > + * Sets @languages as the list of languages to use by default WebKit
> 
> You probably know better than me but just in case: Shouldn't it be @a languages?

Yes, it should be :) Fixed.
Thanks for review

[Grzegorz Czajkowski]

(In reply to comment #21)
> (From update of attachment 162222 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=162222&action=review
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_text_checker_setting.cpp:73
> > +void static _ewk_text_checker_setting_spell_checking_languages_set(const Vector<String>& newLanguages)
> 
> Is there any reason to use *void static* ?  AFAIK, *static void* is more general in WebKit for internal function. By the way, I wonder why this internal function only follows efl coding style for function name.

Thanks, my mistake. Fixed

[Grzegorz Czajkowski]

Created attachment 163324 [details]
add ewk_text_checker_setting_spell_checking_languages_get

Added 'ewk_text_checker_setting_spell_checking_languages_get' method to support unit tests (it ensures possibility to test ewk_text_checker_setting_spell_checking_languages_set method.

[Mario Sanchez Prada]

Comment on attachment 163324 [details]
add ewk_text_checker_setting_spell_checking_languages_get

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

> Source/WebKit2/UIProcess/API/efl/WebKitTextChecker.h:40
> +class WebKitTextChecker {

I've read comment #12, but still I don't see the advantage of defining a class this way. Why having a class full of all static members instead of just having a regular header file with functions implemented in the .cpp file (not using a class)?

[Grzegorz Czajkowski]

(In reply to comment #26)
> (From update of attachment 163324 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=163324&action=review
> 
> > Source/WebKit2/UIProcess/API/efl/WebKitTextChecker.h:40
> > +class WebKitTextChecker {
> 
> I've read comment #12, but still I don't see the advantage of defining a class this way. Why having a class full of all static members instead of just having a regular header file with functions implemented in the .cpp file (not using a class)?

Thanks for your review. This suggestion makes sense to me. I will apply it.

[Grzegorz Czajkowski]

Created attachment 163862 [details]
remove WebKitTextChecker class

Removed WebKitTextChecker class
Added ewk_text_checker_setting_available_spell_checking_languages_get() API based on bug 96518
Moved the client's callback from WebKitTextChecker to ewk_text_checker

[Gyuyoung Kim]

Comment on attachment 163862 [details]
remove WebKitTextChecker class

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

> Source/WebCore/PlatformEfl.cmake:341
> +        platform/text/enchant/TextCheckerEnchant.cpp

I have removed duplicated #ifdef guard. TextCheckerEnchant.cpp file is already guarded by #if ENABLE(SPELLCHECK). So, we don't need to guard this again.

> Source/WebKit2/PlatformEfl.cmake:334
> +    LIST(APPEND WebKit2_INCLUDE_DIRECTORIES

Why do you append WebKit2_INCLUDE_DIRECTORIES twice ?

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

Please use WKEinaSharedString instead of strdup.

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

ditto.

> Source/WebKit2/UIProcess/API/efl/ewk_text_checker_setting.h:163
> ++ * Gets the the list of spell checking languages in use.

Remove '+'

[Gyuyoung Kim]

(In reply to comment #29)
 
> > Source/WebCore/PlatformEfl.cmake:341
> > +        platform/text/enchant/TextCheckerEnchant.cpp
> 
> I have removed duplicated #ifdef guard. TextCheckerEnchant.cpp file is already guarded by #if ENABLE(SPELLCHECK). So, we don't need to guard this again.

WebCore/PlatformEfl.cpp is still using duplicated guard. If you don't wanna change this here, I don't mind.

[Gyuyoung Kim]

Comment on attachment 163862 [details]
remove WebKitTextChecker class

Attachment 163862 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13855002

[Grzegorz Czajkowski]

Created attachment 164090 [details]
apply Gyuyoung's comments

[Grzegorz Czajkowski]

(In reply to comment #29)
> (From update of attachment 163862 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=163862&action=review
> 
> > Source/WebCore/PlatformEfl.cmake:341
> > +        platform/text/enchant/TextCheckerEnchant.cpp
> 
> I have removed duplicated #ifdef guard. TextCheckerEnchant.cpp file is already guarded by #if ENABLE(SPELLCHECK). So, we don't need to guard this again.

Removed the guard from cmake files for both (WebCore/PlatformEfl,cmake and WebKit2/PlatformEfl,cmake).

> 
> > Source/WebKit2/PlatformEfl.cmake:334
> > +    LIST(APPEND WebKit2_INCLUDE_DIRECTORIES
> 
> Why do you append WebKit2_INCLUDE_DIRECTORIES twice ?

Fixed.

> 
> > Source/WebKit2/UIProcess/API/efl/ewk_text_checker_setting.cpp:154
> > +        listOflanguages = eina_list_append(listOflanguages, strdup(languages[i].utf8().data()));
> 
> Please use WKEinaSharedString instead of strdup.
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_text_checker_setting.cpp:178
> > +        listOflanguages = eina_list_append(listOflanguages, strdup(languages[i].utf8().data()));
> 
> ditto.

It's not possible :) WKEinaSharedString was involved to internal usage. Those list are exported to API. Used eina_stringshare_add instead.

> 
> > Source/WebKit2/UIProcess/API/efl/ewk_text_checker_setting.h:163
> > ++ * Gets the the list of spell checking languages in use.
> 
> Remove '+'

Thanks. Fixed.

[Grzegorz Czajkowski]

Created attachment 165373 [details]
changed ewk_text_checker_setting to ewk_text_checker_settings (plural)

1) This patch uses ewk_text_checker_settings.{h.cpp} (plural one) as file which keeps global preferences also uses ewk_settings.
2) Added proper license for the newly created files.

[WebKit Review Bot]

Attachment 165373 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'Source/WebCore/ChangeLog', u..." exit_code: 1
Source/WebKit2/UIProcess/API/efl/ewk_text_checker_settings.h:31:  #ifndef header guard has wrong style, please use: ewk_text_checker_settings_h  [build/header_guard] [5]
Source/WebKit2/UIProcess/API/efl/EWebKit2.h:44:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 2 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Grzegorz Czajkowski]

Created attachment 165377 [details]
fixing coding style

[Gyuyoung Kim]

Comment on attachment 165377 [details]
fixing coding style

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

> Source/WebKit2/UIProcess/API/efl/WebKitTextChecker.cpp:45
> +static OwnPtr<WebCore::TextCheckerEnchant> textCheckerEnchant = WebCore::TextCheckerEnchant::create();

I think it would be good if you use DEFINE_STATIC_LOCAL macro as below,

static OwnPtr<WebPluginLoadObserver>& textCheckerEnchant()
{
    DEFINE_STATIC_LOCAL(OwnPtr<WebCore::TextCheckerEnchant>, nextPluginLoadObserver, WebCore::TextCheckerEnchant::create());
    return textCheckerEnchant;
}

> Source/WebKit2/UIProcess/API/efl/WebKitTextChecker.cpp:46
> +static Ewk_Text_Checker* ewkTextCheckerCallback = ewkTextChecker();

ditto.

> Source/WebKit2/UIProcess/API/efl/WebKitTextChecker.cpp:99
> +    // FIXME: call client's function.

call -> Call ?

> Source/WebKit2/UIProcess/API/efl/WebKitTextChecker.cpp:140
> +            free(item);

Use delete instead of free.

> Source/WebKit2/UIProcess/API/efl/WebKitTextChecker.h:63
> +Vector<String> availableSpellCheckingLanguages();

Is it better to add const keyword ?

> Source/WebKit2/UIProcess/API/efl/WebKitTextChecker.h:65
> +Vector<String> loadedSpellCheckingLanguages();

ditto.

> Source/WebKit2/UIProcess/API/efl/ewk_text_checker_settings.cpp:103
> +void ewk_text_checker_settings_enable_continuous_spell_checking_set(Eina_Bool enable)

In ewk_settings.cpp, enabled is placed in front of _set|_get. I think we need to be consistent with them.

ewk_settings_developer_extras_enabled_get
ewk_settings_javascript_enabled_get

> Source/WebKit2/UIProcess/API/efl/ewk_text_checker_settings.cpp:131
> +void ewk_text_checker_settings_enable_grammar_checking_set(Eina_Bool enable)

ditto.

> Source/WebKit2/UIProcess/API/efl/ewk_text_checker_settings.cpp:146
> +Eina_List* ewk_text_checker_settings_available_spell_checking_languages_get()

ditto ?

> Source/WebKit2/UIProcess/API/efl/ewk_text_checker_settings.cpp:155
> +

Unneeded line.

> Source/WebKit2/UIProcess/efl/TextCheckerEfl.cpp:2
> + * Copyright (C) 2012 Samsung Electronics

Generally, latest copyright is placed to below.

> Source/cmake/OptionsEfl.cmake:171
> +OPTION(ENABLE_SPELLCHECK "Enable spellchecking feature (requires enchant)" ON)

Why not move this to WEBKIT_OPTION_DEFAULT_PORT_VALUE list as well as add this to WebKitFeatures.cmake ?

[Grzegorz Czajkowski]

(In reply to comment #37)
> (From update of attachment 165377 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=165377&action=review
> 
> > Source/WebKit2/UIProcess/API/efl/WebKitTextChecker.cpp:45
> > +static OwnPtr<WebCore::TextCheckerEnchant> textCheckerEnchant = WebCore::TextCheckerEnchant::create();
> 
> I think it would be good if you use DEFINE_STATIC_LOCAL macro as below,
> 
> static OwnPtr<WebPluginLoadObserver>& textCheckerEnchant()
> {
>     DEFINE_STATIC_LOCAL(OwnPtr<WebCore::TextCheckerEnchant>, nextPluginLoadObserver, WebCore::TextCheckerEnchant::create());
>     return textCheckerEnchant;
> }

It's nice suggestion and as I understand your intension is to not define the static variables. IMHO we can not use this macro in this case because of the destructor in never called for the given object. In the shortcut the macro does:
static Foo* foo2 = new Foo(); // no one is take care of deleting it
Destructor will be called when we define object like this:
static Foo foo2; // delete on application exit.

IMO OwnPtr doesn't help us in this case as we create it by new (please correct me If I am wrong).

> 
> > Source/WebKit2/UIProcess/API/efl/WebKitTextChecker.cpp:46
> > +static Ewk_Text_Checker* ewkTextCheckerCallback = ewkTextChecker();
> 
> ditto.

Not possible, ewkTextChecker() it's not the constuctor, this is just simply helper functions avoid calling ewkTextChecker()->userCallbackFunction()

> 
> > Source/WebKit2/UIProcess/API/efl/WebKitTextChecker.cpp:99
> > +    // FIXME: call client's function.
> 
> call -> Call ?

You're right.

> 
> > Source/WebKit2/UIProcess/API/efl/WebKitTextChecker.cpp:140
> > +            free(item);
> 
> Use delete instead of free.

The item is created by application side (using malloc). We have to call free() in this case.

> 
> > Source/WebKit2/UIProcess/API/efl/WebKitTextChecker.h:63
> > +Vector<String> availableSpellCheckingLanguages();
> 
> Is it better to add const keyword ?

The function is not a member of class. We resign of creating a class for keeping a static methods only.

> 
> > Source/WebKit2/UIProcess/API/efl/WebKitTextChecker.h:65
> > +Vector<String> loadedSpellCheckingLanguages();
> 
> ditto.

Ditto :)

> 
> > Source/WebKit2/UIProcess/API/efl/ewk_text_checker_settings.cpp:103
> > +void ewk_text_checker_settings_enable_continuous_spell_checking_set(Eina_Bool enable)
> 
> In ewk_settings.cpp, enabled is placed in front of _set|_get. I think we need to be consistent with them.
> 
> ewk_settings_developer_extras_enabled_get
> ewk_settings_javascript_enabled_get
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_text_checker_settings.cpp:131
> > +void ewk_text_checker_settings_enable_grammar_checking_set(Eina_Bool enable)
> 
> ditto.

Ok, I will apply your suggestion.

> 
> > Source/WebKit2/UIProcess/API/efl/ewk_text_checker_settings.cpp:146
> > +Eina_List* ewk_text_checker_settings_available_spell_checking_languages_get()
> 
> ditto ?

Are you proposing: ewk_text_checker_settings_spell_checking_languages_enabled_get() ?

> 
> > Source/WebKit2/UIProcess/API/efl/ewk_text_checker_settings.cpp:155
> > +
> 
> Unneeded line.

Right.

> 
> > Source/WebKit2/UIProcess/efl/TextCheckerEfl.cpp:2
> > + * Copyright (C) 2012 Samsung Electronics
> 
> Generally, latest copyright is placed to below.

Ok.

> 
> > Source/cmake/OptionsEfl.cmake:171
> > +OPTION(ENABLE_SPELLCHECK "Enable spellchecking feature (requires enchant)" ON)
> 
> Why not move this to WEBKIT_OPTION_DEFAULT_PORT_VALUE list as well as add this to WebKitFeatures.cmake ?

I can try:)

[Grzegorz Czajkowski]

> > Source/WebKit2/UIProcess/API/efl/ewk_text_checker_settings.cpp:103
> > +void ewk_text_checker_settings_enable_continuous_spell_checking_set(Eina_Bool enable)
> 
> In ewk_settings.cpp, enabled is placed in front of _set|_get. I think we need to be consistent with them.
> 
> ewk_settings_developer_extras_enabled_get

ewk_settings API is based on WKPreferences:
WKPreferencesSetDeveloperExtrasEnabled -> ewk_settings_developer_extras_enabled_get

IMHO it's better to use simple tense instead of past, 'enabled' for getter makes sense but for setter doesn't sound well. I followed API from WK1:

ewk_view_setting_enable_developer_extras_get

I'd like to listen all API proposals as changing the API blocks adding unit tests and landing this patch.

Kubo, Christophe could you express your feeling about ewk_text_checker_settings and ewk_text_checker APIs? Thanks

[Grzegorz Czajkowski]

Created attachment 165957 [details]
apply Gyuyoung's comments

This patch doesn't contain any API changes - those should be discussed more.

[Grzegorz Czajkowski]

The spellchecking feature consist of API to:
 1) manipulate settings (toggle both spell and grammar checking, notification about the setting change)
 2) overwrite the default spellchecker implementation - our implementation doesn't support grammar checking (only spell checking), user is able to define own implementation by callback functions. It's the main purpose of WKTextChecker.h - to move the implementation to the client side. We partially deliver it in WebKit internally but exposing those callbacks do not limit client to define own callbacks.

Do you along with them? Feel free to express all your opinions about API.

ewk_text_checker_settings.h File Reference

Typedefs:
typedef void(* 	Ewk_Text_Checker_Settings_Continuous_Spell_Checking_Changed )(Eina_Bool enable)
typedef void(* 	Ewk_Text_Checker_Settings_Grammar_Checking_Changed )(Eina_Bool enable)

Functions:
EAPI void 	ewk_text_checker_settings_continuous_spell_checking_changed_cb_set (Ewk_Text_Checker_Settings_Continuous_Spell_Checking_Changed cb)
EAPI void 	ewk_text_checker_settings_grammar_checking_changed_cb_set (Ewk_Text_Checker_Settings_Grammar_Checking_Changed cb)
EAPI Eina_Bool 	ewk_text_checker_settings_enable_continuous_spell_checking_get (void)
EAPI void 	ewk_text_checker_settings_enable_continuous_spell_checking_set (Eina_Bool enable)
EAPI Eina_Bool 	ewk_text_checker_settings_enable_grammar_checking_get (void)
EAPI void 	ewk_text_checker_settings_enable_grammar_checking_set (Eina_Bool enable)
EAPI Eina_List * 	ewk_text_checker_settings_available_spell_checking_languages_get (void)
EAPI void 	ewk_text_checker_settings_spell_checking_languages_set (const char *languages)
EAPI Eina_List * 	ewk_text_checker_settings_spell_checking_languages_get (void)

ewk_text_checker.h File Reference

Typedefs:
typedef uint64_t(* 	Ewk_Text_Checker_Unique_Spell_Document_Tag_Get )(const Evas_Object *o)
typedef void(* 	Ewk_Text_Checker_Unique_Spell_Document_Tag_Close )(uint64_t tag)
typedef void(* 	Ewk_Text_Checker_String_Spelling_Check )(uint64_t tag, const char *text, int32_t *misspelling_location, int32_t *misspelling_length)
typedef Eina_Bool(* 	Ewk_Text_Checker_UI_Spelling_Status_Get )(void)
typedef void(* 	Ewk_Text_Checker_UI_Spelling_Status_Toggle )(void)
typedef void(* 	Ewk_Text_Checker_UI_Spelling_Misspelled_Word_Update )(uint64_t tag, const char *misspelled_word)
typedef Eina_List *(* 	Ewk_Text_Checker_Word_Guesses_Get )(uint64_t tag, const char *word)
typedef void(* 	Ewk_Text_Checker_Word_Learn )(uint64_t tag, const char *word)
typedef void(* 	Ewk_Text_Checker_Word_Ignore )(uint64_t tag, const char *word)

Functions:
EAPI void 	ewk_text_checker_unique_spell_document_tag_get_cb_set (Ewk_Text_Checker_Unique_Spell_Document_Tag_Get cb)
EAPI void 	ewk_text_checker_unique_spell_document_tag_close_cb_set (Ewk_Text_Checker_Unique_Spell_Document_Tag_Close cb)
EAPI void 	ewk_text_checker_string_spelling_check_cb_set (Ewk_Text_Checker_String_Spelling_Check cb)
EAPI void 	ewk_text_checker_ui_spelling_status_get_cb_set (Ewk_Text_Checker_UI_Spelling_Status_Get cb)
EAPI void 	ewk_text_checker_ui_spelling_status_toggle_cb_set (Ewk_Text_Checker_UI_Spelling_Status_Toggle cb)
EAPI void 	ewk_text_checker_ui_spelling_misspelled_word_update_cb_set (Ewk_Text_Checker_UI_Spelling_Misspelled_Word_Update cb)
EAPI void 	ewk_text_checker_word_guesses_get_cb_set (Ewk_Text_Checker_Word_Guesses_Get cb)
EAPI void 	ewk_text_checker_word_learn_cb_set (Ewk_Text_Checker_Word_Learn cb)
EAPI void 	ewk_text_checker_word_ignore_cb_set (Ewk_Text_Checker_Word_Ignore cb)

[Gyuyoung Kim]

Comment on attachment 165957 [details]
apply Gyuyoung's comments

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

This patch has supported more than text checker API supported by gtk port. Do you think we really need to support these APIs all ?

> Source/WebKit2/UIProcess/API/efl/WebKitTextChecker.cpp:125
> +    // FIXME: call client's function.

Nit : call -> Call ?

> Source/WebKit2/UIProcess/API/efl/WebKitTextChecker.cpp:142
> +

Looks unneeded line.

> Source/WebKit2/UIProcess/API/efl/ewk_text_checker.h:87
> +typedef Eina_Bool (*Ewk_Text_Checker_UI_Spelling_Status_Get)(void);

What does *UI* mean in this callback ?

> Source/WebKit2/UIProcess/API/efl/ewk_text_checker_settings.cpp:29
> +#if ENABLE(SPELLCHECK)

New line ?

> Source/WebKit2/UIProcess/efl/TextCheckerEfl.cpp:-30
> -#include <WebCore/NotImplemented.h>

New line ?

> Source/WebKit2/UIProcess/efl/TextCheckerEfl.cpp:75
> +#endif

You need to use UNUSED_PARAM to avoid unused parameter build warning.

> Source/WebKit2/UIProcess/efl/TextCheckerEfl.cpp:86
> +#endif

ditto.

> Source/WebKit2/UIProcess/efl/TextCheckerEfl.cpp:100
> +#endif

ditto.

> Source/WebKit2/UIProcess/efl/TextCheckerEfl.cpp:107
> +#else

ditto.

> Source/WebKit2/UIProcess/efl/TextCheckerEfl.cpp:158
> +#if ENABLE(SPELLCHECK)

ditto.

[Grzegorz Czajkowski]

Created attachment 168222 [details]
updated patch

1) Apply Gyuyoung's suggestions
2) Remove UI callbacks - WebKit2-EFL doesn't need them.
3) Use new API syntax - ewk_foo_bar_enabled_{get,set} etc.

[Gyuyoung Kim]

(In reply to comment #42)
> (From update of attachment 165957 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=165957&action=review
> 
> This patch has supported more than text checker API supported by gtk port. Do you think we really need to support these APIs all ?

How do you think about my question ?

[Grzegorz Czajkowski]

(In reply to comment #44)
> (In reply to comment #42)
> > (From update of attachment 165957 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=165957&action=review
> > 
> > This patch has supported more than text checker API supported by gtk port. Do you think we really need to support these APIs all ?
> 
> How do you think about my question ?

WK1-Gtk exposes the following callbacks (webkitspellchecker.h):
 - check_spelling_of_string
 - get_guesses_for_word
 - update_spell_checking_languages
 - get_autocorrect_suggestions_for_misspelled_word
 - learn_word
 - ignore_word
Additionally settings are manipulated via (webkitwebsettings.cpp)

WK2-Gtk doesn't expose those callback at all. Only spelling settings can be manipulated via WebKitWebContext.h file.

Whole spelling implementation in WK2 is exposed to the application side (see WKTextChecker.h) The WK2-EFL implementation contains an internal implementation (as WK2-Gtk) based on Enchant implemetation and additionally exposes the callback as (Wk1-GTK). So the client may overwrite the default spelling implementation. IMHO we do not force the client to use Enchant library, he would use any spelling library on demand.

I've already removed UI's nad grammar callbacks.
We can remove grammar settings from (ewk_text_checker_settings.{h.cpp}) too and to move spelling settings to the global settings file (ewk_settings.h). What's you opinion on that?

[Grzegorz Czajkowski]

Created attachment 168409 [details]
reduce implementation

1. Remove 'grammar checking settings'.
   It doesn't make sense to export API which allows to enable 'grammar checking' if the core implementation is missing.

2. Move  spelling settings to the ewk_setting.{h,cpp} files.
   The files mostly manipulate settings which are related to the view object. Text checker settings (in fact spelling settings) are not connected to any view object but keeping them somewhere else may be confusing.

3. ewk_text_checker_settings.{h,cpp} is no needed any more :)

[Grzegorz Czajkowski]

Created attachment 168421 [details]
fix undefined references for API

[Gyuyoung Kim]

Comment on attachment 168421 [details]
fix undefined references for API

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

Looks good to me now except for my comment. However, Someone else might want to have a final look.

> Source/WebKit2/UIProcess/API/efl/ewk_text_checker.h:32
> + * If application wants to enable the feature, API from @a ewk_text_checker_setting.h

ewk_text_checker_setting.h is not needed anymore.

[Grzegorz Czajkowski]

Created attachment 168634 [details]
patch for landing

I will set cq flag when the unit tests (bug 95956) get (in)formal review.

[Chris Dumez]

Comment on attachment 168634 [details]
patch for landing

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

> Source/WebKit2/UIProcess/API/efl/ewk_settings.h:288
> +EAPI void ewk_settings_spell_checking_languages_enabled_set(const char *languages);

This makes it sound like it is taking a boolean in argument. Why the "_enabled" ?

[Grzegorz Czajkowski]

(In reply to comment #50)
> (From update of attachment 168634 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=168634&action=review
> 
> > Source/WebKit2/UIProcess/API/efl/ewk_settings.h:288
> > +EAPI void ewk_settings_spell_checking_languages_enabled_set(const char *languages);
> 
> This makes it sound like it is taking a boolean in argument. Why the "_enabled" ?

There are three functions to manipulate the languages:
- ewk_settings_spell_checking_languages_get(): general getter, gets all languages available on your OS,

- ewk_settings_spell_checking_languages_enabled_set(): sets languages to performing spelling, so languages which are enabled 

- ewk_settings_spell_checking_languages_enabled_get(): gets languages in use (which are enabled)

Shouldn't we use '_enabled' for those API? IMHO it doesn't violate much :) Anyway it was suggestion from comment 35.
Of course we can use for example 'in_use' etc. but IMHO _enabled sounds better in this case.

[Gyuyoung Kim]

Comment on attachment 168634 [details]
patch for landing

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

>>> Source/WebKit2/UIProcess/API/efl/ewk_settings.h:288
>>> +EAPI void ewk_settings_spell_checking_languages_enabled_set(const char *languages);
>> 
>> This makes it sound like it is taking a boolean in argument. Why the "_enabled" ?
> 
> There are three functions to manipulate the languages:
> - ewk_settings_spell_checking_languages_get(): general getter, gets all languages available on your OS,
> 
> - ewk_settings_spell_checking_languages_enabled_set(): sets languages to performing spelling, so languages which are enabled 
> 
> - ewk_settings_spell_checking_languages_enabled_get(): gets languages in use (which are enabled)
> 
> Shouldn't we use '_enabled' for those API? IMHO it doesn't violate much :) Anyway it was suggestion from comment 35.
> Of course we can use for example 'in_use' etc. but IMHO _enabled sounds better in this case.

ewk_settings_spell_checking_languages_set is better for me. If we use _enabled, it looks this API is just enable/disable specific functionality via a flag.

[Grzegorz Czajkowski]

(In reply to comment #52)
> (From update of attachment 168634 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=168634&action=review
> 
> >>> Source/WebKit2/UIProcess/API/efl/ewk_settings.h:288
> >>> +EAPI void ewk_settings_spell_checking_languages_enabled_set(const char *languages);
> >> 
> >> This makes it sound like it is taking a boolean in argument. Why the "_enabled" ?
> > 
> > There are three functions to manipulate the languages:
> > - ewk_settings_spell_checking_languages_get(): general getter, gets all languages available on your OS,
> > 
> > - ewk_settings_spell_checking_languages_enabled_set(): sets languages to performing spelling, so languages which are enabled 
> > 
> > - ewk_settings_spell_checking_languages_enabled_get(): gets languages in use (which are enabled)
> > 
> > Shouldn't we use '_enabled' for those API? IMHO it doesn't violate much :) Anyway it was suggestion from comment 35.
> > Of course we can use for example 'in_use' etc. but IMHO _enabled sounds better in this case.
> 
> ewk_settings_spell_checking_languages_set is better for me. If we use _enabled, it looks this API is just enable/disable specific functionality via a flag.

Ok, thanks for your opinion. I propose the following API:

- ewk_settings_spell_checking_AVAILABLE_languages_get(): general getter, gets all languages available/installed on your OS,

- ewk_settings_spell_checking_languages_set(): sets languages to check spelling
 
- ewk_settings_spell_checking_languages_get(): gets languages in use

What's your opinion on that?

[Gyuyoung Kim]

(In reply to comment #53)
> (In reply to comment #52)
> > (From update of attachment 168634 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=168634&action=review
> > 
> > >>> Source/WebKit2/UIProcess/API/efl/ewk_settings.h:288
> > >>> +EAPI void ewk_settings_spell_checking_languages_enabled_set(const char *languages);
> > >> 
> > >> This makes it sound like it is taking a boolean in argument. Why the "_enabled" ?
> > > 
> > > There are three functions to manipulate the languages:
> > > - ewk_settings_spell_checking_languages_get(): general getter, gets all languages available on your OS,
> > > 
> > > - ewk_settings_spell_checking_languages_enabled_set(): sets languages to performing spelling, so languages which are enabled 
> > > 
> > > - ewk_settings_spell_checking_languages_enabled_get(): gets languages in use (which are enabled)
> > > 
> > > Shouldn't we use '_enabled' for those API? IMHO it doesn't violate much :) Anyway it was suggestion from comment 35.
> > > Of course we can use for example 'in_use' etc. but IMHO _enabled sounds better in this case.
> > 
> > ewk_settings_spell_checking_languages_set is better for me. If we use _enabled, it looks this API is just enable/disable specific functionality via a flag.
> 
> Ok, thanks for your opinion. I propose the following API:
> 
> - ewk_settings_spell_checking_AVAILABLE_languages_get(): general getter, gets all languages available/installed on your OS,
> 
> - ewk_settings_spell_checking_languages_set(): sets languages to check spelling
> 
> - ewk_settings_spell_checking_languages_get(): gets languages in use
> 
> What's your opinion on that?

Looks more clear API names than before.

[Grzegorz Czajkowski]

Created attachment 168945 [details]
change API to more convenient names 

Asking for review once again as API has been changed.

[Gyuyoung Kim]

Comment on attachment 168945 [details]
change API to more convenient names 

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

> Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:45
>  #include <wtf/text/WTFString.h>

New line ?

> Source/WebKit2/UIProcess/API/efl/ewk_settings.cpp:31
>  #include <WebKit2/WKPreferencesPrivate.h>

New line ?

> Source/WebKit2/UIProcess/API/efl/ewk_text_checker.cpp:29
> +#if ENABLE(SPELLCHECK)

New line ?

> Source/WebKit2/UIProcess/API/efl/ewk_text_checker_private.h:29
> +#if ENABLE(SPELLCHECK)

New line ?

[Grzegorz Czajkowski]

Created attachment 169100 [details]
polish the patch regarding to Gyuyoung's comments

[WebKit Review Bot]

Comment on attachment 169100 [details]
polish the patch regarding to Gyuyoung's comments

Rejecting attachment 169100 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
nt): Merge conflict in Source/WebCore/ChangeLog
Failed to merge in the changes.
Patch failed at 0001 Assertion failed on HTMLFormControlElement.cpp: updateFromElementCallback()

When you have resolved this problem run "git rebase --continue".
If you would prefer to skip this patch, instead run "git rebase --skip".
To restore the original branch and stop rebasing run "git rebase --abort".

rebase refs/remotes/origin/master: command returned error: 1

Died at Tools/Scripts/update-webkit line 164.

Full output: http://queues.webkit.org/results/14400382

[Grzegorz Czajkowski]

Comment on attachment 169100 [details]
polish the patch regarding to Gyuyoung's comments

There is no any problems with rebase on my local repository. Let's try to add it one more time.

[WebKit Review Bot]

Comment on attachment 169100 [details]
polish the patch regarding to Gyuyoung's comments

Clearing flags on attachment: 169100

Committed r131579: <http://trac.webkit.org/changeset/131579>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.