Change maxcarlson-20100923-cPE by [email protected] on 2010-09-23 10:50:36 PDT in /Users/maxcarlson/openlaszlo/trunk-clean for http://svn.openlaszlo.org/openlaszlo/trunk
Summary: Clean up lazy click div creation per Tucker's feedback Bugs Fixed: LPP-6892 - Generated File Sizes baseline complete; Performance Tuning ongoing Technical Reviewer: ptw QA Reviewer: hminsky Overview: >From email: On Sep 22, 2010, at 2:38 PM, P T Withington <[email protected]> wrote: > > On 2010-09-22, at 17:32, Max Carlson wrote: > > >> >> On 9/22/10 2:23 PM, P T Withington wrote: >>> >>> Not approved. >>> >>> >>> >>> Issues: >>> >>> >>> >>> 1) There seems like there is a type-oh here. You refer to both >>> >>> `this.clip` and `this._clip` in your test, but you only seem to update >>> >>> `this._clip`? Do we really need both? >> >> >> >> We need both: ._clip has the css representation, while .clip has the >> >> value set by the user when they called setClip(). This is like ._width >> >> and .width... > > > > That doesn't answer my question about the non-parallel code below: > > >>> >>> +LzSprite.prototype._clip = ''; >>> >>> LzSprite.prototype.__updateClip = function() { >>> >>> var quirks = this.quirks; >>> >>> if (this.isroot&& this.quirks.canvas_div_cannot_be_clipped) return; >>> >>> + // default >>> >>> + var clip = ''; >>> >>> if (this.clip&& this.width != null&& this.width>= 0&& this.height >>> >>> != null&& this.height>= 0) { > > > > Here you test clip > > >>> >>> + // have clip, set it >>> >>> + clip = 'rect(0px ' + this._w + ' ' + this._h + ' 0px)'; >>> >>> + } else if (this._clip) { > > > > Here you test _clip > > >>> >>> + // had clip, clear it >>> >>> + clip = quirks.fix_ie_css_syntax ? 'rect(auto auto auto auto)' >>> >>> : ''; >>> >>> + } >>> >>> + if (clip !== this._clip) { >>> >>> + this._clip = clip; > > > > Here you update _clip but not clip. > > > > It seems like this code should either consistently use _clip or clip but > > not one in one place and the other in another place? Ah - right! >>> >>> + this.__LZdiv.style.clip = clip >>> >>> } else { >>> >>> // return so we don't set the containers >>> >>> return; >>> >>> } >>> >>> >>> >>> Questions: >>> >>> >>> >>> 1) Am I understanding correctly here: >>> >>> >>> >>> + var parentcontainer = sprite.__parent&& >>> >>> sprite.__parent[propname]; >>> >>> + if (parentcontainer) { >>> >>> + parentcontainer.appendChild(newdiv); >>> >>> + } >>> >>> >>> >>> parentcontainer will only not exist for the root sprite? Can we have a >>> >>> comment to that effect? >> >> >> >> Actually, parentcontainer can be undefined in a lot of cases. I'll >> >> comment about that. > > > > Really? I guess I don't understand what this code is doing then. I > > thought you needed a chain of containers back to the root? If you don't, > > then why are we iterating through the parents creating containers? Yeah it was kind of unexpected but there are cases where a tree of divs can be created before they're attached to the DOM. Just the way views work I guess... When the sprite is added as a child, the code there ensures it has a valid click tree that does go all the way to the canvas. > > >>> >>> 2) The `||` clauses here: >>> >>> >>> >>> + var left = sprite._x || 0; >>> >>> + if (left) { >>> >>> + to.style.left = left; >>> >>> } >>> >>> + var top = sprite._y || 0; >>> >>> + if (top) { >>> >>> + to.style.top = top; >>> >>> + } >>> >>> + var display = sprite.__csscache.__LZdivdisplay || ''; >>> >>> + if (display) { >>> >>> + to.style.display = display; >>> >>> + } >>> >>> >>> >>> have no effect, since they are falsey values, they will never be >>> >>> installed. If it is important that they get installed, you need to >>> >>> take out the `if`. If not, you might as well delete the `||` clauses. >> >> >> >> Good point! >> >> >>> >>> 3) There are a lot of places where you are testing >>> >>> `(this.quirks.fix_clickable&& this.__LZclickcontainerdiv)`. Is the >>> >>> first test actually necessary? You will never have a clickcontainer >>> >>> div if the quirk is off will you? Maybe the same for >>> >>> `(this.quirks.fix_contextmenu&& this.__LZcontextcontainerdiv)`? >> >> >> >> I was trying to make the code a little more self-documenting, via the >> >> quirks. But you're right, we don't strictly need to do these tests... > > > > Ok, well, that's fine if you leave them; just was wondering because > > elsewhere we are trying to squeeze every last cycle out of the kernel code No I'll take 'em out! Details: Remove extra tests for this.quirks.fix_clickable and this.quirks.fix_contextmenu. Rename clip -> clipcss in __updateClip() to make it less confusing. Improve comment about parent container creation in __createContainerDivs(). Tests: All apps run as before in DHTML. Files: M WEB-INF/lps/lfc/kernel/dhtml/LzSprite.js Changeset: http://svn.openlaszlo.org/openlaszlo/patches/maxcarlson-20100923-cPE.tar
