[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





--- Comment #4 from Carlos Garcia Campos <cgarcia at igalia.com>  2012-08-19 02:05:37 PST ---
(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.

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