868203 - MozElement subclasses should inherit via Object.create()

Status: RESOLVED FIXED

(Testing Graveyard :: Mozmill, defect)

Description

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Right now the MozElement subclasses inherit via the property chain which should not be used anymore given that you will have to call the constructor of the base class to populate the prototype of the subclass. This is always causing trouble if code in the base constructor is awaiting parameters.

This mainly blocks the work on bug 761600.

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Patch v1

With those changes we do no longer unnecessarily call the constructor of MozMillElement.

Comment 2

[cmtalbert]

Comment on attachment 744879 [details] [diff] [review]
Patch v1

Review of attachment 744879 [details] [diff] [review]:
-----------------------------------------------------------------

I don't get this at all, I'm afraid. In what way does this block 761600? That patch required no new parameters to the constructor of MozElement, and in both versions (the old version of this code and the new version of this patch) you must call the constructor of MozElement. You simply have to call the constructor, there is no way around that.

But, given that you already refactored it, and I find it a very slight readability win over the original version of the code, we can take it.  But, please explain the benefits here because I really don't see them at all. r+, pending explanation.

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Clint Talbert ( :ctalbert ) from comment #2)
> I don't get this at all, I'm afraid. In what way does this block 761600?
> That patch required no new parameters to the constructor of MozElement, and
> in both versions (the old version of this code and the new version of this
> patch) you must call the constructor of MozElement. You simply have to call
> the constructor, there is no way around that.

The problem in using the prototype chain in creating new subclasses is that the constructor of the base class is called n+1 times. Each time when you want to get the prototype of the base class and when you create an instance of the class later. The problematic line is:

MozMillCheckBox.prototype = new MozMillElement();

That line issues a call of the constructor of MozMillElement with no parameters given, simply to get it's prototype object. It will cause major issues.  

A good article I just found and which will help you to understand how the ECMA 5 Object.create() method works is: http://dailyjs.com/2012/06/04/js101-object-create/

> But, given that you already refactored it, and I find it a very slight
> readability win over the original version of the code, we can take it.  But,
> please explain the benefits here because I really don't see them at all. r+,
> pending explanation.

This change has nothing to do with a readability win but fixes an underlying problem with inheritance. And with it we could make use of even more features like enumarable, setters/getters, and scopes. Please check the above mentioned article for details about this too.

I hope that clarifies your questions.

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+1]]

I pushed this to master:
https://github.com/mozilla/mozmill/commit/8b14645a1efe38e422ce92b009ed42e098a9944c

Comment 5

[cmtalbert]

(In reply to Henrik Skupin (:whimboo) from comment #3)
> A good article I just found and which will help you to understand how the
> ECMA 5 Object.create() method works is:
> http://dailyjs.com/2012/06/04/js101-object-create/

Thanks, that does make this make a lot more sense.  I like these changes more now. :-)

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Sweet! :) And sorry for not providing that information earlier. I thought you would already know the new APIs.