Bug 96518

Summary: Add method to get the list of all available dictionaries
Product: WebKit Reporter: Grzegorz Czajkowski <g.czajkowski>
Component: WebCore Misc.Assignee: Grzegorz Czajkowski <g.czajkowski>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, cgarcia, gustavo, mario, mrobinson, m.roj, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 91854    
Attachments:
Description Flags
proposed patch none

Grzegorz Czajkowski
Reported 2012-09-12 08:00:12 PDT
There is lack of the method in TextCheckerEnchant class which allows the client to get the list of all available/installed dictionaries. To set the dictionaries the user have to 'guess/hardcode' the names of dictionaries that might be non obvious. The list can be used to set dictionaries used by Enchant. To avoid names conflict this patch proposed a new one for former getSpellCheckingLanguages. It has been changed to loadedSpellCheckingLanguages as it gets the current (in use) dictionaries. Additionally the 'get' prefix has been removed according to WebKit coding style.
Attachments
proposed patch (5.29 KB, patch)
2012-09-13 00:11 PDT, Grzegorz Czajkowski 		no flags
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Grzegorz Czajkowski
Comment 1 2012-09-13 00:11:43 PDT
Created attachment 163795 [details] proposed patch This getter can be used by WebKit's GTK and EFL API for instance: - webkit_web_context_set_spell_checking_languages(), - ewk_text_checker_setting_spell_checking_languages_set() to set dictionaries used by Enchant.
WebKit Review Bot
Comment 2 2012-09-13 00:13:05 PDT
Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API
Mario Sanchez Prada
Comment 3 2012-09-13 02:12:38 PDT
Comment on attachment 163795 [details] proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=163795&action=review The patch looks good to me, although I'm a bit confused because I was expecting to see this new function used in the patch for bug 91854, but I guess that will come in a separate patch? Otherwise, I just have one humble suggestion (see below). > Source/WebCore/platform/text/enchant/TextCheckerEnchant.cpp:196 > + enchant_broker_list_dicts(m_broker, enchantDictDescribeCallback, &allDictionaries); Maybe early return if allDictionaries.isEmpty()?
Grzegorz Czajkowski
Comment 4 2012-09-13 02:38:58 PDT
(In reply to comment #3) > (From update of attachment 163795 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=163795&action=review > > The patch looks good to me, although I'm a bit confused because I was expecting to see this new function used in the patch for bug 91854, but I guess that will come in a separate patch? The last patch for bug 91854 adds ewk_text_checker_setting_spell_checking_languages_get() which gets already loaded dictionaries not available once that this patch proposes. We decided to keep old API as it is and add a new one ewk_text_checker_setting_available_spell_checking_languages_get() to get installed languages. > > Otherwise, I just have one humble suggestion (see below). > > > Source/WebCore/platform/text/enchant/TextCheckerEnchant.cpp:196 > > + enchant_broker_list_dicts(m_broker, enchantDictDescribeCallback, &allDictionaries); > > Maybe early return if allDictionaries.isEmpty()? I thought about it too. IMO this method is very short and if the allDictionaries.isEmpty() the iterations won't happen of course and method returns an empty vector. To early return we have to move 'Vector<String> languages' somewhere above. If you prefer early return here just let me know I will change it. Thanks.
Mario Sanchez Prada
Comment 5 2012-09-13 03:47:46 PDT
(In reply to comment #4) > [...] > The last patch for bug 91854 adds ewk_text_checker_setting_spell_checking_languages_get() which gets already loaded dictionaries not available once that this patch proposes. > We decided to keep old API as it is and add a new one ewk_text_checker_setting_available_spell_checking_languages_get() to get installed languages. Ok. Thanks for the clarification. > [...] > > Maybe early return if allDictionaries.isEmpty()? > I thought about it too. IMO this method is very short and if the allDictionaries.isEmpty() the iterations won't happen of course and method returns an empty vector. To early return we have to move 'Vector<String> languages' somewhere above. If you prefer early return here just let me know I will change it. Thanks. Not a big deal. It already looks good to me as it is now.
Gyuyoung Kim
Comment 6 2012-09-13 23:21:08 PDT
Comment on attachment 163795 [details] proposed patch I think this is the way to go for both gtk and efl ports.
Carlos Garcia Campos
Comment 7 2012-09-13 23:39:47 PDT
Comment on attachment 163795 [details] proposed patch LGTM, thanks!
WebKit Review Bot
Comment 8 2012-09-14 01:09:22 PDT
Comment on attachment 163795 [details] proposed patch Clearing flags on attachment: 163795 Committed r128567: <http://trac.webkit.org/changeset/128567>
WebKit Review Bot
Comment 9 2012-09-14 01:09:26 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.