Closed
Bug 953932
Opened 10 years ago
Closed 10 years ago
purpleIAccount cannot access preferences via JavaScript protocol
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, 5 obsolete files)
7.10 KB,
patch
|
florian
:
review+
|
Details | Diff | Splinter Review |
*** Original post on bio 495 at 2010-09-11 21:05:00 UTC *** purpleIAccount is missing the server information, this makes it impossible for js-proto accounts to actually connect to servers (without hard coding URLs). The server, port and encryption information are all currently unavailable.
Comment 1•10 years ago
|
||
*** Original post on bio 495 at 2010-09-11 21:49:24 UTC *** More generally, we should rethink the way protocol specific options are handled. There are currently only setters in purpleIAccount. We should either add getters, or remove the setters and assume all accounts will listen to notifications of pref changes on the pref branch where the account is stored.
Assignee | ||
Comment 2•10 years ago
|
||
*** Original post on bio 495 at 2010-12-12 04:41:10 UTC *** (In reply to comment #1) > More generally, we should rethink the way protocol specific options are > handled. > > There are currently only setters in purpleIAccount. > We should either add getters, or remove the setters and assume all accounts > will listen to notifications of pref changes on the pref branch where the > account is stored. Yes, we need some way to easily get preferences. I think implementing "getters" might be the best way. Perhaps accounts should observe the preference branch too? I'm not sure.
Summary: purpleIAccount missing server information → purpleIAccount cannot access preferences via JavaScript protocol
Assignee | ||
Comment 3•10 years ago
|
||
*** Original post on bio 495 as attmnt 426 at 2010-12-15 18:27:00 UTC *** This is a very rough cut that I'm requesting an API review on more than anything else. (Also note that this patch not apply properly -- it depends on code from bug 953956 (bio 519).) This code contains implementation of advanced preferences for accounts (for JavaScript protocols) via the Account Properties window (Advanced Options), as well as retrieving those preferences in the protocol, which doesn't fully work -- but I want to make sure I'm going the right direction with this before putting more time into that API. For the creating preferences part, see: https://hg.instantbird.org/experiments/file/IRC-JavaScript/components/ircProtocol.js#l1162 for an example.
Attachment #8352169 -
Flags: review?(florian)
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → clokep
Status: NEW → ASSIGNED
Comment 4•10 years ago
|
||
Comment on attachment 8352169 [details] [diff] [review] Patch v0.1 *** Original change on bio 495 attmnt 426 at 2010-12-16 08:45:40 UTC *** There are details in the code that could be fixed (should be fixed if we decide to go this route), but this time I'm going to comment only on the API. You seem to have done what to can to mimic what's been done for libpurple in purplexpcom. I would suggest we go the opposite way: define what's the best API we can for JS protocols, and then adapt the details both in jsProtoHelper/purplexpcom and the UI code to have an API that feels easy to use. In ircProtocol.js you currently have: getOptions: function() { // XXX need to refer to these in the above code let prefs = [new purplePref("port", "Port", Ci.purpleIPref.typeInt, false, 6667), new purplePref("encoding", "Encodings", Ci.purpleIPref.typeString, false, "UTF-8"), new purplePref("autodetect_utf8", "Auto-detect incoming UTF-8", Ci.purpleIPref.typeBool), new purplePref("username", "Username", Ci.purpleIPref.typeString), new purplePref("realname", "Real name", Ci.purpleIPref.typeString), //new purplePref("quitmsg", "Quit message", // Ci.purpleIPref.typeString), new purplePref("ssl", "Use SSL", Ci.purpleIPref.typeBool)]; return new nsSimpleEnumerator(prefs); }, would this seem better? options: [ {name: "port", label: "Port", default: 6667}, {name: "encoding", label: "Encoding", default: "UTF-8"}, ... ], I don't think we need to specify the type, it can be detected from the JS type of the default value (typeof will return "number", "string", "boolean", or "object" (for arrays that would represent list of possible string values)). The default value would be required. I think this is good for clarity, to understand clearly the default behavior while reading the code. Masked is so rarely used that I think we can make it an optional parameter. Another proposal, shorter but that may lose some information: options: { port: ["Port", 6667], encoding: ["Encoding", "UTF-8"], ... }, I don't really like this because relying only on the position in an array to know what something is has a potential for being confusing, but it's not worse than the proposed solution in the patch that uses the order of parameters to a constructor. I'm not sure if we risk loosing the order of the options by using an object instead of an array, and I'm not even sure we really care about that order. I'm not deciding anything here, just giving food for thoughts, so feel free to comment about what tastes better for you, and what other approaches there could be.
Attachment #8352169 -
Flags: review?(florian) → review-
Assignee | ||
Comment 5•10 years ago
|
||
*** Original post on bio 495 as attmnt 431 at 2010-12-17 22:25:00 UTC *** I have a much better API, based off of Flo's first suggestion above. I'm still unsure about what the best way to get/set these via the protocol would be. Also I've included my UsernameSplit object in here as well. It's not really a preference, but it's necessary -- I can file a separate bug if you wish. :)
Assignee | ||
Comment 6•10 years ago
|
||
Comment on attachment 8352169 [details] [diff] [review] Patch v0.1 *** Original change on bio 495 attmnt 426 at 2010-12-17 22:25:21 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352169 -
Attachment is obsolete: true
Comment 7•10 years ago
|
||
*** Original post on bio 495 at 2010-12-20 12:07:41 UTC *** Comment on attachment 8352174 [details] [diff] [review] (bio-attmnt 431) Patch v0.2 Not doing a full review yet as you haven't requested it, but I figured I should write down the things I observe. >diff --git a/purple/purplexpcom/src/jsProtoHelper.jsm b/purple/purplexpcom/src/jsProtoHelper.jsm >+function UsernameSplit(aLabel, aSeparator, aDefaultValue, aReverse) { >+ this.label = aLabel; >+ this.separator = aSeparator; >+ this.defaultValue = aDefaultValue; >+ this.reverse = aReverse || false; >+} I think I would put the false default value in the prototype and set this.reverse in the constructor only if aReverse !== undefined. This is not necessarily an improvement though :). Or you could also do this.reverse = !!aReverse to ensure this.reverse doesn't contain a string or something else that isn't wanted here. >+function purplePref(name, label, type, defaultValue, masked) { >+ this.name = name; // Preference name >+ this.label = label; // Text to display Can we automate here getting the localized text of the label, or is this something the protocol plugin code will still have to do anyway? >+ this.masked = masked; // Obscured from view Mostly the same case as for this.reverse in UsernameSplit. >+} >+purplePref.prototype = { >+ get typeBool() 1, >+ get typeInt() 2, >+ get typeString() 3, >+ get typeList() 4, What is this for? These constants are defined in the idl interface, aren't they accessible already? >+ getString: function() this._defaultValue, >+ getList: function() (purpleKeyValuePairs(this._defaultValue)) || >+ (new EmptyEnumerator()), Are you sure purpleKeyValuePairs can return null? > const GenericProtocolPrototype = { > // NS_ERROR_XPC_JSOBJECT_HAS_NO_FUNCTION_NAMED errors are too noisy This comment may not be relevant anymore, and is certainly not above a method implemation that isn't a stub doing nothing ;). >- getOptions: function() EmptyEnumerator, >+ getOptions: function() { >+ if (!this.options) >+ return EmptyEnumerator; >+ >+ let purplePrefs = []; >+ for each (let option in this.options) { >+ let type; >+ switch (typeof option.default) { >+ case "boolean": >+ type = Ci.purpleIPref.typeBool; >+ break; >+ case "string": >+ type = Ci.purpleIPref.typeString; >+ break; >+ case "number": >+ type = Ci.purpleIPref.typeInt; >+ break; >+ default: >+ type = Ci.purpleIPref.typeList; >+ break; >+ } What about this shorter implementation? const types = {boolean: "Bool", string: "String", number: "Int", object: "List"}; // XXX I'm not sure if the parenthesis around typeof are needed. // XXX Btw, can this really happen? If it's a coding error, maybe we should throw? if (!((typeof option.default) in types))) continue; type = Ci.purpleIPref["type" + types[typeof option.default]]; >+ purplePrefs.push(new purplePref(option.name, >+ option.label, >+ type, >+ option.default, >+ option.masked || false)); The for each / push combination looks a lot like you wanted to use .map >+ } >+ return new nsSimpleEnumerator(purplePrefs); >+ }, > getUsernameSplit: function() EmptyEnumerator, Shouldn't this be implemented like getOptions? May be better to handle UsernameSplit in another bug if you want to get the options part landed soon. Doesn't matter if you already depend on both though :).
Assignee | ||
Comment 8•10 years ago
|
||
*** Original post on bio 495 as attmnt 459 at 2010-12-31 01:51:00 UTC *** Patch addressing Flo's comments. I will do the UsernameSplit part in a separate bug to make the patch smaller. This patch also switches to using Services in a couple of places, but this is relatively simple and I figured could be added here.
Attachment #8352202 -
Flags: review?(florian)
Assignee | ||
Comment 9•10 years ago
|
||
Comment on attachment 8352174 [details] [diff] [review] Patch v0.2 *** Original change on bio 495 attmnt 431 at 2010-12-31 01:51:45 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352174 -
Attachment is obsolete: true
Comment 10•10 years ago
|
||
Comment on attachment 8352202 [details] [diff] [review] Patch v1.0 *** Original change on bio 495 attmnt 459 at 2010-12-31 18:17:35 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352202 -
Flags: review?(florian) → review-
Comment 11•10 years ago
|
||
*** Original post on bio 495 at 2010-12-31 18:17:38 UTC *** Comment on attachment 8352202 [details] [diff] [review] (bio-attmnt 459) Patch v1.0 >diff --git a/purple/purplexpcom/src/jsProtoHelper.jsm b/purple/purplexpcom/src/jsProtoHelper.jsm > function setTimeout(aFunction, aDelay) >@@ -125,7 +120,11 @@ > _init: function _init(aProtoInstance, aKey, aName) { > this._base = new AccountBase(); > this._base.concreteAccount = this; >+ this._protocol = aProtoInstance; > this._base.init(aKey, aName, aProtoInstance); >+ >+ this._prefs = Services.prefs.getBranch("messenger.account." + this.id + >+ ".options."); This would be executed for all accounts when they are loaded, even those that have no protocol-specific preferences or those that are not going to auto-login. Shouldn't this rather be done with a smart getter? get prefs() this._prefs || (this._prefs = Services. ...), > }, > get base() this._base.purpleIAccountBase, > >@@ -152,6 +151,21 @@ > setBool: function(aName, aVal) this._base.setBool(aName, aVal), > setInt: function(aName, aVal) this._base.setInt(aName, aVal), > setString: function(aName, aVal) this._base.setString(aName, aVal), >+ getInt: function(aName) { >+ if (this._prefs.prefHasUserValue(aName)) >+ return this._prefs.getIntPref(aName); >+ return this.protocol._getOptionDefault(aName); >+ }, >+ getString: function(aName) { >+ if (this._prefs.prefHasUserValue(aName)) >+ return this._prefs.getCharPref(aName); >+ return this.protocol._getOptionDefault(aName); >+ }, >+ getBool: function(aName) { >+ if (this._prefs.prefHasUserValue(aName)) >+ return this._prefs.getBoolPref(aName); >+ return this.protocol._getOptionDefault(aName); >+ }, Will you hate me if I say this is duplicated code? Do you like this? getPref: function(aName, aType) this.prefs.prefHasUserValue(aName) ? this.prefs["get" + aType + "Pref"](aName) : this.protocol._getOptionDefault(aName), getInt: function(aName) this.getPref(aName, "Int"), ... >+function purplePref(name, label, type, defaultValue, masked) { Nit: I would prefer those parameters to have names like aName, aLabel, ... >+// Convert a JavaScript object mapping {"name1": "value1", "name2": "value2"} >+function purpleKeyValuePairs(obj) { >+ return new nsSimpleEnumerator( >+ Object.keys(obj).map(function(key) new purpleKeyValuePair(key, obj[key])) >+ ); >+} You can omit a pair of { } and a return if you want. Not sure if that would add clarity or confusion though. As you prefer :). >+function purpleKeyValuePair(name, value) { Nit: aName, aValue I already commented before that this comment is no longer relevant for your getOptions implementation. Either move it down above the remaining implementation stubs, or remove it. > // NS_ERROR_XPC_JSOBJECT_HAS_NO_FUNCTION_NAMED errors are too noisy >- getOptions: function() EmptyEnumerator, >+ _getOptionDefault: function(aName) { >+ throw "The preference " + aName + >+ " does not have a default value available in " + this.id + "."; If we phrase it as "aName has no default value in this.id", can it fit in a single 80 columns line? >+ return new nsSimpleEnumerator( >+ Object.keys(this._participants) >+ .map(function(key) { >+ let option = this.options[optionName]; >+ if (!((typeof option.default) in types)) >+ throw "Invalid type for preference: " + optionName + "."; >+ >+ let type = Ci.purpleIPref["type" + types[typeof option.default]]; >+ return new purplePref(optionName, >+ option.label, >+ type, >+ option.default, >+ option.masked); >+ }, this) >+ ); I'm afraid my previous review comment about using map wasn't such a good idea here. This is really not easy to read. A for in loop may be better now that it's no longer an object. By the way, what does this._participants have to do with getOptions? Copy/paste mistake?
Comment 12•10 years ago
|
||
*** Original post on bio 495 at 2010-12-31 18:21:42 UTC *** (In reply to comment #6) > >+ getString: function() this._defaultValue, > >+ getList: function() (purpleKeyValuePairs(this._defaultValue)) || > >+ (new EmptyEnumerator()), > Are you sure purpleKeyValuePairs can return null? > > > > const GenericProtocolPrototype = { > > > // NS_ERROR_XPC_JSOBJECT_HAS_NO_FUNCTION_NAMED errors are too noisy > This comment may not be relevant anymore, and is certainly not above a method > implemation that isn't a stub doing nothing ;). I think you missed these 2 comments while updating your patch.
Assignee | ||
Comment 13•10 years ago
|
||
*** Original post on bio 495 as attmnt 461 at 2010-12-31 21:07:00 UTC *** Note that this removes the purpleKeyValuePairs function and places the code inline since it's only a handful of lines anyway. I'm not positive that the "list" property works correctly because I cannot find anything about how the default value of it is chosen? Currently it just uses the first one, but when creating an account on the final page it shows up as "undefined". I'm not sure exactly what needs to be done to fix this, any guidance would help.
Attachment #8352204 -
Flags: review?(florian)
Assignee | ||
Comment 14•10 years ago
|
||
Comment on attachment 8352202 [details] [diff] [review] Patch v1.0 *** Original change on bio 495 attmnt 459 at 2010-12-31 21:07:48 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352202 -
Attachment is obsolete: true
Comment 15•10 years ago
|
||
Comment on attachment 8352204 [details] [diff] [review] Patch v1.1 *** Original change on bio 495 attmnt 461 at 2011-01-06 17:30:39 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352204 -
Flags: review?(florian) → review-
Comment 16•10 years ago
|
||
*** Original post on bio 495 at 2011-01-06 17:30:41 UTC *** Comment on attachment 8352204 [details] [diff] [review] (bio-attmnt 461) Patch v1.1 >diff --git a/purple/purplexpcom/src/jsProtoHelper.jsm b/purple/purplexpcom/src/jsProtoHelper.jsm >--- a/purple/purplexpcom/src/jsProtoHelper.jsm >+++ b/purple/purplexpcom/src/jsProtoHelper.jsm >@@ -46,6 +46,7 @@ > "GenericConvIMPrototype", > "GenericConvChatPrototype", > "GenericConvChatBuddyPrototype", >+ "purpleProxyInfo", I guess this wasn't included on purpose... >@@ -152,15 +149,26 @@ > setBool: function(aName, aVal) this._base.setBool(aName, aVal), > setInt: function(aName, aVal) this._base.setInt(aName, aVal), > setString: function(aName, aVal) this._base.setString(aName, aVal), >+ getPref: function (aName, aType) >+ this.prefs.prefHasUserValue(aName) ? >+ this.prefs["get" + aType + "Pref"](aName) : >+ this.protocol._getOptionDefault(aName), This looks too much like 3 different instructions when reading quickly. I usually write this as: a ? b : c but as the first line would be > 80 columns here, maybe we could do: a ? b : c >+ getList: function() >+ // Convert a JavaScript object map {"name1": "value1", "name2": "value2"} >+ Object.keys(this._defaultValue).length ? new nsSimpleEnumerator( >+ Object.keys(this._defaultValue) >+ .map(function(key) new purpleKeyValuePair(key, this[key]), >+ this._defaultValue) >+ ) : EmptyEnumerator, Code duplication warning! You would put the result of Object.keys(this._defaultValue) in a variable, then test it and return early if !length. (you will need to add back the {} and the explicit return keywords, but it will be much more readable.) >+function purpleKeyValuePair(aName, aValue) { >+ this.name = aName; >+ this.value = aValue; >+} >+purpleKeyValuePair.prototype = { >+ get classDescription() "Key Value Pair for Preferences", >+ getInterfaces: function(countRef) { >+ var interfaces = [Ci.nsIClassInfo, Ci.nsISupports, Ci.purpleIPref]; Your code doesn't work (the bug you mentioned in comment 10) because you forgot to replace purpleIPref by purpleIKeyValuePair here... >+ countRef.value = interfaces.length; >+ return interfaces; >+ }, >+ getHelperForLanguage: function(language) null, >+ implementationLanguage: Ci.nsIProgrammingLanguage.JAVASCRIPT, >+ flags: 0, >+ QueryInterface: XPCOMUtils.generateQI([Ci.purpleIPref, Ci.nsIClassInfo]) >+}; ... and here. >+ getOptions: function() { >+ if (!this.options) >+ return EmptyEnumerator; >+ >+ const types = {boolean: "Bool", string: "String", number: "Int", >+ object: "List"}; I would prefer wrapping the line like this: const types = {boolean: "Bool", string: "String", number: "Int", object: "List"}; >+ for (let optionName in this.options) { >+ let option = this.options[optionName]; >+ if (!((typeof option.default) in types)) { >+ throw "Invalid type for preference: " + optionName + "."; >+ continue; >+ } Do you really expect it to throw AND continue? ;) >+ purplePrefs.push(new purplePref(optionName, >+ option.label, >+ type, >+ option.default, >+ option.masked)); The parameters can all fit on 2 lines. If you don't plan to add a descriptive comment near each parameter, I would prefer: purplePrefs.push(new purplePref(optionName, option.label, type, option.default, option.masked)); I'm a bit confused by the expected value of the default property for list options. I expected it to be: default: {"value": "label", ...} When I tried your patch, I noticed you actually did it the other way around, ie. default: {"label": "value", ...} Is this on purpose? I'm afraid it may cause issues when trying to localize these strings. Well, I guess it depends on how you plan to write the localization code. You should include in this patch an example implementation of the "options" property of the protocol. I will attach shortly the changes I made to jsTestProtocol.js to test your patch.
Comment 17•10 years ago
|
||
*** Original post on bio 495 as attmnt 468 at 2011-01-06 17:33:00 UTC *** The changes I made in jsTestProtocol.js to test your patch. Note that the list option is written the way I expected it to work, not the way it actually works in your patch, so you will need to change it either here or there.
Assignee | ||
Comment 18•10 years ago
|
||
*** Original post on bio 495 as attmnt 469 at 2011-01-07 02:01:00 UTC *** (In reply to comment #11) > (From update of attachment 8352204 [details] [diff] [review] (bio-attmnt 461) [details]) > >diff --git a/purple/purplexpcom/src/jsProtoHelper.jsm b/purple/purplexpcom/src/jsProtoHelper.jsm > >--- a/purple/purplexpcom/src/jsProtoHelper.jsm > >+++ b/purple/purplexpcom/src/jsProtoHelper.jsm > >@@ -46,6 +46,7 @@ > > "GenericConvIMPrototype", > > "GenericConvChatPrototype", > > "GenericConvChatBuddyPrototype", > >+ "purpleProxyInfo", > I guess this wasn't included on purpose... Yes, I have too many patches going right now. Thanks for telling me how to wrap the lines. A lot of times I'm unsure unless it's straight forward. > >+ for (let optionName in this.options) { > >+ let option = this.options[optionName]; > >+ if (!((typeof option.default) in types)) { > >+ throw "Invalid type for preference: " + optionName + "."; > >+ continue; > >+ } > Do you really expect it to throw AND continue? ;) Well, just in case. ;) > I'm a bit confused by the expected value of the default property for list > options. > I expected it to be: > default: {"value": "label", ...} > > When I tried your patch, I noticed you actually did it the other way around, > ie. > default: {"label": "value", ...} > > Is this on purpose? I'm afraid it may cause issues when trying to localize > these strings. Well, I guess it depends on how you plan to write the > localization code. > > You should include in this patch an example implementation of the "options" > property of the protocol. I will attach shortly the changes I made to > jsTestProtocol.js to test your patch. I didn't really think about it (especially not from the localization aspect), but you're right the key should be the value, not the label/name. I've switched my code to what you suggested and included your example in my patch.
Attachment #8352212 -
Flags: review?(florian)
Assignee | ||
Comment 19•10 years ago
|
||
Comment on attachment 8352204 [details] [diff] [review] Patch v1.1 *** Original change on bio 495 attmnt 461 at 2011-01-07 02:01:02 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352204 -
Attachment is obsolete: true
Assignee | ||
Comment 20•10 years ago
|
||
Comment on attachment 8352211 [details] [diff] [review] example *** Original change on bio 495 attmnt 468 at 2011-01-07 02:01:02 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8352211 -
Attachment is obsolete: true
Assignee | ||
Comment 21•10 years ago
|
||
*** Original post on bio 495 at 2011-01-07 22:34:50 UTC *** Comment on attachment 8352212 [details] [diff] [review] (bio-attmnt 469) Patch v2.0 I just realized a small fix that we should probably include: >+ getOptions: function() { >+ if (!this.options) >+ return EmptyEnumerator; >+ >+ const types = >+ {boolean: "Bool", string: "String", number: "Int", object: "List"}; >+ >+ let purplePrefs = []; >+ for (let optionName in this.options) { >+ let option = this.options[optionName]; >+ if (!((typeof option.default) in types)) >+ throw "Invalid type for preference: " + optionName + "."; >+ >+ let type = Ci.purpleIPref["type" + types[typeof option.default]]; >+ purplePrefs.push(new purplePref(optionName, option.label, type, >+ option.default, option.masked)); >+ } >+ return new nsSimpleEnumerator(purplePrefs); >+ }, It should probably be: if (!this.options || !this.options.length)
Assignee | ||
Comment 22•10 years ago
|
||
*** Original post on bio 495 at 2011-01-07 23:06:25 UTC *** (In reply to comment #14) > (From update of attachment 8352212 [details] [diff] [review] (bio-attmnt 469) [details]) > I just realized a small fix that we should probably include: > > It should probably be: > if (!this.options || !this.options.length) Doh, I just realized that this wouldn't work. options is an object not an array. What needs to be ensured is that an EmptyEnumerator is returned if an empty array is sent to nsSimpleEnumerator.
Comment 23•10 years ago
|
||
*** Original post on bio 495 at 2011-01-14 10:07:42 UTC *** (In reply to comment #15) > (In reply to comment #14) > > (From update of attachment 8352212 [details] [diff] [review] (bio-attmnt 469) [details] [details]) > > I just realized a small fix that we should probably include: > > > > It should probably be: > > if (!this.options || !this.options.length) > > Doh, I just realized that this wouldn't work. options is an object not an > array. If the protocol plugin doesn't implement any options, this.options will be undefined here, as there's no default value in the generic prototype. Providing an empty options object should probably work and return an empty enumerator, but it doesn't matter if we are a bit slower and don't catch this in the early return, as it's a coding error anyway... > What needs to be ensured is that an EmptyEnumerator is returned if an > empty array is sent to nsSimpleEnumerator. nsSimpleEnumerator on an empty array already has the same behavior as EmptyEnumerator.
Comment 24•10 years ago
|
||
Comment on attachment 8352212 [details] [diff] [review] Patch v2.0 *** Original change on bio 495 attmnt 469 at 2011-01-14 10:34:18 UTC *** Great work here! 2 nits I'll fix before pushing: * in the "prefs" getter, ".options" should be aligned with "messanger.account." * I would prefer the example to be in jsTestProtocol rather than in overrideTestProtocol (its point is to test only the ForwardProtocolPrototype implementation).
Attachment #8352212 -
Flags: review?(florian) → review+
Assignee | ||
Comment 25•10 years ago
|
||
*** Original post on bio 495 at 2011-01-14 18:05:18 UTC *** Checked in as http://hg.instantbird.org/instantbird/rev/a188a5cc3ff1
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Updated•10 years ago
|
Target Milestone: --- → 0.3a1
You need to log in
before you can comment on or make changes to this bug.
Description
•