Closed
Bug 954083
Opened 11 years ago
Closed 11 years ago
Chat Rooms should be available to JavaScript protocols
Categories
(Chat Core :: General, defect)
Chat Core
General
Tracking
(Not tracked)
RESOLVED
FIXED
0.3a1
People
(Reporter: clokep, Assigned: clokep)
References
Details
Attachments
(1 file, 1 obsolete file)
3.98 KB,
patch
|
florian
:
review+
|
Details | Diff | Splinter Review |
*** 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.
Assignee | ||
Comment 1•11 years ago
|
||
*** 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 | ||
Updated•11 years ago
|
Assignee: nobody → clokep
Status: NEW → ASSIGNED
Comment 2•11 years ago
|
||
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-
Assignee | ||
Comment 3•11 years ago
|
||
*** 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)
Assignee | ||
Comment 4•11 years ago
|
||
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 5•11 years ago
|
||
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+
Assignee | ||
Comment 6•11 years ago
|
||
*** 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: 11 years ago
Resolution: --- → FIXED
Updated•11 years ago
|
Target Milestone: --- → 0.3a1
You need to log in
before you can comment on or make changes to this bug.
Description
•