[webkit-reviews] review granted: [Bug 234155] [Shadow Realms] Use WebCore module loaders for shadow realm importValue : [Attachment 449668] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Joseph Griego <jgriego at igalia.com>'s
request for review:
Bug 234155: [Shadow Realms] Use WebCore module loaders for shadow realm
importValue
https://bugs.webkit.org/show_bug.cgi?id=234155

Attachment 449668: Patch

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




--- Comment #21 from Darin Adler <darin at apple.com> ---
Comment on attachment 449668
  --> https://bugs.webkit.org/attachment.cgi?id=449668
Patch

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

> Source/JavaScriptCore/runtime/ShadowRealmObject.cpp:65
> +    JSGlobalObject* shadowRealmGlobalObject =
globalObject->globalObjectMethodTable()->deriveShadowRealmGlobalObject(globalOb
ject);
> +    object->m_globalObject.set(vm, object, shadowRealmGlobalObject);

I think this would read better without the local variable; also the single line
of code would be shorter than the line defining the local variable, and the
words "shadow realm global object" are in the function name.

> Source/WebCore/bindings/js/JSDOMGlobalObject.cpp:598
> +	   auto document = downcast<Document>(context);

I would write:

    auto& document = downcast<Document>(*context);

The code above already does the null check.

> Source/WebCore/bindings/js/JSShadowRealmGlobalScopeBase.cpp:120
> +    auto incubatingGlobalObject = jsCast<const
JSShadowRealmGlobalScopeBase*>(object)->incubatingRealm();
> +    return
incubatingGlobalObject->globalObjectMethodTable()->supportsRichSourceInfo(
> +	   incubatingGlobalObject
> +    );

Why the vertical format? This would be fine/better on a single line.

Also, in a short function like this I suggest single word variable names. Seems
that using the longer incubatingGlobalObject name is not helpful for
readability.

> Source/WebCore/bindings/js/JSShadowRealmGlobalScopeBase.cpp:128
> +    auto incubatingGlobalObject = jsCast<const
JSShadowRealmGlobalScopeBase*>(object)->incubatingRealm();
> +    return
incubatingGlobalObject->globalObjectMethodTable()->shouldInterruptScript(
> +	   incubatingGlobalObject
> +    );

Ditto.

> Source/WebCore/bindings/js/JSShadowRealmGlobalScopeBase.cpp:136
> +    auto incubatingGlobalObject = jsCast<const
JSShadowRealmGlobalScopeBase*>(object)->incubatingRealm();
> +    return
incubatingGlobalObject->globalObjectMethodTable()->shouldInterruptScriptBeforeT
imeout(
> +	   incubatingGlobalObject
> +    );

Ditto.

> Source/WebCore/bindings/js/JSShadowRealmGlobalScopeBase.cpp:144
> +    auto incubatingGlobalObject = jsCast<const
JSShadowRealmGlobalScopeBase*>(object)->incubatingRealm();
> +    return
incubatingGlobalObject->globalObjectMethodTable()->javaScriptRuntimeFlags(
> +	   incubatingGlobalObject
> +    );

Ditto.

> Source/WebCore/bindings/js/JSShadowRealmGlobalScopeBase.cpp:152
> +    auto incubatingGlobalObject =
jsCast<JSShadowRealmGlobalScopeBase*>(globalObject)->incubatingRealm();
> +    return
incubatingGlobalObject->globalObjectMethodTable()->scriptExecutionStatus(
> +	   incubatingGlobalObject, owner
> +    );

Ditto.

> Source/WebCore/bindings/js/JSShadowRealmGlobalScopeBase.cpp:160
> +    auto incubatingGlobalObject =
jsCast<JSShadowRealmGlobalScopeBase*>(globalObject)->incubatingRealm();
> +   
incubatingGlobalObject->globalObjectMethodTable()->reportViolationForUnsafeEval
(
> +	   incubatingGlobalObject, msg
> +    );

Ditto.

> Source/WebCore/bindings/js/JSShadowRealmGlobalScopeBase.cpp:168
> +    auto incubatingGlobalObject =
jsCast<JSShadowRealmGlobalScopeBase*>(&object)->incubatingRealm();
> +   
incubatingGlobalObject->globalObjectMethodTable()->queueMicrotaskToEventLoop(
> +	   *incubatingGlobalObject, WTFMove(task)
> +    );

Ditto.

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:791
> +sub CreateNeedsJSProxy

This sounds like a function that creates a "needs JS proxy". That’s why we
often use different wording like ShouldIncludeJSProxyAsCreateArgument, but my
name isn’t perfect.

> Source/WebCore/page/ShadowRealmGlobalScope.cpp:41
> +RefPtr<ShadowRealmGlobalScope>
ShadowRealmGlobalScope::tryCreate(JSDOMGlobalObject* wrapper,
ScriptModuleLoader* loader)
> +{
> +    return adoptRef(new ShadowRealmGlobalScope(wrapper, loader));
> +}

This function never returns null. Why not have a create function that returns
Ref<> rather than a tryCreate function that returns RefPtr, that pretends it
can fail, but never can?

> Source/WebCore/page/ShadowRealmGlobalScope.cpp:64
> +JSShadowRealmGlobalScopeBase* ShadowRealmGlobalScope::wrapper()
> +{
> +    return m_wrapper.get();
> +}

I suggest we consider putting this inline in the header, as we did with self().