Bug 117190 – [EFL][WK2] Context menu spellchecking items are not available when "Check Spelling While Typing" is off

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit EFL

[Grzegorz Czajkowski]

Suggestions for the misspelled word, learn and ignore the word should be available in context menu, even continuous spell checking setting is off.

Patch will be added.

[Grzegorz Czajkowski]

Created attachment 203772 [details]
proposed patch

[Chris Dumez]

Comment on attachment 203772 [details]
proposed patch

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

> Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:90
> +    TextCheckerClientEfl::instance().ensureSpellCheckingLanguage();

This change looks a bit "fragile", especially without a comment or unit test for it. Would be nice if we had both :)

[Build Bot]

Comment on attachment 203772 [details]
proposed patch

Attachment 203772 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/745296

New failing tests:
inspector/geolocation-success.html

[Build Bot]

Created attachment 203867 [details]
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-14  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.3

[Grzegorz Czajkowski]

Created attachment 204009 [details]
apply Christophe's suggestions

[Chris Dumez]

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

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

Much better, thanks.

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

Why do we need this const_cast? According the to the doc, EINA_LIST_FOREACH() should do just fine with a const Eina_List*.

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker.cpp:228
> +    Eina_List* listIterator = 0;

Doesn't need initialization.

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker.cpp:229
> +    void* itemData = 0;

Ditto.

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker.cpp:245
> +    return wasContextMenuShown;

I think return true might be clearer.

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker.cpp:276
> +    if (!loadedLanguages)

When can this happen?

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker.cpp:281
> +    data;

What's this statement for?

> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker.cpp:283
> +        eina_stringshare_del(static_cast<const char*>(data));

Cast to Eina_Stringshare* may be clearer.

[Grzegorz Czajkowski]

Created attachment 204021 [details]
apply Christophe's comment #6

[Grzegorz Czajkowski]

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

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

>> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_text_checker.cpp:276
>> +    if (!loadedLanguages)
> 
> When can this happen?

It's described in the same test (260 line). It's possible that user doesn't set the default language or no dictionary is currently installed or just enchant can't use it.

[Chris Dumez]

Comment on attachment 204021 [details]
apply Christophe's comment #6

LGTM, r=me.

[WebKit Commit Bot]

Comment on attachment 204021 [details]
apply Christophe's comment #6

Clearing flags on attachment: 204021

Committed r151314: <http://trac.webkit.org/changeset/151314>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.