Bug 95956

Created attachment 165326 [details] proposed patch This patch covers API from ewk_text_checker_setting.h.

Comment on attachment 165326 [details] proposed patch Attachment 165326 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/13986612

Comment on attachment 165326 [details] proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=165326&action=review > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker_setting.cpp:45 > +static Eina_Bool settingEnabled = false; bool > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker_setting.cpp:46 > +static Ecore_Timer* timeout = 0; timer? or timeoutTimer? > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker_setting.cpp:47 > +static double timeoutValue = 0.5; Please indicate the time unit in the variable name. e.g. defaultTimeoutSeconds > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker_setting.cpp:86 > + ewk_text_checker_setting_enable_continuous_spell_checking_set(EINA_TRUE); true > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker_setting.cpp:93 > + ecore_main_loop_begin(); We avoid using embedded main loops. We usually prefer checking a variable and calling ecore_main_loop_iterate() until the variable state changes. > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker_setting.cpp:109 > + ewk_text_checker_setting_enable_continuous_spell_checking_set(EINA_FALSE); false > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker_setting.cpp:120 > + ewk_text_checker_setting_enable_grammar_checking_set(EINA_TRUE); true > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker_setting.cpp:127 > + ewk_text_checker_setting_enable_grammar_checking_set(EINA_FALSE); false > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker_setting.cpp:136 > + ewk_text_checker_setting_continuous_spell_checking_changed_cb_set( Why 3 lines for this? > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker_setting.cpp:149 > + ecore_main_loop_begin(); main loop iterate. > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker_setting.cpp:162 > + ecore_main_loop_begin(); Ditto. > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker_setting.cpp:174 > + ecore_main_loop_begin(); Ditto. > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker_setting.cpp:185 > + ewk_text_checker_setting_grammar_checking_changed_cb_set( On 1 line? > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker_setting.cpp:251 > + languages.appendLiteral(","); append(',') is more efficient > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker_setting.cpp:279 > + Extra line here > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker_setting.cpp:316 > + const char* lastExpected = static_cast<const char*>(eina_list_nth(availableLanguages, numberOfAvailableLanguages - 1)); eina_list_last? > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker_setting.cpp:322 > + languages.appendLiteral(","); append(',')

Comment on attachment 165326 [details] proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=165326&action=review >> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker_setting.cpp:86 >> + ewk_text_checker_setting_enable_continuous_spell_checking_set(EINA_TRUE); > > true We almost decided to use standard boolean type, In EFL, they avoid to use Eina_Bool except for legacy code.

Created attachment 165609 [details] UT for ewk_text_checker_setting.h

(In reply to comment #3) > (From update of attachment 165326 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=165326&action=review > > > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker_setting.cpp:45 > > +static Eina_Bool settingEnabled = false; > > bool Ok, the unit test is using standard boolean type. EINA_TRUE, EINA_FALSE were replaced to true/false. > > > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker_setting.cpp:46 > > +static Ecore_Timer* timeout = 0; > > timer? or timeoutTimer? timeoutTimer sounds better to me. Thanks. > > > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker_setting.cpp:47 > > +static double timeoutValue = 0.5; > > Please indicate the time unit in the variable name. e.g. defaultTimeoutSeconds Changed to defaultTimeoutInSeconds. > > > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker_setting.cpp:93 > > + ecore_main_loop_begin(); > > We avoid using embedded main loops. We usually prefer checking a variable and calling ecore_main_loop_iterate() until the variable state changes. I was trying to use ecore_main_loope_iterate(). Unfortunately it doesn't process idlers (ecore_idler_add) but some text checker setting are called through idlers. > > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker_setting.cpp:136 > > + ewk_text_checker_setting_continuous_spell_checking_changed_cb_set( > > Why 3 lines for this? Merged. > > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker_setting.cpp:185 > > + ewk_text_checker_setting_grammar_checking_changed_cb_set( > > On 1 line? > Merged. > > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker_setting.cpp:251 > > + languages.appendLiteral(","); > > append(',') is more efficient Right, done. > > > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker_setting.cpp:279 > > + > > Extra line here Removed. > > > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker_setting.cpp:316 > > + const char* lastExpected = static_cast<const char*>(eina_list_nth(availableLanguages, numberOfAvailableLanguages - 1)); > > eina_list_last? Makes sense. > > > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker_setting.cpp:322 > > + languages.appendLiteral(","); > > append(',') Fixed.

Comment on attachment 165609 [details] UT for ewk_text_checker_setting.h Attachment 165609 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/14006523

Created attachment 167953 [details] UT for ewk_text_checker.h

Comment on attachment 167953 [details] UT for ewk_text_checker.h Attachment 167953 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/14258040

Both patches are not built because of dependency Bug 91854. The patches covers the spelling API (ewk_text_checker.h, ewk_text_checker_settings.h) The patches do not cover below API. They are UI specific and aren't well documented. Actually WebKit2-EFL doesn't need them so I am planing to remove them form Bug 91854. 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)

Created attachment 168424 [details] updated patch 1) Merge two patches into one. 2) Adjust to the recent changes in bug 91854

Comment on attachment 168424 [details] updated patch Attachment 168424 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/14265346

Comment on attachment 168424 [details] updated patch Attachment 168424 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/14266354 New failing tests: inspector/profiler/heap-snapshot.html

Comment on attachment 168424 [details] updated patch I think those tests should be moved to a test_ewk2_spellcheck.cpp file. It is big and clutter the test_ewk2_settings.cpp file too much. We already do this for download API for e.g.

Comment on attachment 168424 [details] updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=168424&action=review Looks OK to me. (besides the small typos) > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker.cpp:48 > + * Its values are reseted before each test. "reset" > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker.cpp:73 > + * the client on which object (assosiated to the tag) the spelling is being invoked. "associated"

Created attachment 168702 [details] apply Christophe's suggestions 1) Move the unit tests connected to spelling settings to test_ewk2_text_checker.cpp 2) Fix typos.

Comment on attachment 168702 [details] apply Christophe's suggestions Attachment 168702 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/14294676

Comment on attachment 168702 [details] apply Christophe's suggestions View in context: https://bugs.webkit.org/attachment.cgi?id=168702&action=review > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker.cpp:282 > + // if the SPELLCHECK macro is disabled, the callback is not set. if -> If ? > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker.cpp:409 > + loadUrlSync(environment->urlForResource("spelling_test.html").data()); loadUrlSync needs to use ASSERT_TRUE or EXPECT_TRUE > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker.cpp:477 > + The word has to be selected first. The -> the ?

Created attachment 168893 [details] apply Gyuyoung's comments

Comment on attachment 168893 [details] apply Gyuyoung's comments Attachment 168893 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/14384104

Created attachment 168947 [details] adjust to latest changes from bug 91854

Comment on attachment 168947 [details] adjust to latest changes from bug 91854 Attachment 168947 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/14392147

Comment on attachment 168947 [details] adjust to latest changes from bug 91854 View in context: https://bugs.webkit.org/attachment.cgi?id=168947&action=review LGTM. I think you should land this patch with Bug 91854. > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker.cpp:515 > + Looks unneeded line. > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker.cpp:536 > + ditto.

Created attachment 169149 [details] patch for landing

Comment on attachment 169149 [details] patch for landing Clearing flags on attachment: 169149 Committed r131589: <http://trac.webkit.org/changeset/131589>

All reviewed patches have been landed. Closing bug.