281107 - Google defaults to non-native language

Status: VERIFIED FIXED

(Camino Graveyard :: Toolbars & Menus, defect)

Description

[Kurt Weinschenker]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8b) Gecko/20050203 Camino/0.8+
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8b) Gecko/20050203 Camino/0.8+

When recent nightly builds of Camino are launched for the first time, and a
search term is typed in the Google search term box, Camino heads for the
*Danish* Google site.  My native language is English (US).

New users of Camino may not be amused.

Reproducible: Always

Steps to Reproduce:
1.Launch recent Camino nightly build as downloaded from mozilla.org.
2.Type search term into Google search box in bar.
3.Camino will bring up the Danish Google site.

Actual Results:  
Camino went to the Danish Google site.

Expected Results:  
Camino should go to the default Mac OS X language site.  In my case, that should
be the US English site.

Mac OS 10.3.7.  Previous nightly builds did not display this problem.  It might
be a regression bug.

Comment 1

[Mike Pinkerton (not reading bugmail)]

on what date did it start happening?

Comment 2

[Hiro]

It is displayed that the language is set with preferences in the language of 
hope. 
http://www.google.com/preferences?hl=en

INVALID ?

Comment 3

[Wevah (Nate Weaver)]

This looks like another case of language names being saved with their English
names instead of their ISO codes.

Comment 4

[Kurt Weinschenker]

Mike, I believe it happened in the last two weeks or so.  I tried a nightly
build several days ago, got this bug, and waited a day or two to see if perhaps
something had happened on a nightly.  This behavior, though, seems to have crept
in sometime during January, possibly in the last two weeks of the month.

I try to test nightly builds when I have some free time, but sometimes I don't
get the chance to test nightly builds every night.  Hope that makes sense!  

And thank you Mike, for your work on Camino.  It's a wonderful little browser.

Comment 5

[Bruce Davidson]

This is almost certainly a result of my patches fixing bug 161337 (landed on
31/01/2005) and bug 280814 (landed 03/02/2005). The problem occurs on systems
where AppleLanguages contains English names (e.g. "Dutch") rather than ISO codes
("nl-nl").

At the moment we ignore strings we don't understand when we're building the
accept-language header. This means if your first few languages in the list are
in the (duff) format but later ones aren't, the first language we'll put in the
accept-language header is actually one a long way down your list.

I'm looking at a patch to work around this (either by converting the English
names to ISO codes, or by simply not setting the pref if your first language
isn't valid).

Comment 6

[Bruce Davidson]

Attachment: Proposed patch

This patch ensures that we don't set accept-languages using information from
the system at all if your primary language isn't a valid ISO code. We output a
console message about this to help in identifying the scope of the problem.

Comment 7

[Bruce Davidson]

Attachment: Patch with updated comment

Updated patch adding more detail to comment, per IRC discussion with pink.

Comment 8

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 173564 [details] [diff] [review]
Patch with updated comment

sr=pink

mark this bug as blocking the 083 bug as well

Comment 9

[Asaf Romano (gone)]

Comment on attachment 173564 [details] [diff] [review]
Patch with updated comment

if the array / string is empty, you should set it to "en-us, en", not to an
empty string (fyi, that is the default in seamonkey/ff)

Comment 10

[Bruce Davidson]

Attachment: Updated patch

Following discussion on IRC we're going to fail if any language can't be
understood. This is much easier to help users with than seemingly partial
language support. Also explicitely state our emergency fallback in
all-camino.js.

Comment 11

[Bruce Davidson]

Attachment: Tweak comment and logic

Tweak comments to keep mano happy :-) Change the final if slightly to make the
logic clearer and avoid queries over behaviour of componentsJoinedByString on
empty arrays.

Comment 12

[Asaf Romano (gone)]

Comment on attachment 173655 [details] [diff] [review]
Tweak comment and logic

>       for (unsigned long i = 0; i < [languages count]; ++i) {
>         NSString* language = [PreferenceManager convertLocaleToHTTPLanguage:[languages objectAtIndex:i]];
>         if (language)
>           [acceptableLanguages addObject:language];
>+        else {
...
>+          NSLog( @"Unable to set languages - language '%@' not a valid ISO language identifier", [languages objectAtIndex:i] );
>+          [acceptableLanguages removeAllObjects];
>+          break;
>+        }
>       }

[OPTIONAL] as mentioned on irc, the loop here has a pretty bad design, you're
not supposed to handle the whole array in one round; extra |break|s are not
really good either; Something like the following would be better IMO:

BOOL langsDetectSucces = YES;
...
for (unsigned long i = 0; i < [languages count] && langsDetectSucces; ++i) {
  if (language)
    [acceptableLanguages addObject:language];
  else {
   ...log...
    langsDetectSucces = NO;
  }
}

and then, instead of |if ([acceptLangHeader length] > 0)|, you can do a cleaner
check: |if (langsDetectSucces)|

As mentioned, optional; but readable code is a good thing ;)


>+      // If we understood all the languages in the list set the accept-language header.
>+      // Note that necko will determine quality factors itself
>+      if ([acceptableLanguages count] > 0) {

you may want to mention the "en-us, en" fallback here.

>+pref("intl.accept_languages", "en-us, en" );

please remove extra spcae ;)

otherwise, r=[e-mail].

Comment 13

[Bruce Davidson]

Attachment: Extra space removed

Comment 14

[Mike Pinkerton (not reading bugmail)]

bruce, i know you're going to kill me, but after looking at the code i agree
with mano about the loop. Rather than a break and clearing out the array, just
set a bool and check it as a loop condition and on the |if| before actually
setting the pref. It's cleaner overall, and faster in the fail case.

sorry!!!!!!!!! looks good otherwise.

Comment 15

[Bruce Davidson]

Attachment: Patch addressing pinkerton's comments, and the bit of mano's that I missed

Comment 16

[Asaf Romano (gone)]

Comment on attachment 173758 [details] [diff] [review]
Patch addressing pinkerton's comments, and the bit of mano's that I missed

r=mano

Comment 17

[Mike Pinkerton (not reading bugmail)]

+      if (languagesOkaySoFar && [acceptableLanguages count] > 0) {

nit. is there ever a case where |languagesOkaySoFar| is true but count is 0?

Comment 18

[Asaf Romano (gone)]

(In reply to comment #17)
> +      if (languagesOkaySoFar && [acceptableLanguages count] > 0) {
> 
> nit. is there ever a case where |languagesOkaySoFar| is true but count is 0?

When the language list is empty.

Comment 19

[Asaf Romano (gone)]

Comment on attachment 173758 [details] [diff] [review]
Patch addressing pinkerton's comments, and the bit of mano's that I missed


Bruce:

>+      BOOL languagesOkaySoFar = YES;

please change this to:

>+      BOOL languagesOkaySoFar = [languages count] != 0;

and this one:
>+      if (languagesOkaySoFar && [acceptableLanguages count] > 0) {

to
>+      if (languagesOkaySoFar) {

Comment 20

[Asaf Romano (gone)]

(
BOOL languagesOkaySoFar = [languages count] != 0;
  if you ar wondering why, this expalins "we're note ok with an empty list, fail
to..."
)

Comment 21

[Asaf Romano (gone)]

comment 18 was wrong, there's no way to remove the last language, at least not
on 10.3 (this may or may not make my last two comments invalid... you could
probably just remove the |langauges > 0| check in the pref set part...

Comment 22

[Bruce Davidson]

I don't think we should assume that Apple have sanitised the language list (or
that the user hasn't screwed with the XML) just to save ourselves a few bytes.

I don't mind whether we move the zero length check to above the array (as per
comment 19) or not, though personally I think the current form is more readable.
It avoids overloading the flag to mean "are all languages we've seen valid" and
"have we seen enough languages" at the same time. The overall amount of machine
code and clocks cycles to execute is identical between the two.

Comment 23

[Mike Pinkerton (not reading bugmail)]

landed on trunk and branch

Comment 24

[Bruce Davidson]

*** Bug 283233 has been marked as a duplicate of this bug. ***

Comment 25

[sairuh (rarely reading bugmail)]

when I use camino's search bar it's going to the en-us site for me. verified
fixed using these camino builds on mac os x 10.3.8:

2005031008-trunk
20050212-0.8.3 branch

changing "fixed1.7.6" to "verified1.7.6" since I tested using a camino branch build.

Comment 26

[sairuh (rarely reading bugmail)]

mistakenly removed fixed1.7.6 --pardon the bugspam. set your filter/quicksearch
to "ZippidityDooDahHey" to catch these for easy removal/etc/