Bug 192335

Created attachment 356779 [details] Patch

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

Created attachment 356973 [details] Patch

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!

Created attachment 357018 [details] Patch

Comment on attachment 357018 [details] Patch Clearing flags on attachment: 357018 Committed r239067: <https://trac.webkit.org/changeset/239067>

All reviewed patches have been landed. Closing bug.

<rdar://problem/46617313>