[webkit-reviews] review granted: [Bug 192335] CS Painting API should support multiple worklets. : [Attachment 356779] Patch

[bugzilla-daemon at webkit.org]

Dean Jackson <dino at apple.com> has granted Justin Michaud
<justin_michaud at apple.com>'s request for review:
Bug 192335: CS Painting API should support multiple worklets.
https://bugs.webkit.org/show_bug.cgi?id=192335

Attachment 356779: Patch

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




--- Comment #2 from Dean Jackson <dino at apple.com> ---
Comment on attachment 356779
  --> https://bugs.webkit.org/attachment.cgi?id=356779
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)