Bug 50738 - [GTK] Choose a dictionary in case there is no one defined or the env LANG does not propose one
Summary: [GTK] Choose a dictionary in case there is no one defined or the env LANG doe...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 50739
  Show dependency treegraph
 
Reported: 2010-12-09 01:54 PST by Alejandro G. Castro
Modified: 2010-12-10 09:37 PST (History)
1 user (show)

See Also:


Attachments
Proposed patch (3.05 KB, patch)
2010-12-09 02:06 PST, Alejandro G. Castro
mrobinson: review-
Details | Formatted Diff | Diff
Proposed patch (2.55 KB, patch)
2010-12-09 04:15 PST, Alejandro G. Castro
mrobinson: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alejandro G. Castro 2010-12-09 01:54:25 PST
Currently we are not setting any dictionary and this is causing that the DRT in case the LANG is set to C does not take any dict and spelling tests fail.
Comment 1 Alejandro G. Castro 2010-12-09 02:06:22 PST
Created attachment 76025 [details]
Proposed patch
Comment 2 Martin Robinson 2010-12-09 02:14:53 PST
Comment on attachment 76025 [details]
Proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=76025&action=review

Yay! Thanks for fixing this.

> WebKit/gtk/webkit/webkitwebsettings.cpp:953
> +static void get_avail_dicts_cb(const char * const lang_tag, const char * const provider_name, const char * const provider_desc, const char * const provider_file, void * data)

Should be in WebKit style (camelCase), asterisks with the typenames and use full words instead of abbreviations. :)

> WebKit/gtk/webkit/webkitwebsettings.cpp:1079
> +                // No dictionaries selected, we get one from the list
> +                enchant_broker_list_dicts(broker, get_avail_dicts_cb, &dicts);
> +                if (dicts) {
> +                    GSList* tmp;

Instead of passing a GSList* it might simplify things to pass a point to a Vector<CString> like:

Vector<CString> allDictionaries;
enchant_broker_list_dicts(broker, getAvilableDictionariesCallback, &allDictionaries);
if (!allDictionaries.empty()) {
      ...
     
}
Comment 3 Alejandro G. Castro 2010-12-09 04:14:24 PST
(In reply to comment #2)
> (From update of attachment 76025 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=76025&action=review
> 
> Yay! Thanks for fixing this.
> 

Thanks for the review, I like your proposal.
Comment 4 Alejandro G. Castro 2010-12-09 04:15:47 PST
Created attachment 76043 [details]
Proposed patch
Comment 5 Martin Robinson 2010-12-09 04:33:31 PST
Comment on attachment 76043 [details]
Proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=76043&action=review

Thank you! Please fix the remaining minor issues before landing.

> WebKit/gtk/webkit/webkitwebsettings.cpp:953
> +static void getAvilableDictionariesCallback(const char* const lang_tag, const char* const provider_name, const char* const provider_desc, const char* const provider_file, void* data)

Please change lang_tag to languageTag before landing. You should actually omit provider_name, provider_desc, and provider_file to avoid a warning here too.

> WebKit/gtk/webkit/webkitwebsettings.cpp:955
> +    Vector<CString>* dicts = static_cast< Vector<CString>* >(data);

Please remove the extra space after the static_cast< before landing.
Comment 6 Alejandro G. Castro 2010-12-10 09:37:50 PST
Landed http://trac.webkit.org/changeset/73737