Closed Bug 1389449 Opened 2 years ago Closed 2 years ago

ChatZilla is broken in SeaMonkey 2.54

Categories

(Other Applications :: ChatZilla, defect)

defect
Not set

Tracking

(seamonkey2.54 fixed, firefox57 fixed)

RESOLVED FIXED
mozilla57
Tracking Status
seamonkey2.54 --- fixed
firefox57 --- fixed

People

(Reporter: frg, Assigned: frg)

References

Details

Attachments

(1 file, 4 obsolete files)

+++ 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.
Attached patch 1389449-Sea254.patch (obsolete) — Splinter Review
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.
Attachment #8896219 - Flags: review?(gijskruitbosch+bugs)
Attached patch 1389449-Sea254.patch (obsolete) — Splinter Review
Sorry old patch was incomplete.
Attachment #8896219 - Attachment is obsolete: true
Attachment #8896219 - Flags: review?(gijskruitbosch+bugs)
Attachment #8896221 - Flags: review?(gijskruitbosch+bugs)
Blocks: 1375125
nsIprefBranch2 removal is Bug 1374847.
Blocks: 1374847, 1313625
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 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.
Attachment #8896221 - Flags: review?(gijskruitbosch+bugs) → review+
(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?
> 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.
Attached patch 1389449-irc-Sea254-V2.patch (obsolete) — Splinter Review
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.
Attachment #8896221 - Attachment is obsolete: true
Attachment #8896945 - Flags: review?(gijskruitbosch+bugs)
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.
Attachment #8896945 - Flags: review?(gijskruitbosch+bugs) → review+
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.
(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?
Flags: needinfo?(jorgk)
Hmm you mean:

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

Thought toLocaleString would use it internally.
(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).
Flags: needinfo?(jorgk)
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.
Attachment #8896945 - Flags: review+
Attached patch 1389449-irc-Sea254-V3.patch (obsolete) — Splinter Review
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.
Attachment #8896945 - Attachment is obsolete: true
Attachment #8897127 - Flags: review?(gijskruitbosch+bugs)
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?
Attachment #8897127 - Flags: review?(gijskruitbosch+bugs)
> Ditto. Did you test this code?

Yes in SeaMonkey 2.53
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
(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...
I added the import. Maybe you or someone else can check if this works in XULRunner.
Attachment #8897127 - Attachment is obsolete: true
Attachment #8898017 - Flags: review?(gijskruitbosch+bugs)
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...
Attachment #8898017 - Flags: review?(gijskruitbosch+bugs) → review+
https://hg.mozilla.org/chatzilla/rev/4f72d8882106df55a2f03504bb68fe4ac7f126b4
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
(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. :/
(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.
(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.
Duplicate of this bug: 1392168
You need to log in before you can comment on or make changes to this bug.