It is finally possible to implement missing unit tests as WebKit2-EFL has delivered sub menu (r150254). Add two unit tests for checking onSettingChange callback behaviour, when toggling "Check Spelling While Typing" in context menu.
Created attachment 204892 [details] proposed patch
Comment on attachment 204892 [details] proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=204892&action=review > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker.cpp:238 > + break; maybe we could return here and add a "FAIL();" after the loop to make sure we found the item? > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker.cpp:283 > + selectContextMenuOption(ewk_context_menu_item_submenu_get(item), EWK_CONTEXT_MENU_ITEM_TAG_CHECK_SPELLING_WHILE_TYPING, EWK_CHECKABLE_ACTION_TYPE); why not pass the item directly to selectContextMenuOption() instead of iterating over the list twice? > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker.cpp:284 > + break; same as before, would be nice to return early here and FAIL after the loop. > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker.cpp:373 > + isSettingEnabled = ewk_text_checker_continuous_spell_checking_enabled_get(); those 2 statements should be merged into one. > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker.cpp:409 > + ASSERT_FALSE(waitUntilTrue(callbacksExecutionStats.settingChange, defaultTimeoutInSeconds)); this looks weird. are we waiting to make sure the callback is *not* called? if so, isn't is terribly slow? also, the last arg is likely optional. > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker.cpp:426 > + ASSERT_FALSE(waitUntilTrue(callbacksExecutionStats.settingChange, defaultTimeoutInSeconds)); ditto.
Comment on attachment 204892 [details] proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=204892&action=review >> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker.cpp:238 >> + break; > > maybe we could return here and add a "FAIL();" after the loop to make sure we found the item? I will add ADD_FAILURE(), because FAIL() should not be used in functions that return non void value. >> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker.cpp:283 >> + selectContextMenuOption(ewk_context_menu_item_submenu_get(item), EWK_CONTEXT_MENU_ITEM_TAG_CHECK_SPELLING_WHILE_TYPING, EWK_CHECKABLE_ACTION_TYPE); > > why not pass the item directly to selectContextMenuOption() instead of iterating over the list twice? Because "Check Spelling While Typing" is in sub menu. First we should get sub menu from "Spelling and Grammar", and then toggle "Check Spelling While Typing" option. >> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker.cpp:409 >> + ASSERT_FALSE(waitUntilTrue(callbacksExecutionStats.settingChange, defaultTimeoutInSeconds)); > > this looks weird. are we waiting to make sure the callback is *not* called? if so, isn't is terribly slow? also, the last arg is likely optional. We want to call callback, when *Check Spelling While Typing* is toggled in context menu. When client invokes ewk_text_checker_continuous_spell_checking_enabled_set(), callback should not be called. Yes, the last argument is optional and by default is 10 seconds. I have changed it to 0.5 sec. It is enough. >> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker.cpp:426 >> + ASSERT_FALSE(waitUntilTrue(callbacksExecutionStats.settingChange, defaultTimeoutInSeconds)); > > ditto. Callback should not be called, defaultTimeoutInSeconds = 0.5. Do you suggest to remove this test?
Created attachment 204895 [details] apply suggestions
Comment on attachment 204895 [details] apply suggestions View in context: https://bugs.webkit.org/attachment.cgi?id=204895&action=review From spellchecking side it looks sane. Added some comments. > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker.cpp:240 > + break; It seems that you didn't apply Chris' comment properly. I am in a favor of return here so we could call ADD_FAILURE outside the loop. Moreover, would be nice to add additional indent here. > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker.cpp:290 > + break; Ditto. > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker.cpp:396 > + ASSERT_TRUE(waitUntilTrue(callbacksExecutionStats.settingChange)); Please make additional line here as this is rather part of cleaning. > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker.cpp:416 > + ASSERT_FALSE(waitUntilTrue(callbacksExecutionStats.settingChange, defaultTimeoutInSeconds)); Ditto.
Comment on attachment 204895 [details] apply suggestions View in context: https://bugs.webkit.org/attachment.cgi?id=204895&action=review > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker.cpp:433 > + ASSERT_FALSE(waitUntilTrue(callbacksExecutionStats.settingChange, defaultTimeoutInSeconds)); I think such checks slow down the api tests. maybe we should use 0 as defaultTimeoutInSeconds instead so that we process the event loop only once.
(In reply to comment #6) > (From update of attachment 204895 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=204895&action=review > > > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker.cpp:433 > > + ASSERT_FALSE(waitUntilTrue(callbacksExecutionStats.settingChange, defaultTimeoutInSeconds)); > > I think such checks slow down the api tests. maybe we should use 0 as defaultTimeoutInSeconds instead so that we process the event loop only once. Sounds fine for me.
Created attachment 204914 [details] apply suggestions
Comment on attachment 204914 [details] apply suggestions View in context: https://bugs.webkit.org/attachment.cgi?id=204914&action=review > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker.cpp:279 > + EINA_LIST_FOREACH(contextMenuItems, listIterator, itemData) { Still looks like a lot of duplication with selectContextMenuOption(). How about renaming the utility function to findContextMenuItem() and call it twice?
Comment on attachment 204914 [details] apply suggestions View in context: https://bugs.webkit.org/attachment.cgi?id=204914&action=review > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker.cpp:288 > + ADD_FAILURE(); ADD_FAILURE(); is non fatal. Why cannot we use FAIL() in a function returning a boolean?
Comment on attachment 204914 [details] apply suggestions View in context: https://bugs.webkit.org/attachment.cgi?id=204914&action=review >> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker.cpp:279 >> + EINA_LIST_FOREACH(contextMenuItems, listIterator, itemData) { > > Still looks like a lot of duplication with selectContextMenuOption(). How about renaming the utility function to findContextMenuItem() and call it twice? Sorry for inconvenience, method selectContextMenuOption() will be helpful in next test cases, when option will be selected directly from context menu. >> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker.cpp:288 >> + ADD_FAILURE(); > > ADD_FAILURE(); is non fatal. Why cannot we use FAIL() in a function returning a boolean? When I use FAIL() instead of ADD_FAILURE(), it returns en error at compile time: test_ewk2_text_checker.cpp:288:5: error: void value not ignored as it ought to be
Comment on attachment 204914 [details] apply suggestions View in context: https://bugs.webkit.org/attachment.cgi?id=204914&action=review >>> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker.cpp:279 >>> + EINA_LIST_FOREACH(contextMenuItems, listIterator, itemData) { >> >> Still looks like a lot of duplication with selectContextMenuOption(). How about renaming the utility function to findContextMenuItem() and call it twice? > > Sorry for inconvenience, method selectContextMenuOption() will be helpful in next test cases, when option will be selected directly from context menu. but ewk_context_menu_item_select(contextMenu, findContextMenuItem(contextMenu, action, type)) does the same right? and findContextMenuItem() seems to be more reusable. alternatively you can keep selectContextMenuOption() and have it use findContextMenuItem(). I just want to get rid of the code duplication for iterating over the menu items. >>> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker.cpp:288 >>> + ADD_FAILURE(); >> >> ADD_FAILURE(); is non fatal. Why cannot we use FAIL() in a function returning a boolean? > > When I use FAIL() instead of ADD_FAILURE(), it returns en error at compile time: > test_ewk2_text_checker.cpp:288:5: error: void value not ignored as it ought to be ok.
Created attachment 204920 [details] apply suggestions
Comment on attachment 204920 [details] apply suggestions View in context: https://bugs.webkit.org/attachment.cgi?id=204920&action=review LGTM. r=me with nits. Please let Grzegorz have a look before landing. > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker.cpp:228 > +static Ewk_Context_Menu_Item* findContextMenuItem(Ewk_Context_Menu* contextMenu, const Ewk_Context_Menu_Item_Action& itemAction, const Ewk_Context_Menu_Item_Type& itemType) nit: contextMenu could be a const pointer, and it is overkill to pass enum types by const reference.
Comment on attachment 204920 [details] apply suggestions View in context: https://bugs.webkit.org/attachment.cgi?id=204920&action=review Looks much better, added some comments. >> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker.cpp:228 >> +static Ewk_Context_Menu_Item* findContextMenuItem(Ewk_Context_Menu* contextMenu, const Ewk_Context_Menu_Item_Action& itemAction, const Ewk_Context_Menu_Item_Type& itemType) > > nit: contextMenu could be a const pointer, and it is overkill to pass enum types by const reference. Definitely. > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker.cpp:234 > + nit: I wouldn't add an extra line here. Those variables are related to the loop. Would be nice to keep them closer. Moreover, other text checker tests seem to be used similar syntax. > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker.cpp:241 > + } > + ADD_FAILURE(); But here IMO, a new line will show that this is just protection/notification that an item wasn't found. > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker.cpp:276 > + Ewk_Context_Menu_Item* checkSpellingWhileTypingItem = findContextMenuItem(ewk_context_menu_item_submenu_get(spellingAndGrammarItem), EWK_CONTEXT_MENU_ITEM_TAG_CHECK_SPELLING_WHILE_TYPING, EWK_CHECKABLE_ACTION_TYPE); ewk_context_menu_item_submenu_get(spellingAndGrammarItem) is getting twice (here and 278). It's worth making an additional variable for it, e.g. spellingAndGrammarSubmenu ? > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker.cpp:280 > + ewk_context_menu_item_select(ewk_context_menu_item_submenu_get(spellingAndGrammarItem), checkSpellingWhileTypingItem); > + > + return true; After applying comment above will be nice to call: return ewk_context_menu_item_select(spellingAndGrammarSubmenu, checkSpellingWhileTypingItem); > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker.cpp:368 > + resetCallbacksExecutionStats(); > + > + ewk_text_checker_continuous_spell_checking_change_cb_set(onSettingChange); > + > + isSettingEnabled = !ewk_text_checker_continuous_spell_checking_enabled_get(); > + > + ewkViewClass()->context_menu_show = toogleCheckSpellingWhileTyping; I'd would reorder them a little and remove unnecessary empty lines: resetCallbacksExecutionStats(); ewkViewClass()->context_menu_show = toogleCheckSpellingWhileTyping; ewk_text_checker_continuous_spell_checking_change_cb_set(onSettingChange); isSettingEnabled = !ewk_text_checker_continuous_spell_checking_enabled_get(); > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker.cpp:414 > + Not needed.
Created attachment 204976 [details] apply suggestions
Comment on attachment 204976 [details] apply suggestions View in context: https://bugs.webkit.org/attachment.cgi?id=204976&action=review > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker.cpp:228 > +static Ewk_Context_Menu_Item* findContextMenuItem(const Ewk_Context_Menu* contextMenu, const Ewk_Context_Menu_Item_Action& itemAction, const Ewk_Context_Menu_Item_Type& itemType) Chris mentioned that passing enum (itemAction, itemType) as const reference is not recommended. > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker.cpp:242 > + } > + ADD_FAILURE(); > + > + return 0; Sorry, I clicked wrong line, I meant : } ADD_FAILURE(); return 0;
Created attachment 204983 [details] apply suggestions
Comment on attachment 204983 [details] apply suggestions Thanks.
Comment on attachment 204983 [details] apply suggestions Clearing flags on attachment: 204983 Committed r151729: <http://trac.webkit.org/changeset/151729>
All reviewed patches have been landed. Closing bug.