On 2006-05-17, at 22:58 EDT, Philip Romanik wrote:
> Hi Tucker,
>
> I have updated the code, but I have a few questions for you. When I
> refer to 'current' I mean the existing code that is checked in.
> 'New' indicates my changes:
>
> Thanks!
>
> Phil
>
>
> ==============================
>
>
> LzNode.as
> ---------
>
> 1. I am using 'in' to prevent an undefined reference. Are there any
> assumptions about the code I can make to eliminate using 'in'?
>
> Current:
> var c = arguments.callee.prototype.classChildren;
> if ( c.length ){
>
> New:
> var c = ('classChildren' in arguments.callee.prototype) ?
> arguments.callee.prototype.classChildren : null;
> if ( 'length' in c ){
In this case I think it will suffice to define:
LzNode.prototype.classChildren = [];
Then classChildren will always be defined. From looking at the uses
of classChildren, it appears the code is already careful to make a
copy of the classChildren array before modifying it:
if ( sup.prototype.classChildren &&
sup.prototype.classChildren.length > 0 ){
cc = sup.prototype.classChildren.concat();
} else{
cc = []
}
Which, if we know classChildren is always initialized can be
simplified to simply:
cc = sup.prototype.classChildren.concat();
> 2. Same question as the previous item.
> Current:
> this.setClassEvents( arguments.callee );
>
> New:
> if ('setClassEvents' in this)
> this.setClassEvents( arguments.callee );
This should not be giving a warning, since
LzNode.prototype.setClassEvents is defined. If it is giving a
warning, perhaps you can figure out why by putting in some debug
statements:
if (! ('setClassEvents' in this)) {
Debug.debug('No setClassEvents in %w', this);
}
And then inspect the object to figure out how it could not have a
setClassEvents.
> 3. I'm not sure about this one; maybe my notes are wrong.
> 'this.parent' is
> initialized to null, but I get an error if I don't use 'in'
>
> Current:
> if ( this.parent.__LZpreventSubInit ){
>
> New:
> if ( ('parent' in this) && this.parent.__LZpreventSubInit ){
Strange. Again, perhaps a debug statement would help figure out what
'this' is and how it could not have a .parent field.
> 4. How far should I go with setAttribute? If I go all the way it
> looks like,
>
> LzNode.prototype.setAttribute = function(prop, val) {
> if ( !('prop' in this.setters) || null == this.setters[ prop ] ){
> this[ prop ] = val;
> var ev = 'on' + prop;
> if (ev in this)
> this[ev].sendEvent( val );
> } else {
> this[ this.setters[ prop ] ] ( val );
> }
> }
>
> Should I remove the check on ev because DeclareEvent should/will be
> used everywhere?
No, you should keep that check. There will be 'implicit' events that
are not declared. In theory, any attribute can be listened to, so
every attribute has to see if there is a listener. We are not going
to declare an event for every attribute. The event declaration rule
is for events that are not associated with an attribute.
Don't worry about this being inefficient. I have a task to inline
setAttribute in the compiler -- and function call overhead swamps all
those other checks.
>
> UserClass.as
> ------------
>
> 5. Can the conditional be simplified by using knowledge of the
> architecture?
>
> Current:
> if ( sup.prototype.classChildren.length ){
> cc = sup.prototype.classChildren.concat();
> } else{
> cc = []
> }
>
> New:
> if ( ('classChildren' in sup.prototype) && ('length' in
> sup.prototype.classChildren) &&
> (sup.prototype.classChildren.length) ){
> cc = sup.prototype.classChildren.concat();
> } else{
> cc = []
> }
Yes. See the answer to Q1.
> LzDataElement.as
> ----------------
>
> 6. Is this right? Here's the new code (with the existing code as a
> comment
> if (children == null) {
> this.childNodes = []; // <-- Was: this.childNodes([]);
> } else {
> this.setChildNodes( children );
> }
Oh, I saw that in the review and forgot to ask. That clearly looks
like a brain-oh on someone's part. You might ask Adam, Henry and Max
directly (they are more familiar with the data code) what they think,
but it sure looks like you are right to me.
> LzDataText.lzs
> --------------
>
> 7. I added 'LzDataText.prototype.setters = {};' to the file because
> it isn't
> defined for flash. Can I remove the identical line in the if
> ($dhtml) {}
> block?
>
> if ($dhtml) {
> LzDataText.prototype.setAttribute =
> LzNode.prototype.setAttribute;
> LzDataText.prototype.setters = {};
> }
>
> LzDataText.prototype.setters = {}; // <--- New line of code
> LzDataText.prototype.setters.data = "setData";
I would just unconditionalize the $dhml code and eliminate you new
line. It seems that LzData does not descend from LzNode, hence it is
missing setters and setAttribute, so they have to be provided 'by
hand'. I would even file a bug on cleaning this up further. Perhaps
we could use traits to provide setters so that you can cleanly have
them without having to be a node. Or, I think there is another
proposal on the table to have all nodes derive from data... One way
or the other.
_______________________________________________
Laszlo-dev mailing list
[email protected]
http://www.openlaszlo.org/mailman/listinfo/laszlo-dev
