Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(497)

Issue 29338190: Issue 3697 - Fall back to i18n.getUILanguage if @ui_locale isn't supported (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
3 years, 10 months ago by Oleksandr
Modified:
2 years, 4 months ago
Reviewers:
kzar, Sebastian Noack
Visibility:
Public.

Description

Issue 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 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+16 lines, -3 lines) Patch
M dependencies View 1 2 3 4 5 1 chunk +1 line, -1 line 0 comments Download
M lib/notificationHelper.js View 1 2 3 4 1 chunk +1 line, -1 line 0 comments Download
M lib/utils.js View 1 2 3 4 5 6 7 1 chunk +14 lines, -1 line 0 comments Download

Messages

Total messages: 30
Oleksandr
3 years, 10 months ago (2016-03-14 07:58:07 UTC) #1
Sebastian Noack
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 ...
3 years, 10 months ago (2016-03-14 09:03:07 UTC) #2
Oleksandr
I wasn't able to test on Safari, since I need a developer certificate. But I ...
3 years, 10 months ago (2016-03-17 06:56:03 UTC) #3
Sebastian Noack
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 ...
3 years, 10 months ago (2016-03-17 11:31:56 UTC) #4
Sebastian Noack
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. ...
3 years, 10 months ago (2016-03-17 11:36:11 UTC) #5
Oleksandr
3 years, 9 months ago (2016-04-11 16:42:35 UTC) #6
Sebastian Noack
Sorry for the delay. Looks mostly good to me. However, besides those nits, we also ...
3 years, 8 months ago (2016-05-12 22:14:30 UTC) #7
Oleksandr
3 years, 7 months ago (2016-05-29 00:25:06 UTC) #8
Sebastian Noack
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 ...
2 years, 4 months ago (2017-08-23 12:26:45 UTC) #9
Oleksandr
2 years, 4 months ago (2017-08-26 00:03:19 UTC) #10
Sebastian Noack
You also have to change messageResponder.js in adblockplusui, to use Utils.readingDirection instead of @@bidi_dir. Then ...
2 years, 4 months ago (2017-08-26 07:16:47 UTC) #11
Oleksandr
On 2017/08/26 07:16:47, Sebastian Noack wrote: > You also have to change messageResponder.js in adblockplusui, ...
2 years, 4 months ago (2017-08-28 17:00:19 UTC) #12
Sebastian Noack
On 2017/08/28 17:00:19, Oleksandr wrote: > On 2017/08/26 07:16:47, Sebastian Noack wrote: > > You ...
2 years, 4 months ago (2017-08-29 12:54:06 UTC) #13
Oleksandr
2 years, 4 months ago (2017-08-29 13:36:04 UTC) #14
kzar
On 2017/08/29 13:36:04, Oleksandr wrote: I notice in the issue we mention needing to fall ...
2 years, 4 months ago (2017-08-29 13:44:05 UTC) #15
Sebastian Noack
On 2017/08/29 13:44:05, kzar wrote: > I notice in the issue we mention needing to ...
2 years, 4 months ago (2017-08-29 14:26:48 UTC) #16
kzar
On 2017/08/29 14:26:48, Sebastian Noack wrote: > On 2017/08/29 13:44:05, kzar wrote: > > I ...
2 years, 4 months ago (2017-08-29 14:40:40 UTC) #17
Sebastian Noack
LGTM (as-is or with the caching removed)
2 years, 4 months ago (2017-08-29 16:12:24 UTC) #18
kzar
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 ...
2 years, 4 months ago (2017-08-29 16:17:26 UTC) #19
Sebastian Noack
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 ...
2 years, 4 months ago (2017-08-29 16:47:46 UTC) #20
Sebastian Noack
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() ...
2 years, 4 months ago (2017-08-30 00:22:12 UTC) #21
Oleksandr
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 ...
2 years, 4 months ago (2017-08-30 09:19:12 UTC) #22
kzar
On 2017/08/30 09:19:12, Oleksandr wrote: > I have also updated the ticket to list the ...
2 years, 4 months ago (2017-08-30 10:16:09 UTC) #23
Oleksandr
On 2017/08/30 10:16:09, kzar wrote: > Thanks, but the issue description still needs some work. ...
2 years, 4 months ago (2017-08-30 11:26:52 UTC) #24
kzar
> Updated the issue description with a similar text. Also > added hints for testers. ...
2 years, 4 months ago (2017-08-30 11:54:10 UTC) #25
kzar
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: ...
2 years, 4 months ago (2017-08-30 13:55:03 UTC) #26
Sebastian Noack
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: ...
2 years, 4 months ago (2017-08-30 16:28:24 UTC) #27
Oleksandr
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: > ...
2 years, 4 months ago (2017-08-30 23:00:36 UTC) #28
Sebastian Noack
LGTM
2 years, 4 months ago (2017-08-31 00:12:53 UTC) #29
kzar
2 years, 4 months ago (2017-08-31 11:20:46 UTC) #30
LGTM
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 87257f5