Issue 4871895371481088: Convert region code to uppercase when generating translation file URL on Safari (Closed)
|
Created:
April 10, 2014, 7:40 a.m. by Sebastian Noack Modified:
April 12, 2014, 8:32 a.m. Reviewers:
Wladimir Palant Visibility:
Public. |
DescriptionApparently navigator.language, which we rely on to generate the path for the translation file, is always lowercase on Safari. However Chrome translation files use upper case for the region code. Hence the translations pt_PT, pt_BR, zh_CN, zh_TW and en_GB are currently never used on Safari.
Patch Set 1 #
Total comments: 4
Patch Set 2 : Addressed comments #
Total comments: 4
Patch Set 3 : Added comment regarding dropping the remaining components #
Total comments: 2
Patch Set 4 : Corrected wrong assumptions in comment #MessagesTotal messages: 10
http://codereview.adblockplus.org/4871895371481088/diff/5629499534213120/safa... File safari/ext/common.js (right): http://codereview.adblockplus.org/4871895371481088/diff/5629499534213120/safa... safari/ext/common.js:90: var region = match[2]; It seems that you are still trying to split the string, the following should be more obvious: var bits = navigator.language.split("-", 2); var language = bits[0]; var region = bits.length > 1 ? bits[1] : null; http://codereview.adblockplus.org/4871895371481088/diff/5629499534213120/safa... safari/ext/common.js:96: candidates.splice(candidates.indexOf(defaultLocale) + 1); I had some trouble understanding the purpose of this. So if navigator.language is "en-us" we will return ["en_US"] rather than ["en_US", "en"]? That's inconsistent (and - yes, it looks like the original code got it wrong as well). I'd suggest not trying to be clever, the following code will produce consistent results and also happens to be obvious: if (candidates[0] != defaultLocale) candidates.push(defaultLocale);
http://codereview.adblockplus.org/4871895371481088/diff/5629499534213120/safa... File safari/ext/common.js (right): http://codereview.adblockplus.org/4871895371481088/diff/5629499534213120/safa... safari/ext/common.js:90: var region = match[2]; On 2014/04/10 08:05:34, Wladimir Palant wrote: > It seems that you are still trying to split the string, the following should be > more obvious: > > var bits = navigator.language.split("-", 2); > var language = bits[0]; > var region = bits.length > 1 ? bits[1] : null; Done, but I used ES6 array unpacking for better readability. http://codereview.adblockplus.org/4871895371481088/diff/5629499534213120/safa... safari/ext/common.js:96: candidates.splice(candidates.indexOf(defaultLocale) + 1); On 2014/04/10 08:05:34, Wladimir Palant wrote: > I had some trouble understanding the purpose of this. So if navigator.language > is "en-us" we will return ["en_US"] rather than ["en_US", "en"]? That's > inconsistent (and - yes, it looks like the original code got it wrong as well). > > I'd suggest not trying to be clever, the following code will produce consistent > results and also happens to be obvious: > > if (candidates[0] != defaultLocale) > candidates.push(defaultLocale); Done, but I used candidates.indexOf(defaultLocale) instead. So the code desn't assume that defaultLocale includes a region suffix.
LGTM with the remaining comment addressed. http://codereview.adblockplus.org/4871895371481088/diff/5724160613416960/safa... File safari/ext/common.js (right): http://codereview.adblockplus.org/4871895371481088/diff/5724160613416960/safa... safari/ext/common.js:87: var [language, region] = navigator.language.split("-"); My code was using split("-", 2) - did you remove the second parameter intentionally? I can see arguments both for and against keeping it, but at the very least you should comment that the third component in navigator.language will be dropped if present.
http://codereview.adblockplus.org/4871895371481088/diff/5724160613416960/safa... File safari/ext/common.js (right): http://codereview.adblockplus.org/4871895371481088/diff/5724160613416960/safa... safari/ext/common.js:87: var [language, region] = navigator.language.split("-"); On 2014/04/10 09:18:48, Wladimir Palant wrote: > My code was using split("-", 2) - did you remove the second parameter > intentionally? I can see arguments both for and against keeping it, but at the > very least you should comment that the third component in navigator.language > will be dropped if present. The third component would be dropped anyway, since "foo-bar-baz".split("-", 2) is basically the same as "foo-bar-baz".split("-").slice(0, 2) in JavaScript. That's why I initially preferred a regex, to don't silently drop the remaining components.
http://codereview.adblockplus.org/4871895371481088/diff/5724160613416960/safa... File safari/ext/common.js (right): http://codereview.adblockplus.org/4871895371481088/diff/5724160613416960/safa... safari/ext/common.js:87: var [language, region] = navigator.language.split("-"); On 2014/04/10 09:55:22, Sebastian Noack wrote: > The third component would be dropped anyway, since "foo-bar-baz".split("-", 2) > is basically the same as "foo-bar-baz".split("-").slice(0, 2) in JavaScript. You are right, I got spoiled by Python. So we would actually do region = bits.slice(1).join("_") here. Not that it really makes sense however given that neither Safari nor Chrome support languages with three components IMHO - and you would actually need to add multiple candidates here in order to fall back to ab_XY format. So it makes sense to drop the third component, just with a comment.
http://codereview.adblockplus.org/4871895371481088/diff/5724160613416960/safa... File safari/ext/common.js (right): http://codereview.adblockplus.org/4871895371481088/diff/5724160613416960/safa... safari/ext/common.js:87: var [language, region] = navigator.language.split("-"); On 2014/04/10 10:02:09, Wladimir Palant wrote: > On 2014/04/10 09:55:22, Sebastian Noack wrote: > > The third component would be dropped anyway, since "foo-bar-baz".split("-", 2) > > is basically the same as "foo-bar-baz".split("-").slice(0, 2) in JavaScript. > > You are right, I got spoiled by Python. So we would actually do region = > bits.slice(1).join("_") here. Not that it really makes sense however given that > neither Safari nor Chrome support languages with three components IMHO - and you > would actually need to add multiple candidates here in order to fall back to > ab_XY format. So it makes sense to drop the third component, just with a > comment. Done.
LGTM with the nit addressed. http://codereview.adblockplus.org/4871895371481088/diff/5657382461898752/safa... File safari/ext/common.js (right): http://codereview.adblockplus.org/4871895371481088/diff/5657382461898752/safa... safari/ext/common.js:89: // after the second dash will be dropped, but that case doesn't exist Nit: use "ja-jp-mac" as example? That case does exist, we just don't want to handle it specially. // Note: for languages like "ja-jp-mac" consisting of three parts we drop the third part and only consider language and region codes.
http://codereview.adblockplus.org/4871895371481088/diff/5657382461898752/safa... File safari/ext/common.js (right): http://codereview.adblockplus.org/4871895371481088/diff/5657382461898752/safa... safari/ext/common.js:89: // after the second dash will be dropped, but that case doesn't exist On 2014/04/10 10:17:48, Wladimir Palant wrote: > Nit: use "ja-jp-mac" as example? That case does exist, we just don't want to > handle it specially. > > // Note: for languages like "ja-jp-mac" consisting of three parts we drop the > third part and only consider language and region codes. Done.
Still LGTM (really wasn't necessary to submit this for review once again). 
|
