Bug 228331 – Implement self.structuredClone()

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Domenic Denicola]

Spec PR: https://github.com/whatwg/html/pull/3414
Tests PR: https://github.com/web-platform-tests/wpt/pull/11069
Spec: https://html.spec.whatwg.org/#structured-cloning

[Radar WebKit Bug Importer]

<rdar://problem/81468374>

[Chris Dumez]

Created attachment 436808 [details]
Patch

[Chris Dumez]

Created attachment 436809 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 436809 [details]
Patch

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

> Source/WebCore/page/WindowOrWorkerGlobalScope.cpp:51
> +    auto messageData = SerializedScriptValue::create(globalObject, value, WTFMove(options.transfer), ports, SerializationContext::WindowPostMessage);

It's very unfortunate that we have to serialize to a string just to make the structured clone.
Have we checked the perf of our implementation with other browsers?
We should make sure our implementation isn't orders of magnitude slower than other browser's.

[Darin Adler]

Comment on attachment 436809 [details]
Patch

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

> Source/WebCore/page/StructuredSerializeOptions.h:38
> +    StructuredSerializeOptions() = default;
> +    StructuredSerializeOptions(Vector<JSC::Strong<JSC::JSObject>>&& transfer)
> +        : transfer(WTFMove(transfer))
> +    { }

Do we really need these constructors? Annoying that we can’t program with a struct without doing those.

> Source/WebCore/page/WindowOrWorkerGlobalScope.cpp:43
> +    auto* exception = JSC::jsDynamicCast<JSC::Exception*>(vm, error);
> +    if (!exception)
> +        exception = JSC::Exception::create(vm, error);

Does argument-dependent lookup allow us to omit either of these "JSC::" prefixes?

> Source/WebCore/page/WindowOrWorkerGlobalScope.h:37
> +template<typename T> class ExceptionOr;

No need for the "T" here.

> Source/WebCore/workers/Worker.h:34
> +#include "StructuredSerializeOptions.h"

Is this include needed here? To help the bindings compile? I know the header itself could do with a forward declaration instead.

> Source/WebCore/workers/service/ServiceWorker.h:34
> +#include "StructuredSerializeOptions.h"

Same question.

> Source/WebCore/workers/service/ServiceWorkerClient.h:33
> +#include "StructuredSerializeOptions.h"

I suppose same question again.

[Chris Dumez]

Comment on attachment 436809 [details]
Patch

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

>> Source/WebCore/page/StructuredSerializeOptions.h:38
>> +    { }
> 
> Do we really need these constructors? Annoying that we can’t program with a struct without doing those.

I merely renamed our struct to match the new name in the spec but I'll check.

>> Source/WebCore/page/WindowOrWorkerGlobalScope.cpp:43
>> +        exception = JSC::Exception::create(vm, error);
> 
> Does argument-dependent lookup allow us to omit either of these "JSC::" prefixes?

Will try.

>> Source/WebCore/workers/Worker.h:34
>> +#include "StructuredSerializeOptions.h"
> 
> Is this include needed here? To help the bindings compile? I know the header itself could do with a forward declaration instead.

Again, just renaming here but I'll try and reduce header includes.

[Chris Dumez]

(In reply to Ryosuke Niwa from comment #4)
> Comment on attachment 436809 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=436809&action=review
> 
> > Source/WebCore/page/WindowOrWorkerGlobalScope.cpp:51
> > +    auto messageData = SerializedScriptValue::create(globalObject, value, WTFMove(options.transfer), ports, SerializationContext::WindowPostMessage);
> 
> It's very unfortunate that we have to serialize to a string just to make the
> structured clone.
> Have we checked the perf of our implementation with other browsers?
> We should make sure our implementation isn't orders of magnitude slower than
> other browser's.

I guess I can check once other browsers implement it:
- https://bugs.chromium.org/p/chromium/issues/detail?id=1233571
- https://bugzilla.mozilla.org/show_bug.cgi?id=1722576

Both engines said on the PR that they were interested in implementing this.

[Chris Dumez]

Comment on attachment 436809 [details]
Patch

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

>>> Source/WebCore/page/WindowOrWorkerGlobalScope.cpp:43
>>> +        exception = JSC::Exception::create(vm, error);
>> 
>> Does argument-dependent lookup allow us to omit either of these "JSC::" prefixes?
> 
> Will try.

I couldn't omit JSC:: for either of these without getting compilation errors.

[Chris Dumez]

Comment on attachment 436809 [details]
Patch

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

>>> Source/WebCore/page/StructuredSerializeOptions.h:38
>>> +    { }
>> 
>> Do we really need these constructors? Annoying that we can’t program with a struct without doing those.
> 
> I merely renamed our struct to match the new name in the spec but I'll check.

The problem is that we have several IDLs that look like so:
    [CallWith=GlobalObject] undefined postMessage(any message, sequence<object> transfer);
    [CallWith=GlobalObject] undefined postMessage(any message, optional StructuredSerializeOptions options);

Right now, the implementation objects have a single postMessage() function that takes in a `StructuredSerializeOptions&&` and rely on StructuredSerializeOptions's implicit constructor from a `Vector<JSC::Strong<JSC::JSObject>>&&` for the generated bindings to build.
If I get rid of this constructor, then I have to add postMessage() overloads on those implementation objects, which seems like more code than just keeping the constructor.

[Chris Dumez]

Created attachment 436883 [details]
Patch

[EWS]

Committed r281808 (241144@main): <https://commits.webkit.org/241144@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 436883 [details].