[webkit-reviews] review denied: [Bug 21570] Elements panel needs to be able to preview images : [Attachment 126941] Patch

[bugzilla-daemon at webkit.org]

Pavel Feldman <pfeldman at chromium.org> has denied Alexander Pavlov (apavlov)
<apavlov at chromium.org>'s request for review:
Bug 21570: Elements panel needs to be able to preview images
https://bugs.webkit.org/show_bug.cgi?id=21570

Attachment 126941: Patch
https://bugs.webkit.org/attachment.cgi?id=126941&action=review

------- Additional Comments from Pavel Feldman <pfeldman at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=126941&action=review


> Source/WebCore/inspector/front-end/ElementsPanel.js:359
> +	   // We get here for CSS property treeElements, too, so bail out if
there is no "nodeName" method.

Testing for nodeName is fragile - imagine more tree elements with
representedObjects containing nodeName property. You should only call this
method for ElementsTreeElements.

> Source/WebCore/inspector/front-end/ElementsPanel.js:374
> +		   treeElement._dimensions = { offsetWidth: properties[0],
offsetHeight: properties[1], naturalWidth: properties[2], naturalHeight:
properties[3] };

Do not mutate objects that don't belong to you - you should simply do var
dimetions here and return them in a callback

> Source/WebCore/inspector/front-end/ElementsPanel.js:393
> +	       object.callFunction(dimensions, setDimensionsData);

We now have a returnByValue parameter in callFunctionOn. So that you did not
have to JSON.parse and concatenate the string. Simply return an array and use
it as array.

> Source/WebCore/inspector/front-end/ElementsPanel.js:396
> +	   WebInspector.RemoteObject.resolveNode(node, "", resolvedNode);

Recently, I started using direct flow of callbacks (as in our tests). It is
easier to read the code that way. You should consider giving it a try.

> Source/WebCore/inspector/front-end/ElementsPanel.js:403
> +	       if (listItem.treeElement._dimensions)

No need to cache - things might change.