Issue 29338190: Issue 3697 - Fall back to i18n.getUILanguage if @ui_locale isn't supported (Closed)
|
Created:
March 14, 2016, 7:54 a.m. by Oleksandr Modified:
Aug. 31, 2017, 2:58 p.m. Visibility:
Public. |
DescriptionIssue 3697 - Fall back to i18n.getUILanguage if @ui_locale isn't supported
Note: It would probably make sense to push the change to adblockplusui to use Utils.readingDirection now, if this is accepted.
Patch Set 1 #
Total comments: 3
Patch Set 2 : Remove Safari redundancy and Firefox specific codepath #
Total comments: 3
Patch Set 3 : Address comments. #
Total comments: 2
Patch Set 4 : Add try-catch blocks. Address nits. #
Total comments: 1
Patch Set 5 : Rebase and just use i18n.getUILanguage() #
Total comments: 1
Patch Set 6 : Update the adblockplusui dependency. Add a comment. Change regexp. #
Total comments: 4
Patch Set 7 : Remove redundant underscore replacement #
Total comments: 9
Patch Set 8 : Address the nits #
MessagesTotal messages: 30
https://codereview.adblockplus.org/29338190/diff/29338191/lib/utils.js File lib/utils.js (right): https://codereview.adblockplus.org/29338190/diff/29338191/lib/utils.js#newcode48 lib/utils.js:48: if (locale === "") { Nit: We always use == (instead ===) where possible. This is inline with Mozilla's JavaScript coding style. https://codereview.adblockplus.org/29338190/diff/29338191/lib/utils.js#newcode59 lib/utils.js:59: direction = this.chromeRegistry.isLocaleRTL("adblockplus") ? "rtl" : "ltr"; This code path is specific to Adblock Plus for Firefox. The code here however is only used on Chrome, Opera, Safari and Edge. So you can remove this branch. However, you have to add Utils.readingDirection to the lib/utils.js in Adblock Plus for Firefox as well. https://codereview.adblockplus.org/29338190/diff/29338191/lib/utils.js#newcode64 lib/utils.js:64: direction = /^(ar|fa|he|ug|ur)(_|$)/.test(appLocale) ? "rtl" : "ltr"; This logic is redundant with code in safari/ext/common.js which can be removed now.
I wasn't able to test on Safari, since I need a developer certificate. But I think it should work fine. The Firefox change should be a separate review, I assume.
https://codereview.adblockplus.org/29338190/diff/29338477/lib/utils.js File lib/utils.js (right): https://codereview.adblockplus.org/29338190/diff/29338477/lib/utils.js#newcode59 lib/utils.js:59: direction = /^(ar|fa|he|ug|ur)(_|$)/.test(appLocale) ? "rtl" : "ltr"; Where does the variable appLocale come from? Shouldn't it be this.appLocale? https://codereview.adblockplus.org/29338190/diff/29338477/safari/ext/common.js File safari/ext/common.js (right): https://codereview.adblockplus.org/29338190/diff/29338477/safari/ext/common.j... safari/ext/common.js:129: var bidiDir = Utils.readingDirection(); The idea was to not support @bidi_dir here. But simply use Utils.readingDirection in the calling code.
https://codereview.adblockplus.org/29338190/diff/29338477/lib/utils.js File lib/utils.js (right): https://codereview.adblockplus.org/29338190/diff/29338477/lib/utils.js#newcode57 lib/utils.js:57: var direction = ext.i18n.getMessage("@bidi_dir"); There is an @ missing. It's supposed to be @@bidi_dir.
Sorry for the delay. Looks mostly good to me. However, besides those nits, we also have to adapt the calling code in adblockplusui prior to landing this. https://codereview.adblockplus.org/29338190/diff/29339647/lib/utils.js File lib/utils.js (right): https://codereview.adblockplus.org/29338190/diff/29339647/lib/utils.js#newcode58 lib/utils.js:58: if (direction == "") { Nit: Braces aren't required according to our coding practices in the case here. https://codereview.adblockplus.org/29338190/diff/29339647/lib/utils.js#newcode58 lib/utils.js:58: if (direction == "") { Nit: I would rather check for !direction, this reads a little easier and automatically handles null and undefined along empty strings.
This patch is still relevant, right? It also needs rebasing. https://codereview.adblockplus.org/29338190/diff/29345225/lib/utils.js File lib/utils.js (right): https://codereview.adblockplus.org/29338190/diff/29345225/lib/utils.js#newcode57 lib/utils.js:57: locale = ext.i18n.getUILanguage(); It seems we can use chrome.i18n.getUILanguage() regardless, without checking for getMessage("@@ui_locale") first, since all supported browsers and browser versions seem to support it.
You also have to change messageResponder.js in adblockplusui, to use Utils.readingDirection instead of @@bidi_dir. Then this patch should include the corresponding dependency update for adblockplusui. https://codereview.adblockplus.org/29338190/diff/29527886/lib/utils.js File lib/utils.js (right): https://codereview.adblockplus.org/29338190/diff/29527886/lib/utils.js#newcode53 lib/utils.js:53: if (!direction) We probably should add a comment that this fallback is only necessary for Edge.
On 2017/08/26 07:16:47, Sebastian Noack wrote: > You also have to change messageResponder.js in adblockplusui, to use > Utils.readingDirection instead of @@bidi_dir. Then this patch should include the > corresponding dependency update for adblockplusui. There already is a long hanging codereview for that here: https://codereview.adblockplus.org/29345232/
On 2017/08/28 17:00:19, Oleksandr wrote: > On 2017/08/26 07:16:47, Sebastian Noack wrote: > > You also have to change messageResponder.js in adblockplusui, to use > > Utils.readingDirection instead of @@bidi_dir. Then this patch should include > the > > corresponding dependency update for adblockplusui. > > There already is a long hanging codereview for that here: > https://codereview.adblockplus.org/29345232/ That change just landed. Feel free to update this patch with the dependency update, now.
On 2017/08/29 13:36:04, Oleksandr wrote:
I notice in the issue we mention needing to fall back to
chrome.i18n.getMessage("@ui_locale") for versions of Chrome before 35, but we
don't support versions that old any more. So please could you update the issue
description? Also I wonder if your changes here could be simplified as well, or
if you already took that into account?
On 2017/08/29 13:44:05, kzar wrote:
> I notice in the issue we mention needing to fall back to
> chrome.i18n.getMessage("@ui_locale") for versions of Chrome before 35, but we
> don't support versions that old any more. So please could you update the issue
> description?
I updated the issue.
> Also I wonder if your changes here could be simplified as well, or
> if you already took that into account?
Do you have any particular simplification in mind? The only thing I could think
of would be not caching the reading direction in the Utils object. It is
probably not worth it, on the other hand, it is consistent with other properties
of the Utils object. Either way, fine with me.
On 2017/08/29 14:26:48, Sebastian Noack wrote:
> On 2017/08/29 13:44:05, kzar wrote:
> > I notice in the issue we mention needing to fall back to
> > chrome.i18n.getMessage("@ui_locale") for versions of Chrome before 35, but
we
> > don't support versions that old any more. So please could you update the
issue
> > description?
>
> I updated the issue.
Thanks.
> > Also I wonder if your changes here could be simplified as well, or
> > if you already took that into account?
>
> Do you have any particular simplification in mind? The only thing I could
think
> of would be not caching the reading direction in the Utils object. It is
> probably not worth it, on the other hand, it is consistent with other
properties
> of the Utils object. Either way, fine with me.
Well I didn't review this code, so no. But I just figured if the implementation
followed the issue description then the logic to fall back to
chrome.i18n.getMessage("@ui_locale") could now be removed.
LGTM (as-is or with the caching removed)
https://codereview.adblockplus.org/29338190/diff/29530576/dependencies File dependencies (right): https://codereview.adblockplus.org/29338190/diff/29530576/dependencies#newcode6 dependencies:6: adblockplusui = adblockplusui hg:104a61b1949d git:e691a7d This includes a whole bunch of unrelated changes but they're not listed in the issue. Have you checked through them all to make sure nothing else is being broken / needs to be re-tested by QA?
https://codereview.adblockplus.org/29338190/diff/29530576/dependencies File dependencies (right): https://codereview.adblockplus.org/29338190/diff/29530576/dependencies#newcode6 dependencies:6: adblockplusui = adblockplusui hg:104a61b1949d git:e691a7d On 2017/08/29 16:17:24, kzar wrote: > This includes a whole bunch of unrelated changes but they're not listed in the > issue. Have you checked through them all to make sure nothing else is being > broken / needs to be re-tested by QA? The vast majority of these changes only effect the new options page. When comparing the build after this update, the only unrelated changes I see are setLinks() being moved from firstRun.js to common.js, some whitespace changes, and the changes to the copyright header.
https://codereview.adblockplus.org/29338190/diff/29530576/lib/utils.js File lib/utils.js (right): https://codereview.adblockplus.org/29338190/diff/29530576/lib/utils.js#newcode46 lib/utils.js:46: let locale = ext.i18n.getUILanguage().replace(/_/g, "-"); I just noticed, getUILanguage() as opposed to getMessage("@@ui_locale") returns the locale with dashes instead of underscores already. So the replacement is redundant here.
https://codereview.adblockplus.org/29338190/diff/29530576/dependencies File dependencies (right): https://codereview.adblockplus.org/29338190/diff/29530576/dependencies#newcode6 dependencies:6: adblockplusui = adblockplusui hg:104a61b1949d git:e691a7d On 2017/08/29 16:47:46, Sebastian Noack wrote: > On 2017/08/29 16:17:24, kzar wrote: > > This includes a whole bunch of unrelated changes but they're not listed in the > > issue. Have you checked through them all to make sure nothing else is being > > broken / needs to be re-tested by QA? > > The vast majority of these changes only effect the new options page. When > comparing the build after this update, the only unrelated changes I see are > setLinks() being moved from firstRun.js to common.js, some whitespace changes, > and the changes to the copyright header. I have also updated the ticket to list the imported changes. As Sebastian said, most of the work done in adblockplusui seems to be on the new-options page. We can probably add a hint to testers to test if all the links work, but other than that this import is a minor one.
On 2017/08/30 09:19:12, Oleksandr wrote: > I have also updated the ticket to list the imported changes. As Sebastian said, > most of the work done in adblockplusui seems to be on the new-options page. We > can probably add a hint to testers to test if all the links work, but other than > that this import is a minor one. Thanks, but the issue description still needs some work. The idea is to explain what the other imported changes are, mentioning what functionality could be effected by them. That's what the testers need to know. It's up to you if you list all the other irrelevant changes, but one of the only relevant changes (the setLinks one) isn't listed yet. Here's an example of how it might look: """ Many other changes are imported by this dependency update, but they mostly relate to the new options page and therefore are irrelevant. That said there has been a small change to how the links on the first run page are setup: - https://hg.adblockplus.org/adblockplusui/rev/461ee88245c7 === Hints for testers === - Other stuff... - Please ensure that the links on the first run page still all work. """
On 2017/08/30 10:16:09, kzar wrote: > Thanks, but the issue description still needs some work. The idea is to explain > what the other imported changes are, mentioning what functionality could be > effected by them. That's what the testers need to know. It's up to you if you > list all the other irrelevant changes, but one of the only relevant changes (the > setLinks one) isn't listed yet. Here's an example of how it might look: > > """ > Many other changes are imported by this dependency update, but they mostly > relate to the new options page and therefore are irrelevant. That said there has > been a small change to how the links on the first run page are setup: > > - https://hg.adblockplus.org/adblockplusui/rev/461ee88245c7 > > === Hints for testers === > - Other stuff... > - Please ensure that the links on the first run page still all work. > """ Updated the issue description with a similar text. Also added hints for testers.
> Updated the issue description with a similar text. Also > added hints for testers. Thanks it was looking pretty good and I finished it off for you. Just to check, did you run ESLint on these changes? https://codereview.adblockplus.org/29338190/diff/29531592/lib/utils.js File lib/utils.js (right): https://codereview.adblockplus.org/29338190/diff/29531592/lib/utils.js#newcode52 lib/utils.js:52: let direction = ext.i18n.getMessage("@@bidi_dir"); Shouldn't we also make use of this.readingDirection here? Otherwise I figure we cache it but then never use the cached value. https://codereview.adblockplus.org/29338190/diff/29531592/lib/utils.js#newcode53 lib/utils.js:53: if (!direction) Nit: Please use braces since it spans multiple lines with the comment. https://codereview.adblockplus.org/29338190/diff/29531592/lib/utils.js#newcode55 lib/utils.js:55: direction = /^(?:ar|fa|he|ug|ur)\b/.test(this.appLocale) ? "rtl" : "ltr"; (I assume Sebastian checked this regexp, I have no idea if it's correct.) https://codereview.adblockplus.org/29338190/diff/29531592/lib/utils.js#newcode59 lib/utils.js:59: {value: direction, enumerable: true}); Nit: Please put the closing parenthesis on the following line - same indentation as the start of "Object" above.
https://codereview.adblockplus.org/29338190/diff/29531592/lib/utils.js File lib/utils.js (right): https://codereview.adblockplus.org/29338190/diff/29531592/lib/utils.js#newcode52 lib/utils.js:52: let direction = ext.i18n.getMessage("@@bidi_dir"); On 2017/08/30 11:54:09, kzar wrote: > Shouldn't we also make use of this.readingDirection here? Otherwise I figure we > cache it but then never use the cached value. Argh, disregard that! I see how this works now, we replace the getter with the value.
https://codereview.adblockplus.org/29338190/diff/29531592/lib/utils.js File lib/utils.js (right): https://codereview.adblockplus.org/29338190/diff/29531592/lib/utils.js#newcode53 lib/utils.js:53: if (!direction) On 2017/08/30 11:54:09, kzar wrote: > Nit: Please use braces since it spans multiple lines with the comment. I'd rather just move the comment above. https://codereview.adblockplus.org/29338190/diff/29531592/lib/utils.js#newcode55 lib/utils.js:55: direction = /^(?:ar|fa|he|ug|ur)\b/.test(this.appLocale) ? "rtl" : "ltr"; On 2017/08/30 11:54:09, kzar wrote: > (I assume Sebastian checked this regexp, I have no idea if it's correct.) For reference, this is the regular expression we use on Safari: /^(ar|fa|he|ug|ur)(_|$)/ Using \b (end of word) instead of _|$ is correct, as we have dashes instead of underscores here. However, I didn't check whether this covers all right-to-left languages supported by Windows.
https://codereview.adblockplus.org/29338190/diff/29531592/lib/utils.js File lib/utils.js (right): https://codereview.adblockplus.org/29338190/diff/29531592/lib/utils.js#newcode53 lib/utils.js:53: if (!direction) On 2017/08/30 16:28:24, Sebastian Noack wrote: > On 2017/08/30 11:54:09, kzar wrote: > > Nit: Please use braces since it spans multiple lines with the comment. > > I'd rather just move the comment above. Done. https://codereview.adblockplus.org/29338190/diff/29531592/lib/utils.js#newcode55 lib/utils.js:55: direction = /^(?:ar|fa|he|ug|ur)\b/.test(this.appLocale) ? "rtl" : "ltr"; On 2017/08/30 16:28:24, Sebastian Noack wrote: > On 2017/08/30 11:54:09, kzar wrote: > > (I assume Sebastian checked this regexp, I have no idea if it's correct.) > > For reference, this is the regular expression we use on Safari: > > /^(ar|fa|he|ug|ur)(_|$)/ > > Using \b (end of word) instead of _|$ is correct, as we have dashes instead of > underscores here. However, I didn't check whether this covers all right-to-left > languages supported by Windows. Based on the list here: https://msdn.microsoft.com/en-us/library/aa919356.aspx?f=255&MSPPError=-21472... it looks like we are handling even more languages that Windows supports. So this should be fine.
LGTM
LGTM 
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
