If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Port bug 1356263 to SeaMonkey: Remove use of nsILocaleService/getApplicationLocale()

RESOLVED FIXED in seamonkey2.54

Status

SeaMonkey
General
P3
normal
RESOLVED FIXED
2 months ago
a month ago

People

(Reporter: frg, Assigned: frg)

Tracking

Trunk
seamonkey2.54

SeaMonkey Tracking Flags

(seamonkey2.54 fixed)

Details

Attachments

(3 attachments)

(Assignee)

Description

2 months ago
+++ This bug was initially created as a clone of Bug #1357822 +++

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

See bug 1356263 comment #3:

Only one use of nsILocaleService left in suite:
https://dxr.mozilla.org/comm-central/search?q=getSystemLocale
(Assignee)

Comment 1

2 months ago
Created attachment 8894069 [details] [diff] [review]
1387699-getSystemLocale.patch

Basically the Fx version with a few tweaks. Comments later. Still need to test it.
Assignee: nobody → frgrahl
Status: NEW → ASSIGNED

Comment 2

2 months ago
Comment on attachment 8894069 [details] [diff] [review]
1387699-getSystemLocale.patch

Looks nice.
(Assignee)

Comment 3

2 months ago
Created attachment 8894107 [details] [diff] [review]
1387699-bidiTB.patch

Jorg,

do you want a separate bug for this? This should be about the same spot as in Fx and SM but might need to be moved one statement down. Need to test it first.
Attachment #8894107 - Flags: feedback?(jorgk)

Comment 4

2 months ago
Comment on attachment 8894107 [details] [diff] [review]
1387699-bidiTB.patch

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

Thanks, but I'm a little confused.

What actually happens if BiDi is returned true, then the code controlled by |if (gShowBiDi)| in mail/base/content/utilityOverlay.js is executed, so goUpdateCommand('cmd_switchTextDirection');

What consequences does that have?

::: mail/base/content/mailWindow.js
@@ +172,5 @@
>              .getService(Components.interfaces.nsIActivityManager)
>              .addListener(window.MsgStatusFeedback);
>  
> +  // Initialze BiDi
> +  gShowBiDi = isBidiEnabled();

But var gShowBiDi = false; is in mail/base/content/utilityOverlay.js. That variable is visible here?

::: mail/base/content/utilityOverlay.js
@@ +323,5 @@
>    let url = Services.prefs.getCharPref(kTelemetryInfoUrl);
>    openContentTab(url, where, "^http://www.mozilla.org/");
>  }
> +
> +function getBoolPrefWithDefault(prefname, def) {

Why is that not in the other file?
(Assignee)

Comment 5

2 months ago
> goUpdateCommand('cmd_switchTextDirection');
> What consequences does that have?

This is an interesting question :) Personally I think right to left writing will be enabled. I am baking a TB right now and just want to set it in en-US to see what happens.

> But var gShowBiDi = false; is in mail/base/content/utilityOverlay.js. That variable is visible here?

Should be. Same as Fx and SM. Need to test it first. One of the reasons I didn't set r? yet.

> +function getBoolPrefWithDefault(prefname, def) {

> Why is that not in the other file?

Fx has getBoolPref in utilityoverlay.js. I took an immediate dislike to the name because there it is the same as Services.pref an makes searching via dxr harder. So just a personal preference to name it differently.

Comment 6

2 months ago
Created attachment 8894124 [details] [diff] [review]
1387699-bidiTB.patch - JK

I think we can do without the hunk in utilityOverlay.js.

Setting the BiDi variable does nothing as far as I can see.

Comment 7

a month ago
Comment on attachment 8894107 [details] [diff] [review]
1387699-bidiTB.patch

I gave my feedback by attaching a slightly different patch. We still need to check what that BiDi stuff does.
Attachment #8894107 - Flags: feedback?(jorgk)
(Assignee)

Comment 8

a month ago
Comment on attachment 8894069 [details] [diff] [review]
1387699-getSystemLocale.patch

Tested and works on non bidi system. The pref is never reset but you would need to manually move profiles/language to another machine to notice it. I think that is why Fx did it this way.
Attachment #8894069 - Flags: review?(iann_bugzilla)
(Assignee)

Comment 9

a month ago
Jorg, I tested and also didn't notice a difference. Should I scratch the TB bidi stuff from my radar for now or move it to another bug? This needs probably input from someone who is more familiar with bidi and can actually test it.
Flags: needinfo?(jorgk)

Comment 10

a month ago
Yes, thanks, we pick up the Bidi stuff if we ever need to.
Flags: needinfo?(jorgk)

Comment 11

a month ago
Comment on attachment 8894069 [details] [diff] [review]
1387699-getSystemLocale.patch

>+++ b/suite/common/utilityOverlay.js

>+function getBoolPrefWithDefault(prefname, def) {
>+  try {
>+    return Services.prefs.getBoolPref(prefname);
>+  } catch (er) {
>+    return def;
>+  }
>+}

There is already a function in utilityOverlay.js called GetBoolPref which does this, please use that.
https://dxr.mozilla.org/comm-central/source/suite/common/utilityOverlay.js#1362

r=me with that fixed.
Attachment #8894069 - Flags: review?(iann_bugzilla) → review+

Comment 12

a month ago
Pushed by frgrahl@gmx.net:
https://hg.mozilla.org/comm-central/rev/f43fd1c1c293
Port bug 1356263 to SeaMonkey. Replace getSystemLocale with mozIntl call. r=IanN
Status: ASSIGNED → RESOLVED
Last Resolved: a month ago
Resolution: --- → FIXED
(Assignee)

Comment 13

a month ago
> There is already a function in utilityOverlay.js called GetBoolPref which does this, 

Done and sorry I missed it. The good news is that it can be removed. As of Gecko 54 you can just use the stock api:

https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIPrefBranch

This goes for the others too. I will look where it is used in the tree and file a followup bug later.
status-seamonkey2.54: affected → fixed
Target Milestone: --- → seamonkey2.54

Comment 14

a month ago
Pushed by frgrahl@gmx.net:
https://hg.mozilla.org/comm-central/rev/3983eae3beb5
Fix typo rs=bustage-fix
(Assignee)

Comment 15

a month ago
Message to myself. Test NITs before checking them in...
(Assignee)

Updated

a month ago
Summary: Port bug 1356263 and bug 1313625 to SeaMonkey: Remove use of nsILocaleService/getApplicationLocale() → Port bug 1356263 to SeaMonkey: Remove use of nsILocaleService/getApplicationLocale()
You need to log in before you can comment on or make changes to this bug.