[webkit-reviews] review granted: [Bug 90878] [GTK] Add API to set preferred languages to WebKit2 GTK+ : [Attachment 151451] Patch

[bugzilla-daemon at webkit.org]

Martin Robinson <mrobinson at webkit.org> has granted Carlos Garcia Campos
<cgarcia at igalia.com>'s request for review:
Bug 90878: [GTK] Add API to set preferred languages to WebKit2 GTK+
https://bugs.webkit.org/show_bug.cgi?id=90878

Attachment 151451: Patch
https://bugs.webkit.org/attachment.cgi?id=151451&action=review

------- Additional Comments from Martin Robinson <mrobinson at webkit.org>
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.