Implement IRCv3.2 server-time capability

RESOLVED FIXED in Instantbird 53

Status

Chat Core
IRC
--
enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: freaktechnik, Assigned: freaktechnik)

Tracking

trunk
Instantbird 53

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 10 obsolete attachments)

11.69 KB, patch
clokep
: review+
Details | Diff | Splinter Review
8.09 KB, patch
clokep
: review+
Details | Diff | Splinter Review
(Assignee)

Description

2 years ago
Implement the server-time capability as http://ircv3.net/specs/extensions/server-time-3.2.html specifies.

It should also request znc.in/server-time-iso, since that's what ZNC used before the standard was created and the tag it adds works exactly the same from what I can find.
(Assignee)

Updated

2 years ago
Assignee: nobody → martin
Status: NEW → ASSIGNED
(Assignee)

Comment 1

2 years ago
While testing the time stamps from the spec with my draft patch, I noticed, that leap seconds will result in an "Invalid Date" time instead of an actual time. I wonder what the best thing to do here would be. Either the leap second is ignored (which means parsing the date string and removing the leap second), or the server time is ignored.
(Assignee)

Comment 2

2 years ago
Created attachment 8791148 [details] [diff] [review]
bug1302447-taghandler.patch

This patch adds handler infrastructure to handle tags independent of the command they are on.
Attachment #8791148 - Flags: review?(clokep)
(Assignee)

Comment 3

2 years ago
Created attachment 8791149 [details] [diff] [review]
bug1302447-server-time.patch

This patch implements the capability negotiation and parsing of the tag. Leap seconds are rounded down to the previous non-leap second.
Attachment #8791149 - Flags: review?(clokep)
Comment on attachment 8791148 [details] [diff] [review]
bug1302447-taghandler.patch

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

This is not nearly as gross as I thought it was going to be! I wish there was a way we didn't have to pass tags all over the place though...

::: chat/protocols/irc/irc.js
@@ +179,5 @@
>                        " :\r\n";
>      return this._account.maxMessageLength -
>             this._account.countBytes(baseMessage);
>    },
> +  writeMessage: function(aWho, aMessage, aObject) {

aObject here is an IRC message object?

@@ +193,5 @@
> +        ircHandlers.handleTag(this._account, new TagMessage(msg, tag));
> +      }
> +
> +      delete msg.originalMessage;
> +      aObject = msg;

Why reassign msg to aObject? This seems confusing.

@@ +196,5 @@
> +      delete msg.originalMessage;
> +      aObject = msg;
> +    }
> +    // Remove the IRC tags, as those were added just for this step.
> +    delete aObject.tags;

I think what you're trying to say here is to delete tags since they're an IRC only thing?
Attachment #8791148 - Flags: review?(clokep) → feedback+
Comment on attachment 8791149 [details] [diff] [review]
bug1302447-server-time.patch

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

Thanks for writing tests. :)

::: chat/protocols/irc/ircServerTime.jsm
@@ +20,5 @@
> +
> +  commands: {
> +    "time": function(aMsg) {
> +      // Normalize leap seconds to the next second before it.
> +      const time = aMsg.tagValue.replace(/60.\d{3}(?=Z$)/, "59.999");

This is because JavaScript doesn't support leap seconds?

@@ +40,5 @@
> +      }
> +      return true;
> +    },
> +    "znc.in/server-time-iso": function(aMessage) {
> +      if(aMessage.cap.subcommand == "LS") {

Nit: (Across both patches...) Space after "if", before "(".

::: chat/protocols/irc/test/test_serverTime.js
@@ +3,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +Components.utils.import("resource:///modules/ircServerTime.jsm");
> +
> +function getTags(aRawMsg) {

It'd be nice if we could use the "real" parsing for this.

@@ +52,5 @@
> +      tagName: "time",
> +      tagValue: msg.tags.get("time")
> +    };
> +    tagServerTime.commands.time(tagMessage);
> +    for(let i in msg) {

Nit: Space after for.

@@ +53,5 @@
> +      tagValue: msg.tags.get("time")
> +    };
> +    tagServerTime.commands.time(tagMessage);
> +    for(let i in msg) {
> +      do_check_eq(tagMessage.message[i], msg[i]);

I've been trying to use the Assert.jsm ones now! Let's use those for new code.
Attachment #8791149 - Flags: review?(clokep) → review-
(Assignee)

Comment 6

2 years ago
(In reply to Patrick Cloke [:clokep] from comment #4)
> This is not nearly as gross as I thought it was going to be! I wish there
> was a way we didn't have to pass tags all over the place though...

They are being passed all over the place.
 
> ::: chat/protocols/irc/irc.js
> @@ +179,5 @@
> >                        " :\r\n";
> >      return this._account.maxMessageLength -
> >             this._account.countBytes(baseMessage);
> >    },
> > +  writeMessage: function(aWho, aMessage, aObject) {
> 
> aObject here is an IRC message object?

No, it is a plain object passed in from the handlers. It contains properties to set on the imMessage. It defines things like "incoming" or "system". I added "tags" to all call sites that might be interesting for tag handlers, which is why I had to modify ircCommands.jsm. And why I delete the property later on.

 
> @@ +193,5 @@
> > +        ircHandlers.handleTag(this._account, new TagMessage(msg, tag));
> > +      }
> > +
> > +      delete msg.originalMessage;
> > +      aObject = msg;
> 
> Why reassign msg to aObject? This seems confusing.

This is to preserve potential changes/additions handlers make to the imMessage. I will add a comment explaining that.

> @@ +196,5 @@
> > +      delete msg.originalMessage;
> > +      aObject = msg;
> > +    }
> > +    // Remove the IRC tags, as those were added just for this step.
> > +    delete aObject.tags;
> 
> I think what you're trying to say here is to delete tags since they're an
> IRC only thing?

No, that the tags are passed in on aObject just for the tag handling. I've slightly reworded the comment to make it more obvious.

When I've been able to test an updated version of the server time patch, I'll also attach a patch that has the updated comments.
(Assignee)

Comment 7

2 years ago
(In reply to Martin Giger [:freaktechnik] from comment #6)
> added "tags" to all call sites that might be interesting for tag handlers,
> which is why I had to modify ircCommands.jsm.
 Sorry, not ircCommands.jsm; ircBase.jsm, ircCTCP.jsm and ircNonStandard.jsm are the ones affected by that. It means an IRC message handler can decide whether or not the tag handlers should be ran on the message by passing in the tags property on the object or omitting it.
(Assignee)

Comment 8

2 years ago
Created attachment 8807834 [details] [diff] [review]
bug1302447-taghandler-v2.patch

Tried to re-word some comments and add some to clarify how tag handling works. Fixed up code style.
Attachment #8791148 - Attachment is obsolete: true
Attachment #8807834 - Flags: review?(clokep)
(Assignee)

Comment 9

2 years ago
Created attachment 8807835 [details] [diff] [review]
bug1302447-server-time-v2.patch

> This is because JavaScript doesn't support leap seconds?

Exactly. It's the best method I could come up with to handle them. And since unix timestamps don't have leap seconds either and just do time dilation this is the rightest way to do it.

Switched to Assert.jsm and addressed code style nits.
Attachment #8791149 - Attachment is obsolete: true
Attachment #8807835 - Flags: review?(clokep)
(Assignee)

Comment 10

2 years ago
Created attachment 8807836 [details] [diff] [review]
bug1302447-server-time-v2.1.patch

Noticed that I had the wrong license header in the test.
Attachment #8807835 - Attachment is obsolete: true
Attachment #8807835 - Flags: review?(clokep)
Attachment #8807836 - Flags: review?(clokep)
Comment on attachment 8807836 [details] [diff] [review]
bug1302447-server-time-v2.1.patch

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

::: chat/protocols/irc/test/test_serverTime.js
@@ +2,5 @@
> + * http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +Components.utils.import("resource:///modules/ircServerTime.jsm");
> +Components.utils.import("resource://gre/modules/Services.jsm");
> +Components.utils.import("resource://testing-common/Assert.jsm");

You don't need to import Assert.jsm, you can just directly use the methods.

@@ +44,5 @@
> +      message: "John joined #chan",
> +      get originalMessage() { return "John joined #chan"; },
> +      system: true,
> +      incoming: true
> +    }

How hard would it be to add a test for one WITHOUT tags? (And maybe one with tags being an empty list?)
Attachment #8807836 - Flags: review?(clokep) → review-
(Assignee)

Comment 12

2 years ago
Created attachment 8808424 [details] [diff] [review]
bug1302447-server-time-v3.patch
Attachment #8808424 - Flags: review?(clokep)
Comment hidden (obsolete)
Comment on attachment 8808424 [details] [diff] [review]
bug1302447-server-time-v3.patch

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

::: chat/protocols/irc/irc.js
@@ +185,4 @@
>        const msg = Object.assign({
>          who: aWho,
>          message: aMessage,
> +        get originalMessage() {

This should be in the first patch.

::: chat/protocols/irc/test/test_ircServerTime.js
@@ +7,5 @@
> +var irc = {};
> +Services.scriptloader.loadSubScript("resource:///components/irc.js", irc);
> +
> +function getTags(aRawMsg) {
> +  const { tags } = irc.ircMessage(aRawMsg, "doesnt@matter");

I think we don't normally put spaces around this, but I'm unsure.

@@ +68,5 @@
> +      tagName: "time",
> +      tagValue: msg.tags.get("time")
> +    };
> +    tagServerTime.commands.time(tagMessage);
> +    for (let i in msg) {

Can you add a comment above this for loop about what we're checking for equality.

@@ +71,5 @@
> +    tagServerTime.commands.time(tagMessage);
> +    for (let i in msg) {
> +      equal(tagMessage.message[i], msg[i]);
> +    }
> +    equal("time" in tagMessage.message, kExpectedTimes[m] !== undefined);

And above this for what we're checking.

@@ +73,5 @@
> +      equal(tagMessage.message[i], msg[i]);
> +    }
> +    equal("time" in tagMessage.message, kExpectedTimes[m] !== undefined);
> +
> +    if(kExpectedTimes[m] !== undefined) {

Nit: Space after "if".
Attachment #8808424 - Flags: review?(clokep) → review-
Comment on attachment 8807834 [details] [diff] [review]
bug1302447-taghandler-v2.patch

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

::: chat/protocols/irc/irc.js
@@ +180,4 @@
>      return this._account.maxMessageLength -
>             this._account.countBytes(baseMessage);
>    },
> +  writeMessage: function(aWho, aMessage, aObject) {

Please add a comment above this for what the inputs are (specifically aObject).

@@ +184,5 @@
> +    if ("tags" in aObject && ircHandlers.hasTagHandlers) {
> +      const msg = Object.assign({
> +        who: aWho,
> +        message: aMessage,
> +        get originalMessage {

This has a bug fixed in the next patch.

@@ +185,5 @@
> +      const msg = Object.assign({
> +        who: aWho,
> +        message: aMessage,
> +        get originalMessage {
> +          return aMessage;

Should this be making a copy of aMessage? Why do we need originalMessage here? We seem to create it and then delete it, unless you expect one of the tag handlers to want to access it?

@@ +196,5 @@
> +      }
> +
> +      delete msg.originalMessage;
> +      // Make sure any changes made by the handlers get written to the imMessage.
> +      aObject = msg;

This still seems really confusing to me. I think it'd be clearer if you had an else after the if-statement that said msg = aObject, then just used msg below this line.
Attachment #8807834 - Flags: review?(clokep) → review-
(Assignee)

Comment 16

2 years ago
Created attachment 8810213 [details] [diff] [review]
bug1302447-taghandler-v3.patch

Addresses the nits. I decided not to go for an else due to how scoping works. Alternatively we could just declare messageProps and then conditionally set it.

I think I already know one suggestion you'll have: to not rely on messageProps.who and messageProps.message to just override aWho and aMessage.
Attachment #8807834 - Attachment is obsolete: true
Attachment #8810213 - Flags: review?(clokep)
(Assignee)

Comment 17

2 years ago
Created attachment 8810214 [details] [diff] [review]
bug1302447-sever-time-v4.patch

I also added a test for the znc server-time tag.
Attachment #8808424 - Attachment is obsolete: true
Attachment #8810214 - Flags: review?(clokep)
Comment on attachment 8810213 [details] [diff] [review]
bug1302447-taghandler-v3.patch

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

Can you use 8-lines of context next time?

::: chat/protocols/irc/irc.js
@@ +204,5 @@
> +
> +      // Remove helper prop for tag handlers. We don't want to remove the other
> +      // ones, since they might have been changed and will override aWho and
> +      // aMessage in the imMessage constructor.
> +      delete msg.originalMessage;

msg doesn't exist anymore, I think you wanted this to be messageProps?
Attachment #8810213 - Flags: review?(clokep) → review-
Attachment #8810214 - Flags: review?(clokep) → review+
(Assignee)

Comment 19

2 years ago
Created attachment 8811468 [details] [diff] [review]
bug1302447-taghandler-v4.patch

Yep, totally forgot that when refactoring to better names.
Attachment #8810213 - Attachment is obsolete: true
Attachment #8811468 - Flags: review?(clokep)
Attachment #8811468 - Flags: review?(clokep) → review+
Comment on attachment 8810214 [details] [diff] [review]
bug1302447-sever-time-v4.patch

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

::: chat/protocols/irc/ircServerTime.jsm
@@ +19,5 @@
> +  isEnabled: () => true,
> +
> +  commands: {
> +    "time": function(aMsg) {
> +      if(aMsg.tagValue) {

Nit: This should have a space after "if".

::: chat/protocols/irc/test/test_ircServerTime.js
@@ +36,5 @@
> +      system: true,
> +      incoming: true
> +    },
> +    {
> +      tags: getTags("@znc.in/server-time-iso=2016-11-13T19:20:45.284Z :John!~john@1.2.3.4 JOIN #chan"),

This case is failing for me, I think generating the tag message isn't working correctly...
Attachment #8810214 - Flags: review+ → review-
(Assignee)

Comment 21

2 years ago
Created attachment 8811995 [details] [diff] [review]
bug1302447-server-time-v5.patch

That test for znc.in/server-time-iso was shoddily copy-pasted, but now everything works as expected, I also verified on my ZNC after seeing that the tag handler wasn't even implemented...
Attachment #8810214 - Attachment is obsolete: true
Attachment #8811995 - Flags: review?(clokep)
(Assignee)

Comment 22

2 years ago
Created attachment 8811999 [details] [diff] [review]
bug1302447-server-time-v5.1.patch

Oops, forgot to remove a debugging dump().
Attachment #8811995 - Attachment is obsolete: true
Attachment #8811995 - Flags: review?(clokep)
Attachment #8811999 - Flags: review?(clokep)
Comment on attachment 8811999 [details] [diff] [review]
bug1302447-server-time-v5.1.patch

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

::: chat/protocols/irc/ircServerTime.jsm
@@ +13,5 @@
> +
> +Cu.import("resource:///modules/ircHandlers.jsm");
> +
> +function handleServerTimeTag(aMsg) {
> +  if(aMsg.tagValue) {

Nit: Add a space after "if".

::: chat/protocols/irc/test/test_ircServerTime.js
@@ +83,5 @@
> +
> +    // Ensuring that the expected properties and their values as given in
> +    // kMessages are still the same after the handler.
> +    for (let i in msg) {
> +      equal(tagMessage.message[i], msg[i], "Property '"+i+"' was not modified");

Nit: Spaces around the +.

@@ +86,5 @@
> +    for (let i in msg) {
> +      equal(tagMessage.message[i], msg[i], "Property '"+i+"' was not modified");
> +    }
> +    // The time should only be adjusted when we expect a valid server-time tag.
> +    equal("time" in tagMessage.message, kExpectedTimes[m] !== undefined, "Message time was set when expected");

I think we also need to check for the ZNC tag here?
Attachment #8811999 - Flags: review?(clokep) → review-
(Assignee)

Comment 24

2 years ago
I'll never learn to put spaces after keywords, except maybe import ;)

(In reply to Patrick Cloke [:clokep] from comment #23)
> Comment on attachment 8811999 [details] [diff] [review]
> bug1302447-server-time-v5.1.patch
> 
> Review of attachment 8811999 [details] [diff] [review]:
> -----------------------------------------------------------------
> @@ +86,5 @@
> > +    for (let i in msg) {
> > +      equal(tagMessage.message[i], msg[i], "Property '"+i+"' was not modified");
> > +    }
> > +    // The time should only be adjusted when we expect a valid server-time tag.
> > +    equal("time" in tagMessage.message, kExpectedTimes[m] !== undefined, "Message time was set when expected");
> 
> I think we also need to check for the ZNC tag here?

Nope, that's checking if the imMessage has the time set, not if the tag exists.
(Assignee)

Comment 25

2 years ago
Created attachment 8814243 [details] [diff] [review]
bug1302447-server-time-v6.patch

Addressed the two nits on the last patch.
Attachment #8811999 - Attachment is obsolete: true
Attachment #8814243 - Flags: review?(clokep)
Attachment #8814243 - Flags: review?(clokep) → review+
https://hg.mozilla.org/comm-central/rev/7111a1bd9fa8fd043791eb4f4fb9d43ba802239e
https://hg.mozilla.org/comm-central/rev/08af9c37ea6aea0d51253e3fda5837d994f8d7d8

Thanks! Would it make sense to uplift either of these to 52 to be in the ESR?
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Instantbird 53
Depends on: 1321886
You need to log in before you can comment on or make changes to this bug.