[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

--- Comment #18 from Joseph Griego <jgriego at igalia.com> ---
Comment on attachment 448861
  --> https://bugs.webkit.org/attachment.cgi?id=448861
Patch

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

Darin,

Many thanks for the detailed review.

I'll re-check the style guide and fix the issues you've flagged, apologies.

>> Source/WebCore/bindings/js/JSDOMGlobalObject.cpp:272
>> +        return jsCast<const JSShadowRealmGlobalScopeBase*>(this)->scriptExecutionContext();
> 
> Slightly puzzled by the way this list is sorted; not quite alphabetical. Good that we put the two from "normal web browsing" first, I guess, for performance optimization reasons maybe?
> 
> Eventually we will have to change this so it walks the inheritance chain only once rather than 6 times in a row. Hoping this is not yet a performance bottleneck.

Yeah, this code doesn't seem ideal but this patch was already getting large and I didn't want to go tearing up the floorboards... perhaps I can see if there's a nice way to clean this (and below, in `scriptModuleLoader`) up in a follow-up patch?

>> Source/WebCore/bindings/js/JSDOMGlobalObject.cpp:579
>> +JSC::JSGlobalObject* JSDOMGlobalObject::deriveShadowRealmGlobalObject(JSC::VM& vm, JSC::JSGlobalObject* globalObject)
> 
> Irritating that this function takes JSGlobalObject and needs to "know" it's a JSDOMGlobalObject? I know many other functions do that too, but it’s a frustrating pattern. All those runtime checks for something that is guaranteed. But maybe there’s no simple solution because of the method table.
> 
> Also, why take both a vm and a globalObject? Would be nice to not take the vm unless it’s an important optimization. Eventually all the extra arguments start to make functions harder to read.

I don't think it's necessary to take `vm` as a parameter, will fix.

And yeah, unfortunately this is on the JSGlobalObject method table so unless we cook up some extra machinery it needs to stay `JSGlobalObject*`

>> Source/WebCore/bindings/js/JSDOMGlobalObject.cpp:605
>> +        }
> 
> This loop looks wrong in a risky way. If the worlds are ever not equal, it becomes an infinite loop, since nothing changes doc! And this proves we don’t have extensive-enough test coverage; it might be difficult to construct a test for this, but we need one.
> 
> I think it would be nice if we could find a way to write it as a for loop.
> 
> Also the cast to JSDOMGlobalObject is safe, but why would we have to think about that here. It would be best to change the return type of globalObject in Document, or even some lower base class, so we don’t have to do the cast here.

Oof you're right; that's embarrasing :X

Will fix + try to push the type narrowing further down

>> LayoutTests/ChangeLog:7
>> +        nested, in worker contexts, or in iframes.
> 
> Can these tests be added to Web Platform Tests instead of WebKit-specific tests?
> 
> I personally find our WebKit-specific tests easier to read and maintain, but Web Platform Tests server an important additional purpose of helping us achieve interoperability between web browsers, and so they are strongly preferred.

Is both acceptable? Will be adding coverage in WPT but WebKit is the first implementer and esp. given the awkward lifetime issues I'd like to include tests in this patch.

-- 
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/20220118/12d64710/attachment.htm>