954793 - Abstract the shared methods between ircChannel and ircConversation

Status: RESOLVED FIXED

(Chat Core :: IRC, defect)

Description

[Patrick Cloke [:clokep]]

*** 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).

Comment 1

[Florian Quèze [:florian]]

*** 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,
)

Comment 2

[Patrick Cloke [:clokep]]

*** 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!)

Comment 3

[Florian Quèze [:florian]]

*** 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.

Comment 4

[aleth [:aleth]]

*** 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];

?

Comment 5

[Florian Quèze [:florian]]

*** 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.

Comment 6

[Patrick Cloke [:clokep]]

Attachment: Patch v0.1

*** 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.

Comment 7

[Patrick Cloke [:clokep]]

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.

Comment 8

[aleth [:aleth]]

*** 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.

Comment 9

[Patrick Cloke [:clokep]]

Attachment: Patch v1

*** 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).

Comment 10

[Patrick Cloke [:clokep]]

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 ***

Comment 11

[Patrick Cloke [:clokep]]

*** 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.

Comment 12

[Patrick Cloke [:clokep]]

*** 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. :)

Comment 13

[Patrick Cloke [:clokep]]

*** 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 14

[Patrick Cloke [:clokep]]

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 ***

Comment 15

[Florian Quèze [: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 ***

Comment 16

[Patrick Cloke [:clokep]]

Attachment: Patch v2

*** Original post on bio 1359 as attmnt 1539 at 2012-06-01 02:11:00 UTC ***

With a better function name and unbitrotted.

Comment 17

[Patrick Cloke [:clokep]]

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 ***

Comment 18

[Florian Quèze [:florian]]

*** 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.

Comment 19

[aleth [:aleth]]

*** 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).

Comment 20

[Patrick Cloke [:clokep]]

Attachment: Patch v2.1

*** 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.

Comment 21

[Patrick Cloke [:clokep]]

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 ***

Comment 22

[Florian Quèze [: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.)

Comment 23

[Patrick Cloke [:clokep]]

Attachment: Patch v2.1 unbitrotted

*** Original post on bio 1359 as attmnt 1547 at 2012-06-01 23:13:00 UTC ***

Unbitrotted (I hope).

Comment 24

[Patrick Cloke [:clokep]]

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 ***

Comment 25

[Patrick Cloke [:clokep]]

*** Original post on bio 1359 at 2012-06-01 23:29:50 UTC ***

Committed as http://hg.instantbird.org/instantbird/rev/f25198c3c04a

Comment 26

[Florian Quèze [:florian]]

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).