96518 2012-09-12 08:00:12 -0700 Add method to get the list of all available dictionaries 2012-09-14 01:09:26 -0700 1 1 1 Unclassified WebKit WebCore Misc. 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 91854 1 g.czajkowski g.czajkowski ap cgarcia gustavo mario mrobinson m.roj webkit.review.bot oldest_to_newest 718360 0 g.czajkowski 2012-09-12 08:00:12 -0700 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. 719132 1 163795 g.czajkowski 2012-09-13 00:11:43 -0700 Created attachment 163795 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. 719134 2 webkit.review.bot 2012-09-13 00:13:05 -0700 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 719258 3 163795 mario 2012-09-13 02:12:38 -0700 Comment on attachment 163795 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()? 719276 4 g.czajkowski 2012-09-13 02:38:58 -0700 (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. 719309 5 mario 2012-09-13 03:47:46 -0700 (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. 720349 6 163795 gyuyoung.kim 2012-09-13 23:21:08 -0700 Comment on attachment 163795 proposed patch I think this is the way to go for both gtk and efl ports. 720360 7 163795 cgarcia 2012-09-13 23:39:47 -0700 Comment on attachment 163795 proposed patch LGTM, thanks! 720421 8 163795 webkit.review.bot 2012-09-14 01:09:22 -0700 Comment on attachment 163795 proposed patch Clearing flags on attachment: 163795 Committed r128567: <http://trac.webkit.org/changeset/128567> 720422 9 webkit.review.bot 2012-09-14 01:09:26 -0700 All reviewed patches have been landed. Closing bug. 163795 2012-09-13 00:11:43 -0700 2012-09-14 01:09:22 -0700 proposed patch spelling_api_get_list_of_dicts.patch text/plain 5414 g.czajkowski ZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZyBiL1NvdXJjZS9XZWJDb3JlL0No YW5nZUxvZwppbmRleCBjNjU2YzE5Li5lMjhhOGE1IDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29y ZS9DaGFuZ2VMb2cKKysrIGIvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMzYg QEAKKzIwMTItMDktMTIgIEdyemVnb3J6IEN6YWprb3dza2kgIDxnLmN6YWprb3dza2lAc2Ftc3Vu Zy5jb20+CisKKyAgICAgICAgQWRkIG1ldGhvZCB0byBnZXQgdGhlIGxpc3Qgb2YgYWxsIGF2YWls YWJsZSBkaWN0aW9uYXJpZXMKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19i dWcuY2dpP2lkPTk2NTE4CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisK KyAgICAgICAgVGhlcmUgaXMgbGFjayBvZiB0aGUgbWV0aG9kIGluIHRoZSBUZXh0Q2hlY2tlckVu Y2hhbnQgY2xhc3MgdG8gZ2V0CisgICAgICAgIHRoZSBsaXN0IG9mIGFsbCBhdmFpbGFibGUvaW5z dGFsbGVkIGRpY3Rpb25hcmllcy4gVG8gc2V0IHRoZQorICAgICAgICBkaWN0aW9uYXJpZXMgdGhl IGNsaWVudCBoYXMgdG8gJ2d1ZXNzL2hhcmQgY29kZScgdGhlIG5hbWVzIG9mCisgICAgICAgIGRp Y3Rpb25hcmllcyB0aGF0IGFyZSBub3Qgb2J2aW91cyBhbmQgZGVwZW5kIG9uIE9TIGNvbmZpZ3Vy YXRpb24KKyAgICAgICAgYW5kIGluc3RhbGxlZCBwYWNrYWdlcy4gVGhlIGxpc3QgY2FuIGJlIHVz ZWQgYnkgV2ViS2l0J3MgR1RLIGFuZAorICAgICAgICBFRkwgQVBJIGZvciBpbnN0YW5jZToKKyAg ICAgICAgd2Via2l0X3dlYl9jb250ZXh0X3NldF9zcGVsbF9jaGVja2luZ19sYW5ndWFnZXMoKSwK KyAgICAgICAgZXdrX3RleHRfY2hlY2tlcl9zZXR0aW5nX3NwZWxsX2NoZWNraW5nX2xhbmd1YWdl c19zZXQoKQorICAgICAgICB0byBzZXQgZGljdGlvbmFyaWVzIHVzZWQgYnkgRW5jaGFudC4KKwor ICAgICAgICBUbyBhdm9pZCBuYW1lcyBjb25mbGljdCBhIG5ldyBvbmUgaGFzIGJlZW4gaW50cm9k dWNlZCBmb3IgZm9ybWVyCisgICAgICAgIGdldFNwZWxsQ2hlY2tpbmdMYW5ndWFnZXMuIEl0IGhh cyBiZWVuIGNoYW5nZWQgdG8gbG9hZGVkU3BlbGxDaGVja2luZ0xhbmd1YWdlcworICAgICAgICBh cyBpdCBnZXRzIHRoZSBjdXJyZW50IChpbiB1c2UpIGRpY3Rpb25hcmllcy4gQWRkaXRpb25hbGx5 IHRoZSAnZ2V0JyBwcmVmaXgKKyAgICAgICAgaGFzIGJlZW4gcmVtb3ZlZCBhY2NvcmRpbmcgdG8g V2ViS2l0IGNvZGluZyBzdHlsZS4KKworICAgICAgICAqIHBsYXRmb3JtL3RleHQvZW5jaGFudC9U ZXh0Q2hlY2tlckVuY2hhbnQuY3BwOgorICAgICAgICAoVGV4dENoZWNrZXJFbmNoYW50Ojpsb2Fk ZWRTcGVsbENoZWNraW5nTGFuZ3VhZ2VzKToKKyAgICAgICAgQ2hhbmdlZCBuYW1lIGZyb20gZ2V0 U3BlbGxDaGVja2luZ0xhbmd1YWdlcyBhbmQgYWRkZWQgY29uc3QgbW9kaWZpZXIuCisKKyAgICAg ICAgKFRleHRDaGVja2VyRW5jaGFudDo6YXZhaWxhYmxlU3BlbGxDaGVja2luZ0xhbmd1YWdlcyk6 CisgICAgICAgIEEgbmV3bHkgYWRkZWQgbWV0aG9kIHRvIGdldCBhdmFpbGFibGUvaW5zdGFsbGVk IGRpY3Rpb25hcmllcy4KKyAgICAgICAgSXRzIG91dHB1dCBjYW4gYmUgdXNlZCBhcyBpbnB1dCBm b3IgdXBkYXRlU3BlbGxDaGVja2luZ0xhbmd1YWdlcygpLgorCisgICAgICAgICogcGxhdGZvcm0v dGV4dC9lbmNoYW50L1RleHRDaGVja2VyRW5jaGFudC5oOgorICAgICAgICAoVGV4dENoZWNrZXJF bmNoYW50KToKKwogMjAxMi0wOS0xMiAgSWx5YSBUaWtob25vdnNreSAgPGxvaXNsb0BjaHJvbWl1 bS5vcmc+CiAKIApkaWZmIC0tZ2l0IGEvU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vdGV4dC9lbmNo YW50L1RleHRDaGVja2VyRW5jaGFudC5jcHAgYi9Tb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS90ZXh0 L2VuY2hhbnQvVGV4dENoZWNrZXJFbmNoYW50LmNwcAppbmRleCBmNzJmYjBjLi5kMzE1ZWU5IDEw MDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS90ZXh0L2VuY2hhbnQvVGV4dENoZWNr ZXJFbmNoYW50LmNwcAorKysgYi9Tb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS90ZXh0L2VuY2hhbnQv VGV4dENoZWNrZXJFbmNoYW50LmNwcApAQCAtMTczLDcgKzE3Myw3IEBAIHZvaWQgVGV4dENoZWNr ZXJFbmNoYW50Ojp1cGRhdGVTcGVsbENoZWNraW5nTGFuZ3VhZ2VzKGNvbnN0IFZlY3RvcjxTdHJp bmc+JiBsYW5nCiAgICAgbV9lbmNoYW50RGljdGlvbmFyaWVzID0gc3BlbGxEaWN0aW9uYXJpZXM7 CiB9CiAKLVZlY3RvcjxTdHJpbmc+IFRleHRDaGVja2VyRW5jaGFudDo6Z2V0U3BlbGxDaGVja2lu Z0xhbmd1YWdlcygpCitWZWN0b3I8U3RyaW5nPiBUZXh0Q2hlY2tlckVuY2hhbnQ6OmxvYWRlZFNw ZWxsQ2hlY2tpbmdMYW5ndWFnZXMoKSBjb25zdAogewogICAgIFZlY3RvcjxTdHJpbmc+IGxhbmd1 YWdlczsKICAgICBpZiAobV9lbmNoYW50RGljdGlvbmFyaWVzLmlzRW1wdHkoKSkKQEAgLTE5MCw2 ICsxOTAsMTggQEAgVmVjdG9yPFN0cmluZz4gVGV4dENoZWNrZXJFbmNoYW50OjpnZXRTcGVsbENo ZWNraW5nTGFuZ3VhZ2VzKCkKICAgICByZXR1cm4gbGFuZ3VhZ2VzOwogfQogCitWZWN0b3I8U3Ry aW5nPiBUZXh0Q2hlY2tlckVuY2hhbnQ6OmF2YWlsYWJsZVNwZWxsQ2hlY2tpbmdMYW5ndWFnZXMo KSBjb25zdAoreworICAgIFZlY3RvcjxDU3RyaW5nPiBhbGxEaWN0aW9uYXJpZXM7CisgICAgZW5j aGFudF9icm9rZXJfbGlzdF9kaWN0cyhtX2Jyb2tlciwgZW5jaGFudERpY3REZXNjcmliZUNhbGxi YWNrLCAmYWxsRGljdGlvbmFyaWVzKTsKKworICAgIFZlY3RvcjxTdHJpbmc+IGxhbmd1YWdlczsK KyAgICBmb3IgKFZlY3RvcjxDU3RyaW5nPjo6Y29uc3RfaXRlcmF0b3IgaXRlciA9IGFsbERpY3Rp b25hcmllcy5iZWdpbigpOyBpdGVyICE9IGFsbERpY3Rpb25hcmllcy5lbmQoKTsgKytpdGVyKQor ICAgICAgICBsYW5ndWFnZXMuYXBwZW5kKFN0cmluZzo6ZnJvbVVURjgoaXRlci0+ZGF0YSgpKSk7 CisKKyAgICByZXR1cm4gbGFuZ3VhZ2VzOworfQorCiB2b2lkIFRleHRDaGVja2VyRW5jaGFudDo6 ZnJlZUVuY2hhbnRCcm9rZXJEaWN0aW9uYXJpZXMoKQogewogICAgIGZvciAoVmVjdG9yPEVuY2hh bnREaWN0Kj46OmNvbnN0X2l0ZXJhdG9yIGl0ZXIgPSBtX2VuY2hhbnREaWN0aW9uYXJpZXMuYmVn aW4oKTsgaXRlciAhPSBtX2VuY2hhbnREaWN0aW9uYXJpZXMuZW5kKCk7ICsraXRlcikKZGlmZiAt LWdpdCBhL1NvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL3RleHQvZW5jaGFudC9UZXh0Q2hlY2tlckVu Y2hhbnQuaCBiL1NvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL3RleHQvZW5jaGFudC9UZXh0Q2hlY2tl ckVuY2hhbnQuaAppbmRleCAxNzgwYjQ1Li43ODdmOGJmIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2Vi Q29yZS9wbGF0Zm9ybS90ZXh0L2VuY2hhbnQvVGV4dENoZWNrZXJFbmNoYW50LmgKKysrIGIvU291 cmNlL1dlYkNvcmUvcGxhdGZvcm0vdGV4dC9lbmNoYW50L1RleHRDaGVja2VyRW5jaGFudC5oCkBA IC00Miw3ICs0Miw4IEBAIHB1YmxpYzoKICAgICB2b2lkIGNoZWNrU3BlbGxpbmdPZlN0cmluZyhj b25zdCBTdHJpbmcmLCBpbnQmIG1pc3NwZWxsaW5nTG9jYXRpb24sIGludCYgbWlzc3BlbGxpbmdM ZW5ndGgpOwogICAgIFZlY3RvcjxTdHJpbmc+IGdldEd1ZXNzZXNGb3JXb3JkKGNvbnN0IFN0cmlu ZyYpOwogICAgIHZvaWQgdXBkYXRlU3BlbGxDaGVja2luZ0xhbmd1YWdlcyhjb25zdCBWZWN0b3I8 U3RyaW5nPiYgbGFuZ3VhZ2VzKTsKLSAgICBWZWN0b3I8U3RyaW5nPiBnZXRTcGVsbENoZWNraW5n TGFuZ3VhZ2VzKCk7CisgICAgVmVjdG9yPFN0cmluZz4gbG9hZGVkU3BlbGxDaGVja2luZ0xhbmd1 YWdlcygpIGNvbnN0OworICAgIFZlY3RvcjxTdHJpbmc+IGF2YWlsYWJsZVNwZWxsQ2hlY2tpbmdM YW5ndWFnZXMoKSBjb25zdDsKIAogcHJpdmF0ZToKICAgICBUZXh0Q2hlY2tlckVuY2hhbnQoKTsK ZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJLaXQyL0NoYW5nZUxvZyBiL1NvdXJjZS9XZWJLaXQyL0No YW5nZUxvZwppbmRleCA3ZWJiNDA2Li4zMzI4Mjc2IDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViS2l0 Mi9DaGFuZ2VMb2cKKysrIGIvU291cmNlL1dlYktpdDIvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTUg QEAKKzIwMTItMDktMTIgIEdyemVnb3J6IEN6YWprb3dza2kgIDxnLmN6YWprb3dza2lAc2Ftc3Vu Zy5jb20+CisKKyAgICAgICAgQWRkIG1ldGhvZCB0byBnZXQgdGhlIGxpc3Qgb2YgYWxsIGF2YWls YWJsZSBkaWN0aW9uYXJpZXMKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19i dWcuY2dpP2lkPTk2NTE4CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisK KyAgICAgICAgKiBVSVByb2Nlc3MvQVBJL2d0ay9XZWJLaXRUZXh0Q2hlY2tlci5jcHA6CisgICAg ICAgIChXZWJLaXRUZXh0Q2hlY2tlcjo6Z2V0U3BlbGxDaGVja2luZ0xhbmd1YWdlcyk6CisgICAg ICAgIFVwZGF0ZSB0aGUgbWV0aG9kIG5hbWUgdGhhdCBpcyBjYWxsZWQgaW5zaWRlIGdldFNwZWxs Q2hlY2tpbmdMYW5ndWFnZXMuCisgICAgICAgIFRoaXMgcmVwbGFjZSBkb2Vzbid0IGFmZmVjdCBh bnkgb24gQVBJIGNoYW5nZXMuCisKIDIwMTItMDktMTIgIFNoZXJpZmYgQm90ICA8d2Via2l0LnJl dmlldy5ib3RAZ21haWwuY29tPgogCiAgICAgICAgIFVucmV2aWV3ZWQsIHJvbGxpbmcgb3V0IHIx MjgyODAuCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0Mi9VSVByb2Nlc3MvQVBJL2d0ay9XZWJL aXRUZXh0Q2hlY2tlci5jcHAgYi9Tb3VyY2UvV2ViS2l0Mi9VSVByb2Nlc3MvQVBJL2d0ay9XZWJL aXRUZXh0Q2hlY2tlci5jcHAKaW5kZXggMzlmNDAyMi4uODllNzQ2ZCAxMDA2NDQKLS0tIGEvU291 cmNlL1dlYktpdDIvVUlQcm9jZXNzL0FQSS9ndGsvV2ViS2l0VGV4dENoZWNrZXIuY3BwCisrKyBi L1NvdXJjZS9XZWJLaXQyL1VJUHJvY2Vzcy9BUEkvZ3RrL1dlYktpdFRleHRDaGVja2VyLmNwcApA QCAtMTQwLDcgKzE0MCw3IEBAIHZvaWQgV2ViS2l0VGV4dENoZWNrZXI6OnNldFNwZWxsQ2hlY2tp bmdFbmFibGVkKGJvb2wgZW5hYmxlZCkKIAogY29uc3QgY2hhciogY29uc3QqIFdlYktpdFRleHRD aGVja2VyOjpnZXRTcGVsbENoZWNraW5nTGFuZ3VhZ2VzKCkKIHsKLSAgICBWZWN0b3I8U3RyaW5n PiBzcGVsbENoZWNraW5nTGFuZ3VhZ2VzID0gbV90ZXh0Q2hlY2tlci0+Z2V0U3BlbGxDaGVja2lu Z0xhbmd1YWdlcygpOworICAgIFZlY3RvcjxTdHJpbmc+IHNwZWxsQ2hlY2tpbmdMYW5ndWFnZXMg PSBtX3RleHRDaGVja2VyLT5sb2FkZWRTcGVsbENoZWNraW5nTGFuZ3VhZ2VzKCk7CiAgICAgaWYg KHNwZWxsQ2hlY2tpbmdMYW5ndWFnZXMuaXNFbXB0eSgpKQogICAgICAgICByZXR1cm4gMDsKIAo=