Looks good. Thanks. On 2010-09-23, at 13:55, Max Carlson wrote:
> 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
