Bug 156293

Created attachment 275784 [details] Patch

Attachment 275784 [details] did not pass style-queue: ERROR: Source/WebCore/bindings/scripts/test/ObjC/DOMTestSerializerWithMap.mm:30: Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] ERROR: Source/WebCore/bindings/scripts/test/JS/JSTestSerializer.cpp:45: The parameter name "state" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WebCore/bindings/scripts/test/JS/JSTestSerializer.cpp:138: One line control clauses should not use braces. [whitespace/braces] [4] ERROR: Source/WebCore/bindings/scripts/test/JS/JSTestSerializer.cpp:153: One line control clauses should not use braces. [whitespace/braces] [4] ERROR: Source/WebCore/bindings/scripts/test/JS/JSTestSerializer.cpp:187: One line control clauses should not use braces. [whitespace/braces] [4] ERROR: Source/WebCore/bindings/scripts/test/JS/JSTestSerializer.cpp:204: One line control clauses should not use braces. [whitespace/braces] [4] ERROR: Source/WebCore/bindings/scripts/test/JS/JSTestSerializer.cpp:252: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/bindings/scripts/test/JS/JSTestSerializerWithMap.cpp:45: The parameter name "state" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WebCore/bindings/scripts/test/JS/JSTestSerializerWithMap.cpp:138: One line control clauses should not use braces. [whitespace/braces] [4] ERROR: Source/WebCore/bindings/scripts/test/JS/JSTestSerializerWithMap.cpp:153: One line control clauses should not use braces. [whitespace/braces] [4] ERROR: Source/WebCore/bindings/scripts/test/JS/JSTestSerializerWithMap.cpp:187: One line control clauses should not use braces. [whitespace/braces] [4] ERROR: Source/WebCore/bindings/scripts/test/JS/JSTestSerializerWithMap.cpp:204: One line control clauses should not use braces. [whitespace/braces] [4] ERROR: Source/WebCore/bindings/scripts/test/JS/JSTestSerializerWithMap.cpp:252: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/bindings/scripts/test/ObjC/DOMTestSerializer.mm:30: Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] Total errors found: 14 in 23 files If any of these errors are false positives, please file a bug against check-webkit-style.

Some random quick thoughts. It would be good if we could restrict the size of the generated code of ${serializerNativeFunctionName using a helper function, maybe through templates. To remove the "DOMString/unsigned short" limitation, can we call the js wrapper method directly? Do we want to handle exceptions as well? Also, I am not sure how serializer exactly works, but what happens if JS scripts override the properties of an object. Should the serializer use the property set by the JS script? If so, JS built-ins might help implementing this.

Thanks for the comments. (In reply to comment #3) > Some random quick thoughts. > > It would be good if we could restrict the size of the generated code of > ${serializerNativeFunctionName using a helper function, maybe through > templates. > Ok, I think the part that could be bigger is the loop converting the values to serializable form, maybe I can create a helper to get the string value. I'll check it. > To remove the "DOMString/unsigned short" limitation, can we call the js > wrapper method directly? Do we want to handle exceptions as well? > Hrm, maybe I missed that option that would reduce the code, I'll try to do it, thanks. Also I think maybe we can get rid of the String transformation in case of the numbers. > Also, I am not sure how serializer exactly works, but what happens if JS > scripts override the properties of an object. Should the serializer use the > property set by the JS script? If so, JS built-ins might help implementing > this. AFAIK it should use the new values, the only limitation is that they are serializable types, which have rules to serialize them, there is a list in the webidl spec. Adam probably knows better if there is any limitation in case of the WebRTC objects using this but that would be something each specific object should control.

I forgot to mention one point. If you keep the limitation to number and string, the binding generator should not silently skip the support of other values, it should just fail in some way.

Comment on attachment 275784 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=275784&action=review > Source/WebCore/ChangeLog:25 > + Tests: bindings/scripts/test/TestSerializer.idl > + bindings/scripts/test/TestSerializerWithMap.idl It’s good to have binding script tests, but those are not very effective ways to test what the generated code does. We need tests of the actual generated code in the RTCIceCandidate and RTCSessionDescription classes. > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:1932 > + push(@implContent, "EncodedJSValue JSC_HOST_CALL ${serializerNativeFunctionName}(ExecState* state);\n\n"); Should not have an argument name here in the function declaration. Just ExecState* without a name. > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3358 > + foreach my $attribute (@{$interface->attributes}) { > + my $name = $attribute->signature->name; > + if (grep $_ eq $name, @{$interface->serializable->attributes}) { > + my $type = $attribute->signature->type; > + if ($type eq "DOMString") { > + push(@implContent, " result->putDirect(state->vm(), Identifier::fromString(state, \"${name}\"), jsStringWithCache(state, impl.${name}()));\n"); > + } elsif ($type eq "unsigned short") { > + push(@implContent, " result->putDirect(state->vm(), Identifier::fromString(state, \"${name}\"), jsNumber(impl.${name}().value()).toString(state));\n"); > + } > + > + } > + } state->vm() is an operation that has some associated cost; can do it once instead of over and over again? Calling jsNumber and then toString is an inefficient way to convert a number to a string in JavaScriptCore that unnecessarily converts the number to a JSValue and then back. Should do it a different way. It’s more efficient to call Identifier::fromString with the VM* than with the ExecState*. I don’t understand why the code here calls the function value() in the unsigned short case. What class’s value() function is it calling? Why does this handle only the types DOMString and unsigned short? Worse, there is no error if we see some other type or a missing attribute; we just silently skip putting the string into the new object.

Thanks very much for the review. (In reply to comment #6) > Comment on attachment 275784 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=275784&action=review > > > Source/WebCore/ChangeLog:25 > > + Tests: bindings/scripts/test/TestSerializer.idl > > + bindings/scripts/test/TestSerializerWithMap.idl > > It’s good to have binding script tests, but those are not very effective > ways to test what the generated code does. We need tests of the actual > generated code in the RTCIceCandidate and RTCSessionDescription classes. > Ok, I'll add tests for that. > > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:1932 > > + push(@implContent, "EncodedJSValue JSC_HOST_CALL ${serializerNativeFunctionName}(ExecState* state);\n\n"); > > Should not have an argument name here in the function declaration. Just > ExecState* without a name. > Yep. > > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3358 > > + foreach my $attribute (@{$interface->attributes}) { > > + my $name = $attribute->signature->name; > > + if (grep $_ eq $name, @{$interface->serializable->attributes}) { > > + my $type = $attribute->signature->type; > > + if ($type eq "DOMString") { > > + push(@implContent, " result->putDirect(state->vm(), Identifier::fromString(state, \"${name}\"), jsStringWithCache(state, impl.${name}()));\n"); > > + } elsif ($type eq "unsigned short") { > > + push(@implContent, " result->putDirect(state->vm(), Identifier::fromString(state, \"${name}\"), jsNumber(impl.${name}().value()).toString(state));\n"); > > + } > > + > > + } > > + } > > state->vm() is an operation that has some associated cost; can do it once > instead of over and over again? > I'll do it. > Calling jsNumber and then toString is an inefficient way to convert a number > to a string in JavaScriptCore that unnecessarily converts the number to a > JSValue and then back. Should do it a different way. > Right, actually I can basically use the jsNumber, the spec says basically that every number should be a double. > It’s more efficient to call Identifier::fromString with the VM* than with > the ExecState*. > > I don’t understand why the code here calls the function value() in the > unsigned short case. What class’s value() function is it calling? > It is the Optional class, now I wonder if the null case it is correctly checked this way, I'll check it. > Why does this handle only the types DOMString and unsigned short? Worse, > there is no error if we see some other type or a missing attribute; we just > silently skip putting the string into the new object. The only reason is they are the ones we need for our WebRTC classes in this point, I'm planning to support at least all the basic types and add error handling for the other situations. We can add all the more complex types support but I had doubts adding that code that could be complex makes sense until it is really used. Thanks again for the review.

Created attachment 276314 [details] Patch

Attachment 276314 [details] did not pass style-queue: ERROR: Source/WebCore/bindings/scripts/test/ObjC/DOMTestSerializerWithMap.mm:30: Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] ERROR: Source/WebCore/bindings/scripts/test/JS/JSTestSerializer.cpp:45: The parameter name "state" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WebCore/bindings/scripts/test/JS/JSTestSerializer.cpp:138: One line control clauses should not use braces. [whitespace/braces] [4] ERROR: Source/WebCore/bindings/scripts/test/JS/JSTestSerializer.cpp:153: One line control clauses should not use braces. [whitespace/braces] [4] ERROR: Source/WebCore/bindings/scripts/test/JS/JSTestSerializer.cpp:187: One line control clauses should not use braces. [whitespace/braces] [4] ERROR: Source/WebCore/bindings/scripts/test/JS/JSTestSerializer.cpp:204: One line control clauses should not use braces. [whitespace/braces] [4] ERROR: Source/WebCore/bindings/scripts/test/JS/JSTestSerializer.cpp:252: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/bindings/scripts/test/JS/JSTestSerializerWithMap.cpp:45: The parameter name "state" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/WebCore/bindings/scripts/test/JS/JSTestSerializerWithMap.cpp:138: One line control clauses should not use braces. [whitespace/braces] [4] ERROR: Source/WebCore/bindings/scripts/test/JS/JSTestSerializerWithMap.cpp:153: One line control clauses should not use braces. [whitespace/braces] [4] ERROR: Source/WebCore/bindings/scripts/test/JS/JSTestSerializerWithMap.cpp:187: One line control clauses should not use braces. [whitespace/braces] [4] ERROR: Source/WebCore/bindings/scripts/test/JS/JSTestSerializerWithMap.cpp:204: One line control clauses should not use braces. [whitespace/braces] [4] ERROR: Source/WebCore/bindings/scripts/test/JS/JSTestSerializerWithMap.cpp:252: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/bindings/scripts/test/ObjC/DOMTestSerializer.mm:30: Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] Total errors found: 14 in 28 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 276314 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=276314&action=review I don’t see any tests covering the generated toJSON function, checking that it is enumerable, checking that it appears on the prototype, checking that it can be overridden, etc. > Source/WebCore/ChangeLog:43 > + (parseSerializer): > + (parseSerializerRest): > + (parseSerializationPattern): > + (parseSerializationAttributes): > + (applyMemberList): > + (parseSerializationPatternMap): Deleted. > + (parseSerializationPatternList): Deleted. This list of functions looks quite peculiar without actual comments about what changed in each. And the "Deleted" comments look are even more peculiar since these are just renamed functions, not deleted ones. I know the script generated this, but it’s best to look at these and edit them so that they make sense. It’s easy to think of this machine generated change log as “sacred”, but the intention is that you edit it into something other people will understand. > Source/WebCore/ChangeLog:57 > + (WebKit::kit): > + (WebKit::core): > + (WebKit::wrapTestSerializer): > + (webkit_dom_test_serializer_finalize): > + (webkit_dom_test_serializer_set_property): > + (webkit_dom_test_serializer_get_property): > + (webkit_dom_test_serializer_constructor): > + (webkit_dom_test_serializer_class_init): > + (webkit_dom_test_serializer_init): > + (webkit_dom_test_serializer_get_str_attribute): > + (webkit_dom_test_serializer_set_str_attribute): > + (webkit_dom_test_serializer_get_int_attribute): > + (webkit_dom_test_serializer_set_int_attribute): These lists of function names in expected test result files are not at all useful. Please remove them from the change log. Could also consider teaching our ChangeLog scripts to know not to generate these in the future if you don’t like doing this by hand each time. > Source/WebCore/ChangeLog:73 > + (WebKit::kit): > + (WebKit::core): > + (WebKit::wrapTestSerializerWithMap): > + (webkit_dom_test_serializer_with_map_finalize): > + (webkit_dom_test_serializer_with_map_set_property): > + (webkit_dom_test_serializer_with_map_get_property): > + (webkit_dom_test_serializer_with_map_constructor): > + (webkit_dom_test_serializer_with_map_class_init): > + (webkit_dom_test_serializer_with_map_init): > + (webkit_dom_test_serializer_with_map_get_str_attribute): > + (webkit_dom_test_serializer_with_map_set_str_attribute): > + (webkit_dom_test_serializer_with_map_get_int_attribute): > + (webkit_dom_test_serializer_with_map_set_int_attribute): Ditto. > Source/WebCore/ChangeLog:99 > + (WebCore::JSTestSerializerPrototype::create): > + (WebCore::JSTestSerializerPrototype::createStructure): > + (WebCore::JSTestSerializerPrototype::JSTestSerializerPrototype): > + (WebCore::JSTestSerializerConstructor::prototypeForStructure): > + (WebCore::JSTestSerializerConstructor::initializeProperties): > + (WebCore::JSTestSerializerPrototype::finishCreation): > + (WebCore::JSTestSerializer::JSTestSerializer): > + (WebCore::JSTestSerializer::createPrototype): > + (WebCore::JSTestSerializer::prototype): > + (WebCore::JSTestSerializer::destroy): > + (WebCore::jsTestSerializerStrAttribute): > + (WebCore::jsTestSerializerIntAttribute): > + (WebCore::jsTestSerializerConstructor): > + (WebCore::setJSTestSerializerConstructor): > + (WebCore::setJSTestSerializerStrAttribute): > + (WebCore::setJSTestSerializerIntAttribute): > + (WebCore::JSTestSerializer::getConstructor): > + (WebCore::jsTestSerializerPrototypeFunctionToJSON): > + (WebCore::JSTestSerializerOwner::isReachableFromOpaqueRoots): > + (WebCore::JSTestSerializerOwner::finalize): > + (WebCore::toJSNewlyCreated): > + (WebCore::toJS): > + (WebCore::JSTestSerializer::toWrapped): Ditto. > Source/WebCore/ChangeLog:106 > + (WebCore::JSTestSerializer::create): > + (WebCore::JSTestSerializer::createStructure): > + (WebCore::JSTestSerializer::finishCreation): > + (WebCore::wrapperOwner): > + (WebCore::wrapperKey): > + (WebCore::toJS): Ditto. > Source/WebCore/ChangeLog:130 > + (WebCore::JSTestSerializerWithMapPrototype::create): > + (WebCore::JSTestSerializerWithMapPrototype::createStructure): > + (WebCore::JSTestSerializerWithMapPrototype::JSTestSerializerWithMapPrototype): > + (WebCore::JSTestSerializerWithMapConstructor::prototypeForStructure): > + (WebCore::JSTestSerializerWithMapConstructor::initializeProperties): > + (WebCore::JSTestSerializerWithMapPrototype::finishCreation): > + (WebCore::JSTestSerializerWithMap::JSTestSerializerWithMap): > + (WebCore::JSTestSerializerWithMap::createPrototype): > + (WebCore::JSTestSerializerWithMap::prototype): > + (WebCore::JSTestSerializerWithMap::destroy): > + (WebCore::jsTestSerializerWithMapStrAttribute): > + (WebCore::jsTestSerializerWithMapIntAttribute): > + (WebCore::jsTestSerializerWithMapConstructor): > + (WebCore::setJSTestSerializerWithMapConstructor): > + (WebCore::setJSTestSerializerWithMapStrAttribute): > + (WebCore::setJSTestSerializerWithMapIntAttribute): > + (WebCore::JSTestSerializerWithMap::getConstructor): > + (WebCore::jsTestSerializerWithMapPrototypeFunctionToJSON): > + (WebCore::JSTestSerializerWithMapOwner::isReachableFromOpaqueRoots): > + (WebCore::JSTestSerializerWithMapOwner::finalize): > + (WebCore::toJSNewlyCreated): > + (WebCore::toJS): > + (WebCore::JSTestSerializerWithMap::toWrapped): Ditto. > Source/WebCore/ChangeLog:137 > + (WebCore::JSTestSerializerWithMap::create): > + (WebCore::JSTestSerializerWithMap::createStructure): > + (WebCore::JSTestSerializerWithMap::finishCreation): > + (WebCore::wrapperOwner): > + (WebCore::wrapperKey): > + (WebCore::toJS): Ditto. > Source/WebCore/ChangeLog:146 > + (-[DOMTestSerializer dealloc]): > + (-[DOMTestSerializer strAttribute]): > + (-[DOMTestSerializer setStrAttribute:]): > + (-[DOMTestSerializer intAttribute]): > + (-[DOMTestSerializer setIntAttribute:]): > + (core): > + (kit): Ditto. > Source/WebCore/ChangeLog:156 > + (-[DOMTestSerializerWithMap dealloc]): > + (-[DOMTestSerializerWithMap strAttribute]): > + (-[DOMTestSerializerWithMap setStrAttribute:]): > + (-[DOMTestSerializerWithMap intAttribute]): > + (-[DOMTestSerializerWithMap setIntAttribute:]): > + (core): > + (kit): Ditto. > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:1812 > + my $serializerNativeFunctionName = $codeGenerator->WK_lcfirst($className) . "Prototype" . "Function" . $codeGenerator->WK_ucfirst($serializerFunctionName); Seems a bit peculiar to write "Prototype" . "Function" instead of "PrototypeFunction". > Source/WebCore/bindings/scripts/test/TestSerializer.idl:30 > +// This IDL file is for testing the bindings code generator for the > +// serializer special operation. Do we really need a separate IDL file? Couldn’t we instead add this to an existing test IDL file? > Source/WebCore/bindings/scripts/test/TestSerializerWithMap.idl:30 > +// This IDL file is for testing the bindings code generator for the > +// serializer special operation. Do we really need a separate IDL file? Couldn’t we instead add this to an existing test IDL file? > LayoutTests/fast/mediastream/RTCIceCandidate.html:24 > + shouldBeEqualToString('JSON.stringify(candidate.toJSON())', '{\"candidate\":\"foo\",\"sdpMid\":\"bar\",\"sdpMLineIndex\":6}'); > + candidate.newAttribute = "new value"; > + shouldBeEqualToString('JSON.stringify(candidate.toJSON())', '{\"candidate\":\"foo\",\"sdpMid\":\"bar\",\"sdpMLineIndex\":6}'); > + candidate.sdpMLineIndex = "not a number"; > + shouldBeEqualToString('JSON.stringify(candidate.toJSON())', '{\"candidate\":\"foo\",\"sdpMid\":\"bar\",\"sdpMLineIndex\":0}'); > + candidate.sdpMid = 7; > + shouldBeEqualToString('JSON.stringify(candidate.toJSON())', '{\"candidate\":\"foo\",\"sdpMid\":\"7\",\"sdpMLineIndex\":0}'); > + candidate.sdpMid = null; > + shouldBeEqualToString('JSON.stringify(candidate.toJSON())', '{\"candidate\":\"foo\",\"sdpMid\":\"null\",\"sdpMLineIndex\":0}'); Is there some way to avoid all the \" in these test strings? This seems like a problem in shouldBeEqualToString. Makes both the test itself and the test output hard to read.

Thanks again for the review. (In reply to comment #10) > Comment on attachment 276314 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=276314&action=review > > I don’t see any tests covering the generated toJSON function, checking that > it is enumerable, checking that it appears on the prototype, checking that > it can be overridden, etc. > I'll add them, also now that you name it the spec says nothing about them but it does not seem interesting for the user to modify the function so I'm going to avoid the user can delete or modify the function. Do you know if there is any policy or preference about this? > > Source/WebCore/ChangeLog:43 > > + (parseSerializer): > > + (parseSerializerRest): > > + (parseSerializationPattern): > > + (parseSerializationAttributes): > > + (applyMemberList): > > + (parseSerializationPatternMap): Deleted. > > + (parseSerializationPatternList): Deleted. > > This list of functions looks quite peculiar without actual comments about > what changed in each. And the "Deleted" comments look are even more peculiar > since these are just renamed functions, not deleted ones. I know the script > generated this, but it’s best to look at these and edit them so that they > make sense. It’s easy to think of this machine generated change log as > “sacred”, but the intention is that you edit it into something other people > will understand. > Yep, I'll add comments to that and remove the other functions. > > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:1812 > > + my $serializerNativeFunctionName = $codeGenerator->WK_lcfirst($className) . "Prototype" . "Function" . $codeGenerator->WK_ucfirst($serializerFunctionName); > > Seems a bit peculiar to write "Prototype" . "Function" instead of > "PrototypeFunction". > Right. > > Source/WebCore/bindings/scripts/test/TestSerializer.idl:30 > > +// This IDL file is for testing the bindings code generator for the > > +// serializer special operation. > > Do we really need a separate IDL file? Couldn’t we instead add this to an > existing test IDL file? > I can try to use an existing IDL tests. > > LayoutTests/fast/mediastream/RTCIceCandidate.html:24 > > + shouldBeEqualToString('JSON.stringify(candidate.toJSON())', '{\"candidate\":\"foo\",\"sdpMid\":\"bar\",\"sdpMLineIndex\":6}'); > > + candidate.newAttribute = "new value"; > > + shouldBeEqualToString('JSON.stringify(candidate.toJSON())', '{\"candidate\":\"foo\",\"sdpMid\":\"bar\",\"sdpMLineIndex\":6}'); > > + candidate.sdpMLineIndex = "not a number"; > > + shouldBeEqualToString('JSON.stringify(candidate.toJSON())', '{\"candidate\":\"foo\",\"sdpMid\":\"bar\",\"sdpMLineIndex\":0}'); > > + candidate.sdpMid = 7; > > + shouldBeEqualToString('JSON.stringify(candidate.toJSON())', '{\"candidate\":\"foo\",\"sdpMid\":\"7\",\"sdpMLineIndex\":0}'); > > + candidate.sdpMid = null; > > + shouldBeEqualToString('JSON.stringify(candidate.toJSON())', '{\"candidate\":\"foo\",\"sdpMid\":\"null\",\"sdpMLineIndex\":0}'); > > Is there some way to avoid all the \" in these test strings? This seems like > a problem in shouldBeEqualToString. Makes both the test itself and the test > output hard to read. Not sure if wen can avoid escaping the quotation marks in this case, I'll check if I can improve that.

Created attachment 276390 [details] Patch

(In reply to comment #11) > the spec says nothing about them Here’s what I found: <https://heycam.github.io/webidl/#es-serializer>. This going into detail about the property, its attributes, the location of the property, the the behavior of the function. We should test all that.

Comment on attachment 276390 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=276390&action=review > LayoutTests/fast/mediastream/RTCIceCandidate.html:19 > + shouldBeFalse('Object.getOwnPropertyDescriptor(RTCIceCandidate.prototype, "toJSON").writable'); > + shouldBeFalse('Object.getOwnPropertyDescriptor(RTCIceCandidate.prototype, "toJSON").configurable'); Specification says both of these should be true.

(In reply to comment #14) > Comment on attachment 276390 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=276390&action=review > > > LayoutTests/fast/mediastream/RTCIceCandidate.html:19 > > + shouldBeFalse('Object.getOwnPropertyDescriptor(RTCIceCandidate.prototype, "toJSON").writable'); > > + shouldBeFalse('Object.getOwnPropertyDescriptor(RTCIceCandidate.prototype, "toJSON").configurable'); > > Specification says both of these should be true. Thanks very much for pointing it out, I guess this means our code using the result of the function in webrtc classes has to deal with these situations and test them very carefully. I'll change the patch accordingly and send it again.

Given the restrictions of the current approach, it might be good to think of leaving the meat of the implementation to the DOM class itself. That would remove issues like non-default getters, inheritance, defaut serializer... The binding code could be made responsible to create a JSObject and pass it to the DOM method as a parameter. If it is preferred to keep some separation between JSC and WebCore, something like a Writable WebCore::Dictionary could be introduced.

(In reply to comment #16) > Given the restrictions of the current approach, it might be good to think of > leaving the meat of the implementation to the DOM class itself. That would > remove issues like non-default getters, inheritance, defaut serializer... > > The binding code could be made responsible to create a JSObject and pass it > to the DOM method as a parameter. > If it is preferred to keep some separation between JSC and WebCore, > something like a Writable WebCore::Dictionary could be introduced. I have not experience enough in this code to know if this is a better approach, if some reviewer of this part can comment on it that would be helpful. I'll try to check the limitations you are pointing out anyway.

Here are some more details about what I was proposing. The following cases might break the generated code: - conditional attributes (Conditional=...) - getter raising exception (GetterRaisesException) - attribute whose DOM getter is named differenty (ImplementedAs) - custom and/or cacheable attributes (Custom, Cacheable) - getter needing dedicated parameters (CallWith=...) One possibility is to make the binding generator die whenever a serialized attribute has IDL "extended attributes". But we could also just have serializer in the WebIDL as a shorthand for toJSON. Similarly to promise-based methods, the binding generator would generate a toJSON method that would take an additional "object" parameter. EncodedJSValue jsXXToJSON(ExecState* state) { JSValue thisValue = state->thisValue(); auto castedThis = jsDynamicCast<JSTestObj*>(thisValue); JSObject* object = constructEmptyObject(state); ... castedThis->wrapped().toJSON(*object); // with additional parameters if needed. return JSValue::encode(object); } The DOM class would take a JSObject& or a WritableDictionary& if we prefer to keep separation between WebCore and JSC, something like: void toJSON(WritableDictionary& dictionary) { dictionary.add("key", value); ... } WritableDictionary would be made responsible to convert DOM typed values as JSC::JSValue. I am not sure what is the exact error handling, can add/putDirect fail? The support of serializing inherited attributes would be done at the DOM class, by calling BaseClass::toJSON.

Thanks very much for the detailed explanation, I understand what you are proposing. I'm open to implement any solution you think it fits better in this case, I just had doubts about the interest of supporting this IDL completely considering it has never been used until now, and adding complexity or code that will not be used does not seem very interesting to me. I mean, the javascript API method must be complete robust and follow spec, but I'm not sure about the interest in supporting the whole WebIDL spec in the script, Darin what's your opinion about this? (In reply to comment #18) > Here are some more details about what I was proposing. > > The following cases might break the generated code: > - conditional attributes (Conditional=...) > - getter raising exception (GetterRaisesException) > - attribute whose DOM getter is named differenty (ImplementedAs) > - custom and/or cacheable attributes (Custom, Cacheable) > - getter needing dedicated parameters (CallWith=...) > > One possibility is to make the binding generator die whenever a serialized > attribute has IDL "extended attributes". > I understand, I agree it seems best option to make the generator die in this case for those idls.

The simplest approach might be to let the binding generator generate code similar to "[Custom] void toJSON()" when the serializer keyword is found in the IDL. Developpers would then have to implement JSXX::toJSON(ExecState*). If serializer becomes popular, some refactoring can be made to improve/automate the support.

(In reply to comment #20) > The simplest approach might be to let the binding generator generate code > similar to "[Custom] void toJSON()" when the serializer keyword is found in > the IDL. Developpers would then have to implement JSXX::toJSON(ExecState*). > > If serializer becomes popular, some refactoring can be made to > improve/automate the support. That is a good point, after reviewing the spec I've read the maps and list support is not currently correct, not in the IDL but in the toJSON API, we have to create a map or list with the serialized values converted to jsvalue. Not adding the support to the IDL is one thing but I do not think we should have the API returning wrong values in those cases even if they are not currently used. So probably a helper like the one you proposed could be the best option after all to do the serialization considering it has very specific rules. I'll add that support and check the best approach, thanks again for the comments.

A few thoughts: 1) More bindings code in the DOM classes themselves is the *wrong* direction. In fact, I think we should be eliminating uses of Dictionary, JSDictionary, and possibly ArrayValue in the C++ DOM. We should have the bindings do more of the work for these, and also for enums. Having code in the DOM working directly with these classes seems to be an anti-pattern to me. Long ago we had a blurring of the boundary between the bindings and the DOM, and with these new IDL features like enums and dictionaries, I think we have not done as thorough a job of that as we once did. In the V8 engine era there was more focus on independence from details of the JavaScript engine, which I think is a non-goal for the project now and was before V8, and not enough focus on independence of the DOM itself from the bindings when possible (some cases where it is not). 2) We want to keep hand-written code to a minimum. Any time we have to fall back on [Custom] or [CustomGetter] it has a long term cost. In fact, a general project should be to eliminate every custom binding, one at a time, by enhancing the bindings generator and the classes and functions that it uses. 3) The code generator should generate as little of the code as possible. Instead our aim should be to use the best techniques in C++ to overload for multiple types and use templates and the like. Code generated that checks types and generates different code for each type should be minimized. One way of doing this is to write custom binding code first, then streamline that code as much as possible, and only then write the code generator bits. One of the best techniques is inline functions that ignore parameters as needed, assuming the parameters are values that are already at hand and don’t need to be computed in an expensive way. 4) New features in the code generator should be factored so they work with existing features as much as possible, rather than being completely separate. This can sometimes mitigate some of the problems Youenn is taking about. 5) Geoff Garen told me that we should be looking eventually to replace today’s ExecState (actually a call frame pointer) with the global object pointer as the thing we pass around in the JavaScript runtime. This could increase efficiency and also simplify code a bit. It’s not critical for the code generator to fail when it generates the wrong code, though. While it would be nice, the code generator has never been fully general purpose, just good enough to get the bindings right for the classes we actually have. This patch seems a bit too specific; code for exactly two specific types that can be serialized is a bad sign. Not sure if this answers all the outstanding questions you have about the direction we’d like to go. The people most interested in the future of bindings right now, at least at Apple, seem to be Chris Dumez, Sam Weinig, Geoff Garen, and me.

(In reply to comment #22) > A few thoughts: > > [...] > It’s not critical for the code generator to fail when it generates the wrong > code, though. While it would be nice, the code generator has never been > fully general purpose, just good enough to get the bindings right for the > classes we actually have. > > This patch seems a bit too specific; code for exactly two specific types > that can be serialized is a bad sign. > > Not sure if this answers all the outstanding questions you have about the > direction we’d like to go. > I does answer most of the questions, thanks very much for the detailed description of the current situation in this part of the code. The last version of the patch upload already uses NativeToJSValue function so it basically supports every conversion that function currently supports, so there is no limitation to the 2 types we need anymore. But that function gets the JSValue, which is what we want usually but not sure if it does that for the most complex situations, such as the serialization of maps and lists, it needs to traverse the map or list and convert to JS value the serialized value. I think this fits with new features using existing ones and hopefully conversions of types is the same in every spec so that we can use the same function. Considering the current situation I feel best option to make us feel safe is to create a test with an idl that uses every type and serializes it, that would explain the situation and even every decision we want to make about how we implement the spec. Thanks again for the helpful comments.

Created attachment 277482 [details] Proposed patch New iteration of the patch addressing the issues found in the last ones, namely: - Now the implementation supports all the types supported by NativeToJSValue in the perl script, which means we are converting to JSValue like every other API in webkit. This way we use existing features in the generator, and code already there to control the transformation. - Added tests to check the location of the API is just in the class and not inherited, this API is just inherited for some kind of global objects definition that apparently we do not support in the WebIDL. - Checked the map and list implementation, added tests to check the properties of the generated elements in the serialized map. - Added an informative message and kill the script when someone tries to create a serializer that returns a list, we are not currently supporting this. Only point I think I've missed regarding testing is creating a fake idl but I could not find the test harness to create that, so not sure if that is just interesting. Thanks for the comments.

Comment on attachment 277482 [details] Proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=277482&action=review > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3330 > + my $functionString = "impl.${name}()"; Maybe this is good enough as a first step. If we go that road, see below for an alternative. > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3332 > + $signature->extendedAttributes({}); Why not reusing the extended attributes of the attribute? > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3333 > + $signature->isNullable(1); Should not the nullability be the same as the attribute? > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3335 > + push(@implContent, " result->putDirect(vm, Identifier::fromString(&vm, \"${name}\"), " . NativeToJSValue($signature, 1, $interfaceName, $functionString, $thisObject) . ");\n"); I still wonder whether we could not reuse code already generated for attribute getters. For instance the code generated is something like: EncodedJSValue jsFetchResponseType(ExecState* state, EncodedJSValue thisValue, PropertyName) { UNUSED_PARAM(state); UNUSED_PARAM(thisValue); JSValue decodedThisValue = JSValue::decode(thisValue); auto* castedThis = jsDynamicCast<JSFetchResponse*>(decodedThisValue); if (UNLIKELY(!castedThis)) { return throwGetterTypeError(*state, "FetchResponse", "type"); } auto& impl = castedThis->wrapped(); JSValue result = jsStringWithCache(state, impl.type()); return JSValue::encode(result); } If we could split it (like done for custom attributes) in something like: EncodedJSValue jsFetchResponseType(ExecState* state, EncodedJSValue thisValue, PropertyName) { UNUSED_PARAM(state); UNUSED_PARAM(thisValue); JSValue decodedThisValue = JSValue::decode(thisValue); auto* castedThis = jsDynamicCast<JSFetchResponse*>(decodedThisValue); if (UNLIKELY(!castedThis)) { return throwGetterTypeError(*state, "FetchResponse", "type"); } return JSValue::encode(jsFetchResponseTypeGetter(*state, *castedThis)); } JSValue jsFetchResponseTypeGetter(ExecState&, JSFetchResponse& castedThis) { auto& impl = castedThis->wrapped(); JSValue result = jsStringWithCache(state, impl.type()); return result; } Then, the serializing code could directly use jsFetchResponseTypeGetter instead of "impl.type()". This would solve some of the limitations I pointed out previously. I must say I do not know how straightforward is the splitting. So maybe "impl.type()" is good enough initially.

Thanks very much for the review. (In reply to comment #25) > Comment on attachment 277482 [details] > Proposed patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=277482&action=review > > > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3330 > > + my $functionString = "impl.${name}()"; > > Maybe this is good enough as a first step. > If we go that road, see below for an alternative. > > > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3332 > > + $signature->extendedAttributes({}); > > Why not reusing the extended attributes of the attribute? > Yep, I agree, I'll replace it. > > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3333 > > + $signature->isNullable(1); > > Should not the nullability be the same as the attribute? > I think you are right, I'll change it. > > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3335 > > + push(@implContent, " result->putDirect(vm, Identifier::fromString(&vm, \"${name}\"), " . NativeToJSValue($signature, 1, $interfaceName, $functionString, $thisObject) . ");\n"); > > I still wonder whether we could not reuse code already generated for > attribute getters. > For instance the code generated is something like: > > EncodedJSValue jsFetchResponseType(ExecState* state, EncodedJSValue > thisValue, PropertyName) > { > UNUSED_PARAM(state); > UNUSED_PARAM(thisValue); > JSValue decodedThisValue = JSValue::decode(thisValue); > auto* castedThis = jsDynamicCast<JSFetchResponse*>(decodedThisValue); > if (UNLIKELY(!castedThis)) { > return throwGetterTypeError(*state, "FetchResponse", "type"); > } > auto& impl = castedThis->wrapped(); > JSValue result = jsStringWithCache(state, impl.type()); > return JSValue::encode(result); > } > > If we could split it (like done for custom attributes) in something like: > > EncodedJSValue jsFetchResponseType(ExecState* state, EncodedJSValue > thisValue, PropertyName) > { > UNUSED_PARAM(state); > UNUSED_PARAM(thisValue); > JSValue decodedThisValue = JSValue::decode(thisValue); > auto* castedThis = jsDynamicCast<JSFetchResponse*>(decodedThisValue); > if (UNLIKELY(!castedThis)) { > return throwGetterTypeError(*state, "FetchResponse", "type"); > } > return JSValue::encode(jsFetchResponseTypeGetter(*state, *castedThis)); > } > > JSValue jsFetchResponseTypeGetter(ExecState&, JSFetchResponse& castedThis) > { > auto& impl = castedThis->wrapped(); > JSValue result = jsStringWithCache(state, impl.type()); > return result; > } > > Then, the serializing code could directly use jsFetchResponseTypeGetter > instead of "impl.type()". > This would solve some of the limitations I pointed out previously. > > I must say I do not know how straightforward is the splitting. > So maybe "impl.type()" is good enough initially. Really good point, I'll try that to check if it is possible, I agree it is really a good idea to try to reuse as much code as possible from the current implementation. Thanks again.

Created attachment 279653 [details] Patch

(In reply to comment #27) > Created attachment 279653 [details] > Patch I've added the proposal from youenn, the getter method is splitted in two and we are reusing the part extracting the value in the serializer. This made the patch a little bit more complex for the GenerateImplementation function in the script. Main issue was to check what parameters we needed for the getter, the implementation I did it is generating the function in an array and checking the array to create the function prototype, that is added later to the main contents array. Thanks for your time.

Created attachment 279654 [details] Patch

(In reply to comment #29) > Created attachment 279654 [details] > Patch The generated code looks good to me. Below some additional improvements that can help simplify the generation. It might indeeed be good to separate the handling of static attributes from regular attributes. For regular attributes, the generated helper function could always have the same prototype, taking a JSC::ExecState and a JSXX*. UNUSED_PARAM(state) could be moved in the helper function to handle the case where it is not used. That may simplify the generation script. That may also allow templatize the code found in jsXX getter. To limit the changes in the script, you might also want to consider the possibility to declare the helper function, then generate the jsXX getter, then generate the helper function, something like: static JSC::JSValue jsDOMClassAttributeGetter(JSC::ExecState*, JSDOMClass*); JSC::EncodedJSValue jsDOMClassAttribute(JSC::ExecState* state, JSC::EncodedJSValue thisValue, PropertyName, JSC::JSObject*) { // Generic code that could be put in a template at some point? (in a separate patch I guess) JSValue decodedThisValue = JSValue::decode(thisValue); auto* castedThis = jsDynamicCast<JSDOMClass>decodedThisValue); if (UNLIKELY(!castedThis)) return throwGetterTypeError(*state, "DOMClass", "attribute"); return JSValue::encode(jsDOMClassAttributeGetter(state, castedThis)); } static inline JSC::JSValue jsDOMClassAttributeGetter(JSC::ExecState* state, JSDOMClass* castedThis) { UNUSED_PARAM(state); ... }

Thanks again for the review. (In reply to comment #30) > (In reply to comment #29) > > Created attachment 279654 [details] > > Patch > > The generated code looks good to me. > Below some additional improvements that can help simplify the generation. > > It might indeeed be good to separate the handling of static attributes from > regular attributes. > > For regular attributes, the generated helper function could always have the > same prototype, taking a JSC::ExecState and a JSXX*. > UNUSED_PARAM(state) could be moved in the helper function to handle the case > where it is not used. > That may simplify the generation script. > That may also allow templatize the code found in jsXX getter. > Good point, yeah, this could help the messiest part in the generator, I'll try it. I have concerns because last changes becuase this is a small part of the IDL and I wouldn't like to create more complexity in the generator. > To limit the changes in the script, you might also want to consider the > possibility to declare the helper function, then generate the jsXX getter, > then generate the helper function, something like: > > static JSC::JSValue jsDOMClassAttributeGetter(JSC::ExecState*, JSDOMClass*); > JSC::EncodedJSValue jsDOMClassAttribute(JSC::ExecState* state, > JSC::EncodedJSValue thisValue, PropertyName, JSC::JSObject*) > { > // Generic code that could be put in a template at some point? (in a > separate patch I guess) > JSValue decodedThisValue = JSValue::decode(thisValue); > auto* castedThis = jsDynamicCast<JSDOMClass>decodedThisValue); > if (UNLIKELY(!castedThis)) > return throwGetterTypeError(*state, "DOMClass", "attribute"); > return JSValue::encode(jsDOMClassAttributeGetter(state, castedThis)); > } > > static inline JSC::JSValue jsDOMClassAttributeGetter(JSC::ExecState* state, > JSDOMClass* castedThis) > { > UNUSED_PARAM(state); > ... > } Ok, if we can avoid changing the prototype it could make the trick, I'll test it after modifying the Thanks again.

Created attachment 279753 [details] Patch

Created attachment 279760 [details] Patch

Comment on attachment 279760 [details] Patch The patch keeps improving :) I am wondering what others think about the handling of static attributes though. View in context: https://bugs.webkit.org/attachment.cgi?id=279760&action=review > Source/WebCore/bindings/scripts/test/JS/JSTestGlobalObject.cpp:198 > +JSValue jsTestGlobalObjectEnabledAtRuntimeAttributeGetter(ExecState* state, JSTestGlobalObject* castedThis) castedThis name may be changed to thisObject. That can be done in a follow-up patch. > Source/WebCore/bindings/scripts/test/JS/JSTestInterface.cpp:412 > +JSValue jsTestInterfaceConstructorImplementsStaticReadOnlyAttrGetter(ExecState* state, JSTestInterface* castedThis); I would expect static attribute getter prototype to take only the state parameter, not castedThis. This might simplify the generation so maybe this is acceptable as a first step. Is there any other reason to have it? > Source/WebCore/bindings/scripts/test/JS/JSTestInterface.cpp:421 > + return JSValue::encode(jsTestInterfaceConstructorImplementsStaticReadOnlyAttrGetter(state, castedThis)); I would expect these 3 lines to be just one: return JSValue::encode(jsTestInterfaceConstructorImplementsStaticReadOnlyAttrGetter(state)); If we keep the current prototype, we would go with something like: return JSValue::encode(jsTestInterfaceConstructorImplementsStaticReadOnlyAttrGetter(state, nullptr));

Comment on attachment 279760 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=279760&action=review > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2094 > + if ($interface->serializable) { > + push(@implContent, "EncodedJSValue JSC_HOST_CALL ${serializerNativeFunctionName}(ExecState*);\n\n"); > + } Would be nice to use the single-line if style that the code below uses. > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2556 > + push(@implContent, "JSValue ${getFunctionName}Getter(ExecState* state, ${className}* castedThis);\n\n"); Since this is just a declaration, it should not include the name “state”. Both of these arguments should be references rather than pointers. As Youenn said, I don’t think castedThis is the right name for this. I agree that this should be thisObject. But also, why are we doing this unconditionally? I don’t understand why we want to change this for cases where there is no serializer. > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2571 > + push(@implContent, " JSValue decodedThisValue = JSValue::decode(thisValue);\n"); > + my $castingFunction = $interface->extendedAttributes->{"CustomProxyToJSObject"} ? "to${className}" : GetCastingHelperForThisObject($interface); > + if ($interface->extendedAttributes->{"ImplicitThis"}) { > + push(@implContent, " auto* castedThis = decodedThisValue.isUndefinedOrNull() ? $castingFunction(state->thisValue().toThis(state, NotStrictMode)) : $castingFunction(decodedThisValue);\n"); > + } else { > + push(@implContent, " auto* castedThis = $castingFunction(decodedThisValue);\n"); > + } Seems like a bad idea to do this extra work in cases where the values won’t be used. I don’t understand the rationale here. > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3264 > + if ($interface->serializable) { > + GenerateSerializerFunction($interface, $interfaceName, $className, $serializerFunctionName, $serializerNativeFunctionName); > + } The one line if form would be better. > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3486 > + push(@implContent, " JSValue thisValue = state->thisValue();\n"); > + push(@implContent, " auto castedThis = jsDynamicCast<JS$interfaceName*>(thisValue);\n"); Don’t think we need the local variable named thisValue here. > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3490 > + push(@implContent, " JSObject* result = constructEmptyObject(state);\n"); Would be better to write auto*. > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3499 > + push(@implContent, "\n return JSValue::encode(result);\n"); I don’t think we need the extra blank line here. I would omit the \n at the start of this.

Thanks for the review. (In reply to comment #34) > Comment on attachment 279760 [details] > Patch > > The patch keeps improving :) > > I am wondering what others think about the handling of static attributes > though. > The complete solution for adapted prototypes is in previous patches, it makes the script more complex but adapts the prototype to avoid unused parameters. I'm afraid if we try to cover just some cases we could end up with an if clause that has to follow the logic that controls what code needs more parameters to extract the value, it seems complex to maintain. After your suggestion of taking advantage of the UNUSED I tried this option and I like it better to avoid making the generator more complex, the UNUSED seems something common in the current solution. But anyway, I'm not expert in this code so open to use the solution you think it is best. > > View in context: > https://bugs.webkit.org/attachment.cgi?id=279760&action=review > > > Source/WebCore/bindings/scripts/test/JS/JSTestGlobalObject.cpp:198 > > +JSValue jsTestGlobalObjectEnabledAtRuntimeAttributeGetter(ExecState* state, JSTestGlobalObject* castedThis) > > castedThis name may be changed to thisObject. > That can be done in a follow-up patch. > > > Source/WebCore/bindings/scripts/test/JS/JSTestInterface.cpp:412 > > +JSValue jsTestInterfaceConstructorImplementsStaticReadOnlyAttrGetter(ExecState* state, JSTestInterface* castedThis); > > I would expect static attribute getter prototype to take only the state > parameter, not castedThis. > This might simplify the generation so maybe this is acceptable as a first > step. > Is there any other reason to have it? > Nop, just followed the suggestion to take advantage of the UNUSED and make the generation simpler. > > Source/WebCore/bindings/scripts/test/JS/JSTestInterface.cpp:421 > > + return JSValue::encode(jsTestInterfaceConstructorImplementsStaticReadOnlyAttrGetter(state, castedThis)); > > I would expect these 3 lines to be just one: > return > JSValue:: > encode(jsTestInterfaceConstructorImplementsStaticReadOnlyAttrGetter(state)); > > If we keep the current prototype, we would go with something like: > return > JSValue:: > encode(jsTestInterfaceConstructorImplementsStaticReadOnlyAttrGetter(state, > nullptr)); I'll check it, thanks.

Thanks for the review. (In reply to comment #35) > Comment on attachment 279760 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=279760&action=review > > > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2094 > > + if ($interface->serializable) { > > + push(@implContent, "EncodedJSValue JSC_HOST_CALL ${serializerNativeFunctionName}(ExecState*);\n\n"); > > + } > > Would be nice to use the single-line if style that the code below uses. > Right. > > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2556 > > + push(@implContent, "JSValue ${getFunctionName}Getter(ExecState* state, ${className}* castedThis);\n\n"); > Would be nice to use the single-line if style that the code below uses. > Since this is just a declaration, it should not include the name “state”. > Both of these arguments should be references rather than pointers. As Youenn > said, I don’t think castedThis is the right name for this. I agree that this > should be thisObject. > Right. > But also, why are we doing this unconditionally? I don’t understand why we > want to change this for cases where there is no serializer. > I've followed Youenn's suggestion to simplify the previous solution which was detecting and adapting all the prototypes, taking advantage of using UNUSED allowed to simplify the solution, and I agreed it was simpler to review. But as I said to youenn I have doubts about serializer taking in general a toll in the generator or the code generated, that was the point of the initial patches, considering it does not seem as something we are using a lot. It is an option to do this just for the attributes that are going to be serialized, would that be ok? Probably we should clarify what option is better for us, if we want serializer to support every attribute now we need to modify the generator, in this case if we want a smaller modification in the generator we can use the UNUSED, if we want to avoid that we need a bigger modification. You can see all the solutions in the previous patches posted, depends on the goals we have in this part of the code. > > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2571 > > + push(@implContent, " JSValue decodedThisValue = JSValue::decode(thisValue);\n"); > > + my $castingFunction = $interface->extendedAttributes->{"CustomProxyToJSObject"} ? "to${className}" : GetCastingHelperForThisObject($interface); > > + if ($interface->extendedAttributes->{"ImplicitThis"}) { > > + push(@implContent, " auto* castedThis = decodedThisValue.isUndefinedOrNull() ? $castingFunction(state->thisValue().toThis(state, NotStrictMode)) : $castingFunction(decodedThisValue);\n"); > > + } else { > > + push(@implContent, " auto* castedThis = $castingFunction(decodedThisValue);\n"); > > + } > > Seems like a bad idea to do this extra work in cases where the values won’t > be used. I don’t understand the rationale here. > Hrm, you are right, this seems a mistake when I refactored the code, I'll check it. > > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3264 > > + if ($interface->serializable) { > > + GenerateSerializerFunction($interface, $interfaceName, $className, $serializerFunctionName, $serializerNativeFunctionName); > > + } > > The one line if form would be better. > Right. > > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3486 > > + push(@implContent, " JSValue thisValue = state->thisValue();\n"); > > + push(@implContent, " auto castedThis = jsDynamicCast<JS$interfaceName*>(thisValue);\n"); > > Don’t think we need the local variable named thisValue here. > Right. > > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3490 > > + push(@implContent, " JSObject* result = constructEmptyObject(state);\n"); > > Would be better to write auto*. > Right. > > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3499 > > + push(@implContent, "\n return JSValue::encode(result);\n"); > > I don’t think we need the extra blank line here. I would omit the \n at the > start of this. Right.

Created attachment 279877 [details] Patch

(In reply to comment #38) > Created attachment 279877 [details] > Patch This last patch solves most of the problems pointed out until now but I think we still need to make the decision of what is the best option for this implementation to create a final patch. Basically I see these options: 1. Without support for every attribute in IDL for the generator, it works for what we want in WebRTC, it does not modify current generator part. Not general and it does not reuse current getters. (there is a patch) 2. With support for every attribute in the IDL for the generator: 2.1 Modify current generator for every attribute but with Getter prototypes adapted to the requirements of the Getter code. It is a more complex modification, it modifies every current getter for the serializer which is just used in a small part of all the interface. (there is a patch) 2.2 With general Getter prototypes (2 parameters), we use UNUSED macro to avoid complexity in the generation, it adds Getters to all the current attributes and we use the serializer just for a small part of the IDL. It is easier to review than the last one, smaller amount of changes. (this is the last patch) 2.3 Just modify the IDLs that are defined as serializer, this would require a bigger modification of the current generator, every message would have to be different considering we need new names for the variables. Needs to make a lot of parts that can be adjusted to both options, or rewrite the whole method to make it more flexible. (we still do not have a patch for this) Any other option? Personally I tend to like a smaller modification for this case, considering this is not a very common API nowadays but I guess people with an agenda regarding the generator have a better understanding of the best option. Thanks in advance for your thoughts.

Created attachment 279883 [details] Patch

I would go with option 2.2. As part of this patch or as a follow-up patch, I would differentiate getters in two prototypes: - The ones that only need state (static attributes) - The ones that need state and thisObject (regular attributes) Doing the differentiation will add 1 if statement, which is probably ok in terms of complexity. toJSON being mostly interested in regular attributes, it might be ok to only support regular attributes, until someone actually needs it of couse...

(In reply to comment #41) > I would go with option 2.2. > > As part of this patch or as a follow-up patch, I would differentiate getters > in two prototypes: > - The ones that only need state (static attributes) > - The ones that need state and thisObject (regular attributes) > Doing the differentiation will add 1 if statement, which is probably ok in > terms of complexity. > Thanks for the comment, I think we could go with that. Regarding the static-vs-regular attributes just one comment, there are also attributes that do not need the state, just the thisObject. Also there multiple situations where that could happen. I do not have experience in the code to understand if that could be solved with a clean if clause, as usual when we generate code there are a lot of very special cases.

(In reply to comment #42) > (In reply to comment #41) > > I would go with option 2.2. > > > > As part of this patch or as a follow-up patch, I would differentiate getters > > in two prototypes: > > - The ones that only need state (static attributes) > > - The ones that need state and thisObject (regular attributes) > > Doing the differentiation will add 1 if statement, which is probably ok in > > terms of complexity. > > > > Thanks for the comment, I think we could go with that. > > Regarding the static-vs-regular attributes just one comment, there are also > attributes that do not need the state, just the thisObject. Also there > multiple situations where that could happen. I do not have experience in the > code to understand if that could be solved with a clean if clause, as usual > when we generate code there are a lot of very special cases. I guess the handling of these might add complexity. I would continue outputting UNUSED_PARAM(state) for these cases.

Comment on attachment 279883 [details] Patch Every EWS bot is red. ;)

Created attachment 289544 [details] Patch

After last weeks fixing other issues in the webrtc implementation I would like to try to land this patch. We have finally devoted some more time and implemented the option that does not modify every other API to add the Getter, just the APIs of the objects defined as serialized. It is a bigger change but it seems clear in the patch, the lines protected with the if $interface->serializable, with the EncapsulateReturnTypeIfRequired function and the thisObject variable we could solve the modification of lines without adding new ones. Other than that we left static functions differenciation for next patches when we have situations that uses attributes like those. Thanks in advance for the comments, I hope we can make it now :).

Comment on attachment 289544 [details] Patch I think it goes in the right direction. I would try to simplify the patch by always separating the attribute getter implementation. View in context: https://bugs.webkit.org/attachment.cgi?id=289544&action=review > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2931 > + push(@implContent, " return " . EncapsulateReturnTypeIfRequired($interface->serializable, "jsUndefined()") . ";\n"); That makes quite a lot of switch based on $interface->serializable. How about always generating ${getFunctionName}Getter function, no matter whether supporting serializer or not? > Source/WebCore/bindings/scripts/test/JS/JSTestNode.cpp:172 > +JSValue jsTestNodeNameGetter(ExecState*, JSTestNode*); This should probably be something like: static inline JSValue jsTestNodeNameGetter(ExecState&, JSTestNode&); > Source/WebCore/bindings/scripts/test/JS/JSTestNode.cpp:-179 > - auto& impl = castedThis->wrapped(); It seems to me that this function could be templated as a follow-up patch if we can retrieve the name of the class and the name of the property, something like: EncodedJSValue jsTestNodeName(ExecState* state, EncodedJSValue thisValue, PropertyName propertyName) { return callAttributeGetter<jsTestNodeNameGetter>(*state, *castedThis, propertyName); } Also, If we always split the attribute implementation in two functions, we can remove some UNUSED_PARAM > Source/WebCore/bindings/scripts/test/JS/JSTestNode.cpp:272 > + auto castedThis = jsDynamicCast<JSTestNode*>(state->thisValue()); How about adding an ASSERT(state)? > Source/WebCore/bindings/scripts/test/JS/JSTestNode.cpp:281 > + result->putDirect(vm, Identifier::fromString(&vm, "name"), jsTestNodeNameGetter(state, castedThis)); It might be good to check for any exception that might happen after calling jsTestNodeNameGetter. Either in this patch or as a follow-up patch. > Source/WebCore/bindings/scripts/test/JS/JSTestObj.cpp:1584 > +JSValue jsTestObjReadOnlyTestObjAttrGetter(ExecState*, JSTestObj*); Not related to this patch but there are two lines here while we probably want just one. > Source/WebCore/bindings/scripts/test/JS/JSTestObj.cpp:1610 > +JSValue jsTestObjConstructorStaticReadOnlyLongAttrGetter(ExecState*, JSTestObj*); Two empty lines here.

(In reply to comment #47) > Comment on attachment 289544 [details] > Patch > > I think it goes in the right direction. > I would try to simplify the patch by always separating the attribute getter > implementation. > > > View in context: > https://bugs.webkit.org/attachment.cgi?id=289544&action=review > > > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2931 > > + push(@implContent, " return " . EncapsulateReturnTypeIfRequired($interface->serializable, "jsUndefined()") . ";\n"); > > That makes quite a lot of switch based on $interface->serializable. > How about always generating ${getFunctionName}Getter function, no matter > whether supporting serializer or not? > I'm ok with that option, whatevet feels better for you. > > Source/WebCore/bindings/scripts/test/JS/JSTestNode.cpp:172 > > +JSValue jsTestNodeNameGetter(ExecState*, JSTestNode*); > > This should probably be something like: > static inline JSValue jsTestNodeNameGetter(ExecState&, JSTestNode&); > Right. > > Source/WebCore/bindings/scripts/test/JS/JSTestNode.cpp:-179 > > - auto& impl = castedThis->wrapped(); > > It seems to me that this function could be templated as a follow-up patch if > we can retrieve the name of the class and the name of the property, > something like: > EncodedJSValue jsTestNodeName(ExecState* state, EncodedJSValue thisValue, > PropertyName propertyName) > { > return callAttributeGetter<jsTestNodeNameGetter>(*state, *castedThis, > propertyName); > } > > Also, If we always split the attribute implementation in two functions, we > can remove some UNUSED_PARAM > Right. > > Source/WebCore/bindings/scripts/test/JS/JSTestNode.cpp:272 > > + auto castedThis = jsDynamicCast<JSTestNode*>(state->thisValue()); > > How about adding an ASSERT(state)? > Right. > > > Source/WebCore/bindings/scripts/test/JS/JSTestObj.cpp:1610 > > +JSValue jsTestObjConstructorStaticReadOnlyLongAttrGetter(ExecState*, JSTestObj*); > > Two empty lines here. Right.

Created attachment 289932 [details] Patch

Comment on attachment 289932 [details] Patch I like this patch. Adding an intermediate function between the function used by JS and the DOM function will allow us to do some things like using more references, extracting the code that is casting the this value... Given that this patch is splitting all attribute functions, it would be good to have more eyes looking at it. View in context: https://bugs.webkit.org/attachment.cgi?id=289932&action=review > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2858 > + push(@implContent, "static inline JSValue ${getFunctionName}Getter(ExecState*);\n\n"); In the future, we probably want to pass references for state and object. > Source/WebCore/bindings/scripts/test/JS/JSTestNode.cpp:281 > + result->putDirect(vm, Identifier::fromString(&vm, "name"), jsTestNodeNameGetter(state, castedThis, throwScope)); In the future, we should check for any exception that may happen in the getters.

Comment on attachment 289932 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=289932&action=review > Source/WebCore/bindings/scripts/test/JS/JSTestEventConstructor.cpp:197 > +static inline JSValue jsTestEventConstructorAttr1Getter(ExecState* state, JSTestEventConstructor* thisObject, ThrowScope& throwScope) So now every getter is in two parts, just because of the rare case where we use it for a serializer? This seems very unfortunate. Also, since the function is inline, there is no benefit to code size. Therefore, the only benefit seems to be readability because we generate less code in the case there is a serializer (i.e. avoid duplication). However, I am not convinced by this benefit because: 1. Serializers are rare and we now generate more code in cases where there is no serializer for no good reason. 2. This is generated code so readability (or avoiding code duplication) is not a top priority. Personally, I would just duplicate the attribute getting code in the rare case there is a serializer.

> > Source/WebCore/bindings/scripts/test/JS/JSTestEventConstructor.cpp:197 > > +static inline JSValue jsTestEventConstructorAttr1Getter(ExecState* state, JSTestEventConstructor* thisObject, ThrowScope& throwScope) > > So now every getter is in two parts, just because of the rare case where we > use it for a serializer? This seems very unfortunate. Also, since the > function is inline, there is no benefit to code size. Therefore, the only > benefit seems to be readability because we generate less code in the case > there is a serializer (i.e. avoid duplication). However, I am not convinced > by this benefit because: > 1. Serializers are rare and we now generate more code in cases where there > is no serializer for no good reason. > 2. This is generated code so readability (or avoiding code duplication) is > not a top priority. > > Personally, I would just duplicate the attribute getting code in the rare > case there is a serializer. Maybe it is better to split the patch in two then, one for serializer and another one for the refactoring I have in mind. The idea behind the refactoring I have in mind is: - To templatize the castedThis+throw-if-null code - Apply a different template for private functions/attributes. For these we control the calling site and we do not need checking this again. It is actually bad since we would throw exception with misleading (private) names (bug 158778)

Created attachment 290066 [details] Patch

In this last patch I removed the Getter refactor proposed, and just left the serializer part.

Comment on attachment 290066 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=290066&action=review > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2392 > + push(@implContent, "EncodedJSValue JSC_HOST_CALL ${serializerNativeFunctionName}(ExecState*);\n\n") if $interface->serializable; It should be moved upper with other methods prototypes. > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2552 > + if ($interface->serializable) { Can you move that to GeneratePropertiesHashTable? It might be even simpler to do something similar to iterables: create a toJSON function in IDLParser.pm (see parseOptionalIterableInterface). Then you just need to insert this toJSON function in the functions for which to generate prototype and hash table if serializable. That would leave you with only handling the generation of the implementation of it. > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3797 > + push(@implContent, " result->putDirect(vm, Identifier::fromString(&vm, \"${name}\"), JSValue::decode(${getFunctionName}(state, JSValue::encode(thisValue), Identifier())));\n"); It would be good to check for any exception. Asserting should be good enough for now I guess, something like: childValue = ${getFunctionName}(state, JSValue::encode(thisValue), Identifier()); ASSERT(!throwScope.exception()); result->putDirect(vm, ...., JSValue::decode(childValue)); > LayoutTests/ChangeLog:14 > + * fast/mediastream/RTCSessionDescription.html: Can you add a test verifying the toJSON property, in particular that it has { [[Writable]]: true, [[Enumerable]]: true, [[Configurable]]: true }?

(In reply to comment #55) > Comment on attachment 290066 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=290066&action=review > > > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2392 > > + push(@implContent, "EncodedJSValue JSC_HOST_CALL ${serializerNativeFunctionName}(ExecState*);\n\n") if $interface->serializable; > > It should be moved upper with other methods prototypes. > Right. > > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2552 > > + if ($interface->serializable) { > > Can you move that to GeneratePropertiesHashTable? > > It might be even simpler to do something similar to iterables: create a > toJSON function in IDLParser.pm (see parseOptionalIterableInterface). > Then you just need to insert this toJSON function in the functions for which > to generate prototype and hash table if serializable. > > That would leave you with only handling the generation of the implementation > of it. > Right, I've implemented this approach. > > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3797 > > + push(@implContent, " result->putDirect(vm, Identifier::fromString(&vm, \"${name}\"), JSValue::decode(${getFunctionName}(state, JSValue::encode(thisValue), Identifier())));\n"); > > It would be good to check for any exception. Asserting should be good enough > for now I guess, something like: > > childValue = ${getFunctionName}(state, JSValue::encode(thisValue), > Identifier()); > ASSERT(!throwScope.exception()); > result->putDirect(vm, ...., JSValue::decode(childValue)); > Right. > > LayoutTests/ChangeLog:14 > > + * fast/mediastream/RTCSessionDescription.html: > > Can you add a test verifying the toJSON property, in particular that it has > { [[Writable]]: true, [[Enumerable]]: true, [[Configurable]]: true }? It is already there. Thanks for the review.

Created attachment 290071 [details] Patch

Comment on attachment 290071 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=290071&action=review > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3540 > + Can you move it below iterable since this is the order for the properties hash table and so on. Also would you be able to change the iterable below line as you are doing above: GenerateImplementationIterableFunctions(..) if... ? > Source/WebCore/bindings/scripts/IDLParser.pm:1071 > + $newDataNode = domSerializable->new(); We probably need to create the toJSON function in this case as well. But shouldn't we just die here?

(In reply to comment #58) > Comment on attachment 290071 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=290071&action=review > > > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3540 > > + > > Can you move it below iterable since this is the order for the properties > hash table and so on. > Also would you be able to change the iterable below line as you are doing > above: GenerateImplementationIterableFunctions(..) if... ? > Right. > > Source/WebCore/bindings/scripts/IDLParser.pm:1071 > > + $newDataNode = domSerializable->new(); > > We probably need to create the toJSON function in this case as well. > But shouldn't we just die here? We are creating the function with all the attributes in this case as required in the spec, we add the attributes later when applying the list to make sure they are all parsed. + if ($interface->serializable) { + my $numSerializerAttributes = @{$interface->serializable->attributes}; + if ($numSerializerAttributes == 0) { + foreach my $attribute (@{$interface->attributes}) { + push(@{$interface->serializable->attributes}, $attribute->signature->name); + } + }

Created attachment 290075 [details] Patch

Comment on attachment 290075 [details] Patch r=me, let's ship it!

Created attachment 290079 [details] Patch for landing

Awesome, thanks everyone that contributed to the review! :)

Comment on attachment 290079 [details] Patch for landing Clearing flags on attachment: 290079 Committed r206514: <http://trac.webkit.org/changeset/206514>

All reviewed patches have been landed. Closing bug.

(In reply to comment #52) > > > Source/WebCore/bindings/scripts/test/JS/JSTestEventConstructor.cpp:197 > > > +static inline JSValue jsTestEventConstructorAttr1Getter(ExecState* state, JSTestEventConstructor* thisObject, ThrowScope& throwScope) > > > > So now every getter is in two parts, just because of the rare case where we > > use it for a serializer? This seems very unfortunate. Also, since the > > function is inline, there is no benefit to code size. Therefore, the only > > benefit seems to be readability because we generate less code in the case > > there is a serializer (i.e. avoid duplication). However, I am not convinced > > by this benefit because: > > 1. Serializers are rare and we now generate more code in cases where there > > is no serializer for no good reason. > > 2. This is generated code so readability (or avoiding code duplication) is > > not a top priority. > > > > Personally, I would just duplicate the attribute getting code in the rare > > case there is a serializer. > > Maybe it is better to split the patch in two then, one for serializer and > another one for the refactoring I have in mind. > > The idea behind the refactoring I have in mind is: > - To templatize the castedThis+throw-if-null code > - Apply a different template for private functions/attributes. For these we > control the calling site and we do not need checking this again. It is > actually bad since we would throw exception with misleading (private) names > (bug 158778) Some of the refactoring is now done. Bug 163325 is updating the serializer code to directly reuse these direct attribute getters.

Should this have fixed "serializer = { attribute };", as used by DOMRectReadOnly? It doesn't seem to work.

(In reply to comment #67) > Should this have fixed "serializer = { attribute };", as used by > DOMRectReadOnly? It doesn't seem to work. Yep, it should work, if it doesn't it is a bug. I'll check it, thanks for reporting.

Fixing via bug 163466.

(In reply to comment #69) > Fixing via bug 163466. Oh, attribute is the special keyword for the serializable attributes, that was not supported in the parser. We implemented serializer keyword and the map of attribute identifiers.