[Webkit-unassigned] [Bug 90878] [GTK] Add API to set preferred languages to WebKit2 GTK+

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=90878


Martin Robinson <mrobinson at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #151451|review?                     |review+
               Flag|                            |




--- Comment #3 from Martin Robinson <mrobinson at webkit.org>  2012-08-17 07:40:44 PST ---
(From update of attachment 151451)
View in context: https://bugs.webkit.org/attachment.cgi?id=151451&action=review

Looks good, but I have a few minor comments and I still don't understand the changes in the platform-independent code.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebContext.cpp:425
> +    return g_strdelimit(g_ascii_strdown(language, -1), "_", '-');

You treat the return value here as UTF-8 and the annotation for webkit_web_context_set_preferred_languages says that it takes UTF-8, so maybe this should be g_utf8_strdown?

Another thing you could do is to just return a string:
return String::fromUTF8(language).lower().replace("_", "-");

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebContext.cpp:221
> +    GList* languages = g_list_prepend(0, (gpointer)"dE");
> +    languages = g_list_prepend(languages, (gpointer)"ES_es");
> +    languages = g_list_prepend(languages, (gpointer)"en");
> +    webkit_web_context_set_preferred_languages(webkit_web_context_get_default(), languages);

The casts to gpointer are almost certainly unecessary since the strings are const char*. Maybe you could double-check here?

> Source/WebKit2/WebProcess/WebProcess.cpp:316
> +#if PLATFORM(GTK)
> +    languageDidChange();
> +#endif

I'm not sure I understand completely why this change is necessary for GTK+ only.

> Source/WebKit2/WebProcess/gtk/WebProcessGtk.cpp:126
> +    // Fallback to "en" is the list is empty.

is the list -> if the list

> Source/WebKit2/WebProcess/gtk/WebProcessGtk.cpp:145
> +        builder.append(languages[i]);

You should probably use languages[i].utf8() for the sake of safety.

> Source/WebKit2/WebProcess/gtk/WebProcessGtk.cpp:148
> +        if (quality && quality < 100) {

It seems the only way that quality could be >= 100 is if i or delta is <= 0. It doesn't look like tihs can happen, so perhaps you can remove the quality < 0 check here.

> Source/WebKit2/WebProcess/gtk/WebProcessGtk.cpp:151
> +            char buffer[8];
> +            g_ascii_formatd(buffer, 8, "%.2f", quality / 100.0);
> +            builder.append(String::format(";q=%s", buffer));

Hrm. why not just use string.format here? builder.append(String::format(";q=%.2f", quality / 100.0)));

It's probably a good idea to use

> Source/WebKit2/WebProcess/gtk/WebProcessGtk.cpp:162
> +    SoupSession* session = WebCore::ResourceHandle::defaultSession();
> +    CString acceptLanguages = buildAcceptLanguages(languages);
> +    g_object_set(G_OBJECT(session), "accept-language", acceptLanguages.data(), NULL);

This could probably be one line sanely.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.