This is another soup session property that needs to be exposed by our API.
Created attachment 151451 [details] Patch
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
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.
(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.
Committed r125972: <http://trac.webkit.org/changeset/125972>
See https://bugs.webkit.org/show_bug.cgi?id=94428
Re-opened since this is blocked by 94431
Created attachment 159360 [details] Updated patch to fix the mac build Try to fix mac build.
(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.
Committed r126017: <http://trac.webkit.org/changeset/126017>