Bug 225180 – Web Inspector: Graphics: add instrumentation for new `CanvasRenderingContext2DSettings`

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Devin Rousso]

.

[Radar WebKit Bug Importer]

<rdar://problem/77587429>

[Devin Rousso]

Created attachment 435467 [details]
Patch

[EWS Watchlist]

This patch modifies the inspector protocol. Please ensure that any frontend changes appropriately use feature checks for new protocol features.

[Devin Rousso]

Created attachment 435499 [details]
Patch

[Patrick Angle]

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.

[Devin Rousso]

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.

[Blaze Burg]

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.

[Devin Rousso]

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.

[EWS]

Patch 435499 does not build

[Devin Rousso]

Created attachment 439015 [details]
Patch

[EWS]

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].