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...
+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?
Actually, parentcontainer can be undefined in a lot of cases. I'll
comment about that.
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...
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
