| Summary: | [WK2][EFL] Implementation of spellchecking feature. | ||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Grzegorz Czajkowski <g.czajkowski> | ||||||||||||||||||||||||||||||||||||||
| Component: | WebKit2 | Assignee: | Grzegorz Czajkowski <g.czajkowski> | ||||||||||||||||||||||||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||||||||||||
| Severity: | Enhancement | CC: | cdumez, dglazkov, d-r, gyuyoung.kim, kenneth, m.roj, rakuco, sw0524.lee, webkit.review.bot | ||||||||||||||||||||||||||||||||||||||
| Priority: | P2 | ||||||||||||||||||||||||||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||||||||||||||||||||||||||
| Hardware: | All | ||||||||||||||||||||||||||||||||||||||||
| OS: | Linux | ||||||||||||||||||||||||||||||||||||||||
| Bug Depends on: | 94320, 95436, 96518 | ||||||||||||||||||||||||||||||||||||||||
| Bug Blocks: | 61838, 81042, 93611, 95956 | ||||||||||||||||||||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||||||||||||||||||||
|
Description
Grzegorz Czajkowski
2012-07-20 06:36:52 PDT
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. Comment on attachment 161908 [details] proposed patch Attachment 161908 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/13745105 Created attachment 161911 [details]
add missing FindEnchant.cmake file
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 (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. 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. (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. (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. (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. Created attachment 161930 [details]
refactoring of FindEnchant.cmake
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 ? (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 Created attachment 161980 [details]
apply Gyuyoung's suggestions
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 I install libenchant-dev to efl ews. Please submit patch again to test it. Created attachment 162222 [details]
no change, just to make efl ews happy
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. 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 (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. 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? 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. Created attachment 162506 [details]
apply Christophe's and Gyuyoung suggestions
(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 (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 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.
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)? (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. 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 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 '+' (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. Comment on attachment 163862 [details] remove WebKitTextChecker class Attachment 163862 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/13855002 Created attachment 164090 [details]
apply Gyuyoung's comments
(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. 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.
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.
Created attachment 165377 [details]
fixing coding style
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 ? (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:) > > 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
Created attachment 165957 [details]
apply Gyuyoung's comments
This patch doesn't contain any API changes - those should be discussed more.
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) 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. 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.
(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 ? (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? 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 :)
Created attachment 168421 [details]
fix undefined references for API
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. Created attachment 168634 [details] patch for landing I will set cq flag when the unit tests (bug 95956) get (in)formal review. 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" ? (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. 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. (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? (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. Created attachment 168945 [details]
change API to more convenient names
Asking for review once again as API has been changed.
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 ? Created attachment 169100 [details]
polish the patch regarding to Gyuyoung's comments
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 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.
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> All reviewed patches have been landed. Closing bug. |