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?
>> + 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?
>> 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.
