Bug 117794 – [EFL][WK2] Implement unit test callback: onWordGuesses

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Krzysztof Wolanski]

It is finally possible to implement missing unit tests as WebKit2-EFL
has delivered sub menu (r150254).

[Krzysztof Wolanski]

Created attachment 204997 [details]
proposed patch

[Grzegorz Czajkowski]

Comment on attachment 204997 [details]
proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=204997&action=review

Added some comments before review.

> Source/WebKit2/ChangeLog:9
> +        It is finally possible to implement missing unit tests as WebKit2-EFL
> +        has delivered sub menu (r150254).

nit: suggestions do not need sub menu, they appear in the primary context menu.

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker.cpp:188
> +        char* suggestion = (char*)malloc(strlen(clientSuggestionsForWord[i]) + 1);
> +        strcpy(suggestion, clientSuggestionsForWord[i]);

At first sight, it looks wired that we are using C here. But WebKit2-EFL calls free() for them as this part of API.

Anyway I'd duplicate it using:
char* suggestion = strdup(clientSuggestionsForWord[i]);

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker.cpp:289
> +static Eina_Bool checkClientSuggestionsForWord(Ewk_View_Smart_Data*, Evas_Coord, Evas_Coord, Ewk_Context_Menu* contextMenuItems)

Ewk_Context_Menu* contextMenuItems ? 
You've probably thought of contextMenu.

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

nit: list -> contextMenuItems

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

int -> unsigned or size_t

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker.cpp:629
> +    wasContextMenuShown = false;

That it's confusing. We shouldn't assume that the callback (that overwrite this value) will be called asynchronously. I'd move it somewhere around resetCallbacksExecutionStats().

[Krzysztof Wolanski]

Created attachment 205063 [details]
apply suggestions

[Chris Dumez]

Comment on attachment 205063 [details]
apply suggestions

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.

[Krzysztof Wolanski]

Comment on attachment 205063 [details]
apply suggestions

View in context: https://bugs.webkit.org/attachment.cgi?id=205063&action=review

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

contextMunuItems contains all items, not only suggestions. Suggestions are on the top. Would you like to check how many items are to *Ignore Spelling* item (*Ignore Spelling* is first item after suggestions)?

[Krzysztof Wolanski]

Comment on attachment 205063 [details]
apply suggestions

View in context: https://bugs.webkit.org/attachment.cgi?id=205063&action=review

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

I don't want to iterate whole list, only the suggestions on the top.

[Chris Dumez]

Comment on attachment 205063 [details]
apply suggestions

View in context: https://bugs.webkit.org/attachment.cgi?id=205063&action=review

>>> 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.
> 
> contextMunuItems contains all items, not only suggestions. Suggestions are on the top. Would you like to check how many items are to *Ignore Spelling* item (*Ignore Spelling* is first item after suggestions)?

Ok, then an assertion that WTF_ARRAY_LENGTH(clientSuggestionsForWord) <= eina_list_count(contextMenuItem) ? just to make sure we abort before the loop in this error case.

[Chris Dumez]

Comment on attachment 205063 [details]
apply suggestions

View in context: https://bugs.webkit.org/attachment.cgi?id=205063&action=review

>>> 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.
> 
> I don't want to iterate whole list, only the suggestions on the top.

Ok, it wasn't clear to me. Maybe a comment would help understand what you are actually testing then :) apparently, it is not self-explanatory :)

[Krzysztof Wolanski]

Created attachment 205073 [details]
apply suggestions

[Chris Dumez]

(In reply to comment #9)
> Created an attachment (id=205073) [details]
> apply suggestions

Let's fix the API first. Please file a bug and mark it as a dependency as discussed with grzegorz on IRC.

[Grzegorz Czajkowski]

(In reply to comment #10)
> (In reply to comment #9)
> > Created an attachment (id=205073) [details] [details]
> > apply suggestions
> 
> Let's fix the API first. Please file a bug and mark it as a dependency as discussed with grzegorz on IRC.

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

[Chris Dumez]

Comment on attachment 205073 [details]
apply suggestions

LGTM, r=me but please let grzegorz have one last look before landing.

[Grzegorz Czajkowski]

Comment on attachment 205073 [details]
apply suggestions

View in context: https://bugs.webkit.org/attachment.cgi?id=205073&action=review

The code itself looks fine. Added one nit regarding to the EFL's spellchecking.

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker.cpp:629
>      ewk_text_checker_word_guesses_get_cb_set(onWordGuesses);

Setting this callback affects other text checker tests that will be launched after this one. Could you invalidate it by setting 0 at the end of the test? You've already done it in onSettingChange test. Would be nice to add an additional line then, to point clearing part of the test.

[Krzysztof Wolanski]

Created attachment 205278 [details]
apply suggestions

[Grzegorz Czajkowski]

Comment on attachment 205278 [details]
apply suggestions

Thanks for the changes.

[WebKit Commit Bot]

Comment on attachment 205278 [details]
apply suggestions

Clearing flags on attachment: 205278

Committed r151904: <http://trac.webkit.org/changeset/151904>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.