[webkit-reviews] review granted: [Bug 35003] Web Inspector: Implement evaluate-on-hover for scripts panel. : [Attachment 49009] [PATCH] Proposed change.

[bugzilla-daemon at webkit.org]

Timothy Hatcher <timothy at hatcher.name> has granted Pavel Feldman
<pfeldman at chromium.org>'s request for review:
Bug 35003: Web Inspector: Implement evaluate-on-hover for scripts panel.
https://bugs.webkit.org/show_bug.cgi?id=35003

Attachment 49009: [PATCH] Proposed change.
https://bugs.webkit.org/attachment.cgi?id=49009&action=review

------- Additional Comments from Timothy Hatcher <timothy at hatcher.name>
> +	   this.contentElement.positionAt(0, 0);
> +	   document.body.appendChild(this.contentElement);
> +	   var preferredWidth = preferredWidth ||
this.contentElement.offsetWidth;
> +	   var preferredHeight = preferredHeight ||
this.contentElement.offsetHeight;

A comment here explaining you do this for measuement would be good.

> +	   // Short tooltips are not pretty, their arrow location is not nice.
> +	   preferredWidth = Math.max(preferredWidth, 50);

Don't you mean "skinny"? Short is for height, but you are padding the width.

> +	   var totalWidth = window.innerWidth;
> +	   var totalHeight = window.innerHeight;

const

> +	   // User has 500ms to reach the popup.
> +	   if (this._popup) {
> +	       var self = this;
> +	       this._hidePopupTimer = setTimeout(function() {
self._hidePopup(); delete self._hidePopupTimer; }, 500);
> +	   }

Usually tooltips stay up longer so you can just look at them without moving.
5-10 seconds even. A named function would be nice for that timeout.

> +		   this._popup.show(element, 300, 250);

These could be named constants.

> -	   total += element.offsetLeft;
> +	   total += element.offsetLeft + element.clientLeft;

You should fix the totalOffsetTop function to match.