Bug 90878

Summary: [GTK] Add API to set preferred languages to WebKit2 GTK+
Product: WebKit Reporter: Carlos Garcia Campos <cgarcia>
Component: WebKit2Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: danw, eric.carlson, gustavo, gyuyoung.kim, mrobinson, webkit.review.bot, xan.lopez
Priority: P2 Keywords: Gtk
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Linux   
Bug Depends on: 94431    
Bug Blocks:    
Attachments:
Description Flags
Patch
mrobinson: review+
Updated patch to fix the mac build none

Carlos Garcia Campos
Reported 2012-07-10 06:24:37 PDT
This is another soup session property that needs to be exposed by our API.
Attachments
Patch (16.19 KB, patch)
2012-07-10 06:32 PDT, Carlos Garcia Campos
mrobinson: review+
Updated patch to fix the mac build (17.18 KB, patch)
2012-08-20 01:26 PDT, Carlos Garcia Campos
no flags
Carlos Garcia Campos
Comment 1 2012-07-10 06:32:04 PDT
WebKit Review Bot
Comment 2 2012-07-10 06:37:41 PDT
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 3 2012-08-17 07:40:44 PDT
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
Comment 4 2012-08-19 02:05:37 PDT
(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
Comment 5 2012-08-19 02:24:24 PDT
Eric Carlson
Comment 6 2012-08-19 10:10:15 PDT
WebKit Review Bot
Comment 7 2012-08-19 11:27:36 PDT
Re-opened since this is blocked by 94431
Carlos Garcia Campos
Comment 8 2012-08-20 01:26:58 PDT
Created attachment 159360 [details] Updated patch to fix the mac build Try to fix mac build.
Carlos Garcia Campos
Comment 9 2012-08-20 03:21:19 PDT
(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
Comment 10 2012-08-20 03:27:44 PDT
Note You need to log in before you can comment on or make changes to this bug.