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

[bugzilla-daemon at webkit.org]

Pavel Feldman <pfeldman at chromium.org> has denied  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 43866: Patch with manual test
https://bugs.webkit.org/attachment.cgi?id=43866&action=review

------- Additional Comments from Pavel Feldman <pfeldman at chromium.org>
A handful of style nits, otherwise good to go. Please put // TODO: Replace with
InjectedScriptAccess.getBasicProperties(obj, [names]) in place of the extended
Q&A block. That will save few bytes on loading / parsing.

>  
> +WebInspector.ElementsTreeElement.createTooltipForImageNode =
function(properties)
> +{

It is Ok to define this one of prototype even though it does not use 'this'. We
do that in other places.

> +    var tooltipText = null;
> +    var offsetHeight = properties["offsetHeight"];

You can use properties.offsetHeight (and there is no need in explicit
intermediate variable I guess).


>  WebInspector.ElementsTreeElement.prototype = {
>      get highlighted()
>      {
> @@ -787,14 +801,30 @@ WebInspector.ElementsTreeElement.prototy
>	   if (this._editing)
>	       return;
>  

> +	       return callback();

No need to return it, just call it.

> +	   var objectProxy = new
WebInspector.ObjectProxy(this.representedObject.id);
> +	   WebInspector.ObjectProxy.getPropertiesAsync(objectProxy, callback);

It would be even better if you provided the names of the properties needed
here, so that it would
be easier to understand what the 'properties' object you carry is needed for.



> +	   var result = [];
> +	   for (e in propertiesPayload)

We typically iterate over arrays using index 'for (var i = 0; i <
properties.length; ++i)'