[webkit-reviews] review denied: [Bug 173436] Web Inspector: Only enable the "Show Grid" navigation item if the image has transparent pixels : [Attachment 313016] Patch

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has denied Devin Rousso
<drousso at apple.com>'s request for review:
Bug 173436: Web Inspector: Only enable the "Show Grid" navigation item if the
image has transparent pixels
https://bugs.webkit.org/show_bug.cgi?id=173436

Attachment 313016: Patch

https://bugs.webkit.org/attachment.cgi?id=313016&action=review




--- Comment #3 from Joseph Pecoraro <joepeck at webkit.org> ---
Comment on attachment 313016
  --> https://bugs.webkit.org/attachment.cgi?id=313016
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=313016&action=review

> Source/WebInspectorUI/UserInterface/Base/ImageUtilities.js:55
> +WebInspector.imageHasTransparency = function(imageElement, skip = 100)

I think this may be unnecessarily complex. I'd think the expensive part would
be getImageData, not looping over to detect if the alpha bit is < 255. So the
skipping might not be necessary and might just lead to misdiagnosing an image
with transparency.

I think we should also be able to write a simple test for this with some tiny
images.

> Source/WebInspectorUI/UserInterface/Base/ImageUtilities.js:71
> +    for (let i = 3; i < imageData.data.length; i += 4 * skip) {

If `skip` is 0 then this will infinite loop. Seems like it is easy to mis-use
this function.

> Source/WebInspectorUI/UserInterface/Views/ImageResourceContentView.js:70
> +	   this._imageElement.addEventListener("load", (event) => {
> +	       let imageHasTransparency =
WebInspector.imageHasTransparency(this._imageElement);
> +	       this._showGridButtonNavigationItem.enabled =
imageHasTransparency;
> +	       if (imageHasTransparency)
> +		   this._toggleImageGrid();
> +
> +	       URL.revokeObjectURL(objectURL);
> +	   });

Style: Put this after setting the more direct attributes. The code is easier to
read that way.