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

Description Grzegorz Czajkowski 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.
Comment 1 Grzegorz Czajkowski 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.
Comment 2 WebKit Review Bot 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
Comment 3 Mario Sanchez Prada 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()?
Comment 4 Grzegorz Czajkowski 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.
Comment 5 Mario Sanchez Prada 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.
Comment 6 Gyuyoung Kim 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.
Comment 7 Carlos Garcia Campos 2012-09-13 23:39:47 PDT
Comment on attachment 163795 [details]
proposed patch

LGTM, thanks!
Comment 8 WebKit Review Bot 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>
Comment 9 WebKit Review Bot 2012-09-14 01:09:26 PDT
All reviewed patches have been landed.  Closing bug.