Not approved, yet. This is great work, but a big change to the core,
so we need to be careful.
For testing, I would ensure that all the demo apps still work as
expected. I realize this assumes you know how the demos are expected
to work. Perhaps Amy could commandeer some QA resources to do this.
Basically, my concern here is that we are mucking with the core of
the system, and we need more coverage than just the smoke test to
verify that we have not broken anything.
Also, I spotted a few things that I would do slightly differently,
and what I think might be a pervasive transformation error. In a
number of places you have transformed:
if (foo.bar)
into:
if ('bar' in foo)
but that is not semantically equivalent. You would have to say:
if ('bar' in foo && foo.bar)
I think there may be places where because of surrounding code the
more verbose transformation is not needed, but I would like you to
double-check the places where you have made this change. I call out
some particular cases I spotted, below; but I think a thorough check
is warranted.
Detailed comments:
In LzView Would this:
< this.immediateparent.onremovesubview.sendEvent( this );
---
> if ('onremovesubview' in this.immediateparent)
> this.immediateparent.onremovesubview.sendEvent( this );
be better handled by declaring the event?
Here:
901c951,952
< this.__LZmaskClip._yscale = v;
---
> if (this.__LZmaskClip && typeof(this.__LZmaskClip._yscale) !=
'undefined')
> this.__LZmaskClip._yscale = v;
906,907c957,960
< this.setButtonSize( "height" , v );
< this.__LZbgRef._yscale = v;
---
> if (this.setButtonSize)
> this.setButtonSize( "height" , v );
> if (typeof(this.__LZbgRef._yscale) != 'undefined')
> this.__LZbgRef._yscale = v;
909c962,963
you used typeof != 'undefined', rather than `in`. `in` should be
more efficient.
In LzDatasource:
< if (typeof(forset.timeout) != "undefined" && forset.timeout !=
null) {
---
> if (tloader && typeof(forset.timeout) != "undefined" &&
forset.timeout != null) {
I would use `in` rather than typeof.
In LzFocus:
< if ( v.focusable ){
---
> if ( 'focusable' in v ){
appears to change the sense of the test. `in` only tells you the
property is there, if the property can be true or false, you would
need to say ('focusable' in v && v.focusable). [Perhaps in this
particular case focusable should be a defaulted property?]
The same error appears in LzIdle:
< if (! this.regNext ){
---
> if (!('regNext' in this)) {
I won't claim that I have spotted all of these, so you should double-
check all the places where you have used this idiom to make sure.
Same issue in LzInput6. Maybe in LzLoader, LzModeManager, LzNode,
too. In some of these places, there may be further logic that tests
the actual value.
In LzNode:
< this[ "on" + a ].sendEvent( args[ a ] );
---
> if (("on" + a) in this)
> this[ "on" + a ].sendEvent( args[ a ] );
> // this[ "on" + a ].sendEvent( args[ a ] );
Our compiler is only about as smart as an early 70's C compiler, so
you should do your own common sub-exprs:
var ev = 'on' + a;
if (ev in this) { this[ev].sendEvent(args[a]) };
In UserClass:
< if ( sup.prototype.classChildren.length ){
---
> if ( ('classChildren' in sup.prototype) && ('length' in
sup.prototype.classChildren) && (sup.prototype.classChildren.length) ){
I wonder if classChildren should be defaulted in the prototype?
On 2006-05-16, at 13:39 EDT, Philip Romanik wrote:
> I'm attaching a new changelist for this bug. 7 files differ from
> the earlier changelist
>
> LzNode.as
> LzDataset.as
> LzLoader.as
> LzHistory.as
> LzLoadQueue.as
> LaszloCanvas.as
> LaszloView.as
>
> These files now use DeclareEvent() to declare events, and the
> conditional (to check for null) before calling sendEvent() has been
> removed.
>
> Phil
>
>
>> Change 42262 by [EMAIL PROTECTED] on 2006/05/15 17:17:10 *pending*
>>
>> Summary: Cleanup LFC Warnings
>>
>> Bugs Fixed:
>> LPP-2034 Cleanup LFC Warnings
>>
>> Technical Reviewer: ptw (pending)
>> QA Reviewer: jgrandy (pending)
>> Doc Reviewer: jsundman (pending)
>>
>> Documentation:
>>
>> I followed the instructions listed in LPP-2034 to build a
>> runtime
>> that reports warnings and errors when simple.lzw is
>> executed. Most
>> errors are caused by uninitialized variables or improperly
>> testing
>> if a property or method is defined.
>>
>> In some cases I added initial values to the object and set
>> them to
>> null. For example, many classes call events that have not
>> been
>> initialized. In addition to initializing them to null, the
>> code now
>> checks by,
>>
>> if (this.onload)
>> this.onload.sendEvent (this);
>>
>> In other places, the 'in' operator is used to test a variable
>> before it is used. I only changed code that reported a
>> warning
>> or error, or was similar to the code that produced a warning
>> or error.
>>
>> Tests:
>>
>> test/smoke/simple.lzx
>> test/smoke/delay.lzx
>>
>> Affected files ...
>>
>> //depot/lps-legals/WEB-INF/lps/lfc/core/Library.lzs#3 edit
>> //depot/lps-legals/WEB-INF/lps/lfc/core/LzClass.as#6 edit
>> //depot/lps-legals/WEB-INF/lps/lfc/core/LzNode.as#7 edit
>> //depot/lps-legals/WEB-INF/lps/lfc/core/UserClass.as#5 edit
>> //depot/lps-legals/WEB-INF/lps/lfc/data/LzDataElement.lzs#2 edit
>> //depot/lps-legals/WEB-INF/lps/lfc/data/LzDataText.lzs#2 edit
>> //depot/lps-legals/WEB-INF/lps/lfc/data/LzDatapath.as#2 edit
>> //depot/lps-legals/WEB-INF/lps/lfc/data/LzDataset.as#3 edit
>> //depot/lps-legals/WEB-INF/lps/lfc/data/LzDatasource.as#4 edit
>> //depot/lps-legals/WEB-INF/lps/lfc/data/LzHTTPDatasource.as#3 edit
>> //depot/lps-legals/WEB-INF/lps/lfc/data/LzLoader.as#4 edit
>> //depot/lps-legals/WEB-INF/lps/lfc/events/LaszloEvents.lzs#3 edit
>> //depot/lps-legals/WEB-INF/lps/lfc/glue/LaszloInitiator.as#3 edit
>> //depot/lps-legals/WEB-INF/lps/lfc/services/LzBrowser.as#2 edit
>> //depot/lps-legals/WEB-INF/lps/lfc/services/LzFocus.as#3 edit
>> //depot/lps-legals/WEB-INF/lps/lfc/services/LzFontManager.as#2 edit
>> //depot/lps-legals/WEB-INF/lps/lfc/services/LzHistory.as#3 edit
>> //depot/lps-legals/WEB-INF/lps/lfc/services/LzIdle.as#1 edit
>> //depot/lps-legals/WEB-INF/lps/lfc/services/LzInstantiator.as#3 edit
>> //depot/lps-legals/WEB-INF/lps/lfc/services/LzKeys.as#5 edit
>> //depot/lps-legals/WEB-INF/lps/lfc/services/LzLoadQueue.as#6 edit
>> //depot/lps-legals/WEB-INF/lps/lfc/services/LzModeManager.as#2 edit
>> //depot/lps-legals/WEB-INF/lps/lfc/services/LzTimer.as#2 edit
>> //depot/lps-legals/WEB-INF/lps/lfc/views/LaszloCanvas.as#5 edit
>> //depot/lps-legals/WEB-INF/lps/lfc/views/LaszloView.as#11 edit
>> <changeset-42262.zip>
_______________________________________________
Laszlo-dev mailing list
[email protected]
http://www.openlaszlo.org/mailman/listinfo/laszlo-dev
