Bug 95956 – [WK2][EFL] Add unit tests for Spelling

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Grzegorz Czajkowski]

This bug covers the tests for API from:
 - ewk_text_checker.h,
 - ewk_text_checker_setting.h.

[Grzegorz Czajkowski]

Created attachment 165326 [details]
proposed patch

This patch covers API from ewk_text_checker_setting.h.

[Gyuyoung Kim]

Comment on attachment 165326 [details]
proposed patch

Attachment 165326 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13986612

[Chris Dumez]

Comment on attachment 165326 [details]
proposed patch

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

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker_setting.cpp:45
> +static Eina_Bool settingEnabled = false;

bool

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker_setting.cpp:46
> +static Ecore_Timer* timeout = 0;

timer? or timeoutTimer?

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker_setting.cpp:47
> +static double timeoutValue = 0.5;

Please indicate the time unit in the variable name. e.g. defaultTimeoutSeconds

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker_setting.cpp:86
> +    ewk_text_checker_setting_enable_continuous_spell_checking_set(EINA_TRUE);

true

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker_setting.cpp:93
> +    ecore_main_loop_begin();

We avoid using embedded main loops. We usually prefer checking a variable and calling ecore_main_loop_iterate() until the variable state changes.

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker_setting.cpp:109
> +    ewk_text_checker_setting_enable_continuous_spell_checking_set(EINA_FALSE);

false

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker_setting.cpp:120
> +    ewk_text_checker_setting_enable_grammar_checking_set(EINA_TRUE);

true

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker_setting.cpp:127
> +    ewk_text_checker_setting_enable_grammar_checking_set(EINA_FALSE);

false

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker_setting.cpp:136
> +    ewk_text_checker_setting_continuous_spell_checking_changed_cb_set(

Why 3 lines for this?

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker_setting.cpp:149
> +    ecore_main_loop_begin();

main loop iterate.

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker_setting.cpp:162
> +    ecore_main_loop_begin();

Ditto.

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker_setting.cpp:174
> +    ecore_main_loop_begin();

Ditto.

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker_setting.cpp:185
> +    ewk_text_checker_setting_grammar_checking_changed_cb_set(

On 1 line?

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker_setting.cpp:251
> +            languages.appendLiteral(",");

append(',') is more efficient

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker_setting.cpp:279
> +

Extra line here

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker_setting.cpp:316
> +    const char* lastExpected = static_cast<const char*>(eina_list_nth(availableLanguages, numberOfAvailableLanguages - 1));

eina_list_last?

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker_setting.cpp:322
> +    languages.appendLiteral(",");

append(',')

[Gyuyoung Kim]

Comment on attachment 165326 [details]
proposed patch

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

>> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker_setting.cpp:86
>> +    ewk_text_checker_setting_enable_continuous_spell_checking_set(EINA_TRUE);
> 
> true

We almost decided to use standard boolean type, In EFL, they avoid to use Eina_Bool except for legacy code.

[Grzegorz Czajkowski]

Created attachment 165609 [details]
UT for ewk_text_checker_setting.h

[Grzegorz Czajkowski]

(In reply to comment #3)
> (From update of attachment 165326 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=165326&action=review
> 
> > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker_setting.cpp:45
> > +static Eina_Bool settingEnabled = false;
> 
> bool

Ok, the unit test is using standard boolean type. EINA_TRUE, EINA_FALSE were replaced to true/false.

> 
> > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker_setting.cpp:46
> > +static Ecore_Timer* timeout = 0;
> 
> timer? or timeoutTimer?

timeoutTimer sounds better to me. Thanks.

> 
> > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker_setting.cpp:47
> > +static double timeoutValue = 0.5;
> 
> Please indicate the time unit in the variable name. e.g. defaultTimeoutSeconds

Changed to defaultTimeoutInSeconds.

> 
> > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker_setting.cpp:93
> > +    ecore_main_loop_begin();
> 
> We avoid using embedded main loops. We usually prefer checking a variable and calling ecore_main_loop_iterate() until the variable state changes.

I was trying to use ecore_main_loope_iterate(). Unfortunately it doesn't process idlers (ecore_idler_add) but some text checker setting are called through idlers.

> > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker_setting.cpp:136
> > +    ewk_text_checker_setting_continuous_spell_checking_changed_cb_set(
> 
> Why 3 lines for this?

Merged.

> > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker_setting.cpp:185
> > +    ewk_text_checker_setting_grammar_checking_changed_cb_set(
> 
> On 1 line?
> 

Merged.

> > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker_setting.cpp:251
> > +            languages.appendLiteral(",");
> 
> append(',') is more efficient

Right, done.

> 
> > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker_setting.cpp:279
> > +
> 
> Extra line here

Removed.

> 
> > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker_setting.cpp:316
> > +    const char* lastExpected = static_cast<const char*>(eina_list_nth(availableLanguages, numberOfAvailableLanguages - 1));
> 
> eina_list_last?

Makes sense.

> 
> > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker_setting.cpp:322
> > +    languages.appendLiteral(",");
> 
> append(',')

Fixed.

[Gyuyoung Kim]

Comment on attachment 165609 [details]
UT for ewk_text_checker_setting.h

Attachment 165609 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14006523

[Grzegorz Czajkowski]

Created attachment 167953 [details]
UT for ewk_text_checker.h

[Gyuyoung Kim]

Comment on attachment 167953 [details]
UT for ewk_text_checker.h

Attachment 167953 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14258040

[Grzegorz Czajkowski]

Both patches are not built because of dependency Bug 91854.
The patches covers the spelling API (ewk_text_checker.h, ewk_text_checker_settings.h)

The patches do not cover below API. They are UI specific and aren't well documented. Actually WebKit2-EFL doesn't need them so I am planing to remove them form Bug 91854.

typedef Eina_Bool (*Ewk_Text_Checker_UI_Spelling_Status_Get )(void)
typedef void (*Ewk_Text_Checker_UI_Spelling_Status_Toggle )(void)
typedef void (*Ewk_Text_Checker_UI_Spelling_Misspelled_Word_Update )(uint64_t tag, const char *misspelled_word)

[Grzegorz Czajkowski]

Created attachment 168424 [details]
updated patch

1) Merge two patches into one.
2) Adjust to the recent changes in bug 91854

[Gyuyoung Kim]

Comment on attachment 168424 [details]
updated patch

Attachment 168424 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14265346

[Build Bot]

Comment on attachment 168424 [details]
updated patch

Attachment 168424 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14266354

New failing tests:
inspector/profiler/heap-snapshot.html

[Chris Dumez]

Comment on attachment 168424 [details]
updated patch

I think those tests should be moved to a test_ewk2_spellcheck.cpp file. It is big and clutter the test_ewk2_settings.cpp file too much. We already do this for download API for e.g.

[Chris Dumez]

Comment on attachment 168424 [details]
updated patch

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

Looks OK to me. (besides the small typos)

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker.cpp:48
> + * Its values are reseted before each test.

"reset"

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker.cpp:73
> + * the client on which object (assosiated to the tag) the spelling is being invoked.

"associated"

[Grzegorz Czajkowski]

Created attachment 168702 [details]
apply Christophe's suggestions

1) Move the unit tests connected to spelling settings to test_ewk2_text_checker.cpp
2) Fix typos.

[Gyuyoung Kim]

Comment on attachment 168702 [details]
apply Christophe's suggestions

Attachment 168702 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14294676

[Gyuyoung Kim]

Comment on attachment 168702 [details]
apply Christophe's suggestions

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

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker.cpp:282
> +    // if the SPELLCHECK macro is disabled, the callback is not set.

if -> If ?

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker.cpp:409
> +    loadUrlSync(environment->urlForResource("spelling_test.html").data());

loadUrlSync needs to use ASSERT_TRUE or EXPECT_TRUE

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker.cpp:477
> +           The word has to be selected first.

The -> the ?

[Grzegorz Czajkowski]

Created attachment 168893 [details]
apply Gyuyoung's comments

[Gyuyoung Kim]

Comment on attachment 168893 [details]
apply Gyuyoung's comments

Attachment 168893 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14384104

[Grzegorz Czajkowski]

Created attachment 168947 [details]
adjust to latest changes from bug 91854

[Gyuyoung Kim]

Comment on attachment 168947 [details]
adjust to latest changes from bug 91854

Attachment 168947 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14392147

[Gyuyoung Kim]

Comment on attachment 168947 [details]
adjust to latest changes from bug 91854

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

LGTM. I think you should land this patch with Bug 91854.

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker.cpp:515
> +    

Looks unneeded line.

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker.cpp:536
> +    

ditto.

[Grzegorz Czajkowski]

Created attachment 169149 [details]
patch for landing

[WebKit Review Bot]

Comment on attachment 169149 [details]
patch for landing

Clearing flags on attachment: 169149

Committed r131589: <http://trac.webkit.org/changeset/131589>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.