Bug 190979 – Add new global object and preliminary Worklets support for CSS painting api

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Justin Michaud]

Add support for CSS.paintWorklet.addModule(), which should run with its own global object on the main thread.

[Justin Michaud]

Created attachment 353217 [details]
Patch

[EWS Watchlist]

Comment on attachment 353217 [details]
Patch

Attachment 353217 [details] did not pass bindings-ews (mac):
Output: https://webkit-queues.webkit.org/results/9745508

New failing tests:
Please see test output for results

[Justin Michaud]

Created attachment 353305 [details]
Patch

[EWS Watchlist]

Comment on attachment 353305 [details]
Patch

Attachment 353305 [details] did not pass bindings-ews (mac):
Output: https://webkit-queues.webkit.org/results/9770503

New failing tests:
Please see test output for results

[Justin Michaud]

Created attachment 353311 [details]
Patch

[EWS Watchlist]

Comment on attachment 353311 [details]
Patch

Attachment 353311 [details] did not pass bindings-ews (mac):
Output: https://webkit-queues.webkit.org/results/9770987

New failing tests:
Please see test output for results

[Justin Michaud]

Created attachment 353315 [details]
Patch

[Don Olmstead]

Comment on attachment 353315 [details]
Patch

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

This is great! Someone with more JSC binding experience should look at the code more but looks ok from where I stand. Just saw a few formatting issues.

> Source/WebCore/DerivedSources.make:1029
> +	$(WebCore)/worklets/PaintWorkletGlobalScope.idl \
> +    $(WebCore)/worklets/Worklet.idl \
> +	$(WebCore)/worklets/WorkletGlobalScope.idl \

You got some weird spacing here.

> Source/WebCore/DerivedSources.make:1162
> +	$(WORKLETGLOBALSCOPE_CONSTRUCTORS_FILE) \

Same weird spacing.

> Source/WebCore/worklets/PaintWorkletGlobalScope.h:35
> +

Remove blank line here?

[EWS Watchlist]

Comment on attachment 353315 [details]
Patch

Attachment 353315 [details] did not pass bindings-ews (mac):
Output: https://webkit-queues.webkit.org/results/9771232

New failing tests:
Please see test output for results

[Justin Michaud]

Created attachment 353316 [details]
Patch

[Don Olmstead]

Comment on attachment 353316 [details]
Patch

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

> Source/WebCore/DerivedSources.make:1162
> +	$(WORKLETGLOBALSCOPE_CONSTRUCTORS_FILE) \

Still has the weird spacing

[EWS Watchlist]

Comment on attachment 353316 [details]
Patch

Attachment 353316 [details] did not pass bindings-ews (mac):
Output: https://webkit-queues.webkit.org/results/9771356

New failing tests:
Please see test output for results

[Justin Michaud]

(In reply to Don Olmstead from comment #11)
> Comment on attachment 353316 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=353316&action=review
> 
> > Source/WebCore/DerivedSources.make:1162
> > +	$(WORKLETGLOBALSCOPE_CONSTRUCTORS_FILE) \
> 
> Still has the weird spacing

Heh, Xcode isn't cooperating. I'll fix it with vim.

[Justin Michaud]

Created attachment 353318 [details]
Patch

[Dean Jackson]

Comment on attachment 353318 [details]
Patch

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

> Source/WebCore/bindings/js/JSWorkletGlobalScopeBase.cpp:42
> +namespace WebCore {
> +using namespace JSC;

Nit: We usually put a blank line between these.

> Source/WebCore/css/CSSPaintImageValue.cpp:53
> +    auto* selectedGlobalScope = renderElement.document().getCSSPaintWorkletGlobalScope();

Shouldn't this be getPaintWorkletGlobalScope() now?

> Source/WebCore/css/DOMCSSPaintWorklet.idl:30
> +    [ImplementedAs=ensurePaintWorklet,CallWith=Document] static readonly attribute Worklet paintWorklet;

Should DOMCSSPaintWorklet.* be DOMPaintWorklet.* now?

> Source/WebCore/dom/Document.cpp:8381
> +Worklet& Document::ensureCSSPaintWorklet()

ensurePaintWorklet?

> Source/WebCore/worklets/PaintWorkletGlobalScope.cpp:45
> +    return 1.0;

Why not fetch this from the document?

> Source/WebCore/worklets/WorkletGlobalScope.h:55
> +    String origin() const final { ASSERT_NOT_REACHED(); return ""; }

emptyString()

> Source/WebCore/worklets/WorkletScriptController.cpp:170
> +        // This FIXME is from in WorkerScriptController.

Typo: in

[Chris Dumez]

Comment on attachment 353318 [details]
Patch

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

> Source/WebCore/worklets/Worklet.cpp:49
> +    auto context = PaintWorkletGlobalScope::create(makeWeakPtr(document), ScriptSourceCode(moduleURL));

We usually prefer to pass the object and let the callee decide to take a WeakPtr. So:
auto context = PaintWorkletGlobalScope::create(document, ScriptSourceCode(moduleURL));

here

and then makeWeakPtr() inside the PaintWorkletGlobalScope constructor when assigning it to the member.

[Justin Michaud]

Created attachment 353423 [details]
Patch

[Chris Dumez]

Comment on attachment 353423 [details]
Patch

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

> Source/WebCore/Sources.txt:388
> +bindings/js/JSPaintWorkletGlobalScopeCustom.cpp

Bad sorting now.

> Source/WebCore/bindings/js/JSPaintWorkletGlobalScopeCustom.cpp:47
> +void JSPaintWorkletGlobalScope::visitAdditionalChildren(JSC::SlotVisitor& visitor)

I hear these visit methods can get called on a background thread. As a result, you'll likely need to protect all accesses / modifications to paintDefinitionMap with a Lock.

> Source/WebCore/bindings/js/JSPaintWorkletGlobalScopeCustom.cpp:54
> +JSValue JSPaintWorkletGlobalScope::registerPaint(ExecState& state)

Please add a comment explaining why this requires custom code. What is the bindings generator missing to generate the code you need? We don't like to add new custom code unless there is a very good reason to.

> Source/WebCore/bindings/js/JSPaintWorkletGlobalScopeCustom.cpp:58
> +    PaintWorkletGlobalScope& workletGlobalScope = wrapped();

auto&

> Source/WebCore/bindings/js/JSWorkletGlobalScopeBase.cpp:87
> +    JSWorkletGlobalScopeBase* thisObject = jsCast<JSWorkletGlobalScopeBase*>(cell);

auto*

> Source/WebCore/bindings/js/JSWorkletGlobalScopeBase.cpp:120
> +    const JSWorkletGlobalScopeBase* thisObject = jsCast<const JSWorkletGlobalScopeBase*>(object);

auto*

> Source/WebCore/bindings/js/JSWorkletGlobalScopeBase.cpp:134
> +    JSWorkletGlobalScope* contextWrapper = script->workletGlobalScopeWrapper();

auto*

> Source/WebCore/bindings/js/JSWorkletGlobalScopeBase.h:40
> +    typedef JSDOMGlobalObject Base;

using Base = JSDOMGlobalObject;

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:509
> +    return 0 if $interface->type->name eq "WorkletGlobalScope";

Please add a comment before this line like so:
# For worklets, the global object is a PaintWorkletGlobalScope.

> Source/WebCore/dom/Document.cpp:8390
> +    m_paintWorkletGlobalScope = WTFMove(scope);

Should we ASSERT(!m_paintWorkletGlobalScope); before this? or is it OK to overwrite a previous WorkletGlobalScope?

> Source/WebCore/dom/Document.h:1526
> +    PaintWorkletGlobalScope* getPaintWorkletGlobalScope() { return m_paintWorkletGlobalScope.get(); }

Getters in WebKit do not use the 'get' prefix. Should be:
PaintWorkletGlobalScope* paintWorkletGlobalScope();

> Source/WebCore/worklets/PaintWorkletGlobalScope.cpp:38
> +    return adoptRef(*new PaintWorkletGlobalScope(makeWeakPtr(document), WTFMove(code)));

return adoptRef(*new PaintWorkletGlobalScope(document, WTFMove(code)));

> Source/WebCore/worklets/PaintWorkletGlobalScope.cpp:42
> +    : WorkletGlobalScope(WTFMove(document), WTFMove(code))

makeWeakPtr(document)

> Source/WebCore/worklets/PaintWorkletGlobalScope.h:43
> +    bool isPaintWorkletGlobalScope() const final { return true; }

Should probably be private.

> Source/WebCore/worklets/PaintWorkletGlobalScope.h:58
> +    PaintWorkletGlobalScope(WeakPtr<Document>&&, ScriptSourceCode&&);

Document&

> Source/WebCore/worklets/Worklet.cpp:42
> +

Extra blank line here.

> Source/WebCore/worklets/Worklet.cpp:49
> +    auto context = PaintWorkletGlobalScope::create(document, ScriptSourceCode(moduleURL));

Does this need a FIXME comment indicating that moduleURL should actually be a URL and not source code?

> Source/WebCore/worklets/Worklet.cpp:51
> +    document.setPaintWorkletGlobalScope(WTFMove(context));

What if the document already had a PaintWorkletGlobalScope?

> Source/WebCore/worklets/Worklet.h:33
> +class Worklet : public RefCounted<Worklet> {

Should probably subclass ScriptWrappable since it can have a JS wrapper.

> Source/WebCore/worklets/Worklet.h:37
> +    void addModule(Document&, String moduleURL);

const String&

> Source/WebCore/worklets/Worklet.h:41
> +

No need for this blank line.

> Source/WebCore/worklets/Worklet.h:45
> +#endif

Missing blank line before this.

> Source/WebCore/worklets/WorkletGlobalScope.cpp:49
> +WorkletGlobalScope::WorkletGlobalScope(WeakPtr<Document>&& document, ScriptSourceCode&& code)

Document&

> Source/WebCore/worklets/WorkletGlobalScope.cpp:50
> +    : m_document(WTFMove(document))

makeWeakPtr(document)

> Source/WebCore/worklets/WorkletGlobalScope.cpp:52
> +    , m_script(std::make_unique<WorkletScriptController>(this))

if m_script cannot be null then we should use UniqueRef instead of unique_ptr.

> Source/WebCore/worklets/WorkletGlobalScope.cpp:59
> +    Frame* frame = m_document->frame();

auto&

> Source/WebCore/worklets/WorkletGlobalScope.cpp:60
> +    m_jsRuntimeFlags = (frame ? frame->settings().javaScriptRuntimeFlags() : JSC::RuntimeFlags());

No need for the surrounding brackets.

> Source/WebCore/worklets/WorkletGlobalScope.cpp:75
> +    return m_topOrigin->toString();

Isn't the origin of the WorkletGlobalScope the origin of its document?

The call to `setSecurityOriginPolicy(SecurityOriginPolicy::create(m_document->securityOrigin()));` in the constructor made me believe so.

> Source/WebCore/worklets/WorkletGlobalScope.cpp:110
> +    // Always use UTF-8 in Workers.

Drop this comment, this is not a worker.

> Source/WebCore/worklets/WorkletGlobalScope.h:50
> +    virtual ~WorkletGlobalScope();

no need for the 'virtual'.

> Source/WebCore/worklets/WorkletGlobalScope.h:52
> +    virtual bool isPaintWorkletGlobalScope() const { return false; }

Should probably be private.

> Source/WebCore/worklets/WorkletGlobalScope.h:59
> +    IDBClient::IDBConnectionProxy* idbConnectionProxy() final { ASSERT_NOT_REACHED(); return nullptr; }

Should probably be private.

> Source/WebCore/worklets/WorkletGlobalScope.h:62
> +    void postTask(Task&&) final { ASSERT_NOT_REACHED(); }

Should probably be private.

> Source/WebCore/worklets/WorkletScriptController.cpp:188
> +    JSC::ExecState* exec = m_workletGlobalScopeWrapper->globalExec();

auto*

[Chris Dumez]

Comment on attachment 353423 [details]
Patch

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

>> Source/WebCore/bindings/js/JSPaintWorkletGlobalScopeCustom.cpp:47
>> +void JSPaintWorkletGlobalScope::visitAdditionalChildren(JSC::SlotVisitor& visitor)
> 
> I hear these visit methods can get called on a background thread. As a result, you'll likely need to protect all accesses / modifications to paintDefinitionMap with a Lock.

You should probably confirm this with JSC engineer (like Mark Lam) before doing the extra work since I am only 90% certain.

[Saam Barati]

Comment on attachment 353423 [details]
Patch

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

>>> Source/WebCore/bindings/js/JSPaintWorkletGlobalScopeCustom.cpp:47
>>> +void JSPaintWorkletGlobalScope::visitAdditionalChildren(JSC::SlotVisitor& visitor)
>> 
>> I hear these visit methods can get called on a background thread. As a result, you'll likely need to protect all accesses / modifications to paintDefinitionMap with a Lock.
> 
> You should probably confirm this with JSC engineer (like Mark Lam) before doing the extra work since I am only 90% certain.

This can.

>> Source/WebCore/bindings/js/JSPaintWorkletGlobalScopeCustom.cpp:54
>> +JSValue JSPaintWorkletGlobalScope::registerPaint(ExecState& state)
> 
> Please add a comment explaining why this requires custom code. What is the bindings generator missing to generate the code you need? We don't like to add new custom code unless there is a very good reason to.

Isn't this just copied from the previous implementation?

> Source/WebCore/bindings/js/JSWorkletGlobalScopeBase.h:84
> +} // namespace WebCore

nit: You should add a newline above this

> Source/WebCore/bindings/js/JSWorkletGlobalScopeBase.h:85
> +#endif

style nit: You should comment what this endif is for. You should also add a newline above this.

> Source/WebCore/worklets/WorkletGlobalScope.h:109
> +    bool wrapCryptoKey(const Vector<uint8_t>&, Vector<uint8_t>&) final { RELEASE_ASSERT_NOT_REACHED(); return false; }
> +    bool unwrapCryptoKey(const Vector<uint8_t>&, Vector<uint8_t>&) final { RELEASE_ASSERT_NOT_REACHED(); return false; }

Do you have a bug for this?

> Source/WebCore/worklets/WorkletGlobalScope.h:123
> +    // FIXME: This is not implemented properly, it just satisfies the compiler.

Should have a bug #

> Source/WebCore/worklets/WorkletScriptController.cpp:105
> +    JSLockHolder lock(m_vm.get());

might as well be consistent with above and do:
JSLockHolder lock { vm() };

> Source/WebCore/worklets/WorkletScriptController.cpp:109
> +    // Explicitly protect the global object's prototype so it isn't collected
> +    // when we allocate the global object. (Once the global object is fully
> +    // constructed, it can mark its own prototype.)

This Strong<> is not needed. It'll either be alive in the stack or in a register since it's used below. Also, you pass in the "structure" below, which will mark its prototype. That "structure" is live until the global object sets its value. So this can just be a regular JSGlobalScopePrototype*

> Source/WebCore/worklets/WorkletScriptController.cpp:170
> +        // This FIXME is from WorkerScriptController.

This is ambiguous. You should link to a bug

[Justin Michaud]

Created attachment 353551 [details]
Patch

[Chris Dumez]

Comment on attachment 353551 [details]
Patch

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

> Source/WebCore/worklets/PaintWorkletGlobalScope.h:42
> +    bool isPaintWorkletGlobalScope() const final { return true; }

Why is this public?

> Source/WebCore/worklets/PaintWorkletGlobalScope.h:54
> +    HashMap<String, std::unique_ptr<PaintDefinition>>& paintDefinitionMap() { return m_paintDefinitionMap; }

You should assert that m_paintDefinitionMapLock is locked here.

> Source/WebCore/worklets/PaintWorkletGlobalScope.h:55
> +    Lock& lock() { return m_lock; }

name is too generic.

> Source/WebCore/worklets/PaintWorkletGlobalScope.h:61
> +    Lock m_lock;

m_lock seems to generic a name. I'd call this m_paintDefinitionMapLock;

[Justin Michaud]

Comment on attachment 353423 [details]
Patch

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

>>> Source/WebCore/bindings/js/JSPaintWorkletGlobalScopeCustom.cpp:54
>>> +JSValue JSPaintWorkletGlobalScope::registerPaint(ExecState& state)
>> 
>> Please add a comment explaining why this requires custom code. What is the bindings generator missing to generate the code you need? We don't like to add new custom code unless there is a very good reason to.
> 
> Isn't this just copied from the previous implementation?

No, this is new.

>> Source/WebCore/dom/Document.cpp:8390
>> +    m_paintWorkletGlobalScope = WTFMove(scope);
> 
> Should we ASSERT(!m_paintWorkletGlobalScope); before this? or is it OK to overwrite a previous WorkletGlobalScope?

I added a FIXME

>> Source/WebCore/worklets/PaintWorkletGlobalScope.h:43
>> +    bool isPaintWorkletGlobalScope() const final { return true; }
> 
> Should probably be private.

This can't be, it is used in WorkletScriptController

>> Source/WebCore/worklets/WorkletGlobalScope.h:109
>> +    bool unwrapCryptoKey(const Vector<uint8_t>&, Vector<uint8_t>&) final { RELEASE_ASSERT_NOT_REACHED(); return false; }
> 
> Do you have a bug for this?

No. It looks like it isn't actually a bug, since Worklets don't allow access to indexeddb.

[Justin Michaud]

Created attachment 353557 [details]
Patch

[Justin Michaud]

(In reply to Chris Dumez from comment #22)
> Comment on attachment 353551 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=353551&action=review
> 
> > Source/WebCore/worklets/PaintWorkletGlobalScope.h:42
> > +    bool isPaintWorkletGlobalScope() const final { return true; }
> 
> Why is this public?

It is needed by WorkletScriptController. I tried to use is<>() instead, but the type traits would also require it to be public.

> > Source/WebCore/worklets/PaintWorkletGlobalScope.h:54
> > +    HashMap<String, std::unique_ptr<PaintDefinition>>& paintDefinitionMap() { return m_paintDefinitionMap; }
> 
> You should assert that m_paintDefinitionMapLock is locked here.

Great idea! I put in a RELEASE_ASSERT, although I am not sure if that should be a regular ASSERT instead. I am not sure if a race here could cause a security issue.

[Chris Dumez]

Comment on attachment 353557 [details]
Patch

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

> Source/WebCore/worklets/PaintWorkletGlobalScope.h:54
> +    HashMap<String, std::unique_ptr<PaintDefinition>>& paintDefinitionMap() { RELEASE_ASSERT(m_paintDefinitionLock.isLocked()); return m_paintDefinitionMap; }

I would use a debug assertion for performance reasons here.

[Chris Dumez]

(In reply to Justin Michaud from comment #25)
> (In reply to Chris Dumez from comment #22)
> > Comment on attachment 353551 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=353551&action=review
> > 
> > > Source/WebCore/worklets/PaintWorkletGlobalScope.h:42
> > > +    bool isPaintWorkletGlobalScope() const final { return true; }
> > 
> > Why is this public?
> 
> It is needed by WorkletScriptController. I tried to use is<>() instead, but
> the type traits would also require it to be public.

Does not make much sense, it should be public in the base class, and private in the subclass because it makes no sense to call this method on the subclass (if you are, you are doing a useless check). And Yes, you should be using is<>() / downcast<>() whenever possible but is<>() does usually rely on having such a virtual function defined.

[Justin Michaud]

Created attachment 353560 [details]
Patch

[Chris Dumez]

Comment on attachment 353560 [details]
Patch

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

> Source/WebCore/bindings/js/JSPaintWorkletGlobalScopeCustom.cpp:55
> +// This custom binding is required to handle the paint callback conversion according to the spec.

Still not clear for me why we need custom bindings. We have plenty of API that takes callbacks without custom bindings (e.g. MutationObserver constructor, FileSystemEntry.getParent, Geolocation.getCurrentPosition).
You still have not convinced me you NEED custom bindings.

> Source/WebCore/bindings/js/JSPaintWorkletGlobalScopeCustom.cpp:127
> +    auto paintDefinition = std::unique_ptr<PaintDefinition>(new PaintDefinition { name, paint.releaseNonNull(), inputProperties, inputArguments });

heh.. std::make_unique<PaintDefinition>(name, paint.releaseNonNull(), inputProperties, inputArguments)

> Source/WebCore/bindings/js/JSWorkletGlobalScopeBase.cpp:120
> +    const auto* thisObject = jsCast<const JSWorkletGlobalScopeBase*>(object);

auto*, we like to omit 'const' whenever possible.

> Source/WebCore/bindings/js/JSWorkletGlobalScopeBase.cpp:143
> +    const ClassInfo* classInfo = asObject(value)->classInfo(vm);

auto*

> Source/WebCore/bindings/js/JSWorkletGlobalScopeBase.h:84
> +} // namespace WebCore

Missing blank line before this.

> Source/WebCore/dom/ScriptExecutionContext.cpp:488
> +    RELEASE_ASSERT_NOT_REACHED();

Does this really need to be a RELEASE_ASSERT?

> Source/WebCore/dom/ScriptExecutionContext.cpp:532
> +        WorkerGlobalScope& workerGlobalScope = downcast<WorkerGlobalScope>(*this);

This local variable is not needed, then you don't need the curly brackets either.

> Source/WebCore/dom/ScriptExecutionContext.cpp:537
> +        WorkletGlobalScope& workletGlobalScope = downcast<WorkletGlobalScope>(*this);

This local variable is not needed.

> Source/WebCore/worklets/PaintWorkletGlobalScope.cpp:49
> +    ASSERT(m_document->domWindow());

These assertions look super risky to me and I'd suggest null checks instead. You can probably return 1.0 when document or window are null.

> Source/WebCore/worklets/PaintWorkletGlobalScope.h:44
> +    double devicePixelRatio();

const ?

> Source/WebCore/worklets/PaintWorkletGlobalScope.idl:37
> +    [Custom, MayThrowException] void registerPaint(DOMString name, Function paintCtor);

paintCtor -> paintConstructor

> Source/WebCore/worklets/WorkletConsoleClient.h:46
> +protected:

It makes no sense for a class marked as final to have protected members. If the class is really final, then these members should be private.

> Source/WebCore/worklets/WorkletConsoleClient.h:47
> +    void messageWithTypeAndLevel(MessageType, MessageLevel, JSC::ExecState*, Ref<Inspector::ScriptArguments>&&) override;

override -> final, here and below.

> Source/WebCore/worklets/WorkletGlobalScope.cpp:54
> +    , m_identifier("WorkletGlobalScope:" + Inspector::IdentifiersFactory::createIdentifier())

makeString("WorkletGlobalScope:"_s, Inspector::IdentifiersFactory::createIdentifier())

> Source/WebCore/worklets/WorkletGlobalScope.cpp:58
> +    auto* frame = m_document->frame();

I'd rather you use document.frame() here as it looks safer than m_document->

> Source/WebCore/worklets/WorkletGlobalScope.cpp:60
> +    ASSERT(m_document->page());

ditto.

> Source/WebCore/worklets/WorkletGlobalScope.cpp:79
> +    ASSERT(m_document);

This assertion looks risky to me, I'd suggest a null check. Same comment in other places. Or tell me what guarantees that you always have a document.

> Source/WebCore/worklets/WorkletGlobalScope.h:49
> +class WorkletGlobalScope : public RefCounted<WorkletGlobalScope>, public Supplementable<WorkletGlobalScope>, public ScriptExecutionContext, public EventTargetWithInlineData {

I don't think you need this to be Supplementable, do you?

> Source/WebCore/worklets/WorkletGlobalScope.h:64
> +    WorkletScriptController* script() { return &m_script.get(); }

Why doesn't this return a WorkletScriptController& instead of a WorkletScriptController* ? I can never return null.

> Source/WebCore/worklets/WorkletGlobalScope.h:81
> +    WeakPtr<Document> m_document;

Having protected data members is bad practice. Data members should be private. You can add a protected getter for that data member if needed.

> Source/WebCore/worklets/WorkletGlobalScope.h:126
> +    mutable WorkerEventQueue m_eventQueue;

Why does this need to be mutable?

> Source/WebCore/worklets/WorkletScriptController.cpp:187
> +#endif

Missing blank line before this.

[Justin Michaud]

Comment on attachment 353560 [details]
Patch

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

>> Source/WebCore/bindings/js/JSPaintWorkletGlobalScopeCustom.cpp:127
>> +    auto paintDefinition = std::unique_ptr<PaintDefinition>(new PaintDefinition { name, paint.releaseNonNull(), inputProperties, inputArguments });
> 
> heh.. std::make_unique<PaintDefinition>(name, paint.releaseNonNull(), inputProperties, inputArguments)

It's a struct, so that doesn't work. Should I leave it as-is, or add a constructor?

>> Source/WebCore/dom/ScriptExecutionContext.cpp:488
>> +    RELEASE_ASSERT_NOT_REACHED();
> 
> Does this really need to be a RELEASE_ASSERT?

I wasn't sure if commonVM() was safe to be returned, but I needed to return a reference to something to satisfy the compiler.

>> Source/WebCore/worklets/WorkletGlobalScope.h:126
>> +    mutable WorkerEventQueue m_eventQueue;
> 
> Why does this need to be mutable?

The signature of the getter is:
EventQueue& eventQueue() const
For some reason. I have no clue why, but I can't change it without changing ScriptExecutionContext

[Justin Michaud]

Created attachment 353643 [details]
Patch

[Justin Michaud]

Comment on attachment 353643 [details]
Patch

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

> Source/WebCore/worklets/WorkletScriptController.cpp:157
> +void WorkletScriptController::evaluate(const ScriptSourceCode& sourceCode, NakedPtr<JSC::Exception>& returnedException, String* returnedExceptionMessage)

Can you confirm this is correct? Worklets have a unique opaque origin, so normally this would always sanitize exception messages. At the same time, it is important that exceptions show up in the inspector. As far as I know, there is nowhere where a worklet can import a cross-origin script (do ES6 modules allow cross-origin imports?), so this is safe.

[Chris Dumez]

Comment on attachment 353643 [details]
Patch

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

r=me

>> Source/WebCore/worklets/WorkletScriptController.cpp:157
>> +void WorkletScriptController::evaluate(const ScriptSourceCode& sourceCode, NakedPtr<JSC::Exception>& returnedException, String* returnedExceptionMessage)
> 
> Can you confirm this is correct? Worklets have a unique opaque origin, so normally this would always sanitize exception messages. At the same time, it is important that exceptions show up in the inspector. As far as I know, there is nowhere where a worklet can import a cross-origin script (do ES6 modules allow cross-origin imports?), so this is safe.

You are not exposing this exception message to JS, are you? Logging it in WebInspector does not count as passing it to JS.

[Justin Michaud]

Created attachment 353751 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 353751 [details]
Patch

Clearing flags on attachment: 353751

Committed r237766: <https://trac.webkit.org/changeset/237766>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/45780764>