[Webkit-unassigned] [Bug 117794] [EFL][WK2] Implement unit test callback: onWordGuesses

[bugzilla-daemon at webkit.org]

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





--- Comment #4 from Christophe Dumez <dchris at gmail.com>  2013-06-20 03:05:31 PST ---
(From update of attachment 205063)
View in context: https://bugs.webkit.org/attachment.cgi?id=205063&action=review

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker.cpp:185
> +    unsigned numberOfSuggestions = WTF_ARRAY_LENGTH(clientSuggestionsForWord);

I would prefer to use size_t rather than unsigned (http://en.cppreference.com/w/cpp/language/sizeof).

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker.cpp:186
> +    for (unsigned i = 0; i < numberOfSuggestions; ++i) {

ditto

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker.cpp:187
> +        char* suggestion = strdup(clientSuggestionsForWord[i]);

You don't really need to strdup with the current API but I would fix the API first (see below). Then strdup() will be needed but I would remove this temporary variable.

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker.cpp:192
>      return suggestionsForWord;

According to the doc:
 * @return a list of dynamically allocated strings (as char*) and caller is responsible for destroying them.

The doc is not clear if we need to free the list or not but it does say we need to free the items. Currently, you free neither.
BTW, I think the API should be fixed so that the caller does not need to free the strings. This is very inconvenient in a callback to return strings and later free them.

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker.cpp:290
> +    const Eina_List* contextMenuItems = ewk_context_menu_items_get(contextMenu);

Would be nice to add a check to make sure contextMenuItems and clientSuggestionsForWord are the same size.

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker.cpp:292
> +    unsigned numberOfSuggestions = WTF_ARRAY_LENGTH(clientSuggestionsForWord);

size_t

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker.cpp:293
> +    for (unsigned i = 0; i < numberOfSuggestions; ++i) {

ditto

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker.cpp:294
> +        Ewk_Context_Menu_Item* item = static_cast<Ewk_Context_Menu_Item*>(eina_list_nth(contextMenuItems, i));

I think eina_list_nth may iterate through the list to find the nth item, in which case we would be better off using EINA_LIST_FOREACH() + i counter.

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