1389449 - ChatZilla is broken in SeaMonkey 2.54

Status: RESOLVED FIXED

(Other Applications :: ChatZilla, defect)

Description

[Frank-Rainer Grahl (:frg)]

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

Numerous breakages. Some older deprecated apis were already removed from the mozilla source tree and need to be replaced.

Comment 1

[Frank-Rainer Grahl (:frg)]

Attachment: 1389449-Sea254.patch

Basically 1:1 replacements for now. I am not so sure about the
Date.prototype.toString = function()
but it didn't throw an exception during brief use. I think function() could be removed too.

Comment 2

[Frank-Rainer Grahl (:frg)]

Attachment: 1389449-Sea254.patch

Sorry old patch was incomplete.

Comment 3

[Frank-Rainer Grahl (:frg)]

nsIprefBranch2 removal is Bug 1374847.

Comment 4

[Frank-Rainer Grahl (:frg)]

If the patch is basically ok I should put an alternate or better the old path in for Bug 1313625 and older pre 56 Gecko versions.

Comment 5

[:Gijs (he/him)]

Comment on attachment 8896221 [details] [diff] [review]
1389449-Sea254.patch

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

r=me with the timezone removed.

::: xul/content/static.js
@@ +249,5 @@
>      try
>      {
> +        const dtOptions = { year: "numeric", month: "long", day: "numeric",
> +                          hour: "numeric", minute: "numeric", second: "numeric",
> +                          timeZoneName: "short", weekday: "short" };

I don't think this is necessarily right. On my current CZ on an old version of XULRunner, I get:

[16:09:22]	[EVAL-IN]	x.toString()
[16:09:22]	[EVAL-OUT]	11 August 2017 at 16:09:16
[16:09:57]	[EVAL-IN]	x.toLocaleString(undefined, { year: "numeric", month: "long", day: "numeric", hour: "numeric", minute: "numeric", second: "numeric",timeZoneName: "short", weekday: "short" })
[16:09:57]	[EVAL-OUT]	Fri, 11 August 2017 16:09:16 BST

But then again, it seems that the defaults are maybe OS-dependent, or OS-settings-dependent, so I don't know how to make this always be the same - I don't see some way to read the long date format from the OS now that scriptableDateFormat is gone, and we have strftime-style prefs for the log and window timestamps, which are the most important, so I'm not sure how much would even be affected by minor changes here.

However, it seems we should at least omit the timezone, which doesn't add much here.

Comment 6

[:Gijs (he/him)]

(In reply to Frank-Rainer Grahl (:frg) from comment #4)
> If the patch is basically ok I should put an alternate or better the old
> path in for Bug 1313625 and older pre 56 Gecko versions.

I'm not clear on what this comment means... does toLocaleString() not work properly in older geckos?

Comment 7

[Frank-Rainer Grahl (:frg)]

> I'm not clear on what this comment means... does toLocaleString() not work properly in older geckos?

As far as I know this specific implementation wasn't even there before 51. The current one uses MozIntl. Jorgk should know.

Comment 8

[Frank-Rainer Grahl (:frg)]

Attachment: 1389449-irc-Sea254-V2.patch

Could you check this one out. It uses nsIScriptableDate before 56. I moved getting the version up higher in static.js and only changed the code for the date formatting there compared to the first patch

I think the Date.Prototype is ineffective. Seems not to work. But same in 2.49 which corresponds to ESR 52 so likely caused by a different bug.

Comment 9

[:Gijs (he/him)]

Comment on attachment 8896945 [details] [diff] [review]
1389449-irc-Sea254-V2.patch

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

::: xul/content/static.js
@@ +249,5 @@
>      }
>  
>      try
>      {
> +       // nsIScriptableDateFormat was removed from Gecko 57.

Nit: in this block you seem to be using 3-space indent some of the time. Please use 4-space indent.

@@ +252,5 @@
>      {
> +       // nsIScriptableDateFormat was removed from Gecko 57.
> +       // The replacement function was not available in a stable version
> +       // prior to Gecko 56.
> +       if (Services.vc.compare(version.hostVersion, "56.0") < 0) {

Nit: braces on the next line per CZ style.

Comment 10

[Jorg K (CEST = GMT+2)]

Comment on attachment 8896945 [details] [diff] [review]
1389449-irc-Sea254-V2.patch

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

::: xul/content/static.js
@@ +278,5 @@
> +
> +          // Mmmm, fun. This ONLY affects the ChatZilla window, don't worry!
> +          Date.prototype.toStringInt = Date.prototype.toString;
> +          Date.prototype.toString = function() {
> +              this.toLocaleString(undefined, dtOptions);

If this is 56+, why not use mozIntl here like the rest of the chrome code.

Comment 11

[:Gijs (he/him)]

(In reply to Jorg K (GMT+2) from comment #10)
> Comment on attachment 8896945 [details] [diff] [review]
> 1389449-irc-Sea254-V2.patch
> 
> Review of attachment 8896945 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: xul/content/static.js
> @@ +278,5 @@
> > +
> > +          // Mmmm, fun. This ONLY affects the ChatZilla window, don't worry!
> > +          Date.prototype.toStringInt = Date.prototype.toString;
> > +          Date.prototype.toString = function() {
> > +              this.toLocaleString(undefined, dtOptions);
> 
> If this is 56+, why not use mozIntl here like the rest of the chrome code.

Which "rest of the chrome code"? What would that look like, and why would it be better?

Comment 12

[Frank-Rainer Grahl (:frg)]

Hmm you mean:

> const dateTimeFormatter = Services.intl.createDateTimeFormat(undefined, { ...

Thought toLocaleString would use it internally.

Comment 13

[Jorg K (CEST = GMT+2)]

(In reply to :Gijs from comment #11)
> Which "rest of the chrome code"? What would that look like, and why would it
> be better?
Just talking in general about chrome code in Firefox, see bug 1354445 and bug 1383463. Dates/times displayed in the UI follow operating system settings, so I don't see why CZ should behave differently.

toLocaleString() is ECMA-402 and doesn't follow OS settings like mozIntl (quoted in comment #12).

Comment 14

[:Gijs (he/him)]

Comment on attachment 8896945 [details] [diff] [review]
1389449-irc-Sea254-V2.patch

If we have a better way of getting the OS format here via mozIntl (which I was not aware of) we should use that instead.

Comment 15

[Frank-Rainer Grahl (:frg)]

Attachment: 1389449-irc-Sea254-V3.patch

Good that I looked it over. Forgot to set the client variable which would have resulted in an undefined variable

This one uses mozIntl but the Date.prototype does still not work so this part is imho inoperative.

Comment 16

[:Gijs (he/him)]

Comment on attachment 8897127 [details] [diff] [review]
1389449-irc-Sea254-V3.patch

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

::: xul/content/static.js
@@ +250,5 @@
>      {
> +        // nsIScriptableDateFormat was removed from Gecko 57
> +        // The replacement function was not available in a stable version
> +        // prior to Gecko 56.
> +        if (Services.vc.compare(Services.appinfo.platformVersion, "56.0") < 0)

Services isn't available in this scope, I don't think.

@@ +271,5 @@
> +            }
> +        }
> +        else
> +        {
> +            client.dtFormatter = Services.intl.createDateTimeFormat(

Ditto. Did you test this code?

Comment 17

[Frank-Rainer Grahl (:frg)]

> Ditto. Did you test this code?

Yes in SeaMonkey 2.53

Comment 18

[Frank-Rainer Grahl (:frg)]

It is usually loaded from somewhere but maybe an explicide import should be done like here:

https://hg.mozilla.org/dom-inspector/rev/a69769cacd8dbf7b2d41ff361c25d3712c72355d

Not sure about Xulrunner.

FRG

Comment 19

[:Gijs (he/him)]

(In reply to Frank-Rainer Grahl (:frg) from comment #17)
> > Ditto. Did you test this code?
> 
> Yes in SeaMonkey 2.53

Interesting. I don't see anywhere where we import Services.jsm in the CZ codebase...

Comment 20

[Frank-Rainer Grahl (:frg)]

Attachment: 1389449-irc-Sea254-V4.patch

I added the import. Maybe you or someone else can check if this works in XULRunner.

Comment 21

[:Gijs (he/him)]

Comment on attachment 8898017 [details] [diff] [review]
1389449-irc-Sea254-V4.patch

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

r=me. At some point we should switch all our ugly Cc[] invocations over to use Services etc. - or we could just switch to webextensions...

Comment 22

[Frank-Rainer Grahl (:frg)]

https://hg.mozilla.org/chatzilla/rev/4f72d8882106df55a2f03504bb68fe4ac7f126b4

Comment 23

[James Ross]

(In reply to :Gijs from comment #21)
> or we could just switch to webextensions...

Working on it! But it feels somewhat pointless when we don't have working sockets or file access. :/

Comment 24

[:Gijs (he/him)]

(In reply to James Ross from comment #23)
> (In reply to :Gijs from comment #21)
> > or we could just switch to webextensions...
> 
> Working on it! But it feels somewhat pointless when we don't have working
> sockets or file access. :/

It feels like we could improvise the file thing by doing indexeddb storage and offering export to file (which I think is workable with plain HTML, so should be workable for us?).

The sockets thing is harder. We'd need an API definition and implementation in bug 1247628. My understanding is that the webextensions team would take a patch but doesn't have the bandwidth to work on it themselves today.

Comment 25

[James Ross]

(In reply to :Gijs from comment #24)

Yeah, agreed. I'm focused on bug 785723 (restartless) first, but it's not like any of us have the time to work on a big feature like sockets.