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
<rdar://problem/81468374>
Created attachment 436808 [details] Patch
Created attachment 436809 [details] Patch
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.
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.
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.
(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.
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.
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.
Created attachment 436883 [details] Patch
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].