CS Painting API should support multiple worklets.

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Justin Michaud]

CS Painting API should support multiple worklets. Right now, they are just overridden.

[Justin Michaud]

Created attachment 356779 [details]
Patch

[Dean Jackson]

Comment on attachment 356779 [details]
Patch

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

> Source/WebCore/css/StyleResolver.cpp:1732
> +        if (document().paintWorkletGlobalScope(name)) {
> +            auto& paintWorklet = *document().paintWorkletGlobalScope(name);
> +            auto locker = holdLock(paintWorklet.paintDefinitionLock());
> +            if (auto* registration = paintWorklet.paintDefinitionMap().get(name)) {
> +                for (auto& property : registration->inputProperties)
> +                    state.style()->addCustomPaintWatchProperty(property);
> +            }
>          }

What happens if there isn't a worklet global scope with this name? Should we throw an error/ASSERT?

> Source/WebCore/dom/Document.cpp:8522
> +PaintWorkletGlobalScope* Document::paintWorkletGlobalScope(const String& name)

I wonder if this should be paintWorkletGlobalScopeForName/WithName? or maybe I've just been looking at ObjC too much recently.

> Source/WebCore/worklets/Worklet.cpp:55
> +    auto locker = holdLock(context->paintDefinitionLock());
> +    for (auto& name : context->paintDefinitionMap().keys())
> +        document.setPaintWorkletGlobalScope(name, makeRef(context.get()));

Should we check that the name doesn't already exist?

> LayoutTests/fast/css-custom-paint/multiple-worklets.html:4
> +<meta name="assert" content="test hidpi scaling">

Is this correct?

> LayoutTests/fast/css-custom-paint/multiple-worklets.html:40
> +<script id="code1" type="text/worklet">
> +registerPaint('my-paint', class {
> +  paint(ctx, geom, properties) {
> +    ctx.fillStyle = 'purple';
> +    ctx.fillRect(0, 0, geom.width, geom.height);
> +  }
> +});
> +</script>
> +
> +<script id="code2" type="text/worklet">
> +registerPaint('my-paint2', class {
> +  paint(ctx, geom, properties) {
> +    ctx.fillStyle = 'green';
> +    ctx.fillRect(0, 0, geom.width, geom.height);
> +  }
> +});
> +</script>

I'm not sure how this test exercises they are isolated scopes. Could you try to write to a global variable in one, and then read the value in the other? (or something on a built-in prototype)

[Simon Fraser (smfr)]

Comment on attachment 356779 [details]
Patch

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

> Source/WebCore/css/StyleResolver.cpp:1726
> +        if (document().paintWorkletGlobalScope(name)) {
> +            auto& paintWorklet = *document().paintWorkletGlobalScope(name);

Two hash lookups here. Should be:

if (auto* workout = document().paintWorkletGlobalScope(name))
  auto& paintWorklet = *worklet.

> Source/WebCore/dom/Document.cpp:2567
> +    for (auto& scope : m_paintWorkletGlobalScopes.values()) {

No braces.

[Justin Michaud]

Created attachment 356973 [details]
Patch

[Dean Jackson]

Comment on attachment 356973 [details]
Patch

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

> LayoutTests/fast/css-custom-paint/multiple-worklets-isolation.html:29
> +    console.log("Paint 1: " + typeof(shouldOnlyBeInCode1));

Did you want to leave this in?

> LayoutTests/fast/css-custom-paint/multiple-worklets-isolation.html:34
> +    if (typeof(shouldOnlyBeInCode1) == 'function') {
> +      ctx.fillStyle = 'green';
> +    } else {
> +      ctx.fillStyle = 'red';
> +    }

ctx.fillStyle = (typeof(shouldOnlyBeInCode1) == 'function') ? 'green' : 'red';

> LayoutTests/fast/css-custom-paint/multiple-worklets-isolation.html:43
> +    if (typeof(shouldOnlyBeInCode1) == 'undefined') {

Nice!

[Justin Michaud]

Created attachment 357018 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 357018 [details]
Patch

Clearing flags on attachment: 357018

Committed r239067: <https://trac.webkit.org/changeset/239067>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/46617313>