WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED WONTFIX
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
Summary
Web Inspector: Only enable the "Show Grid" navigation item if the image has t...
Devin Rousso
Reported
2017-06-15 14:18:48 PDT
This will prevent awkward scenarios where the user clicks on the button, only for it to have no effect since the grid is not visible.
Attachments
Patch
(6.59 KB, patch)
2017-06-15 15:04 PDT
,
Devin Rousso
no flags
Details
Formatted Diff
Diff
Patch
(4.58 KB, patch)
2017-06-15 15:06 PDT
,
Devin Rousso
joepeck
: review-
Details
Formatted Diff
Diff
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
Devin Rousso
Comment 1
2017-06-15 15:04:47 PDT
Created
attachment 313015
[details]
Patch
Devin Rousso
Comment 2
2017-06-15 15:06:26 PDT
Created
attachment 313016
[details]
Patch Oops. Included some extra stuff :P
Joseph Pecoraro
Comment 3
2017-06-15 16:17:42 PDT
Comment on
attachment 313016
[details]
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.
Joseph Pecoraro
Comment 4
2017-06-15 18:46:48 PDT
After discussion we may just not want to take this approach at all and just always leave the button enabled?
Devin Rousso
Comment 5
2017-06-15 23:36:27 PDT
(In reply to Joseph Pecoraro from
comment #4
)
> After discussion we may just not want to take this approach at all and just > always leave the button enabled?
Agreed. In the case that the image has no transparency, the fact that clicking the Show Grid button has no visible effect is a good indicator in itself.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug