Bug 190979 - Add new global object and preliminary Worklets support for CSS painting api
Summary: Add new global object and preliminary Worklets support for CSS painting api
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-10-26 20:34 PDT by Justin Michaud
Modified: 2018-11-02 21:11 PDT (History)
13 users (show)

See Also:


Attachments
Patch (176.88 KB, patch)
2018-10-26 20:47 PDT, Justin Michaud
no flags Details | Formatted Diff | Diff
Patch (177.34 KB, patch)
2018-10-29 11:57 PDT, Justin Michaud
no flags Details | Formatted Diff | Diff
Patch (177.37 KB, patch)
2018-10-29 12:57 PDT, Justin Michaud
no flags Details | Formatted Diff | Diff
Patch (178.72 KB, patch)
2018-10-29 13:16 PDT, Justin Michaud
no flags Details | Formatted Diff | Diff
Patch (178.88 KB, patch)
2018-10-29 13:26 PDT, Justin Michaud
no flags Details | Formatted Diff | Diff
Patch (183.48 KB, patch)
2018-10-29 13:55 PDT, Justin Michaud
no flags Details | Formatted Diff | Diff
Patch (183.76 KB, patch)
2018-10-30 16:00 PDT, Justin Michaud
no flags Details | Formatted Diff | Diff
Patch (184.15 KB, patch)
2018-10-31 16:32 PDT, Justin Michaud
no flags Details | Formatted Diff | Diff
Patch (184.28 KB, patch)
2018-10-31 16:47 PDT, Justin Michaud
no flags Details | Formatted Diff | Diff
Patch (184.67 KB, patch)
2018-10-31 17:11 PDT, Justin Michaud
no flags Details | Formatted Diff | Diff
Patch (184.27 KB, patch)
2018-11-01 14:14 PDT, Justin Michaud
no flags Details | Formatted Diff | Diff
Patch (184.35 KB, patch)
2018-11-02 17:30 PDT, 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-10-26 20:34:11 PDT
Add support for CSS.paintWorklet.addModule(), which should run with its own global object on the main thread.
Comment 1 Justin Michaud 2018-10-26 20:47:18 PDT Comment hidden (obsolete)
Comment 2 Build Bot 2018-10-26 20:56:05 PDT Comment hidden (obsolete)
Comment 3 Justin Michaud 2018-10-29 11:57:39 PDT Comment hidden (obsolete)
Comment 4 Build Bot 2018-10-29 12:17:55 PDT Comment hidden (obsolete)
Comment 5 Justin Michaud 2018-10-29 12:57:26 PDT Comment hidden (obsolete)
Comment 6 Build Bot 2018-10-29 13:07:12 PDT Comment hidden (obsolete)
Comment 7 Justin Michaud 2018-10-29 13:16:49 PDT
Created attachment 353315 [details]
Patch
Comment 8 Don Olmstead 2018-10-29 13:22:07 PDT
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?
Comment 9 Build Bot 2018-10-29 13:22:44 PDT Comment hidden (obsolete)
Comment 10 Justin Michaud 2018-10-29 13:26:35 PDT
Created attachment 353316 [details]
Patch
Comment 11 Don Olmstead 2018-10-29 13:29:18 PDT
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
Comment 12 Build Bot 2018-10-29 13:31:17 PDT Comment hidden (obsolete)
Comment 13 Justin Michaud 2018-10-29 13:34:56 PDT
(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.
Comment 14 Justin Michaud 2018-10-29 13:55:29 PDT
Created attachment 353318 [details]
Patch
Comment 15 Dean Jackson 2018-10-30 12:49:14 PDT
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
Comment 16 Chris Dumez 2018-10-30 14:35:45 PDT
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.
Comment 17 Justin Michaud 2018-10-30 16:00:29 PDT
Created attachment 353423 [details]
Patch
Comment 18 Chris Dumez 2018-10-30 17:02:29 PDT
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*
Comment 19 Chris Dumez 2018-10-30 17:06:52 PDT
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.
Comment 20 Saam Barati 2018-10-30 18:18:54 PDT
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
Comment 21 Justin Michaud 2018-10-31 16:32:32 PDT
Created attachment 353551 [details]
Patch
Comment 22 Chris Dumez 2018-10-31 16:38:03 PDT
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;
Comment 23 Justin Michaud 2018-10-31 16:41:13 PDT
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.
Comment 24 Justin Michaud 2018-10-31 16:47:37 PDT
Created attachment 353557 [details]
Patch
Comment 25 Justin Michaud 2018-10-31 16:54:53 PDT
(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.
Comment 26 Chris Dumez 2018-10-31 17:02:12 PDT
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.
Comment 27 Chris Dumez 2018-10-31 17:03:46 PDT
(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.
Comment 28 Justin Michaud 2018-10-31 17:11:59 PDT
Created attachment 353560 [details]
Patch
Comment 29 Chris Dumez 2018-11-01 09:44:40 PDT
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.
Comment 30 Justin Michaud 2018-11-01 14:10:41 PDT
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
Comment 31 Justin Michaud 2018-11-01 14:14:56 PDT
Created attachment 353643 [details]
Patch
Comment 32 Justin Michaud 2018-11-02 11:09:22 PDT
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.
Comment 33 Chris Dumez 2018-11-02 14:40:24 PDT
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.
Comment 34 Justin Michaud 2018-11-02 17:30:17 PDT
Created attachment 353751 [details]
Patch
Comment 35 WebKit Commit Bot 2018-11-02 21:01:39 PDT
Comment on attachment 353751 [details]
Patch

Clearing flags on attachment: 353751

Committed r237766: <https://trac.webkit.org/changeset/237766>
Comment 36 WebKit Commit Bot 2018-11-02 21:01:41 PDT
All reviewed patches have been landed.  Closing bug.
Comment 37 Radar WebKit Bug Importer 2018-11-02 21:11:23 PDT
<rdar://problem/45780764>