[Webkit-unassigned] [Bug 117728] [EFL][WK2] Implement unit test callback: onSettingChange

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=117728





--- Comment #15 from Grzegorz Czajkowski <g.czajkowski at samsung.com>  2013-06-19 00:33:17 PST ---
(From update of attachment 204920)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.