Bug 192335 - CS Painting API should support multiple worklets.
Summary: CS Painting API should support multiple worklets.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Justin Michaud
URL:
Keywords: InRadar
Depends on:
Blocks: 190217
  Show dependency treegraph
 
Reported: 2018-12-03 15:00 PST by Justin Michaud
Modified: 2019-04-25 09:23 PDT (History)
8 users (show)

See Also:


Attachments
Patch (10.44 KB, patch)
2018-12-06 19:48 PST, Justin Michaud
no flags Details | Formatted Diff | Diff
Patch (22.52 KB, patch)
2018-12-10 11:28 PST, Justin Michaud
no flags Details | Formatted Diff | Diff
Patch (22.35 KB, patch)
2018-12-10 16:46 PST, Justin Michaud
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Justin Michaud 2018-12-03 15:00:14 PST
CS Painting API should support multiple worklets. Right now, they are just overridden.
Comment 1 Justin Michaud 2018-12-06 19:48:44 PST
Created attachment 356779 [details]
Patch
Comment 2 Dean Jackson 2018-12-07 11:07:45 PST
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)
Comment 3 Simon Fraser (smfr) 2018-12-07 17:45:10 PST
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.
Comment 4 Justin Michaud 2018-12-10 11:28:06 PST
Created attachment 356973 [details]
Patch
Comment 5 Dean Jackson 2018-12-10 15:52:29 PST
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!
Comment 6 Justin Michaud 2018-12-10 16:46:15 PST
Created attachment 357018 [details]
Patch
Comment 7 WebKit Commit Bot 2018-12-10 19:55:56 PST
Comment on attachment 357018 [details]
Patch

Clearing flags on attachment: 357018

Committed r239067: <https://trac.webkit.org/changeset/239067>
Comment 8 WebKit Commit Bot 2018-12-10 19:55:58 PST
All reviewed patches have been landed.  Closing bug.
Comment 9 Radar WebKit Bug Importer 2018-12-10 19:56:23 PST
<rdar://problem/46617313>