GNOME Bugzilla – Bug 602547
Can't load correctly the language of a webpage (Debian)
Last modified: 2010-08-18 19:38:57 UTC
Created attachment 148199 [details] identica language, Epiphany and Arora In the image can see the diferent language in the identi.ca website with the same user. Top ------> Epiphany Bottom --> Arora
This should work now since we implement correctly the Accept-Language HTTP header. Try with 2.29.95 or later and reopen if you see any issue.
As discussed with msanchez in #webkit-gtk, this is still a problem. You can check by going to debian.org, as well. The problem is our header does not include q=x.x values for each language, so they are treated the same, as far as priority goes, according to danw.
Yes, libsoup will only take care of inserting the quality values in the list of languages when automatically getting them from the system (through accept_languages_from_system() in SoupSession) but not when the client app using libsoup explicitly specifies a "Accept-Language" string to be used in the session (and this is the case with epiphany) One approach to fix this issue could be to make it so that libsoup could add those quality values to the accept-language string when being set in the session in those cases they were not specified by the app itself. This way, whenever a application just sent something like "en, es, it" it would be internally translated inside libsoup to something like "en, es;q=0.9, it;q=0.8" Other option would be to set the quality values right in epiphany, which would mean to duplicate some code already present in libsoup but that would have the advantage not to make the library to assume anything when receiving an string as "en, es, it". What do you guys think?
Created attachment 156490 [details] [review] Patch proposal (for epiphany) Attaching a patch proposal for epiphany, which basically makes sure the quality values are added along with each language as specified in the preferences dialog. The problem with this patch is that it duplicates some code already present in SoupSession, but perhaps it's better to keep the library indepedent.
(In reply to comment #3) > Other option would be to set the quality values right in epiphany, which would > mean to duplicate some code already present in libsoup but that would have the > advantage not to make the library to assume anything when receiving an string > as "en, es, it". I think that is the only sane option. Making libsoup try to guess the browser's intention is bound to break some usage scenarios.
Review of attachment 156490 [details] [review]: The idea is the best one IMO, but I think there's a bit of going back and forward in the code. ::: embed/ephy-embed-prefs.c @@ +374,3 @@ ephy_langs_sanitise (array); langs = (char **) g_array_free (array, FALSE); You are freeing this array here, but then the first thing you do inside that function is to create an array, shouldn't you reuse this array instead? @@ +376,2 @@ langs = (char **) g_array_free (array, FALSE); + langs_str = accept_languages_set_quality_values ((const gchar**) langs); I think this function name is misleading. I think the name should imply what it does, instead: it builds the accept language header string with quality values from an ordered list of languages, so build_accept_languages_header sounds better to my ears.
Would be good to have danw look at this patch, as well.
the guts of the patch are mostly just copied from the libsoup code, so it all looks right. of course, if it turns out the libsoup code is wrong then we have to fix it in two places, but... it's probably not wrong at this point. :)
(In reply to comment #6) > Review of attachment 156490 [details] [review]: > > The idea is the best one IMO, but I think there's a bit of going back and > forward in the code. > > ::: embed/ephy-embed-prefs.c > @@ +374,3 @@ > ephy_langs_sanitise (array); > > langs = (char **) g_array_free (array, FALSE); > > You are freeing this array here, but then the first thing you do inside that > function is to create an array, shouldn't you reuse this array instead? Sure. I didn't bother too much about it since I was more interested in the proposal itself than in it acceptance at this stage. But obviously I agree it's better to reuase the GArray in here. > @@ +376,2 @@ > langs = (char **) g_array_free (array, FALSE); > + langs_str = accept_languages_set_quality_values ((const gchar**) langs); > > I think this function name is misleading. I think the name should imply what it > does, instead: it builds the accept language header string with quality values > from an ordered list of languages, so build_accept_languages_header sounds > better to my ears. I'm terrible with function names, sorry, and yours sounds better as well to my ears so yes, "I buy it to you" :-) Today I'm no longer at home nor in the office so I'll come up with a better patch on Monday, unless I find some time on the weekend, but I can swear on it :-) Thanks for reviewing.
Created attachment 156641 [details] [review] Patch proposal Second version of the patch addressing the issues pointed out by Gustavo at comment #6.
Review of attachment 156641 [details] [review]: OK, let's go ahead with it =) ::: embed/ephy-embed-prefs.c @@ +369,3 @@ + /* Free memory */ + array_data = (char **) g_array_free (array, FALSE); + g_strfreev (array_data); Is the two-step free really necessary? I assume so...
Review of attachment 156641 [details] [review]: Actually, we're in hard code freeze, right?
(In reply to comment #11) > Review of attachment 156641 [details] [review]: > > OK, let's go ahead with it =) > > ::: embed/ephy-embed-prefs.c > @@ +369,3 @@ > + /* Free memory */ > + array_data = (char **) g_array_free (array, FALSE); > + g_strfreev (array_data); > > Is the two-step free really necessary? I assume so... Well... I need to free both the char** and every char* so I think it's more practical not to free it along with g_array_free and then use g_strfreev for the data.
Comment on attachment 156641 [details] [review] Patch proposal Hardcode freeze is over, hence restoring patch status.
Maybe it is now a good time to commit this. I've tested this patch locally against 2.30.2 build, and it works fine.
(In reply to comment #15) > Maybe it is now a good time to commit this. I've tested this patch locally > against 2.30.2 build, and it works fine. I vote for it... and I'm not biased at all, I swear! :-)
The patch is incorrect. When the locale setting is not C, the locale’s setting will be prepended to the list, without a q setting. This leads to a funny behavior, where loading the browser in French locale displays the website in English, while loading it in English displays the website in French.
(In reply to comment #17) > The patch is incorrect. When the locale setting is not C, the locale’s setting > will be prepended to the list, without a q setting. This is of course not the problem since the RFC allows this. The real problem is that g_strdup_printf obeys to LC_NUMERIC and will add e.g. "fr;0,9" instead of "fr;0.9".
Created attachment 165157 [details] [review] Same patch using locale-independent functions
Review of attachment 165157 [details] [review]: I'm pro landing this.
Is there anything pending on this topic? I think it would be nice to get this patch landed for the next release. Pinging just in case O:-)
Comment on attachment 165157 [details] [review] Same patch using locale-independent functions None in my view, so I'll tag it accepted, please go ahead =)
(In reply to comment #22) > (From update of attachment 165157 [details] [review]) > None in my view, so I'll tag it accepted, please go ahead =) Just in case, pointing out that I have no commit permissions yet, so someone else (but not me) would have to commit this :-)
Committed to master as 626d441.
Closing as fixed and marking Josseling's patch as committed.