[webkit-reviews] review granted: [Bug 231205] Web Inspector: Show color space for canvases in the Graphics tab on the overview cards : [Attachment 440146] Patch v1.0

[bugzilla-daemon at webkit.org]

Devin Rousso <drousso at apple.com> has granted Patrick Angle <pangle at apple.com>'s
request for review:
Bug 231205: Web Inspector: Show color space for canvases in the Graphics tab on
the overview cards
https://bugs.webkit.org/show_bug.cgi?id=231205

Attachment 440146: Patch v1.0

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




--- Comment #5 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 440146
  --> https://bugs.webkit.org/attachment.cgi?id=440146
Patch v1.0

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

r=me

On one hand, it's nice to expose info like this at a cursory glance as that can
make finding the right `<canvas>` easier.  On the other hand, why are we
picking this as the only thing to show of all the context attributes?  2D is
also gonna get support for `alpha` soon™.  Should we also show that?  I
personally think `colorSpace` is likely more useful than `alpha`, but that's
just my experience and may not represent the way other developers use
`<canvas>`.  I guess it just feels a bit odd that we'd pick one over the other,
especially when most pages probably don't use more than a handful of `<canvas>`
that likely can be distinguished based on the current content.

It's also odd that we don't have anything for WebGL either.  But that's an even
more complex problem since there's more context attributes :/

I guess in summary I personally wouldn't add this as it's kinda "picking
favorites" IMO, but I can see a use for it so I'll let you decide :)

> Source/JavaScriptCore/inspector/protocol/Canvas.json:21
> +	       "enum": ["srgb", "display-p3" ]

Style: space before `]`

> Source/WebInspectorUI/UserInterface/Models/Canvas.js:473
> +}

Style: missing `;`

> Source/WebInspectorUI/UserInterface/Views/CanvasContentView.js:111
> +		   subtitle.textContent =
`(${WI.Canvas.displayNameForColorSpace(this.representedObject.contextAttributes
.colorSpace)})`;

NIT: I'd personally use `"(" + ... + ")"` as it's clearer to me that you're
wrapping a string in parenthesis