Closed Bug 1179117 Opened 9 years ago Closed 9 years ago

Support incoming IRCv3.2 tags

Categories

(Chat Core :: IRC, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Instantbird 42

People

(Reporter: freaktechnik, Assigned: freaktechnik)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 3 obsolete files)

Attached patch incoming_irc_tags-patch.diff (obsolete) — Splinter Review
The chat core currently doesn't know about tags and the message parsing breaks, if a capability with tags is activated.

The spec for tags can be found here: http://ircv3.net/specs/core/message-tags-3.2.html
Attached patch incoming_irc_tags.diff (obsolete) — Splinter Review
Forgot to remove some debug output in the tests.
Attachment #8628114 - Attachment is obsolete: true
Attachment #8628119 - Flags: review?(clokep)
Assignee: nobody → martin
Severity: normal → enhancement
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Comment on attachment 8628119 [details] [diff] [review]
incoming_irc_tags.diff

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

Overall this looks awesome! There's some comments below, which are mostly style things.

Please add another line to the block comment above ircMessage [1] saying that tags are a property.

[1] http://mxr.mozilla.org/comm-central/source/chat/protocols/irc/irc.js#22

::: chat/protocols/irc/irc.js
@@ +50,1 @@
>    // is required. A raw string looks like:

We should this this opportunity to make this sentence make sense:
"Splits the raw string into five parts. The third part, the command, is required.

@@ +82,5 @@
>    //   :(servername|(nickname[[!user]@host]))
> +  [message.origin, message.user, message.host] = temp[2].split(/[!@]/);
> +
> +  // Store the tags in a Map, if there are tags.
> +  // For the tags spec, see IRCv3.2 Message Tags.

"Store the tags in a Map, see IRCv3.2 Message Tags." is enough of a comment.

@@ +88,5 @@
> +
> +  if (temp[1]) {
> +    let tags = temp[1].split(";");
> +    tags.forEach((tag) => {
> +      tag = tag.split("=");

It might be clearer to do:
let [tag, value] = tag.split("=");
if (value) {
  // Unescape it.
  value = ...
}
message.tags.set(tag, value);

In this case, "value" will automatically be come undefined if there's no set value.

@@ +92,5 @@
> +      tag = tag.split("=");
> +
> +      // A tag doesn't need to have a value.
> +      let value = null;
> +      if(tag.length > 1) {

Nit: Space before (.

@@ +115,5 @@
> +          // ignore the backslash, not specified by the spec, but as it says
> +          // backslashes must be escaped this case should not occur in a valid
> +          // tag value.
> +          else
> +            return type;

This means that "\." will get replace with just ".", is that expected? (The spec doesn't seem to say anything about this and I can't find test vectors including escaped characters.)

Nit: Please start all comments with a capital letter and end them in punctuation.

::: chat/protocols/irc/test/test_ircMessage.js
@@ +157,5 @@
>    let result = true;
>    for (let fieldName in aObject1) {
>      let field1 = aObject1[fieldName];
> +    let field2;
> +    if (aObject2 instanceof Map)

It just took me a few minutes to figure this out, could you add a brief comment? Also please add a comment above this method (or change the variable names) so it is clear that aObject2 must be the ircMessage, while aObject1 is the expected value.

@@ +257,5 @@
>  
>    run_next_test();
>  }
> +
> +function testTags() {

Did these come from anywhere in particular? Are any of the test vectors from https://github.com/DanielOaks/irc-parser-tests/blob/8fb0cc33ddc22e631c7380425b98db69abd9c6b1/tests/msg-split.yaml#L128-L148 useful? (In particular, I'd like to see one that has an equal sign and no value added: @a=;b=c.)
Attachment #8628119 - Flags: review?(clokep) → review-
Attached patch incoming_irc_tags.diff (obsolete) — Splinter Review
(In reply to Patrick Cloke [:clokep] from comment #2)
> @@ +115,5 @@
> > +          // ignore the backslash, not specified by the spec, but as it says
> > +          // backslashes must be escaped this case should not occur in a valid
> > +          // tag value.
> > +          else
> > +            return type;
> 
> This means that "\." will get replace with just ".", is that expected? (The
> spec doesn't seem to say anything about this and I can't find test vectors
> including escaped characters.)

This is to gracefully handle invalid tag values, as the spec doesn't specify what to do with invalid escape sequences.

> @@ +257,5 @@
> >  
> >    run_next_test();
> >  }
> > +
> > +function testTags() {
> 
> Did these come from anywhere in particular? Are any of the test vectors from
> https://github.com/DanielOaks/irc-parser-tests/blob/
> 8fb0cc33ddc22e631c7380425b98db69abd9c6b1/tests/msg-split.yaml#L128-L148
> useful? (In particular, I'd like to see one that has an equal sign and no
> value added: @a=;b=c.)
The existing tests are from the spec itself, the third one is one I made up to test decoding.

I've added the test from the irc-parser-tests with the empty tag value, the other one doesn't test anything new. I've also added an example from the IRCv3.2 spec for server timestamps.

I've also addressed all the other comments in the suggested manner.
Attachment #8628119 - Attachment is obsolete: true
Attachment #8628267 - Flags: review?(clokep)
Comment on attachment 8628267 [details] [diff] [review]
incoming_irc_tags.diff

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

A couple of nits to fix and this is good to go!

::: chat/protocols/irc/irc.js
@@ +91,5 @@
> +    let tags = temp[1].split(";");
> +    tags.forEach((tag) => {
> +      let [key, value] = tag.split("=");
> +
> +      if(value) {

Nit: Space before (. (There's a few places below this has to be changed too.)

@@ +100,5 @@
> +        // \s = SPACE
> +        // \: = ;
> +        // everything else stays identical
> +        value = value.replace(/\\(.)/g, (str, type) => {
> +          if (type=="\\")

Nit: Spaces around ==.
Attachment #8628267 - Flags: review?(clokep) → review-
Attachment #8628267 - Attachment is obsolete: true
Attachment #8628906 - Flags: review?(clokep)
Comment on attachment 8628906 [details] [diff] [review]
incoming_irc_tags.diff

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

I changed a minor thing or two before checkin. Thanks!
Attachment #8628906 - Flags: review?(clokep) → review+
https://hg.mozilla.org/comm-central/rev/80f9837f63ad
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.6
Updating milestone from 1.6 to 42.
Target Milestone: 1.6 → Instantbird 42
Blocks: ircv3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: