Bug 106800 – [V8] Make an Isolate parameter mandatory in SerializedScriptValue methods

• Status: RESOLVED
• Resolution: INVALID
• Component: WebCore JavaScript

[Kentaro Hara]

SerializedScriptValue methods are used by both V8 bindings and WebCore. So they should support both an Isolate-version method and an non-Isolate version method.

There are two ways to accomplish that:

[1] Use an optional Isolate parameter.

[2] Implement two versions and delegate the non-Isolate version method to the Isolate version method.

I would prefer the approach [2], because I'd like to statically make sure that we never passe a 0 Isolate. If we take the approach [1], we need to insert ASSERT(isolate) here and there.

[Kentaro Hara]

Created attachment 182588 [details]
Patch

[Adam Barth]

Comment on attachment 182588 [details]
Patch

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

Ok, but we should be able to remove the non-isolate version of many of these methods once we've updated all the callers.  (Notice that the JSC versions pass an ExecState.)

> Source/WebCore/ChangeLog:10
> +        SerializedScriptValue methods are used by both V8 bindings
> +        and WebCore. So they should support both an Isolate-version
> +        method and an non-Isolate version method.

The JSC version needs an ExecState, which plays a similar roll to the isolate in the V8 version.

> Source/WebCore/ChangeLog:20
> +        make sure that we never passe a 0 Isolate. If we take the approach

passe -> pass

[Kentaro Hara]

Comment on attachment 182588 [details]
Patch

Landed in r139735.

[Kentaro Hara]

(In reply to comment #2)
> Ok, but we should be able to remove the non-isolate version of many of these methods once we've updated all the callers.  (Notice that the JSC versions pass an ExecState.)

Yes. Given that these methods can be called from WebCore with a ScriptState parameter, one way to solve the problem would be to implement a mechanism to retrieve an Isolate from a ScriptState. JSC already implemented it (i.e. toJS(toRef(ScriptState*) gives us an ExecState).

[Adam Barth]

Yeah, I think we're going to want to keep the Isolate in the ScriptState for a bunch of other cases as well.

[Brian Burg]

Closing some V8-related work items.