Bug 231205 - Web Inspector: Show color space for canvases in the Graphics tab on the overview cards
Summary: Web Inspector: Show color space for canvases in the Graphics tab on the overv...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Patrick Angle
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-10-04 18:39 PDT by Patrick Angle
Modified: 2021-10-05 15:13 PDT (History)
11 users (show)

See Also:


Attachments
Patch v1.0 (9.76 KB, patch)
2021-10-04 19:29 PDT, Patrick Angle
no flags Details | Formatted Diff | Diff
Screenshot of Patch v1.0 /w Display P3 Color Space (894.59 KB, image/png)
2021-10-04 19:30 PDT, Patrick Angle
no flags Details
Screenshot of Patch v1.0 /w sRGB Color Space and no color space (1.14 MB, image/png)
2021-10-04 19:31 PDT, Patrick Angle
no flags Details
Patch v1.1 (9.76 KB, patch)
2021-10-05 14:32 PDT, Patrick Angle
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Patrick Angle 2021-10-04 18:39:57 PDT
<rdar://81828683>
Comment 1 Patrick Angle 2021-10-04 19:29:41 PDT
Created attachment 440146 [details]
Patch v1.0
Comment 2 Patrick Angle 2021-10-04 19:30:34 PDT
Created attachment 440148 [details]
Screenshot of Patch v1.0 /w Display P3 Color Space
Comment 3 EWS Watchlist 2021-10-04 19:30:59 PDT
This patch modifies the inspector protocol generator. Please ensure that you have rebaselined any generator test results (i.e., by running `Tools/Scripts/run-inspector-generator-tests --reset-results`)

This patch modifies the inspector protocol. Please ensure that any frontend changes appropriately use feature checks for new protocol features.
Comment 4 Patrick Angle 2021-10-04 19:31:22 PDT
Created attachment 440149 [details]
Screenshot of Patch v1.0 /w sRGB Color Space and no color space
Comment 5 Devin Rousso 2021-10-05 12:10:29 PDT
Comment on attachment 440146 [details]
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
Comment 6 Patrick Angle 2021-10-05 12:44:37 PDT
(In reply to Devin Rousso from comment #5)
> Comment on attachment 440146 [details]
> Patch v1.0
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=440146&action=review
> 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 :)

There is certainly some amount of "picking favorites" happening here, and we will never be able to show every attribute of a context on the overview card, but I feel Display P3 (and rich color in general) to be an important enough bit to surface here. Part of this comes down to awareness - it will benefit folks who didn't realize you could control the colorspace to see the current value on the overview, which will hopefully lead them to say "oh, colorspace is a thing for this context?" and consider for themselves whether their content and users would benefit from a wider color space.
Comment 7 Devin Rousso 2021-10-05 12:52:19 PDT
(In reply to Patrick Angle from comment #6)
> (In reply to Devin Rousso from comment #5)
> > Comment on attachment 440146 [details]
> > Patch v1.0
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=440146&action=review
> > 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 :)
> 
> There is certainly some amount of "picking favorites" happening here, and we will never be able to show every attribute of a context on the overview card, but I feel Display P3 (and rich color in general) to be an important enough bit to surface here. Part of this comes down to awareness - it will benefit folks who didn't realize you could control the colorspace to see the current value on the overview, which will hopefully lead them to say "oh, colorspace is a thing for this context?" and consider for themselves whether their content and users would benefit from a wider color space.

you could make that same argument for many (if not all) of the context attributes tho 😅 (e.g. "oh hey I can make my context not have a transparent background!")

with (current) 2D controls I think yeah `colorSpace` is probably the most important (tho `alpha` could be _very_ useful for a wide variety of situations), but for WebGL I'm not as convinced (tho yeah `colorSpace` doesn't exist there, but might soon?)
Comment 8 Patrick Angle 2021-10-05 14:32:16 PDT
Created attachment 440269 [details]
Patch v1.1
Comment 9 Patrick Angle 2021-10-05 14:36:49 PDT
Comment on attachment 440269 [details]
Patch v1.1

I'd like to move forward with this for the time being - we should absolutely re-evaluate what extra info we show here as more attributes are supported, as WebGL color space support possibly becomes a thing, etc..
Comment 10 EWS 2021-10-05 15:13:04 PDT
Committed r283572 (242536@main): <https://commits.webkit.org/242536@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 440269 [details].