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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Sun Nov 22 08:29:30 PST 2009


Timothy Hatcher <timothy at hatcher.name> 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 43646: Patch with manual test
https://bugs.webkit.org/attachment.cgi?id=43646&action=review

------- Additional Comments from Timothy Hatcher <timothy at hatcher.name>
> -	   var title = this._nodeTitleInfo(this.representedObject,
this.hasChildren, WebInspector.linkifyURL).title;
> +	   var self = this;
> +	   var callback = function(propertiesPayload) {
> +	       var getPropertyValue = function(propertyName) {
> +		   for (e in propertiesPayload)
> +		       if (propertiesPayload[e].name == propertyName)
> +			   return propertiesPayload[e].value.description;
> +		   return null;
> +	       };
> +	       self._updateTitleWithRespectToNodeProperties(getPropertyValue);
> +	   };
> +	   var prototype = new
WebInspector.ObjectProxy(this.representedObject.id, [], 0);
> +	   InjectedScriptAccess.getProperties(prototype, true, callback);

This is going to slow down the Elements panel, since it will delay updating the
title waiting for the async InjectedScriptAccess.getProperties. (Slower in
Chrome where the async is corss process. So we need to minimize these calls.)

We are paying a heavy cost of InjectedScriptAccess.getProperties for every DOM
element just to filter it later for image elements. The
InjectedScriptAccess.getProperties call was not designed for one-off property
access, as you found, since it has .name properties. It is really only used for
property inspection. I am not sure what the best solution is.

Also these should use named functions, with the brace o nthe next line like:

> +	   function callback(propertiesPayload)
> +	   {
> +	       function getPropertyValue(propertyName)
> +	       {

> +			       if (node.nodeName.toLowerCase() == "img") {

Use === in cases like this.

> +				   tooltipText = offsetWidth + "px x " +
offsetHeight + "px";

This should use \u00d7 for the multiplication sign, not "x". I don't think "px"
is needed twice. You could do "123x456 pixels" like Safari, but that will need
to be localized with a WebInspector.UIString. Or just leave off the units.

> +				   if (offsetHeight != naturalHeight ||
offsetWidth != naturalWidth)

Use !==.

> +				       tooltipText += " (natural: " +
naturalWidth + "px x " + naturalHeight + "px)";

This text will need to be localized, and should not be appened to another
string that might be localized. Other languages mgiht want to show them in a
different order.

So: WebInspector.UIString("%d\u00d7%d", naturalWidth, naturalHeight)


More information about the webkit-reviews mailing list