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
