Bug 95956 - [WK2][EFL] Add unit tests for Spelling
Summary: [WK2][EFL] Add unit tests for Spelling
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Grzegorz Czajkowski
URL:
Keywords:
Depends on: 91854
Blocks: 90451
  Show dependency treegraph
 
Reported: 2012-09-06 02:34 PDT by Grzegorz Czajkowski
Modified: 2012-10-17 05:06 PDT (History)
5 users (show)

See Also:


Attachments
proposed patch (14.61 KB, patch)
2012-09-24 01:12 PDT, Grzegorz Czajkowski
gyuyoung.kim: commit-queue-
Details | Formatted Diff | Diff
UT for ewk_text_checker_setting.h (14.83 KB, patch)
2012-09-25 07:14 PDT, Grzegorz Czajkowski
gyuyoung.kim: commit-queue-
Details | Formatted Diff | Diff
UT for ewk_text_checker.h (12.63 KB, patch)
2012-10-10 01:15 PDT, Grzegorz Czajkowski
gyuyoung.kim: commit-queue-
Details | Formatted Diff | Diff
updated patch (22.46 KB, patch)
2012-10-12 08:34 PDT, Grzegorz Czajkowski
gyuyoung.kim: commit-queue-
Details | Formatted Diff | Diff
apply Christophe's suggestions (21.53 KB, patch)
2012-10-15 06:35 PDT, Grzegorz Czajkowski
gyuyoung.kim: commit-queue-
Details | Formatted Diff | Diff
apply Gyuyoung's comments (21.60 KB, patch)
2012-10-16 01:54 PDT, Grzegorz Czajkowski
gyuyoung.kim: commit-queue-
Details | Formatted Diff | Diff
adjust to latest changes from bug 91854 (21.69 KB, patch)
2012-10-16 08:04 PDT, Grzegorz Czajkowski
gyuyoung.kim: review+
gyuyoung.kim: commit-queue-
Details | Formatted Diff | Diff
patch for landing (21.68 KB, patch)
2012-10-17 03:48 PDT, Grzegorz Czajkowski
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Grzegorz Czajkowski 2012-09-06 02:34:20 PDT
This bug covers the tests for API from:
 - ewk_text_checker.h,
 - ewk_text_checker_setting.h.
Comment 1 Grzegorz Czajkowski 2012-09-24 01:12:39 PDT
Created attachment 165326 [details]
proposed patch

This patch covers API from ewk_text_checker_setting.h.
Comment 2 Gyuyoung Kim 2012-09-24 01:16:36 PDT
Comment on attachment 165326 [details]
proposed patch

Attachment 165326 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13986612
Comment 3 Chris Dumez 2012-09-24 05:16:52 PDT
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(',')
Comment 4 Gyuyoung Kim 2012-09-24 19:07:48 PDT
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.
Comment 5 Grzegorz Czajkowski 2012-09-25 07:14:35 PDT
Created attachment 165609 [details]
UT for ewk_text_checker_setting.h
Comment 6 Grzegorz Czajkowski 2012-09-25 07:21:21 PDT
(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.
Comment 7 Gyuyoung Kim 2012-09-25 07:22:22 PDT
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
Comment 8 Grzegorz Czajkowski 2012-10-10 01:15:46 PDT
Created attachment 167953 [details]
UT for ewk_text_checker.h
Comment 9 Gyuyoung Kim 2012-10-10 01:34:39 PDT
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
Comment 10 Grzegorz Czajkowski 2012-10-10 01:48:55 PDT
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)
Comment 11 Grzegorz Czajkowski 2012-10-12 08:34:17 PDT
Created attachment 168424 [details]
updated patch

1) Merge two patches into one.
2) Adjust to the recent changes in bug 91854
Comment 12 Gyuyoung Kim 2012-10-12 08:39:55 PDT
Comment on attachment 168424 [details]
updated patch

Attachment 168424 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14265346
Comment 13 Build Bot 2012-10-12 08:59:58 PDT
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
Comment 14 Chris Dumez 2012-10-15 00:08:24 PDT
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.
Comment 15 Chris Dumez 2012-10-15 00:21:07 PDT
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"
Comment 16 Grzegorz Czajkowski 2012-10-15 06:35:14 PDT
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.
Comment 17 Gyuyoung Kim 2012-10-15 06:43:13 PDT
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
Comment 18 Gyuyoung Kim 2012-10-16 01:20:11 PDT
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 ?
Comment 19 Grzegorz Czajkowski 2012-10-16 01:54:27 PDT
Created attachment 168893 [details]
apply Gyuyoung's comments
Comment 20 Gyuyoung Kim 2012-10-16 02:10:42 PDT
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
Comment 21 Grzegorz Czajkowski 2012-10-16 08:04:34 PDT
Created attachment 168947 [details]
adjust to latest changes from bug 91854
Comment 22 Gyuyoung Kim 2012-10-16 08:14:52 PDT
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
Comment 23 Gyuyoung Kim 2012-10-17 02:05:35 PDT
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.
Comment 24 Grzegorz Czajkowski 2012-10-17 03:48:26 PDT
Created attachment 169149 [details]
patch for landing
Comment 25 WebKit Review Bot 2012-10-17 05:06:48 PDT
Comment on attachment 169149 [details]
patch for landing

Clearing flags on attachment: 169149

Committed r131589: <http://trac.webkit.org/changeset/131589>
Comment 26 WebKit Review Bot 2012-10-17 05:06:55 PDT
All reviewed patches have been landed.  Closing bug.