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!
