New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 819369 link

Starred by 4 users

Issue metadata

Status: Fixed
Owner:
Closed: Mar 2018
Cc:
Components:
EstimatedDays: ----
NextAction: 2018-03-12
OS: Mac
Pri: 1
Type: Bug-Regression



Sign in to add a comment

Locale override broke for Mac

Project Member Reported by sky@chromium.org, Mar 6 2018

Issue description

https://chromium-review.googlesource.com/c/chromium/src/+/882558 broke overriding the locale on Mac. Specifically 882558 reordered things such that ResourceBundle is created before l10n_util::OverrideLocaleWithCocoaLocale() . 882558 changes the ordering so that l10n_util::OverrideLocaleWithCocoaLocale() is called before ResourceBundle is created. It looks like OverrideLocaleWithCocoaLocale sets global (g_overridden_locale), which is used by ResourceBundle's GetApplicationLocale (used during creation). So, if we don't call OverrideLocaleWithCocoaLocale() before creating the ResourceBundle the override-locale is ignored. It's not entirely clear to me what environments this impacts.
 
Steps to reproduce the problem are here:
https://chromium-review.googlesource.com/c/chromium/src/+/882558/23/chrome/browser/chrome_browser_main.cc#689
Specifically I've tested this on the russian system locale

Comment 2 by sky@chromium.org, Mar 7 2018

Which is to switch the OS to a non-English locale, right? This would mean anyone not running US would see the US locale, right? Again, I'm surprised we haven't seen more reports.

Comment 3 by sky@chromium.org, Mar 7 2018

Are overriding the locale on the command line?

Comment 4 by sky@chromium.org, Mar 7 2018

Cc: a...@chromium.org
Labels: -OS-Chrome OS-Mac
There's no special setup that I'm aware of.

Some more specifics, just in case:
MacOS High Sierra (10.13.3)
The preferred system language is Russian:
https://drive.google.com/file/d/1z6BubPJxehPSjKDc6sOT8dvPCIN3-5JL/view?usp=sharing

Currently chromium starts like this:
https://drive.google.com/file/d/1EWOHMay3Len5b2sq9U8xKYXwPQ_Sl2qT/view?usp=sharing

Previous versions (no bug) started like this:
https://drive.google.com/file/d/1agRxo3PWcNeBYAzke6D3TxQzNNR54jPF/view?usp=sharing

I'm doing nothing on the command line, just starting Chromium.app without arguments

Comment 6 by sky@chromium.org, Mar 8 2018

Thanks for the details!
Cc: linds...@chromium.org sky@chromium.org
Owner: manoranj...@chromium.org
Status: Assigned (was: Started)
Hi Mano, 
can you ptal at this change? The manual test check here would be for testing on mac canary with system language other than english to verify if the strings shown to the user in various ui surfaces (menus, settings...) match the system locale.
Thanks,
Cc: ellyjo...@chromium.org gov...@chromium.org
Components: UI>Localization
Labels: -Type-Bug -Pri-2 Target-67 RegressedIn-66 M-66 ReleaseBlock-Beta FoundIn-66 Target-66 Pri-1 Type-Bug-Regression
Owner: sky@chromium.org
This seems to be regressed in M66 and below is the bisect result:

Good# 65.0.3325.146
Bad# 66.0.3355.0

https://chromium.googlesource.com/chromium/src/+log/382f6d5b5ca966fcc9913054cb8d6fd535d7d83c..f55c1c3b1ecb51b4ff6e7fd3775b87924b0234d2

sky@, can you please look into this change (https://chromium.googlesource.com/chromium/src/+/875789eaa469e365ac6fb7ac069f4ca6c0676cee) ?

Thank you!

Comment 9 by sky@chromium.org, Mar 9 2018

Thanks for verifying this. Fix is here: https://chromium-review.googlesource.com/c/chromium/src/+/951708 . I will land now and ask for merge after it bakes for a couple of days.
Project Member

Comment 10 by bugdroid1@chromium.org, Mar 9 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/740c2633585839e76fdc5d22964046b71d93dd8c

commit 740c2633585839e76fdc5d22964046b71d93dd8c
Author: Scott Violet <sky@chromium.org>
Date: Fri Mar 09 04:11:42 2018

mac: change initialization so locale set before local state

This likely leads to using the wrong locale for non-english systems.

BUG= 819369 
TEST=none

Change-Id: Ie432f038567f077ab10a18257b4b89c8a1a3282e
Reviewed-on: https://chromium-review.googlesource.com/951708
Reviewed-by: Avi Drissman <avi@chromium.org>
Commit-Queue: Scott Violet <sky@chromium.org>
Cr-Commit-Position: refs/heads/master@{#542018}
[modify] https://crrev.com/740c2633585839e76fdc5d22964046b71d93dd8c/chrome/browser/chrome_browser_main_mac.mm

NextAction: 2018-03-12
Pls request a merge to M66 on Monday morning once change is baked in canary so we can pick it up for next week beta release. Thank you.

Comment 12 Deleted

M66 Beta promotion is coming VERY soon next week. Your bug is labelled as Beta ReleaseBlock, pls make sure to land the fix and request a merge into the release branch latest by 5:00 PM PT Monday, 03/12. Thank you.

Comment 14 by sky@chromium.org, Mar 9 2018

The patch just landed on trunk. Do you want me to request merge now?
No, pls request a merge on Monday morning after canary verification. Thank you.
Components: -UI>Localization UI>Browser
Labels: Needs-TestConfirmation
Hi,

This is not localization issue. Removing UI>Localization.
I will be looping in the Engineering Team for review.

Regards!
Labels: -Needs-TestConfirmation
- 'Needs-TestConfirmation'. 'chrome-te' has already provided the bisect in c#8.
The NextAction date has arrived: 2018-03-12
Labels: TE-Verified-M67 TE-Verified-67.0.3368.0
Tested this issue on Mac 10.13.3 using chrome Canary-67.0.3368.0 as per C#0 & C#5.

steps:
-----
1. Set mac system language to Russian
2. Restart system
3. Now launch chrome

Observed that chrome launched in Russian language & its options also in Russian language only.As it is working as intended adding TE Verified labels.

Please find the attached screencast for reference.

Thanks..!


819369-Mac.mp4
2.9 MB View Download

Comment 20 by sky@chromium.org, Mar 12 2018

Labels: Merge-Request-66
Requesting merge for very visible user facing regression.

Comment 21 by ajha@chromium.org, Mar 13 2018

Cc: abdulsyed@chromium.org
M-66 will be promoted to Beta in 2 days time and we need to get this merged to M-66. This is already verified in M-67, looping abdul@ for merge-approval as per C#20.

Comment 22 by sdy@chromium.org, Mar 13 2018

Cc: lgrey@chromium.org
 Issue 820537  has been merged into this issue.
Project Member

Comment 23 by sheriffbot@chromium.org, Mar 13 2018

Labels: -Merge-Request-66 Merge-Approved-66 Hotlist-Merge-Approved
Your change meets the bar and is auto-approved for M66. Please go ahead and merge the CL to branch 3359 manually. Please contact milestone owner if you have questions.
Owners: cmasso@(Android), cmasso@(iOS), josafat@(ChromeOS), abdulsyed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Project Member

Comment 24 by bugdroid1@chromium.org, Mar 13 2018

Labels: -merge-approved-66 merge-merged-3359
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a13716b3397eb82d78a74176410320841a921237

commit a13716b3397eb82d78a74176410320841a921237
Author: Scott Violet <sky@chromium.org>
Date: Tue Mar 13 17:49:39 2018

merge: mac: change initialization so locale set before local state

This likely leads to using the wrong locale for non-english systems.

BUG= 819369 
TEST=none
TBR=avi@chromium.org

Change-Id: Ie432f038567f077ab10a18257b4b89c8a1a3282e
Reviewed-on: https://chromium-review.googlesource.com/951708
Reviewed-by: Avi Drissman <avi@chromium.org>
Commit-Queue: Scott Violet <sky@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#542018}(cherry picked from commit 740c2633585839e76fdc5d22964046b71d93dd8c)
Reviewed-on: https://chromium-review.googlesource.com/960938
Reviewed-by: Scott Violet <sky@chromium.org>
Cr-Commit-Position: refs/branch-heads/3359@{#210}
Cr-Branched-From: 66afc5e5d10127546cc4b98b9117aff588b5e66b-refs/heads/master@{#540276}
[modify] https://crrev.com/a13716b3397eb82d78a74176410320841a921237/chrome/browser/chrome_browser_main_mac.mm

Comment 25 by sky@chromium.org, Mar 13 2018

Status: Fixed (was: Assigned)
Special thanks for Alex for identifying this and letting me know!
Labels: TE-Verified-M66 TE-Verified-66.0.3359.33
Tested this issue on Mac OS 10.13.1 Pro on the bad build 66.0.3355.0 and latest M-66 build 66.0.3359.33
Able to reproduce this issue on 66.0.3355.0 and the issue is fixed on the latest M-66 build 66.0.3359.33

On changing the system language to Russian, can see the options in Chrome in Russian language.
Attached is the screen shot for reference.

Hence adding TE verified labels as the fix is working as intended.

Thanks..
819369-M66.png
43.2 KB View Download

Sign in to add a comment