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?
+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) {
- var s = 'rect(0px ' + this._w + ' ' + this._h + ' 0px)';
- this.__LZdiv.style.clip = s
- } else if (this.__LZdiv.style.clip) {
- var s = quirks.fix_ie_css_syntax ? 'rect(auto auto auto auto)' : '';
- this.__LZdiv.style.clip = s;
+ // have clip, set it
+ clip = 'rect(0px ' + this._w + ' ' + this._h + ' 0px)';
+ } else if (this._clip) {
+ // had clip, clear it
+ clip = quirks.fix_ie_css_syntax ? 'rect(auto auto auto auto)' : '';
+ }
+ if (clip !== this._clip) {
+ this._clip = clip;
+ 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?
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.
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)`?
On 2010-09-15, at 17:54, Max Carlson wrote:
> Change 20100915-maxcarlson-x by maxcarl...@friendly on 2010-09-15 14:46:56 PDT
> in /Users/maxcarlson/openlaszlo/trunk-clean
> for http://svn.openlaszlo.org/openlaszlo/trunk
>
> Summary: Create click containers only as needed
>
> Bugs Fixed: LPP-6892 - Generated File Sizes baseline complete; Performance
> Tuning ongoing
>
> Technical Reviewer: ptw
> QA Reviewer: hminsky
>
> Overview: This change only creates click containers for clickable sprites.
> This drastically reduces the number of divs for each application.
>
> Details: LzSprite - When addChild() sees a child sprite that has a clickdiv,
> it creates a clickdiv container tree and appends it. setClickable() creates
> a click tree as-needed. All setters, e.g. setX() only update the click div
> if present. __updateClip() now tracks clip css in the _clip attribute.
> setContextMenu() now uses the __createContainerDivs() to create container
> divs. __copystyles() tries to avoid copying unneeded styles and uses cached
> sprite css values.
>
> LzTextSprite - Don't create click divs.
>
> LzInputTextSprite - Delay creating click divs until they're needed.
>
> Tests: All apps run as before (but faster) in dhtml.
>
> Files:
> M WEB-INF/lps/lfc/kernel/dhtml/LzSprite.js
> M WEB-INF/lps/lfc/kernel/dhtml/LzTextSprite.js
> M WEB-INF/lps/lfc/kernel/dhtml/LzInputTextSprite.js
>
> Changeset:
> http://svn.openlaszlo.org/openlaszlo/patches/20100915-maxcarlson-x.tar
