[Webkit-unassigned] [Bug 234155] [Shadow Realms] Use WebCore module loaders for shadow realm importValue
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Fri Jan 21 11:06:06 PST 2022
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>
More information about the webkit-unassigned
mailing list