[webkit-reviews] review denied: [Bug 31207] Web Inspector: Datatip should be a tree : [Attachment 42657] preliminary patch

[bugzilla-daemon at webkit.org]

Timothy Hatcher <timothy at hatcher.name> has denied  review:
Bug 31207: Web Inspector: Datatip should be a tree
https://bugs.webkit.org/show_bug.cgi?id=31207

Attachment 42657: preliminary patch
https://bugs.webkit.org/attachment.cgi?id=42657&action=review

------- Additional Comments from Timothy Hatcher <timothy at hatcher.name>

> +	   this.element.contentWindow.addEventListener("scroll",
WebInspector.datatip.hide.bind(WebInspector.datatip), false);

This listener should be registered in WebInspector.Datatip.show() and removed
in WebInspector.Datatip.hide().


> +	   WebInspector.panels.scripts.evaluateInSelectedCallFrame(expression,
false, "console", callback.bind(this));

Make the change Pavel sugested, using "datatip" instead of "console" and call
InspectorController.releaseWrapperObjectGroup("datatip") on mouseout.


> +	       WebInspector.datatip.show();
> +	       WebInspector.datatip.element.style.left =
(event.target.totalOffsetLeft + this.sourceFrame.element.totalOffsetLeft) +
"px";
> +	       WebInspector.datatip.element.style.top =
(event.target.offsetHeight + event.target.totalOffsetTop +
this.sourceFrame.element.totalOffsetTop -
this.sourceFrame.element.contentDocument.body.scrollTop) + "px";

Maybe you should have show() take an element, and it will do the positioning?
But maybe it is tricky to get right for all cases, since this is about the
iframe.

> +    this.visible = false;

Consider making visible a getter/setter that calls show()/hide(). We do this
for WebInpector.View, so maybe copy the style from there. And let it be
undefined (same as false) by default.

> +    this._object = undefined;

It will be undefined by default, so you can remove this.

> +    this.hideTimer = null;

Just let it be undefined by default, so remove this.


> +	   this.clearObject();
> +	   this._object = obj;
> +	   this.element.appendChild(new
WebInspector.ObjectPropertiesSection(obj, obj.description, null,
true).element);
> +	   this.element.addEventListener("mouseover",
this.clearHideTimer.bind(this), false);
> +	   this.element.addEventListener("mouseout",
this.hideAfterDelay.bind(this), false);

This will register the event listeners again and again if the Datatip is reused
(and it is). So you need to unregister them, or just register them once when
this.element is created. I prefer just registering them once.

Add a line between this._object and the appendChild.

> +    get object(obj)
> +    {
> +	   return obj;
> +    },

This is wrong. Getters don't take arguments. It should be:

    get object()
    {
	return this._object;
    },

> +    clearObject: function()
> +    {
> +	   this.element.removeChildren();
> +	   delete this._object;
> +    },

Just do this in set object(), and tremove this function. It isn't needed by any
other caller.

> +    show: function()
> +    {
> +	   this.clearHideTimer();
> +	   if (this.visible)
> +	       return;
> +
> +	   this.visible = true;
> +	   document.body.appendChild(this.element);
> +    },

It seems bad to append and assume the caller will position, but maybe that is
ok? Maybe the caller should position before calling show, or have show do the
positioning?

> +    hide: function(a)

Remove the a argument.


> +	   this.hideTimer = setTimeout(this.hide.bind(this, true), 500);

The true argument isn't needed.