Approved. On 2010-04-24, at 18:34, Max Carlson wrote:
> Change 20100421-maxcarlson-B by maxcarl...@friendly on 2010-04-21 16:42:17 PDT > in /Users/maxcarlson/openlaszlo/trunk-clean > for http://svn.openlaszlo.org/openlaszlo/trunk > > Summary: UPDATED ^2: Update construct() to halt early for placed nodes > > Bugs Fixed: LPP-8929 - Prevent construction of destroyed LzNode subclasses > > Technical Reviewer: [email protected] > QA Reviewer: ptw > > Details: Updated to address Tucker's review: > Question: > > 1) This only addresses the issue of placement aborting construction. We also > have the issue of replication aborting construction. Is there a similar > public interface that replication is using that needs similar treatment, so > we don't need so many checks against __LZdeleted sprinkled throughout the LFC? > > I wasn't able to pull this off in swf8 - there was an issue with using too > many try/catch/throws(). > > Issues: > > 1) I would rather you use a distinct object than a string for your throw > value: Something like: > > var __LzConstructAbort = { toString: function () { return 'Constructor > Abort'; } }; > > Done. > > 2) If you catch something _other_ than this distinguished object, you should > re-throw it, so you don't silently ignore errors. (If the language were more > powerful, you could ask to only catch your specific throw, but JS is not.) > > Fixed. > > 3) You could simplify the control flow to just: > > if (this.__LZdeleted) { throw __LzConstructAbort; } > > Otherwise the same except the changeset was titled 'Update basecomponent to > not set styles, construct() to automatically halt for deleted nodes': > > Broke into separate changesets, updated to address Andre's issues: > > 2) strict equality check to avoid string coercion, re-throw error if e !=== > '__LZdeleted'! >> + } catch(e) { >> + // Construct may, through many tangled webs of replication and >> + // placement, actually end up deleting us! Bail out completely. >> + if (e == '__LZdeleted') { >> + return; >> + } >> + } > > Fixed. > > 3) There's an important devnote right before the added > "throw('__LZdeleted');": >> // @devnote only add to subnodes if this node is not deleted which >> // may happen as a side-effect of calling determinePlacement(). >> // We still need to set 'immediateparent' because legacy constructors >> // expect to see this property. > > I cleaned up the devnote and removed the unnecessary __LZdeleted test. > > LzNode - Throw a custom exception when a node is deleted due to placement > changes to halt any superclass construct() calls. Catch exceptions in > construct() and forward if they're not our custom __LZdeleted exception. > > LzInputText, LzText, LaszloView - Remove unneeded __LZdeleted test in > construct(). > > LzDatapath - Remove extra test for onDocumentChange.ready > > LzSelectionManager - Prevent null dereference for invalid selections > > LzDataSelectionManager, LzSelectionManager, LzState, LaszloLayout, > LzDatapointer, LzDataAttrBind - Return from destroy if __LZdeleted. > > Tests: See examples/components/component_sampler.lzx?debug=true, smokecheck > in all runtimes > > Files: > M WEB-INF/lps/lfc/core/LzNode.lzs > M WEB-INF/lps/lfc/views/LzInputText.lzs > M WEB-INF/lps/lfc/views/LzText.lzs > M WEB-INF/lps/lfc/views/LaszloView.lzs > M WEB-INF/lps/lfc/helpers/LzDataSelectionManager.lzs > M WEB-INF/lps/lfc/helpers/LzSelectionManager.lzs > M WEB-INF/lps/lfc/helpers/LzState.lzs > M WEB-INF/lps/lfc/controllers/LaszloLayout.lzs > M WEB-INF/lps/lfc/data/LzDatapointer.lzs > M WEB-INF/lps/lfc/data/LzDatapath.lzs > M WEB-INF/lps/lfc/data/LzDataAttrBind.lzs > > Changeset: > http://svn.openlaszlo.org/openlaszlo/patches/20100421-maxcarlson-B.tar >
