Bug 90878 – [GTK] Add API to set preferred languages to WebKit2 GTK+

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Carlos Garcia Campos]

This is another soup session property that needs to be exposed by our API.

[Carlos Garcia Campos]

Created attachment 151451 [details]
Patch

[WebKit Review Bot]

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

[Martin Robinson]

Comment on attachment 151451 [details]
Patch

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.

[Carlos Garcia Campos]

(In reply to comment #3)
> (From update of attachment 151451 [details])
> 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.

Thanks for reviewing.

> > 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("_", "-");

This looks much nicer :-)

> > 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?

They are indeed needed, I've replaced the C style cast for this beautiful C++ style 

cast const_cast<gpointer>(static_cast<const void*>()

> > 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.

because only the gtk+ port uses a language observer in the web process, but I agree it's harmless for tother ports, so I've removed the #ifdef

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

Ok.

> > Source/WebKit2/WebProcess/gtk/WebProcessGtk.cpp:145
> > +        builder.append(languages[i]);
> 
> You should probably use languages[i].utf8() for the sake of safety.

I don't think so, the function receives a vecor of String, and StringBuilder uses String, so we only need to convert the final string.

> > 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.

I'm not sure I understand this, I guess you mean I can remove the quality < 100 check. When i is 0 quality is always 100, and that always happens for the first iteration of the loop.

> > 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)));

Because g_ascii_formatd() is locale independent, String::format() would return a ',' instead of '.' in my spanish system for example.

> 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.

OK.

[Carlos Garcia Campos]

Committed r125972: <http://trac.webkit.org/changeset/125972>

[Eric Carlson]

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

[WebKit Review Bot]

Re-opened since this is blocked by 94431

[Carlos Garcia Campos]

Created attachment 159360 [details]
Updated patch to fix the mac build

Try to fix mac build.

[Carlos Garcia Campos]

(In reply to comment #8)
> Created an attachment (id=159360) [details]
> Updated patch to fix the mac build
> 
> Try to fix mac build.

All boxes are green now, let's give it another try.

[Carlos Garcia Campos]

Committed r126017: <http://trac.webkit.org/changeset/126017>