Bug 225180 - Web Inspector: Graphics: add instrumentation for new `CanvasRenderingContext2DSettings`
Summary: Web Inspector: Graphics: add instrumentation for new `CanvasRenderingContext2...
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: Devin Rousso
URL:
Keywords: InRadar
Depends on: 225173
Blocks:
  Show dependency treegraph
 
Reported: 2021-04-28 19:07 PDT by Devin Rousso
Modified: 2021-09-23 12:19 PDT (History)
13 users (show)

See Also:


Attachments
Patch (29.74 KB, patch)
2021-08-12 19:11 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (99.00 KB, patch)
2021-08-13 11:53 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (98.94 KB, patch)
2021-09-22 23:35 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Devin Rousso 2021-04-28 19:07:38 PDT
.
Comment 1 Radar WebKit Bug Importer 2021-05-05 19:08:13 PDT
<rdar://problem/77587429>
Comment 2 Devin Rousso 2021-08-12 19:11:13 PDT
Created attachment 435467 [details]
Patch
Comment 3 EWS Watchlist 2021-08-12 19:12:05 PDT
This patch modifies the inspector protocol. Please ensure that any frontend changes appropriately use feature checks for new protocol features.
Comment 4 Devin Rousso 2021-08-13 11:53:32 PDT
Created attachment 435499 [details]
Patch
Comment 5 Patrick Angle 2021-08-13 13:17:02 PDT
Comment on attachment 435499 [details]
Patch

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

Looks good, just a few nits, and maybe some tests to reenable in another bug. Provisional r+ from me.

> Source/WebCore/ChangeLog:14
> +        Unify the two cases where we fetch canvas attributes into a single method so that both get
> +        the same data. This means that the canvas recorder now also uses `Canvas.ContextAttributes`.

Nice.

> Source/WebCore/inspector/InspectorCanvas.cpp:831
> +#endif

NIT: `#endif // ENABLE(DESTINATION_COLOR_SPACE_DISPLAY_P3)`

> Source/WebCore/inspector/InspectorCanvas.cpp:860
> +            contextAttributesPayload->setPowerPreference("default"_s);

Maybe power preference values should be an enum in the Protocol?

> Source/WebCore/inspector/InspectorCanvas.cpp:863
> +            contextAttributesPayload->setPowerPreference("low-power"_s);

Ditto :860

> Source/WebCore/inspector/InspectorCanvas.cpp:866
> +            contextAttributesPayload->setPowerPreference("high-performance"_s);

Ditto :860

> LayoutTests/ChangeLog:24
> +        * inspector/canvas/context-attributes-expected.txt:
> +        * inspector/canvas/recording-2d-frameCount-expected.txt:
> +        * inspector/canvas/recording-2d-full-expected.txt:
> +        * inspector/canvas/recording-2d-memoryLimit-expected.txt:
> +        * inspector/canvas/recording-bitmaprenderer-frameCount-expected.txt:
> +        * inspector/canvas/recording-bitmaprenderer-full-expected.txt:
> +        * inspector/canvas/recording-bitmaprenderer-memoryLimit-expected.txt:
> +        * inspector/canvas/recording-html-2d-expected.txt:
> +        * inspector/canvas/recording-webgl-frameCount-expected.txt:
> +        * inspector/canvas/recording-webgl-full-expected.txt:
> +        * inspector/canvas/recording-webgl-memoryLimit-expected.txt:
> +        * inspector/canvas/recording-webgl-snapshots-expected.txt:
> +        * inspector/canvas/recording-webgl2-frameCount-expected.txt:
> +        * inspector/canvas/recording-webgl2-full-expected.txt:
> +        * inspector/canvas/recording-webgl2-memoryLimit-expected.txt:
> +        * inspector/canvas/recording-webgl2-snapshots-expected.txt:

Looking at the `Mac` `TestExpectations` file shows that some of these tests are `[Pass Timeout]` or `[Pass Failure Timeout]`. If any of these shouldn't be any more, we should probably update the expectations in another bug. Particularly the timeouts given that any modified test or test expectations get stress-tested in EWS now.
Comment 6 Devin Rousso 2021-08-27 14:15:07 PDT
Comment on attachment 435499 [details]
Patch

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

>> Source/WebCore/inspector/InspectorCanvas.cpp:860
>> +            contextAttributesPayload->setPowerPreference("default"_s);
> 
> Maybe power preference values should be an enum in the Protocol?

Hmm, we could?  We don't actually explicitly use these values manually anywhere in the frontend.  It'd just be a sanity check on the backend really.  The frontend pipes this data directly into a `canvasElement.createContext(type, attributes)`.  Frankly we could replace `Canvas.ContextAttributes` with an anonymous object and it'd work the same.
Comment 7 BJ Burg 2021-09-22 09:01:38 PDT
Comment on attachment 435499 [details]
Patch

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

r=me

The Windows EWS failure looks like a clean build is needed, for some reason the InspectorProtocolObjects.h file is not being regenerated. Please confirm before landing.

https://ews-build.webkit.org/#/builders/10/builds/102675

>>> Source/WebCore/inspector/InspectorCanvas.cpp:860
>>> +            contextAttributesPayload->setPowerPreference("default"_s);
>> 
>> Maybe power preference values should be an enum in the Protocol?
> 
> Hmm, we could?  We don't actually explicitly use these values manually anywhere in the frontend.  It'd just be a sanity check on the backend really.  The frontend pipes this data directly into a `canvasElement.createContext(type, attributes)`.  Frankly we could replace `Canvas.ContextAttributes` with an anonymous object and it'd work the same.

I'm with Devin here. I don't see much point in making an enum if we aren't verifying the values that come through.
Comment 8 Devin Rousso 2021-09-22 20:41:03 PDT
Comment on attachment 435499 [details]
Patch

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

>> Source/WebCore/inspector/InspectorCanvas.cpp:831
>> +#endif
> 
> NIT: `#endif // ENABLE(DESTINATION_COLOR_SPACE_DISPLAY_P3)`

While I personally would prefer doing this, I've been told in the past that WebKit style is (usually) to not include this unless there's a large gap between the `#if` and `#endif`.

>> LayoutTests/ChangeLog:24
>> +        * inspector/canvas/recording-webgl2-snapshots-expected.txt:
> 
> Looking at the `Mac` `TestExpectations` file shows that some of these tests are `[Pass Timeout]` or `[Pass Failure Timeout]`. If any of these shouldn't be any more, we should probably update the expectations in another bug. Particularly the timeouts given that any modified test or test expectations get stress-tested in EWS now.

I think this could be said for _all_ Web Inspector tests :P

Definitely worth looking at this, but I'd likely do that in a separate bug so as to not cause additional issues.
Comment 9 EWS 2021-09-22 21:30:27 PDT
Patch 435499 does not build
Comment 10 Devin Rousso 2021-09-22 23:35:50 PDT
Created attachment 439015 [details]
Patch
Comment 11 EWS 2021-09-23 12:19:51 PDT
Committed r282984 (242069@main): <https://commits.webkit.org/242069@main>

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