Closed Bug 954083 Opened 8 years ago Closed 8 years ago

Chat Rooms should be available to JavaScript protocols

Categories

(Chat Core :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: clokep, Assigned: clokep)

References

Details

Attachments

(1 file, 1 obsolete file)

*** 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.
Blocks: 953944
Attached patch v1.0 (obsolete) — Splinter Review
*** 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.
Attachment #8352218 - Flags: review?(florian)
Assignee: nobody → clokep
Status: NEW → ASSIGNED
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.
Attachment #8352218 - Flags: review?(florian) → review-
Attached patch v2.0Splinter Review
*** 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.
Attachment #8352223 - Flags: review?(florian)
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 ***
Attachment #8352218 - Attachment is obsolete: true
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).
Attachment #8352223 - Flags: review?(florian) → review+
*** Original post on bio 648 at 2011-01-14 18:09:18 UTC ***

Checked in http://hg.instantbird.org/instantbird/rev/61fc80a569d3
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → 0.3a1
Blocks: 954121
You need to log in before you can comment on or make changes to this bug.