954083 - Chat Rooms should be available to JavaScript protocols

Status: RESOLVED FIXED

(Chat Core :: General, defect)

Description

[Patrick Cloke [:clokep]]

*** Original post on bio 648 at 2011-01-07 02:19:00 UTC ***

This bug will handle making purpleIChatRoomField and purpleIChatRoomFieldValue available to JavaScript protocols.

See http://lxr.instantbird.org/instantbird/source/purple/purplexpcom/public/purpleIAccount.idl#66 for the interface.

Comment 1

[Patrick Cloke [:clokep]]

Attachment: v1.0

*** Original post on bio 648 as attmnt 475 at 2011-01-14 04:59:00 UTC ***

Implements ChatRoomField and ChatRoomFieldValues and sets some samples for jsTestProtocol.  This might not apply properly with some of the other changes to jsProtoHelper.jsm.

Comment 2

[Florian Quèze [:florian]]

Comment on attachment 8352218 [details] [diff] [review]
v1.0

*** Original change on bio 648 attmnt 475 at 2011-01-14 15:33:13 UTC ***

>diff --git a/purple/purplexpcom/src/jsProtoHelper.jsm b/purple/purplexpcom/src/jsProtoHelper.jsm

>+      let type = typeof chatRoomField.default;
>+      if (type == "number")
That's confusing. I would prefer:
let type;
if ((typeof chatRoomField.default) == "number")

(not sure if the parenthesis around the typeof operator are needed by the way)

>+      chatRoomFields.push(new ChatRoomField(
>+        chatRoomField.label, chatRoomFieldName, type, chatRoomField.required,
>+        chatRoomField.min, chatRoomField.max
>+      ));
The comment I made in the bug about UserSplits could apply here too. the ChatRoomField constructor could take only 2 parameters: the identifier and the values.
But we did it this way for options, so... anyway...

>+  getChatRoomDefaultFieldValues: function(aDefaultChatName) {
>+    if (!this.chatRoomFields)
>+      return EmptyEnumerator;
>+
>+    let chatRoomDefaultFieldValues = [];
>+    for (let chatRoomFieldName in this.chatRoomFields) {
>+      let chatRoomField = this.chatRoomFields[chatRoomFieldName];
>+      chatRoomDefaultFieldValues[chatRoomFieldName] = chatRoomField.default;
>+    }
>+    return new ChatRoomFieldValues(chatRoomDefaultFieldValues);
I think this code would be more readable if you removed the "chatRoom" prefix from the local variables.


>+ChatRoomFieldValues.prototype = {

>+  getValue: function(aIdentifier) {
>+    if (this.values.hasOwnProperty(aIdentifier))
>+        return this.values[aIdentifier];
The indent isn't right here. You have two unneeded spaces.

>+    return null;

I think this could fit in 80 columns by the way:
    this.values.hasOwnProperty(aIdentifier) ? this.values[aIdentifier] : null

>diff --git a/purple/purplexpcom/src/jsTestProtocol.js b/purple/purplexpcom/src/jsTestProtocol.js

>+  chatRoomFields: {
>+    "channel": {"label": "_Channel Field", "required": true},
>+    "channelDefault": {"label": "_Channel Field (with default)",
>+                       "default": "Default Value"},
>+    "password": {"label": "_Password Field", "default": "", "isPassword": true,
>+                 "required": false},
>+    "sampleIntField": {"label": "_Int Field", "default": 4, "required": true,
>+                       "min": 0, "max": 10}
I think the " could be removed from the keys here, as we did in the UsernameSplit bug.

Comment 3

[Patrick Cloke [:clokep]]

Attachment: v2.0

*** Original post on bio 648 as attmnt 480 at 2011-01-14 15:58:00 UTC ***

Fixes the comments and simplifies the code in a few other instances.

Comment 4

[Patrick Cloke [:clokep]]

Comment on attachment 8352218 [details] [diff] [review]
v1.0

*** Original change on bio 648 attmnt 475 at 2011-01-14 15:58:57 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 5

[Florian Quèze [:florian]]

Comment on attachment 8352223 [details] [diff] [review]
v2.0

*** Original change on bio 648 attmnt 480 at 2011-01-14 17:05:41 UTC ***

Looks good, with a few nits that I'll fix before pushing (missing semicolon, trailing comma, this.min/max lines that could be moved inside the INT specific block, canJoinChat: true needed to make the example work).

Comment 6

[Patrick Cloke [:clokep]]

*** Original post on bio 648 at 2011-01-14 18:09:18 UTC ***

Checked in http://hg.instantbird.org/instantbird/rev/61fc80a569d3