Closed Bug 954793 Opened 8 years ago Closed 8 years ago

Abstract the shared methods between ircChannel and ircConversation

Categories

(Chat Core :: IRC, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: clokep, Assigned: clokep)

Details

Attachments

(1 file, 4 obsolete files)

*** Original post on bio 1359 at 2012-04-04 12:40:00 UTC ***

The shared code between ircChannel and ircConversation is getting kind of unwieldy (with the landing of bug 954764 (bio 1332)) and we need to abstract it somehow.

Unfortunately JavaScript doesn't support multiple inheritance so there's a couple of options I see:
1. Create a GenericIRCConversation object with these methods and copy all of it's methods/properties to ircChannel/ircConversation in the constructor of each.
2. Create a GenericIRCConversation object with these methods, use this as the __proto__ of both ircChannel/ircConversation and have it set it's __proto__ dynamically to GenericConversationPrototype or GenericChatPrototype in it's init() function call (which would also call the init() function of it's prototype).
3. Mook suggested on IRC using JavaScript Proxies, I looked into these a bit and they seem confusing as hell; but maybe it's an option.

I'm willing to hear other ideas! Right now I'm leaning towards idea 2 (assuming it works).
*** Original post on bio 1359 at 2012-04-04 13:00:58 UTC ***

I would do something like this:

function conversation_sendMsg() {
  // shared code that can use "this"
}
...
ircChannel.prototype = {
  ...
  sendMsg: conversation_sendMsg,
  ...
};

ircConversation.prototype = {
  ...
  sendMsg: conversation_sendMsg,
  ...
};


(Or if you are concerned with the global namespace pollution that would cause, use this instead:
const genericConversation = {
  sendMsg: function() { /* shared code */ }
  ...
};

and then
  sendMsg: genericConversation.sendMsg,
)
*** Original post on bio 1359 at 2012-04-04 13:26:32 UTC ***

(In reply to comment #0)
> 2. Create a GenericIRCConversation object with these methods, use this as the
> __proto__ of both ircChannel/ircConversation and have it set it's __proto__
> dynamically to GenericConversationPrototype or GenericChatPrototype in it's
> init() function call (which would also call the init() function of it's
> prototype).

flo asked for an extend definition of idea 2 over IRC.

GenericIRCConversation = {
  _init: function(aAccount, aName, aNick) {
    if (aNick) {
      this.__proto__ = GenericConvChatPrototype;
      this.call(GenericConvChatPrototype.init, this, aAccount, aName, aNick);
    } else {
      this.__proto__ = GenericConvIMPrototype;
      this.call(GenericConvIMPrototype.init, this, aAccount, aName);
    }
  },
  ...
  sendMsg: function() {...}
  ...
}

function ircChannel(aAccount, aName, aNick) {
  this._init(aAccount, aName, aNick);
}
ircChannel.prototype = {
  __proto__: GenericIRCConversation,
  ...
}

function ircConversation(aAccount, aName) {
  this._init(aAccount, aName);
}
ircConversation.prototype = {
  __proto__: GenericIRCConversation,
  ...
}

Or something along those lines. It would pretty much keep us from having to overwrite each function individually, I'm not positive this will work easily though. (I'm confident it will though!)
Status: NEW → ASSIGNED
*** Original post on bio 1359 at 2012-04-04 13:29:11 UTC ***

(In reply to comment #2)
> (In reply to comment #0)
> > 2. Create a GenericIRCConversation object with these methods, use this as the
> > __proto__ of both ircChannel/ircConversation and have it set it's __proto__
> > dynamically to GenericConversationPrototype or GenericChatPrototype in it's
> > init() function call (which would also call the init() function of it's
> > prototype).
> 
> flo asked for an extend definition of idea 2 over IRC.
> 
> GenericIRCConversation = {
>   _init: function(aAccount, aName, aNick) {
>     if (aNick) {
>       this.__proto__ = GenericConvChatPrototype;

After executing this line, GenericIRCConversation is no longer in the prototype chain.
*** Original post on bio 1359 at 2012-04-04 14:17:56 UTC ***

Could 1) be combined with comment 1 along the lines of 

for (let i in GenericIRCConversation)
  if (GenericIRCConversation.hasOwnProperty(i))
    this.prototype[i] = GenericIRCConversation[i];

?
*** Original post on bio 1359 at 2012-04-04 14:19:52 UTC ***

Possibly, but the getters and setters may be annoying to do that.
Attached patch Patch v0.1 (obsolete) — Splinter Review
*** Original post on bio 1359 as attmnt 1298 at 2012-04-05 00:22:00 UTC ***

This is what I came up with as a first pass...any feedback is appreciated.
Attachment #8353051 - Flags: review?
Comment on attachment 8353051 [details] [diff] [review]
Patch v0.1

*** Original change on bio 1359 attmnt 1298 at 2012-04-11 19:57:24 UTC ***

This has bit rotted.
Attachment #8353051 - Flags: review?
*** Original post on bio 1359 at 2012-04-11 20:22:16 UTC ***

(In reply to comment #6)
If you don't copy the properties to the prototype (as in comment #4), you may break things like deleting objects which should have default values in the prototype.
Attached patch Patch v1 (obsolete) — Splinter Review
*** Original post on bio 1359 as attmnt 1456 at 2012-05-08 23:55:00 UTC ***

A patch that seems to work OK.

It copies another object into the prototype after declaring the prototype (instead of copying it each time the object is instantiated).
Attachment #8353209 - Flags: review?(florian)
Comment on attachment 8353051 [details] [diff] [review]
Patch v0.1

*** Original change on bio 1359 attmnt 1298 at 2012-05-08 23:55:34 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353051 - Attachment is obsolete: true
*** Original post on bio 1359 at 2012-05-08 23:56:28 UTC ***

(In reply to comment #9)
> Created attachment 8353209 [details] [diff] [review] (bio-attmnt 1456) [details]
> Patch v1

Also, this probably depends on the patch in bug 953761 (bio 318) to apply.
*** Original post on bio 1359 at 2012-05-29 22:05:39 UTC ***

This patch is almost definitely full of bitrot, but I'd appreciate some thoughts on whether this is acceptable before updating it. :)
*** Original post on bio 1359 at 2012-05-31 13:18:48 UTC ***

Florian thinks this style looks good, but finds the function name confusing. I'll add a comment and rename it to:
copySharedBaseToPrototype(aBase, aPrototype)

Which would be called as:
copySharedBaseToPrototype(GenericIRCConversation, ircConversation.prototype);

Or maybe just as:
copySharedBaseToPrototype(GenericIRCConversation, ircConversation);
Comment on attachment 8353209 [details] [diff] [review]
Patch v1

*** Original change on bio 1359 attmnt 1456 at 2012-05-31 13:19:04 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353209 - Flags: review?(florian)
Comment on attachment 8353209 [details] [diff] [review]
Patch v1

*** Original change on bio 1359 attmnt 1456 at 2012-05-31 13:20:50 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353209 - Flags: feedback+
Attached patch Patch v2 (obsolete) — Splinter Review
*** Original post on bio 1359 as attmnt 1539 at 2012-06-01 02:11:00 UTC ***

With a better function name and unbitrotted.
Attachment #8353294 - Flags: review?(florian)
Comment on attachment 8353209 [details] [diff] [review]
Patch v1

*** Original change on bio 1359 attmnt 1456 at 2012-06-01 02:11:33 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353209 - Attachment is obsolete: true
*** Original post on bio 1359 at 2012-06-01 09:42:06 UTC ***

Comment on attachment 8353294 [details] [diff] [review] (bio-attmnt 1539)
Patch v2

>diff --git a/chat/protocols/irc/irc.js b/chat/protocols/irc/irc.js

>+// This copies all the properties of aBase to aPrototype (which is expected to
>+// be the prototype of an object). This is necessary because JavaScript does not
>+// support multiple inheritance and both conversations objects

Not sure, but should that be "both conversation objects" instead of "both conversations objects"?

>+// shared code (but need to inherit from different XPCOM classes).

"XPCOM classes" feels wrong. maybe "XPCOM interfaces" instead? That doesn't feel exactly right either.
Or longer: "inherit from objects exposing different XPCOM interfaces"?

>+  unInitIRC: function() {

This method name was a bit surprising to me, maybe rename to unInitIRCConversation?


Looks OK (with me at least, aleth may want to have a look too) otherwise.
*** Original post on bio 1359 at 2012-06-01 10:00:59 UTC ***

We discussed this a bit on IRC, looks good.

Should _setMode be moved into this shared object as well now we have it? (I'm not sure as it doesn't really need to be a property).
Attached patch Patch v2.1 (obsolete) — Splinter Review
*** Original post on bio 1359 as attmnt 1541 at 2012-06-01 10:41:00 UTC ***

Fixed comment & updated method name.

I haven't moved _setMode into the prototype (I thought about it), because I expected it'll be needed for the account object as wel.
Attachment #8353296 - Flags: review?(florian)
Comment on attachment 8353294 [details] [diff] [review]
Patch v2

*** Original change on bio 1359 attmnt 1539 at 2012-06-01 10:41:21 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353294 - Attachment is obsolete: true
Attachment #8353294 - Flags: review?(florian)
Comment on attachment 8353296 [details] [diff] [review]
Patch v2.1

*** Original change on bio 1359 attmnt 1541 at 2012-06-01 10:43:44 UTC ***

Looks good, thanks! (note: I haven't tested this at all.)
Attachment #8353296 - Flags: review?(florian) → review+
*** Original post on bio 1359 as attmnt 1547 at 2012-06-01 23:13:00 UTC ***

Unbitrotted (I hope).
Attachment #8353302 - Flags: review?(florian)
Comment on attachment 8353296 [details] [diff] [review]
Patch v2.1

*** Original change on bio 1359 attmnt 1541 at 2012-06-01 23:13:45 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353296 - Attachment is obsolete: true
*** Original post on bio 1359 at 2012-06-01 23:29:50 UTC ***

Committed as http://hg.instantbird.org/instantbird/rev/f25198c3c04a
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.2
Comment on attachment 8353302 [details] [diff] [review]
Patch v2.1 unbitrotted

*** Original change on bio 1359 attmnt 1547 at 2012-06-02 21:26:05 UTC ***

Attachment 8353296 [details] [diff] (bio-attmnt 1541) is what I actually reviewed here; removing the flag from attachment 8353302 [details] [diff] [review] (bio-attmnt 1547).
Attachment #8353302 - Flags: review?(florian)
You need to log in before you can comment on or make changes to this bug.