[webkit-reviews] review canceled: [Bug 35003] Web Inspector: Implement evaluate-on-hover for scripts panel. : [Attachment 48912] [PATCH] Proposed fix (work in progress, couple of nits I need to fix).

[bugzilla-daemon at webkit.org]

Timothy Hatcher <timothy at hatcher.name> has canceled 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 48912: [PATCH] Proposed fix (work in progress, couple of nits I need
to fix).
https://bugs.webkit.org/attachment.cgi?id=48912&action=review

------- Additional Comments from Timothy Hatcher <timothy at hatcher.name>
So this popover will be used for conditional breakpoints too?

> +	   preferredWidth = Math.max(preferredWidth, 100 - 2 * 23);
> +	   preferredHeight = Math.max(preferredHeight, 100 - 2 * 23);
> +	   var newElementPosition = { x: 0, y: 0, width: preferredWidth + 2 *
27, height: preferredHeight + 2 * 27 };
> +		   newElementPosition.width += 11;
> +	       newElementPosition.y += 10;
> +		   newElementPosition.width += 11;
> +	       newElementPosition.y -= 10;
> +	       newElementPosition.x = Math.max(0, anchorBox.x) - 40;
> +	       this._popupArrowElement.style.right = totalWidth - anchorBox.x -
25 - anchorBox.width + "px";
> +	       this._popupArrowElement.style.left = anchorBox.x - 25 + "px";
> +	   newElementPosition.height += 11;

You should put these numbers in named constants at the top of the file.

> +	   this._hoverTimer = setTimeout(this._mouseHover.bind(this), 1000);

Should it be longer or better match tooltips? Put in a named constant at the
top of the file.

> +	       InspectorBackend.releaseWrapperObjectGroup(0, "hovercard");

Maybe "popover" instead of "hovercard" would be better. And a constant so it
can be shared with the other place in this file.

> +	   if (element.hasStyleClass("webkit-javascript-keyword")) {
> +	       if (element.textContent === "this")
> +		   this._showPopup(element, "this");
> +	       return;
> +	   }

I don't understand this.

> +	   var offset = 0;
> +	   var node =
lineRow.lastChild.traverseNextTextNode(lineRow.lastChild);
> +	   while (node && node !== element.firstChild) {
> +	       offset += node.nodeValue.length;
> +	       node = node.traverseNextTextNode(lineRow.lastChild);
> +	   }

Some comments here would help.

> +	   var prefix = this._textModel.line(lineNumber).substring(0, offset +
element.textContent.length);
> +	   var reversedPrefix = prefix.split("").reverse().join("");
> +	   var match = /[a-zA-Z\x80-\xFF\_$0-9.]+/.exec(reversedPrefix);
> +	   if (!match)
> +	       return;
> +	   var expression = match[0].split("").reverse().join("");

And here. Why reverse?


> +    this.headerElement.className = "hidden";

Should you use addStyleClass just so the previous classes are preserved? I
guess it isn't an issue.

> +WebInspector.HoverPropertiesSection.prototype = {
> +    update: function()
> +    {
> +    }
> +}

I assume this is will do more later.


> +.webkit-javascript-ident {
> +    color: rgb(0, 0, 0);
> +}

Use black.

> +    z-index: 100;

Use 1000, or something higher than 100.