Bug 117190 - [EFL][WK2] Context menu spellchecking items are not available when "Check Spelling While Typing" is off
Summary: [EFL][WK2] Context menu spellchecking items are not available when "Check Spe...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Grzegorz Czajkowski
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-06-04 05:07 PDT by Grzegorz Czajkowski
Modified: 2013-06-07 05:26 PDT (History)
7 users (show)

See Also:


Attachments
proposed patch (1.89 KB, patch)
2013-06-05 02:13 PDT, Grzegorz Czajkowski
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2 (735.12 KB, application/zip)
2013-06-05 11:16 PDT, Build Bot
no flags Details
apply Christophe's suggestions (9.58 KB, patch)
2013-06-07 00:44 PDT, Grzegorz Czajkowski
no flags Details | Formatted Diff | Diff
apply Christophe's comment #6 (9.61 KB, patch)
2013-06-07 03:51 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 2013-06-04 05:07:24 PDT
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.
Comment 1 Grzegorz Czajkowski 2013-06-05 02:13:39 PDT
Created attachment 203772 [details]
proposed patch
Comment 2 Chris Dumez 2013-06-05 04:57:00 PDT
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 :)
Comment 3 Build Bot 2013-06-05 11:16:52 PDT
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
Comment 4 Build Bot 2013-06-05 11:16:55 PDT
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
Comment 5 Grzegorz Czajkowski 2013-06-07 00:44:43 PDT
Created attachment 204009 [details]
apply Christophe's suggestions
Comment 6 Chris Dumez 2013-06-07 02:59:46 PDT
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.
Comment 7 Grzegorz Czajkowski 2013-06-07 03:51:06 PDT
Created attachment 204021 [details]
apply Christophe's comment #6
Comment 8 Grzegorz Czajkowski 2013-06-07 03:57:15 PDT
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.
Comment 9 Chris Dumez 2013-06-07 04:31:36 PDT
Comment on attachment 204021 [details]
apply Christophe's comment #6

LGTM, r=me.
Comment 10 WebKit Commit Bot 2013-06-07 05:26:06 PDT
Comment on attachment 204021 [details]
apply Christophe's comment #6

Clearing flags on attachment: 204021

Committed r151314: <http://trac.webkit.org/changeset/151314>
Comment 11 WebKit Commit Bot 2013-06-07 05:26:10 PDT
All reviewed patches have been landed.  Closing bug.