Port bug 1356263 and bug 1313625 to C-C: Remove use of nsILocaleService/getApplicationLocale() - tweaks after removal of nsIScriptableDateFormat

RESOLVED FIXED in Thunderbird 57.0

Status

Thunderbird
General
P3
normal
RESOLVED FIXED
7 months ago
3 months ago

People

(Reporter: Jorg K [Almost not working on Thunderbird (some bustage-fix only) due to non-renewal of contract], Assigned: Jorg K [Almost not working on Thunderbird (some bustage-fix only) due to non-renewal of contract])

Tracking

52 Branch
Thunderbird 57.0
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

+++ This bug was initially created as a clone of Bug #1356263 +++

See bug 1356263 comment #3:

Uses of nsILocaleService in C-C:
https://dxr.mozilla.org/comm-central/search?q=getApplicationLocale
https://dxr.mozilla.org/comm-central/search?q=getSystemLocale

Comment 1

7 months ago
From bug 1356263 comment 4:
> https://dxr.mozilla.org/comm-central/search?q=getApplicationLocale
You seem to only have one use of this in StringBundle.js. You should switch to Services.locale.getAppLocaleAsLangTag.

> https://dxr.mozilla.org/comm-central/search?q=getSystemLocale
You seem to really only have one use of this in navigator.js - you should switch it to mozIOSPreferences systemLocale attribute.

Aryx, would like to take this?
Flags: needinfo?(aryx.bugmail)
Jorg, this is becoming a bit more urgent as with the 57 window opening, I'd like to start working on bug 1356263.

It seems to me after a brief search, that Tb has no uses of the APIs left (the webspeech is also m-c, and I'll remove it in the patch in bug 1356263).

Can you confirm that it's ok for me to start working on removing nsILocaleService, nsLocaleService, nsILocale and nsLocale?
Flags: needinfo?(jorgk)

Comment 3

4 months ago
(In reply to :aceman from comment #1)
> From bug 1356263 comment 4:
> > https://dxr.mozilla.org/comm-central/search?q=getApplicationLocale
> You seem to only have one use of this in StringBundle.js. You should switch
> to Services.locale.getAppLocaleAsLangTag.

mailnews/base/util/StringBundle.js is unnecessary to use nsILocaleService.  Services.strings.createBundle has only 1 parameter.
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #2)
> Can you confirm that it's ok for me to start working on removing
> nsILocaleService, nsLocaleService, nsILocale and nsLocale?
Sure, go for it, I'm here to fix things if they break in TB.
Flags: needinfo?(jorgk)
Flags: needinfo?(aryx.bugmail)
Created attachment 8893707 [details] [diff] [review]
1357822-nsILocaleService.patch

Bug 1356263 is on autoland, so we need to hurry.

FRG, I'm not going to do the SM part here:
https://dxr.mozilla.org/comm-central/source/suite/browser/navigator.js#2629
Assignee: nobody → jorgk
Status: NEW → ASSIGNED
Flags: needinfo?(frgrahl)
Attachment #8893707 - Flags: review?(acelists)
> FRG, I'm not going to do the SM part here:

Thanks for the heads up. Need to take care of some other things too. Keeping the needinfo to not forget this one.
Bug 1356263 had landed and it's causing C++ bustage in nsMsgDBView.h:
  static nsDateFormatSelector  m_dateFormatDefault;
  static nsDateFormatSelector  m_dateFormatThisWeek;
  static nsDateFormatSelector  m_dateFormatToday;

I'll look into it.
nsDateFormatSelector is still there, it just moved here
https://hg.mozilla.org/mozilla-central/rev/afc4e2d4eb96#l3.24
in bug 1313625: https://hg.mozilla.org/mozilla-central/rev/afc4e2d4eb96

Comment 9

4 months ago
Comment on attachment 8893707 [details] [diff] [review]
1357822-nsILocaleService.patch

Review of attachment 8893707 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks.
Attachment #8893707 - Flags: review?(acelists) → review+
OK, all we need to do is add mozilla::

Patch coming.
Summary: Remove use of nsLocaleService, nsILocaleService, nsLocale, and nsPosixLocale from TB → Port bug 1356263 and bug 1313625 to C-C: Remove use of nsILocaleService/getApplicationLocale() - tweaks after removal of nsIScriptableDateFormat
Created attachment 8893955 [details] [diff] [review]
1357822-nsILocaleService.patch (v2)

Sorry for lumping to ports into the same bug. This compiles so far.
Attachment #8893707 - Attachment is obsolete: true

Comment 12

4 months ago
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/4b99bbec6414
Port bug 1356263: remove nsILocaleService/use of getApplicationLocale(); Port bug 1313625: add namespace to date/time format. r=aceman
Status: ASSIGNED → RESOLVED
Last Resolved: 4 months ago
Resolution: --- → FIXED
Second and third bustage of the day, times are getting tough :-(
Target Milestone: --- → Thunderbird 57.0
Sorry to hear that Jorg!

It may come as a consolation to you that this is probably the end of Locale related changes for quite a while! We have the new API ready and complete with all features and old APIs removed. :)

Thank you so much for your help with getting it working for Tb and Sm!
... and FF, you forgot: Bug 1383463, bug 1351427, bug 1355465.

One personal comment: Why doesn't anyone fix bug 1355977? That's ten lines at most and it's ugly.
Jorg: I didn't! I just meant in this bug for the removal of the old APIs. You've helped a lot during the whole process. Thank you:)

> Why doesn't anyone fix bug 1355977? That's ten lines at most and it's ugly.

Lower priority than other things I guess.
Blocks: 1387699

Updated

3 months ago
Attachment #8893955 - Flags: review+
Flags: needinfo?(frgrahl)
You need to log in before you can comment on or make changes to this bug.