Max, clearly these all need to be addressed, either before you check in or in
an amendment to your checkin.
Thank you as usual André for a most thorough review!
On 2010-03-29, at 16:45, André Bargull wrote:
> Did anybody said comments are no longer allowed to delay the checkin? :-P
>
> LzTextSprite (swf8):
>> + if (! this['scrolldel']) {
>> + this.scrolldel = new LzDelegate(this, "__LZupdateScrollAttrs");
>> + }
> and later:
>> + if (this.scrolldel.hasevents) {
>> + this.scrolldel.destroy();
>> + }
> I'm asking myself why you've used the "save" property access (bracket syntax)
> when no harm can occur, but later the "brute force" method (scrolldel could
> be undefined)? I'd just remove the bracket syntax and use direct property
> access.
>
>
> LzXMLLoader
>> + // To keep delegates from resurrecting us. See LzDelegate#execute
>> + this.__LZdeleted = true;
> LzXMLLoader is a LzLoader, which is a LzEventable, so you must not set
> _LZdeleted.
>
> LzDelegate
>> - } else {
>> - keep.push( ev );
>> + this.__events.splice(i, 1);
>> + break;
>> }
> You cannot use 'break' here, a delegate can be registered to an event more
> than once.
>
> LzHTTPDataProvider
>> + /**
>> + * @access private
>> + * @param LzHTTPDataRequest dreq:
>> + * @param LzDataElement data:
>> + */
>> + override function destroy() {
> Wrong comment..
>
> LzDataset
>> override function destroy () {
>> this.$lzc$set_childNodes([ ]);
>> + this.dataprovider.destroy();
> You must not destroy the dataprovider, dataproviders are singleton objects!
> For example the default dataprovider for a dataset is
> canvas.defaultdataprovider, so all datasets share the same dataprovider!
>
>
>
>
>
> On 3/29/2010 8:05 PM, P T Withington wrote:
>> Approved.
>>
>> Let's check this in so we can get some QA on it (and before someone thinks
>> up another amendment to delay the vote).
>>
>> Nice work on a thorny problem!
>>
>> On 2010-03-24, at 17:45, Max Carlson wrote:
>>
>>
>>> Change 20100314-maxcarlson-0 by maxcarl...@bank on 2010-03-14 00:54:51 PST
>>> in /Users/maxcarlson/openlaszlo/trunk-clean
>>> for http://svn.openlaszlo.org/openlaszlo/trunk
>>>
>>> Summary: UPDATE 6: Automatically remove invalid delegates in
>>> LzEventable.destroy()
>>>
>>> Bugs Fixed: LPP-8822 - Improve layout performance
>>>
>>> Technical Reviewer: ptw
>>> QA Reviewer: hminsky
>>>
>>> Release Notes: It is no longer necessary to manually track and release
>>> delegates whose target is 'this' to prevent memory leaks. This is handled
>>> automatically by the event system.
>>>
>>> Details: Updated to address Tucker's comments:
>>> General comment:
>>>
>>> It really sucks that LzDataAttrBind pretends to be an LzDelegate but isn't
>>> really. We probably should use interface/implements to make that more
>>> explicit. At least then the swf10 back-end could give us some help.
>>>
>>> Nits:
>>>
>>> 1) LzEventable#105 Not sure what this means:
>>>
>>>
>>>>> // uncomment to LFC delegates cleaned up, e.g. datapath and inputtext
>>>>> //if ($debug) Debug.warn('removing %w delegates from %w.',
>>>>> this.__delegates.length, this);
>>>>>
>>> I removed this.
>>>
>>> 2) LzNode#400:
>>>
>>> /** Tracks constraint delegates
>>> @access private */
>>>
>>> Might be better to say:
>>>
>>> /** Tracks delegates and data bindings for states so they can be
>>> applied/removed
>>>
>>> because that is what we have reduced this to. Perhaps someday we can come
>>> up with a less kludgey mechanism than state reaching out and grabbing these
>>> things from the node constructor.
>>>
>>> Done.
>>>
>>> Issues:
>>>
>>> 1) LZNode#2080
>>>
>>>>> //remove __LZconstraintdelegates
>>>>> // We have to do this to remove LzDataAttrBinds
>>>>>
>>> I don't follow. It seems to me that in LzNode/dataBindAttribute, the
>>> LzDataAttrBind object needs to go on __LZconstraintDelegates, so it can be
>>> applied/unapplied by LzState. But LzDataAttrBind's constructor should
>>> register the binding on __delegates so that it can be auto-cleaned when the
>>> node it is binding is destroyed. We really want to separate these two
>>> tasks so future generations don't have to work this out again. Then you
>>> can truly just let __LZconstraintdelegates drop on the floor in
>>> LzNode/destroy.
>>>
>>> Yeah, you're right! Done.
>>>
>>> 2) LzDataAttrBind/unregisterAll: Does anyone actually call this any more?
>>> LzEventable/destroy should just be calling LzDataAttrBind/destroy
>>>
>>> Removed unregisterAll().
>>>
>>> 3) LaszloEvents#227: There needs to be a comment here. Something like
>>> "Delegates between LzEventables can cause memory leaks. We record such
>>> delegates so they can be cleaned up in destroy".
>>>
>>> Done.
>>>
>>> 4) LaszloEvents#227: Do you not have to test that the context _is_
>>> actually an LzEventable here? At least in the runtimes that don't enforce
>>> types?
>>>
>>> Ooh good point! I improved the testing and ensured LzDelegate.__tracked is
>>> correctly maintained.
>>>
>>> 5) LaszloView#1795: Why did you un-deprecate releaseLayouts? I thought we
>>> were trying to get rid of this?
>>>
>>> I removed it since it's deprecated in 4.7.1 - but there doesn't seem to be
>>> a replacement. It could be useful when you want to move things around
>>> without layouts interfering - but you could lock/unlock the layouts
>>> instead. Maybe we should make the layouts array public so folks could do
>>> this? The replication manager does this during updates...
>>>
>>> Otherwise, the same as r5 (Updated to address Andre's comments):
>>> LzDelegate:
>>>
>>>> + if (eventSender !== this.c) {
>>>> + var _dels:Array = this.c['__delegates'];
>>>> + if (_dels == null) {
>>>> + this.c.__delegates = [ this ];
>>>> + } else {
>>>> + _dels.push( this );
>>>> + }
>>>> + }
>>>>
>>> 1) This will create multiple entries for each delegate, you should add a
>>> boolean flag to track whether the delegate was already added to the
>>> __delegates array to prevent more than one entry.
>>>
>>> Done.
>>>
>>> 2) What about the issue with the swf8 kernel using LzDelegates?
>>>
>>> I updated the swf8 kernel to be very careful about unregistering all
>>> delegates used on instances.
>>>
>>>
>>> LzDelegate:
>>>
>>>> +public function destroy ():void {
>>>> + this.unregisterAll();
>>>> + this.c = null;
>>>> + this.m = null;
>>>> +}
>>>>
>>> Please add "@access private" to hide the function from the docs - can you
>>> remind me why functions are added to the docs by default?! There is a JIRA
>>> issue to switch this default behaviour, isn't it?
>>>
>>> Done. I think it's that way to force developers to notice that they need
>>> to write documentation...
>>>
>>> LzNode:
>>>
>>>> + for ( var i = dels.length - 1; i>= 0; i-- ){
>>>> + if (dels[i].c) {
>>>> + dels[ i ].destroy();
>>>> + }
>>>> }
>>>>
>>> Won't work for LzDataAttrBind - there is no "c" attribute, so the if-test
>>> will fail.
>>>
>>> Done. I added a __LZdeleted flag to LzDelegate that can be used instead.
>>>
>>> LaszloEvents - Add __events array to track events registered on the
>>> delegate and make LzDelegate static. Add hasevents boolean to track
>>> whether calls to unregisterAll() are needed. Add __tracked flag to prevent
>>> adding to __delegates more than once. Add __LZdeleted flag to track when
>>> to destroy(), mirroring LzEventable.
>>>
>>> LzHTTPDataProvider - Track LzHTTPLoaders and call destroy() on each one
>>> when destroyed.
>>>
>>> LzDataset - Call destroy() on the dataprovider when destroyed.
>>>
>>> kernel/*/LzHTTPLoader - Add destroy() method which is called when a loader
>>> should be destroyed. Add stub methods for swf9 and dhtml.
>>>
>>> LzState, LzNode - Use LzDelegate/LzDataAttrBind.__LZdeleted to decide
>>> whether to call destroy().
>>>
>>> kernel/swf/* - Ensure all delegates not used by global services are tracked
>>> and destroyed. Use new hasevents flag to avoid unneeded calls to
>>> unregisterAll().
>>>
>>> Otherwise, the same:
>>>
>>> LzNode,LzState - Rename __LZdelegates -> __LZconstraintdelegates, shorten
>>> array lookups where possible, use LzDelegate.destroy() instead of
>>> unregisterAll().
>>>
>>> LaszloEvents - Add check/warning when a delegate is created for an invalid
>>> context. Move __delegates tracking to register() and only add LzEventables
>>> that are registering for a different context. Add warnings for unneeded
>>> unregisterAll() calls, but comment them out due to too many warnings. Add
>>> destroy() API that neuters and unregisters the delegate.
>>>
>>> LzDataAttrBind - Add destroy() API for symmetry with LzDelegate.
>>>
>>> LzDatapath - Use super.destroy() for efficiency.
>>>
>>> LaszloView - releaseLayouts() now calls layout.destroy().
>>>
>>> LzState - Rename __LZstatedelegates -> __LZstateconstraintdelegates.
>>>
>>> Otherwise, the same as before:
>>>
>>> LzInputText,LzAnimatorGroup,LzDatapointer,LzDataset - Remove extra delegate
>>> unregistration as it's now handled by the event system.
>>>
>>> LzEventable - Add _delegates array to hold delegates that have a context
>>> pointing to this object. Clean out delegate contexts in destroy().
>>>
>>> LaszloEvents - LzDelegate registers on its contexts' _delegates array.
>>>
>>> LaszloLayout,resizelayout - don't track delegates anymore.
>>>
>>>
>>> Tests: Run modified leak-components test in swf10 - the memory monitor in
>>> the upper left shows much less leakage with this patch: you can click the
>>> monitor to force garbage collection. Also run smokecheck across runtimes.
>>>
>>> Files:
>>> M test/components/lz/leak-components.lzx
>>> M WEB-INF/lps/lfc/kernel/swf/LzTextSprite.as
>>> M WEB-INF/lps/lfc/kernel/swf/LzMediaLoader.lzs
>>> M WEB-INF/lps/lfc/kernel/swf/LzInputTextSprite.as
>>> M WEB-INF/lps/lfc/kernel/swf/LzXMLLoader.lzs
>>> M WEB-INF/lps/lfc/kernel/swf/LzMakeLoadSprite.as
>>> M WEB-INF/lps/lfc/kernel/swf/LzHTTPLoader.as
>>> M WEB-INF/lps/lfc/kernel/swf/LzSprite.as
>>> M WEB-INF/lps/lfc/kernel/dhtml/LzHTTPLoader.js
>>> M WEB-INF/lps/lfc/kernel/swf9/LzHTTPLoader.as
>>> M WEB-INF/lps/lfc/core/LzNode.lzs
>>> M WEB-INF/lps/lfc/core/LzEventable.lzs
>>> M WEB-INF/lps/lfc/views/LzInputText.lzs
>>> M WEB-INF/lps/lfc/views/LaszloView.lzs
>>> M WEB-INF/lps/lfc/helpers/LzState.lzs
>>> M WEB-INF/lps/lfc/events/LaszloEvents.lzs
>>> M WEB-INF/lps/lfc/controllers/LzAnimatorGroup.lzs
>>> M WEB-INF/lps/lfc/controllers/LaszloLayout.lzs
>>> M WEB-INF/lps/lfc/data/LzDatapointer.lzs
>>> M WEB-INF/lps/lfc/data/LzHTTPDataProvider.lzs
>>> M WEB-INF/lps/lfc/data/LzDataset.lzs
>>> M WEB-INF/lps/lfc/data/LzDatapath.lzs
>>> M WEB-INF/lps/lfc/data/LzDataAttrBind.lzs
>>> M docs/src/developers/delegates.dbk
>>> M lps/components/rpc/ajax.lzx
>>> M lps/components/extensions/html.lzx
>>> M lps/components/utils/layouts/resizelayout.lzx
>>> M lps/components/utils/replicator/replicator.lzx
>>> M lps/components/av/rtmpstatus.lzx
>>> M lps/components/base/basetabelement.lzx
>>> M lps/components/base/basescrollbar.lzx
>>> M lps/components/base/basecomponent.lzx
>>> M lps/components/base/basewindow.lzx
>>> M lps/components/base/submit.lzx
>>> M lps/components/base/basetrackgroup.lzx
>>> M lps/components/base/basedatacombobox.lzx
>>>
>>> Changeset:
>>> http://svn.openlaszlo.org/openlaszlo/patches/20100314-maxcarlson-0.tar
>>>
>>>
>>
>>
>>
