[Webkit-unassigned] [Bug 234155] [Shadow Realms] Use WebCore module loaders for shadow realm importValue

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=234155

Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #449668|review?                     |review+
              Flags|                            |

--- 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(globalObject);
> +    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()->shouldInterruptScriptBeforeTimeout(
> +        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().

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20220121/99d8b190/attachment.htm>