[webkit-reviews] review denied: [Bug 21554] Hovering the "src" attribute for an image should show the image dimensions : [Attachment 43881] Patch with test case

[bugzilla-daemon at webkit.org]

Pavel Feldman <pfeldman at chromium.org> has denied Daniel Bates
<dbates at webkit.org>'s request for review:
Bug 21554: Hovering the "src" attribute for an image should show the image
dimensions
https://bugs.webkit.org/show_bug.cgi?id=21554

Attachment 43881: Patch with test case
https://bugs.webkit.org/attachment.cgi?id=43881&action=review

------- Additional Comments from Pavel Feldman <pfeldman at chromium.org>
Thanks for addressing the comments. It now looks really good and clean. Test is
very good too. Couple of minot nits and re are ready to land:

img-tooltip.html should be called elements-img-tooltip (we try to group things
by panels)

> +    function createPropertiesMapThenCallback(propertiesPayload)
> +    {

Since the interaction between front-end and injected script is asynchronous,
node that you passed into getProperties might
be already deleted. You should be ready for that. 'propertiesPayload' will be
'false' in that case and you should pass it further
as undefined or []. You should then be able to handle missing properties when
generating tooltip.

> +
> +    var innerMapping = WebInspector.domAgent._idToDOMNode;
> +    for (var nodeId in innerMapping) {
> +	   if (innerMapping[nodeId].nodeName === "IMG") {
> +	      
WebInspector.ElementsTreeElement.prototype.createTooltipForImageNode(innerMappi
ng[nodeId], callback);
> +	       break;
> +	   }
> +    }
> +}

Nit: You should do return in place of break and call
testController.notifyDone(); after the loop. This way when elements panel
breaks and has no IMG, you will not need to wait for timieout.

You probably need to store img inline in order not to wait for load event, but
you should make it more simple. I'll attach a white rectangle of the same size
to the bug. it is 80 times smaller.