[webkit-reviews] review denied: [Bug 190979] Add new global object and preliminary Worklets support for CSS painting api : [Attachment 353423] Patch

[bugzilla-daemon at webkit.org]

Chris Dumez <cdumez at apple.com> has denied Justin Michaud
<justin_michaud at apple.com>'s request for review:
Bug 190979: Add new global object and preliminary Worklets support for CSS
painting api
https://bugs.webkit.org/show_bug.cgi?id=190979

Attachment 353423: Patch

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




--- Comment #18 from Chris Dumez <cdumez at apple.com> ---
Comment on attachment 353423
  --> https://bugs.webkit.org/attachment.cgi?id=353423
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->securityOrigi
n()));` 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*