Summary: | [EFL][WK2] Context menu spellchecking items are not available when "Check Spelling While Typing" is off | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Grzegorz Czajkowski <g.czajkowski> | ||||||||||
Component: | WebKit EFL | Assignee: | Grzegorz Czajkowski <g.czajkowski> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | buildbot, cdumez, commit-queue, gyuyoung.kim, lucas.de.marchi, rakuco, rniwa | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Attachments: |
|
Description
Grzegorz Czajkowski
2013-06-04 05:07:24 PDT
Created attachment 203772 [details]
proposed patch
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 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 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
Created attachment 204009 [details]
apply Christophe's suggestions
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. Created attachment 204021 [details] apply Christophe's comment #6 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 on attachment 204021 [details] apply Christophe's comment #6 LGTM, r=me. Comment on attachment 204021 [details] apply Christophe's comment #6 Clearing flags on attachment: 204021 Committed r151314: <http://trac.webkit.org/changeset/151314> All reviewed patches have been landed. Closing bug. |