[WebIDL] Support serializing sequences and FrozenArrays of non-interfaces
rdar://problem/35983035
Created attachment 353247 [details] Patch
Comment on attachment 353247 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=353247&action=review > Source/WebCore/bindings/scripts/test/TestSerialization.idl:42 > + attribute sequence<TestSerializationInheritFinal> twelthInterfaceSequenceAttribute; twelth!
(In reply to Wenson Hsieh from comment #3) > Comment on attachment 353247 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=353247&action=review > > > Source/WebCore/bindings/scripts/test/TestSerialization.idl:42 > > + attribute sequence<TestSerializationInheritFinal> twelthInterfaceSequenceAttribute; > > twelth 🤦♂️
Created attachment 353254 [details] Patch
Comment on attachment 353254 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=353254&action=review > Source/WebCore/bindings/scripts/test/JS/JSTestSerialization.cpp:646 > + auto tenthFrozenArrayAttributeValue = jsTestSerializationTenthFrozenArrayAttributeGetter(state, thisObject, throwScope); > + throwScope.assertNoException(); > + result->putDirect(vm, Identifier::fromString(&vm, "tenthFrozenArrayAttribute"), tenthFrozenArrayAttributeValue); > + > + auto eleventhSequenceAttributeValue = jsTestSerializationEleventhSequenceAttributeGetter(state, thisObject, throwScope); > + throwScope.assertNoException(); > + result->putDirect(vm, Identifier::fromString(&vm, "eleventhSequenceAttribute"), eleventhSequenceAttributeValue); Is this actually putting the raw array on the serialized object? Should this need to be copied/cloned somewhere? I'd expect if there is: interface TestSerialization { attribute sequence<DOMString> seq; serializer = { attribute }; } That if I have an instance that looked like: let instance = new TestSerialization; instance.seq = ["one", "two"]; The toJSON's seq should be separate from the instance's seq: assert( instance.toJSON().seq !== instance.seq ); // (1) Such that modifications to the toJSON result should not affect the instance's: let json = instance.toJSON(); assert( instance.seq.length === 2 ); json.seq.push("test"); assert( instance.seq.length === 2 ); // (2) Right now it looks like we are directly putting the value on to the JSON. So I'd think (1) and (2) would fail. -- Honestly the spec doesn't seem too clear to me on this. (Note serializer became toJSON a while back): https://heycam.github.io/webidl/#idl-tojson-operation It's example only uses primitives, where getting the underlying value is a copy. It doesn't show what to do with non-primitives, or I'm missing it.
(In reply to Joseph Pecoraro from comment #6) > Comment on attachment 353254 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=353254&action=review > > > Source/WebCore/bindings/scripts/test/JS/JSTestSerialization.cpp:646 > > + auto tenthFrozenArrayAttributeValue = jsTestSerializationTenthFrozenArrayAttributeGetter(state, thisObject, throwScope); > > + throwScope.assertNoException(); > > + result->putDirect(vm, Identifier::fromString(&vm, "tenthFrozenArrayAttribute"), tenthFrozenArrayAttributeValue); > > + > > + auto eleventhSequenceAttributeValue = jsTestSerializationEleventhSequenceAttributeGetter(state, thisObject, throwScope); > > + throwScope.assertNoException(); > > + result->putDirect(vm, Identifier::fromString(&vm, "eleventhSequenceAttribute"), eleventhSequenceAttributeValue); > > Is this actually putting the raw array on the serialized object? eleventhSequenceAttributeValue (for example) is a newly-created JSValue. jsTestSerializationEleventhSequenceAttributeGetter() calls toJS() on the wrapped object's Vector, which converts the vector's items into JSValues in a MarkedArgumentBuffer then creates a new array with JSC::constructArray(). But now that I think about it, we won't always get a new value if the attribute uses [CachedAttribute], [CustomGetter], or [CustomSetter]. I'll think about how to handle those. In the meantime, I wrote a layout test to check your questions: > I'd expect if there is: > > interface TestSerialization { > attribute sequence<DOMString> seq; > serializer = { attribute }; > } > > That if I have an instance that looked like: > > let instance = new TestSerialization; > instance.seq = ["one", "two"]; > > The toJSON's seq should be separate from the instance's seq: > > assert( instance.toJSON().seq !== instance.seq ); // (1) This assertion is true (the sequences are separate). > > Such that modifications to the toJSON result should not affect the > instance's: > > let json = instance.toJSON(); > assert( instance.seq.length === 2 ); > json.seq.push("test"); > assert( instance.seq.length === 2 ); // (2) These two assertions are also true (the sequences are separate).
> > The toJSON's seq should be separate from the instance's seq: > > > > assert( instance.toJSON().seq !== instance.seq ); // (1) > This assertion is true (the sequences are separate). > > > > > Such that modifications to the toJSON result should not affect the > > instance's: > > > > let json = instance.toJSON(); > > assert( instance.seq.length === 2 ); > > json.seq.push("test"); > > assert( instance.seq.length === 2 ); // (2) > These two assertions are also true (the sequences are separate). Awesome, then this is looking good to me.
Created attachment 361462 [details] Patch
Comment on attachment 361462 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=361462&action=review r=me > Source/WebCore/ChangeLog:14 > + left that as a follow-up task, since I don't see any IDLs that currently need this. If there are specs that support this idea, I think we should file a Bugzilla now that tracks this, and reference it in your FIXME comment. But, if no IDL needs this at all I guess we don't have to track work that isn't even conceptually needed. > Source/WebCore/bindings/scripts/CodeGenerator.pm:957 > + # FIXME: Teach GenerateSerializerDefinition to serialize sequences of interfaces, then remove this line. FIXME(Bug), please...
Comment on attachment 361462 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=361462&action=review >> Source/WebCore/ChangeLog:14 >> + left that as a follow-up task, since I don't see any IDLs that currently need this. > > If there are specs that support this idea, I think we should file a Bugzilla now that tracks this, and reference it in your FIXME comment. > > But, if no IDL needs this at all I guess we don't have to track work that isn't even conceptually needed. Filed bug #194439 and will add a comment below. Thanks for reviewing!
Created attachment 361511 [details] Patch
Comment on attachment 361511 [details] Patch Clearing flags on attachment: 361511 Committed r241198: <https://trac.webkit.org/changeset/241198>
All reviewed patches have been landed. Closing bug.