After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 602547 - Can't load correctly the language of a webpage (Debian)
Can't load correctly the language of a webpage (Debian)
Status: RESOLVED FIXED
Product: epiphany
Classification: Core
Component: Backend
2.28.x
Other Linux
: Normal normal
: ---
Assigned To: Xan Lopez
Epiphany Maintainers
Depends on:
Blocks:
 
 
Reported: 2009-11-20 23:48 UTC by Gabriel
Modified: 2010-08-18 19:38 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
identica language, Epiphany and Arora (108.92 KB, image/png)
2009-11-20 23:48 UTC, Gabriel
  Details
Patch proposal (for epiphany) (2.62 KB, patch)
2010-03-18 17:11 UTC, Mario Sánchez Prada
needs-work Details | Review
Patch proposal (2.71 KB, patch)
2010-03-20 22:32 UTC, Mario Sánchez Prada
accepted-commit_now Details | Review
Same patch using locale-independent functions (2.29 KB, patch)
2010-07-03 00:29 UTC, Josselin Mouette
committed Details | Review

Description Gabriel 2009-11-20 23:48:44 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
Comment 1 Xan Lopez 2010-02-12 13:09:59 UTC
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.
Comment 2 Gustavo Noronha (kov) 2010-02-18 17:50:59 UTC
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.
Comment 3 Mario Sánchez Prada 2010-03-18 15:06:24 UTC
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?
Comment 4 Mario Sánchez Prada 2010-03-18 17:11:13 UTC
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.
Comment 5 Gustavo Noronha (kov) 2010-03-19 14:44:38 UTC
(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.
Comment 6 Gustavo Noronha (kov) 2010-03-19 14:50:34 UTC
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.
Comment 7 Gustavo Noronha (kov) 2010-03-19 14:51:04 UTC
Would be good to have danw look at this patch, as well.
Comment 8 Dan Winship 2010-03-19 16:32:25 UTC
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. :)
Comment 9 Mario Sánchez Prada 2010-03-19 21:05:20 UTC
(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.
Comment 10 Mario Sánchez Prada 2010-03-20 22:32:09 UTC
Created attachment 156641 [details] [review]
Patch proposal

Second version of the patch addressing the issues pointed out by Gustavo at comment #6.
Comment 11 Gustavo Noronha (kov) 2010-03-24 23:36:58 UTC
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...
Comment 12 Gustavo Noronha (kov) 2010-03-24 23:38:05 UTC
Review of attachment 156641 [details] [review]:

Actually, we're in hard code freeze, right?
Comment 13 Mario Sánchez Prada 2010-03-25 08:20:02 UTC
(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 14 André Klapper 2010-05-18 21:03:49 UTC
Comment on attachment 156641 [details] [review]
Patch proposal

Hardcode freeze is over, hence restoring patch status.
Comment 15 Tommi Vainikainen 2010-06-30 07:55:55 UTC
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.
Comment 16 Mario Sánchez Prada 2010-06-30 09:14:59 UTC
(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! :-)
Comment 17 Josselin Mouette 2010-07-02 23:28:10 UTC
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.
Comment 18 Josselin Mouette 2010-07-02 23:50:01 UTC
(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".
Comment 19 Josselin Mouette 2010-07-03 00:29:38 UTC
Created attachment 165157 [details] [review]
Same patch using locale-independent functions
Comment 20 Gustavo Noronha (kov) 2010-07-16 13:57:13 UTC
Review of attachment 165157 [details] [review]:

I'm pro landing this.
Comment 21 Mario Sánchez Prada 2010-08-10 08:31:55 UTC
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 22 Gustavo Noronha (kov) 2010-08-10 17:11:37 UTC
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 =)
Comment 23 Mario Sánchez Prada 2010-08-10 20:31:58 UTC
(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 :-)
Comment 24 Josselin Mouette 2010-08-16 16:49:17 UTC
Committed to master as 626d441.
Comment 25 Diego Escalante Urrelo (not reading bugmail) 2010-08-18 19:38:48 UTC
Closing as fixed and marking Josseling's patch as committed.