Bug 222532 – Fix inspector viewing of canvas save/restore stack, and tighten and simplify CanvasRenderingContext2DBase

• Component: Canvas
• Status: RESOLVED FIXED

[Darin Adler]

Fix inspector viewing of canvas save/restore stack, and tighten and simplify CanvasRenderingContext2DBase

[Darin Adler]

Created attachment 421782 [details]
Patch

[Sam Weinig]

Comment on attachment 421782 [details]
Patch

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

> Source/WebCore/html/canvas/CanvasRenderingContext2DBase.cpp:278
> +const AtomString& CanvasRenderingContext2DBase::defaultFontFamily()
> +{
> +    static NeverDestroyed<AtomString> family = AtomString { DefaultFontFamily, AtomString::ConstructFromLiteral };
> +    return family.get();
> +}

I am not sure if this is safe to do since the OffscreenCanvasRenderingContext2D inherits from this class and is used on a non-main thread.

> Source/WebCore/platform/graphics/cairo/PathCairo.cpp:417
> +FloatRect Path::strokeBoundingRect(const Optional<Function<void(GraphicsContext&)>>& strokeStyleApplier) const

WTF::Function has null state (though it is debatable whether that is a good thing). Can that be used rather than the Optional?

> Source/WebCore/rendering/svg/RenderSVGShape.cpp:-54
> -class BoundingRectStrokeStyleApplier final : public StrokeStyleApplier {

I wonder if we have other one off delegate patterns like this that can be replaced with function objects?

[Darin Adler]

Comment on attachment 421782 [details]
Patch

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

>> Source/WebCore/html/canvas/CanvasRenderingContext2DBase.cpp:278
>> +}
> 
> I am not sure if this is safe to do since the OffscreenCanvasRenderingContext2D inherits from this class and is used on a non-main thread.

Your broader point about this class is definitely correct and does call into question the design pattern I used here.

For the actual safety of the code change, the only caller of this function is CanvasRenderingContext2D::setFont, which uses it to call setOneFamily, which uses an AtomString. For safety, we could move this function into CanvasRenderingContext2D and instead export the const char* that it uses to initialize the AtomString, which I suppose is how it was working before, except that it was making a new AtomString every time, so I would want to add a global there for efficiency.

> Source/WebCore/html/canvas/CanvasRenderingContext2DBase.cpp:282
> +    static NeverDestroyed<String> font = DefaultFont;

Using the same global String across threads is also unsafe, not just AtomString. So I think your comment about the function above is actually correctly pointing out a serious problem with this one! Creating a new String every time we make a CanvasRenderingContext2DBase::State is inefficient but I guess it’s required to keep things thread-safe! That is frustrating.

>> Source/WebCore/platform/graphics/cairo/PathCairo.cpp:417
>> +FloatRect Path::strokeBoundingRect(const Optional<Function<void(GraphicsContext&)>>& strokeStyleApplier) const
> 
> WTF::Function has null state (though it is debatable whether that is a good thing). Can that be used rather than the Optional?

Yes, I will switch to that.

>> Source/WebCore/rendering/svg/RenderSVGShape.cpp:-54
>> -class BoundingRectStrokeStyleApplier final : public StrokeStyleApplier {
> 
> I wonder if we have other one off delegate patterns like this that can be replaced with function objects?

I am certain that we do.

[Devin Rousso]

Comment on attachment 421782 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        Not sure how we make regression tests for the inspector: added no new tests.

The tests for Web Inspector canvas recording are `LayoutTests/inspector/canvas/recording-*.html`.

> Source/WebCore/inspector/InspectorCanvas.cpp:879
> +            // FIXME: This is wrong: it will repeat the context's current path for every level in the stack, ignoring saved paths.

IIRC, `Path` is not saved as part of canvas 2D state.  As such, it should probably only be added to the first (i.e. "current") state.  I'd probably do something like this:
```
    for (auto& state : context2d.stateStack()) {
        ...
    }

    if (statesPayload->length()) {
        if (auto firstStatePayload = statesPayload->get(0)->asObject()) {
            auto setPath = JSON::ArrayOf<JSON::Value>::create();
            ...
            firstStatePayload->setArray(stringIndexForKey("setPath"_s), WTFMove(setPath));
        }
    }
```

[Darin Adler]

Comment on attachment 421782 [details]
Patch

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

>> Source/WebCore/inspector/InspectorCanvas.cpp:879
>> +            // FIXME: This is wrong: it will repeat the context's current path for every level in the stack, ignoring saved paths.
> 
> IIRC, `Path` is not saved as part of canvas 2D state.  As such, it should probably only be added to the first (i.e. "current") state.  I'd probably do something like this:
> ```
>     for (auto& state : context2d.stateStack()) {
>         ...
>     }
> 
>     if (statesPayload->length()) {
>         if (auto firstStatePayload = statesPayload->get(0)->asObject()) {
>             auto setPath = JSON::ArrayOf<JSON::Value>::create();
>             ...
>             firstStatePayload->setArray(stringIndexForKey("setPath"_s), WTFMove(setPath));
>         }
>     }
> ```

It’s saved, but by the platform. For example, Core Graphics saves it as an effect of the CGContextSaveGState function. And the platform does not let us introspect the saved state stack.

[Darin Adler]

Comment on attachment 421782 [details]
Patch

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

>> Source/WebCore/ChangeLog:8
>> +        Not sure how we make regression tests for the inspector: added no new tests.
> 
> The tests for Web Inspector canvas recording are `LayoutTests/inspector/canvas/recording-*.html`.

Do you know if there is any test that already tries to cover the state stack? I’ll go look, but I’d love to update an existing test to have better coverage instead of writing a whole new one.

[Devin Rousso]

Comment on attachment 421782 [details]
Patch

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

>>> Source/WebCore/ChangeLog:8
>>> +        Not sure how we make regression tests for the inspector: added no new tests.
>> 
>> The tests for Web Inspector canvas recording are `LayoutTests/inspector/canvas/recording-*.html`.
> 
> Do you know if there is any test that already tries to cover the state stack? I’ll go look, but I’d love to update an existing test to have better coverage instead of writing a whole new one.

I think `LayoutTests/inspector/canvas/recording-2d-saves.html` is the main test for "the initial state contains multiple saves".

>>> Source/WebCore/inspector/InspectorCanvas.cpp:879
>>> +            // FIXME: This is wrong: it will repeat the context's current path for every level in the stack, ignoring saved paths.
>> 
>> IIRC, `Path` is not saved as part of canvas 2D state.  As such, it should probably only be added to the first (i.e. "current") state.  I'd probably do something like this:
>> ```
>>     for (auto& state : context2d.stateStack()) {
>>         ...
>>     }
>> 
>>     if (statesPayload->length()) {
>>         if (auto firstStatePayload = statesPayload->get(0)->asObject()) {
>>             auto setPath = JSON::ArrayOf<JSON::Value>::create();
>>             ...
>>             firstStatePayload->setArray(stringIndexForKey("setPath"_s), WTFMove(setPath));
>>         }
>>     }
>> ```
> 
> It’s saved, but by the platform. For example, Core Graphics saves it as an effect of the CGContextSaveGState function. And the platform does not let us introspect the saved state stack.

Ah!  TIL.  As an FYI, the purpose of the Web Inspector canvas recorder is to instrument actions at the JS/bindings layer so that developers can have a better understanding of how each of the `CanvasRenderingContext2D` APIs affect what's displayed in the `<canvas>` (e.g. "why did this JS `fillRect` call draw a red square instead of a blue one?").  As such, since the concept of "state" in the `CanvasRenderingContext2D` API explicitly doesn't include the idea of "path" [1], I don't think Web Inspector needs to (or even should) either:
> The current default path and the rendering context's bitmaps are not part of the drawing state. The current default path is persistent, and can only be reset using the beginPath() method. The bitmaps depend on whether and how the rendering context is bound to a canvas element.
In fact, the only reason that Web Inspector has this here right now is so that it can more accurately "encode" the initial state of the `CanvasRenderingContext2D` for recreation/replay inside Web Inspector.  If there was a regular `CanvasRenderingContext2D` API for getting/setting the `Path`, then Web Inspector would be using that instead [2][3].

Also, for what it's worth, if Web Inspector wants to add a feature for introspecting the behavior of the underlying native graphics context and/or bitmap, then I think that's a different discussion (and one I'd also likely be in support of).

[1]: https://html.spec.whatwg.org/multipage/canvas.html#the-canvas-state
[2]: https://github.com/whatwg/html/issues/2897
[3]: https://github.com/whatwg/html/issues/2896

[Darin Adler]

Comment on attachment 421782 [details]
Patch

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

>>>> Source/WebCore/inspector/InspectorCanvas.cpp:879
>>>> +            // FIXME: This is wrong: it will repeat the context's current path for every level in the stack, ignoring saved paths.
>>> 
>>> IIRC, `Path` is not saved as part of canvas 2D state.  As such, it should probably only be added to the first (i.e. "current") state.  I'd probably do something like this:
>>> ```
>>>     for (auto& state : context2d.stateStack()) {
>>>         ...
>>>     }
>>> 
>>>     if (statesPayload->length()) {
>>>         if (auto firstStatePayload = statesPayload->get(0)->asObject()) {
>>>             auto setPath = JSON::ArrayOf<JSON::Value>::create();
>>>             ...
>>>             firstStatePayload->setArray(stringIndexForKey("setPath"_s), WTFMove(setPath));
>>>         }
>>>     }
>>> ```
>> 
>> It’s saved, but by the platform. For example, Core Graphics saves it as an effect of the CGContextSaveGState function. And the platform does not let us introspect the saved state stack.
> 
> Ah!  TIL.  As an FYI, the purpose of the Web Inspector canvas recorder is to instrument actions at the JS/bindings layer so that developers can have a better understanding of how each of the `CanvasRenderingContext2D` APIs affect what's displayed in the `<canvas>` (e.g. "why did this JS `fillRect` call draw a red square instead of a blue one?").  As such, since the concept of "state" in the `CanvasRenderingContext2D` API explicitly doesn't include the idea of "path" [1], I don't think Web Inspector needs to (or even should) either:

The way these things are saved and restored is an implementation detail. It’s a happy coincidence that developers don’t want or need to see the path, since it’s one thing saved and restored that we can’t implement well. It would be great to remove it since it’s not accurate. Should I just take it out?

[Darin Adler]

I’m just fixing a broken implementation that I spotted here by code inspection that claims to represent saved state, but wasn’t. My personal aim is *not* to set a future architecture or feature direction for this.

[Devin Rousso]

Comment on attachment 421782 [details]
Patch

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

(In reply to Darin Adler from comment #9)
> I’m just fixing a broken implementation that I spotted here by code inspection that claims to represent saved state, but wasn’t. My personal aim is *not* to set a future architecture or feature direction for this.

I appreciate that and thank you for taking the time to do so!  It is not my intention for myself or to ask you to set a future architecture/feature direction for this either.

>>>>> Source/WebCore/inspector/InspectorCanvas.cpp:879
>>>>> +            // FIXME: This is wrong: it will repeat the context's current path for every level in the stack, ignoring saved paths.
>>>> 
>>>> IIRC, `Path` is not saved as part of canvas 2D state.  As such, it should probably only be added to the first (i.e. "current") state.  I'd probably do something like this:
>>>> ```
>>>>     for (auto& state : context2d.stateStack()) {
>>>>         ...
>>>>     }
>>>> 
>>>>     if (statesPayload->length()) {
>>>>         if (auto firstStatePayload = statesPayload->get(0)->asObject()) {
>>>>             auto setPath = JSON::ArrayOf<JSON::Value>::create();
>>>>             ...
>>>>             firstStatePayload->setArray(stringIndexForKey("setPath"_s), WTFMove(setPath));
>>>>         }
>>>>     }
>>>> ```
>>> 
>>> It’s saved, but by the platform. For example, Core Graphics saves it as an effect of the CGContextSaveGState function. And the platform does not let us introspect the saved state stack.
>> 
>> Ah!  TIL.  As an FYI, the purpose of the Web Inspector canvas recorder is to instrument actions at the JS/bindings layer so that developers can have a better understanding of how each of the `CanvasRenderingContext2D` APIs affect what's displayed in the `<canvas>` (e.g. "why did this JS `fillRect` call draw a red square instead of a blue one?").  As such, since the concept of "state" in the `CanvasRenderingContext2D` API explicitly doesn't include the idea of "path" [1], I don't think Web Inspector needs to (or even should) either:
> 
> The way these things are saved and restored is an implementation detail. It’s a happy coincidence that developers don’t want or need to see the path, since it’s one thing saved and restored that we can’t implement well. It would be great to remove it since it’s not accurate. Should I just take it out?

We definitely don't want to remove it because then Web Inspector may not have an accurate representation of the initial state of the `CanvasRenderingContext2D` when the recording starts (e.g. if the developer starts recording in the middle of an interactive drawing which has already called `moveTo` or `lineTo`).  The goal of my original comment was just to avoid adding a FIXME since I think the solution is simple enough that it could be fixed now.

For what it's worth, I don't think there will be any issues if you leave it as you have it now, as all it would do is cause Web Inspector to set the path of the `<canvas>` multiple times to the same value before replaying the rest of the recorded actions instead of just once (to give some context, during replay in Web Inspector the initial state is applied before any actions, so calling `setPath` multiple times shouldn't do anything negative).

[Darin Adler]

(In reply to Devin Rousso from comment #7)
> I think `LayoutTests/inspector/canvas/recording-2d-saves.html` is the main
> test for "the initial state contains multiple saves".

I see the problem now. That only tests fillStyle, and only fill styles that are solid colors. There are 20 or so other properties to test! And two other modes for fillStyle.

Daunting to write all those missing tests.

[Darin Adler]

Created attachment 422395 [details]
Patch

[Darin Adler]

Looks like things are passing on EWS and this patch should address everything I discussed with Sam and Devin. Well, not *everything* I discussed with Devin, but everything you’d probably *expect* me to address.

[Radar WebKit Bug Importer]

<rdar://problem/75152728>

[Darin Adler]

Committed r274101 (235035@main): <https://commits.webkit.org/235035@main>