Bug 117728 – [EFL][WK2] Implement unit test callback: onSettingChange

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

[Krzysztof Wolanski]

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.

[Krzysztof Wolanski]

Created attachment 204892 [details]
proposed patch

[Chris Dumez]

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.

[Krzysztof Wolanski]

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?

[Krzysztof Wolanski]

Created attachment 204895 [details]
apply suggestions

[Grzegorz Czajkowski]

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.

[Chris Dumez]

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.

[Grzegorz Czajkowski]

(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.

[Krzysztof Wolanski]

Created attachment 204914 [details]
apply suggestions

[Chris Dumez]

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?

[Chris Dumez]

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?

[Krzysztof Wolanski]

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

[Chris Dumez]

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.

[Krzysztof Wolanski]

Created attachment 204920 [details]
apply suggestions

[Chris Dumez]

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.

[Grzegorz Czajkowski]

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.

[Krzysztof Wolanski]

Created attachment 204976 [details]
apply suggestions

[Grzegorz Czajkowski]

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;

[Krzysztof Wolanski]

Created attachment 204983 [details]
apply suggestions

[Grzegorz Czajkowski]

Comment on attachment 204983 [details]
apply suggestions

Thanks.

[WebKit Commit Bot]

Comment on attachment 204983 [details]
apply suggestions

Clearing flags on attachment: 204983

Committed r151729: <http://trac.webkit.org/changeset/151729>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.