[EFL] Change format of return value of navigator.language

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Kihong Kwon]

window.navigator.language returns a value like en-US.UTF-8 on the EFL port.
But, all other ports and browsers returns a value like en-US.
I think it is better to change format of navigator.language like xx-XX like other ports(gtk, chromium).

[Kihong Kwon]

Created attachment 148742 [details]
Patch.

[Gyuyoung Kim]

Comment on attachment 148742 [details]
Patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=148742&action=review

> Source/WebCore/ChangeLog:11
> +        But, it has been moved to under platform/qt(navigator-language.html).

I can't agree to change language locale without clear reason. If you wanna support this test, you just add navigator-language-expected.txt for EFL port.

[Kihong Kwon]

(In reply to comment #2)
> (From update of attachment 148742 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=148742&action=review
> 
> > Source/WebCore/ChangeLog:11
> > +        But, it has been moved to under platform/qt(navigator-language.html).
> 
> I can't agree to change language locale without clear reason. If you wanna support this test, you just add navigator-language-expected.txt for EFL port.

I meant navigator-language.html has been moved to platform/qt.
Therefore we can't test this by adding only navigator-language-expected.txt.
We need to make a same test case in the platform/efl.
How do you think about this?

[Gyuyoung Kim]

(In reply to comment #3)

> I meant navigator-language.html has been moved to platform/qt.
> Therefore we can't test this by adding only navigator-language-expected.txt.
> We need to make a same test case in the platform/efl.
> How do you think about this?

If there is clear reason to change locale value, I agree to land this patch. But, if not, I think we just add navigator-language.html and -expected.txt to platform/efl like qt port.

[Kihong Kwon]

(In reply to comment #4)
> (In reply to comment #3)
> 
> > I meant navigator-language.html has been moved to platform/qt.
> > Therefore we can't test this by adding only navigator-language-expected.txt.
> > We need to make a same test case in the platform/efl.
> > How do you think about this?
> 
> If there is clear reason to change locale value, I agree to land this patch. But, if not, I think we just add navigator-language.html and -expected.txt to platform/efl like qt port.

(In reply to comment #4)
> (In reply to comment #3)
> 
> > I meant navigator-language.html has been moved to platform/qt.
> > Therefore we can't test this by adding only navigator-language-expected.txt.
> > We need to make a same test case in the platform/efl.
> > How do you think about this?
> 
> If there is clear reason to change locale value, I agree to land this patch. But, if not, I think we just add navigator-language.html and -expected.txt to platform/efl like qt port.

It is a problem only EFL returns locale like xx-XX.UTF8.(Some web developer could not care about this kind of locale value.), and removing '.UTF8' from locale value is already applied to the GTK.
I think this change is as simple as you can check by seeing once.
That's why I didn't add a test case.
But, if you think really need to add a test case, that's not a problem~:)

[Gyuyoung Kim]

(In reply to comment #5)

> It is a problem only EFL returns locale like xx-XX.UTF8.(Some web developer could not care about this kind of locale value.), and removing '.UTF8' from locale value is already applied to the GTK.
> I think this change is as simple as you can check by seeing once.
> That's why I didn't add a test case.
> But, if you think really need to add a test case, that's not a problem~:)

I already checked GTK port locale value. It looks this can be decided by port. If  many web developer doesn't want to use .XXX value, I agree to land this. However, you should add test case like qt port.

[Kihong Kwon]

Created attachment 148958 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 148958 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=148958&action=review

> Source/WebCore/platform/efl/LanguageEfl.cpp:46
> +    return String(localeDefault).replace('_', '-').left(String(localeDefault).find('.'));

I'm a little confused to manipulate string in a line. I think it is good to use new string variable. See also, blackberry port.
http://trac.webkit.org/browser/trunk/Source/WebCore/platform/blackberry/LocalizedStringsBlackBerry.cpp#L63

[Kihong Kwon]

Created attachment 149154 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 149154 [details]
Patch

Looks good to me now.

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 149154 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=149154&action=review

> Source/WebCore/ChangeLog:8
> +        Change format of return value of navigator.language from xx-XX.UTF-8 to xx-XX.

You need to explain why you are doing this. Please also note that 'UTF-8' is not hardcoded, it just happens to be the encoding you are using on your system. A more generic description would be along the lines of "Remove the encoding from the language returned by navigator.language".

The `language' attribute of the navigator object does not seem to be part of any standard, so different browsers seem to return different values anyway. Do you know if "en-US" is currently more common than "en", for example?

> Source/WebCore/ChangeLog:9
> +        Need a short description and bug URL (OOPS!)

Wrong line.

> Source/WebCore/ChangeLog:11
> +        Test: platform/efl/fast/js/navigator-language.html

I object to copying this test from Qt; it was moved to platform/qt in r113892 because it apparently tested something which was relevant only to that port.

[Kihong Kwon]

(In reply to comment #11)
> (From update of attachment 149154 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=149154&action=review
> 
> > Source/WebCore/ChangeLog:8
> > +        Change format of return value of navigator.language from xx-XX.UTF-8 to xx-XX.
> 
> You need to explain why you are doing this. Please also note that 'UTF-8' is not hardcoded, it just happens to be the encoding you are using on your system. A more generic description would be along the lines of "Remove the encoding from the language returned by navigator.language".
> 
> The `language' attribute of the navigator object does not seem to be part of any standard, so different browsers seem to return different values anyway. Do you know if "en-US" is currently more common than "en", for example?
> 
I think there is no correct answer for this now.
But there is no browser displaying encoding with language.
IMHO, It is better to remove encoding.
xx(en) : Chrome(win/mac), Opera(all).
xx-XX(en-US) : Firefox(all), Chrome(linux), safari(win).
xx-xx(en-us) : safari(mac).

> > Source/WebCore/ChangeLog:9
> > +        Need a short description and bug URL (OOPS!)
> 
> Wrong line.
OK.

> 
> > Source/WebCore/ChangeLog:11
> > +        Test: platform/efl/fast/js/navigator-language.html
> 
> I object to copying this test from Qt; it was moved to platform/qt in r113892 because it apparently tested something which was relevant only to that port.

Did you mean we don't need to add this test case to the EFL?

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 149154 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=149154&action=review

>>> Source/WebCore/ChangeLog:8
>>> +        Change format of return value of navigator.language from xx-XX.UTF-8 to xx-XX.
>> 
>> You need to explain why you are doing this. Please also note that 'UTF-8' is not hardcoded, it just happens to be the encoding you are using on your system. A more generic description would be along the lines of "Remove the encoding from the language returned by navigator.language".
>> 
>> The `language' attribute of the navigator object does not seem to be part of any standard, so different browsers seem to return different values anyway. Do you know if "en-US" is currently more common than "en", for example?
> 
> I think there is no correct answer for this now.
> But there is no browser displaying encoding with language.
> IMHO, It is better to remove encoding.
> xx(en) : Chrome(win/mac), Opera(all).
> xx-XX(en-US) : Firefox(all), Chrome(linux), safari(win).
> xx-xx(en-us) : safari(mac).

OK thanks, that's what I asked. In this case, it makes more sense to return "en-US" instead of just "en" indeed.

>>> Source/WebCore/ChangeLog:11
>>> +        Test: platform/efl/fast/js/navigator-language.html
>> 
>> I object to copying this test from Qt; it was moved to platform/qt in r113892 because it apparently tested something which was relevant only to that port.
> 
> Did you mean we don't need to add this test case to the EFL?

Yes, I don't like keeping two almost identical copies of the same test in the tree; if the test also applies to ports other than Qt, it should be moved back to fast/js with a proper explanation (and perhaps skipped in ports where it does not make sense), otherwise another, specific test should be written.

> Source/WebCore/platform/efl/LanguageEfl.cpp:47
> +    size_t position = locale.find('-');

Shouldn't you be looking for '_' here?

[Kihong Kwon]

(In reply to comment #13)
> (From update of attachment 149154 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=149154&action=review
> 
> >>> Source/WebCore/ChangeLog:8
> >>> +        Change format of return value of navigator.language from xx-XX.UTF-8 to xx-XX.
> >> 
> >> You need to explain why you are doing this. Please also note that 'UTF-8' is not hardcoded, it just happens to be the encoding you are using on your system. A more generic description would be along the lines of "Remove the encoding from the language returned by navigator.language".
> >> 
> >> The `language' attribute of the navigator object does not seem to be part of any standard, so different browsers seem to return different values anyway. Do you know if "en-US" is currently more common than "en", for example?
> > 
> > I think there is no correct answer for this now.
> > But there is no browser displaying encoding with language.
> > IMHO, It is better to remove encoding.
> > xx(en) : Chrome(win/mac), Opera(all).
> > xx-XX(en-US) : Firefox(all), Chrome(linux), safari(win).
> > xx-xx(en-us) : safari(mac).
> 
> OK thanks, that's what I asked. In this case, it makes more sense to return "en-US" instead of just "en" indeed.
> 
> >>> Source/WebCore/ChangeLog:11
> >>> +        Test: platform/efl/fast/js/navigator-language.html
> >> 
> >> I object to copying this test from Qt; it was moved to platform/qt in r113892 because it apparently tested something which was relevant only to that port.
> > 
> > Did you mean we don't need to add this test case to the EFL?
> 
> Yes, I don't like keeping two almost identical copies of the same test in the tree; if the test also applies to ports other than Qt, it should be moved back to fast/js with a proper explanation (and perhaps skipped in ports where it does not make sense), otherwise another, specific test should be written.

Basically, you are absolutely right.
But, in this case, return value of navigator.language is different in each ports.
For example, chromium-win port never pass this test case because of difference of return value.
Therefore, I think it is better test case adds under platform/efl.
(I can simplify my test case but it is not much different with qt because this is very simple patch and test.(Actually I don't think this test is really needed.)
How do you think about this?

> 
> > Source/WebCore/platform/efl/LanguageEfl.cpp:47
> > +    size_t position = locale.find('-');
> 
> Shouldn't you be looking for '_' here?
OK. It was my mistack. I will fix it.

[Kihong Kwon]

Created attachment 149467 [details]
Patch

[Chris Dumez]

Comment on attachment 149467 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=149467&action=review

> Source/WebCore/platform/efl/LanguageEfl.cpp:47
> +    size_t position = locale.find('_');

Do we really need this find()? Can't we simply call locale.replace('_', '-'); directly?

[Kihong Kwon]

(In reply to comment #16)
> (From update of attachment 149467 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=149467&action=review
> 
> > Source/WebCore/platform/efl/LanguageEfl.cpp:47
> > +    size_t position = locale.find('_');
> 
> Do we really need this find()? Can't we simply call locale.replace('_', '-'); directly?

Yes, we can. I will remove that statement~  ;-)

[Raphael Kubo da Costa (:rakuco)]

(In reply to comment #14)
> (In reply to comment #13)
> > Yes, I don't like keeping two almost identical copies of the same test in the tree; if the test also applies to ports other than Qt, it should be moved back to fast/js with a proper explanation (and perhaps skipped in ports where it does not make sense), otherwise another, specific test should be written.
> 
> Basically, you are absolutely right.
> But, in this case, return value of navigator.language is different in each ports.
> For example, chromium-win port never pass this test case because of difference of return value.
> Therefore, I think it is better test case adds under platform/efl.
> (I can simplify my test case but it is not much different with qt because this is very simple patch and test.(Actually I don't think this test is really needed.)
> How do you think about this?

I still think that if Qt's test is still relevant for other ports, it's better to move it back to the main LayoutTests directory to prevent needless duplication of tests (you can talk to the people who added the tests and/or to the ones who moved it to platform/qt before that as well to confirm that this makes sense).

[Kihong Kwon]

Created attachment 150151 [details]
Patch

[Raphael Kubo da Costa (:rakuco)]

Comment on attachment 150151 [details]
Patch

Looks fine to me.

[Gyuyoung Kim]

LGTM too.

[Chris Dumez]

Skipping fast/js/navigator-language.html in Bug 90365. Please unskip the test with your fix.

[Kihong Kwon]

(In reply to comment #22)
> Skipping fast/js/navigator-language.html in Bug 90365. Please unskip the test with your fix.

OK, I will do that before landing.

[Kihong Kwon]

Created attachment 150548 [details]
Patch

[Chris Dumez]

Comment on attachment 150548 [details]
Patch

LGTM. Thanks.

[WebKit Review Bot]

Comment on attachment 150548 [details]
Patch

Clearing flags on attachment: 150548

Committed r121833: <http://trac.webkit.org/changeset/121833>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.