Pass reference instead of pointer to IDL attribute setters if not nullable.
Created attachment 277874 [details] Patch
Created attachment 277877 [details] Patch
Created attachment 277878 [details] Patch
Another attempt to make the GObject bindings happy :/
Comment on attachment 277878 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=277878&action=review > Source/WebCore/ChangeLog:12 > + There is not intended Web-exposed behavior change. should be "no intended" > Source/WebCore/ChangeLog:38 > + (WebCore::setJSTestObjTestObjAttr): > + (WebCore::setJSTestObjLenientTestObjAttr): > + (WebCore::setJSTestObjXMLObjAttr): > + (WebCore::setJSTestObjTypedArrayAttr): > + (WebCore::setJSTestObjStrictTypeCheckingAttribute): > + (WebCore::setJSTestObjWithScriptExecutionContextAttribute): > + (WebCore::setJSTestObjWithScriptStateAttributeRaises): > + (WebCore::setJSTestObjWithScriptExecutionContextAttributeRaises): > + (WebCore::setJSTestObjWithScriptExecutionContextAndScriptStateAttribute): > + (WebCore::setJSTestObjWithScriptExecutionContextAndScriptStateAttributeRaises): > + (WebCore::setJSTestObjWithScriptExecutionContextAndScriptStateWithSpacesAttribute): > + (WebCore::setJSTestObjWithScriptArgumentsAndCallStackAttribute): > + (WebCore::setJSTestObjWithCallWithAndSetterCallWithAttribute): Deleted. I suggest removing the lists of function names for the regenerated test output. > Source/WebCore/bindings/js/JSDataCueCustom.cpp:92 > return JSValue::encode(JSValue()); So random to use this value here unlike other similar call sites. I think we need a more standard thing to return when we have exceptions. JSValue::encode(jsUndefined()) is so long and it shouldn’t matter what we return. > Source/WebCore/bindings/scripts/CodeGeneratorGObject.pm:1113 > + $paramName = "*$paramName" if ($codeGenerator->ShouldPassWrapperByReference($param, $parentNode)); perl coding style omits the parentheses after if when doing it like this, and I like that > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2867 > + push(@implContent, " return false;\n"); Maybe we should return { } in all these places where there is an exception and it doesn’t matter what we return. > Source/WebCore/dom/NodeIterator.h:63 > + NodePointer() = default; > + NodePointer(Node&, bool); Do we really need these constructors at all? Could we just use aggregate initialization syntax instead and write { &node, false }. Where are the call sites that this makes better? > Source/WebCore/dom/TreeWalker.h:45 > + Node& currentNode() { return m_current.get(); } > + const Node& currentNode() const { return m_current.get(); } This need to overload irritates me a bit. It seems fine to me to get a Node& out of a const TreeWalker; just seems this is a sort of “bug” in Ref; would not be an issue if it was an actual reference. I’d prefer not to have these two overloads. Maybe we can use const_cast or something. > Source/WebCore/dom/TreeWalker.h:62 > + Node* setCurrent(Ref<Node>&&); The return value for this could be Node&.
Comment on attachment 277878 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=277878&action=review >> Source/WebCore/ChangeLog:12 >> + There is not intended Web-exposed behavior change. > > should be "no intended" OK >> Source/WebCore/ChangeLog:38 >> + (WebCore::setJSTestObjWithCallWithAndSetterCallWithAttribute): Deleted. > > I suggest removing the lists of function names for the regenerated test output. OK >> Source/WebCore/bindings/js/JSDataCueCustom.cpp:92 >> return JSValue::encode(JSValue()); > > So random to use this value here unlike other similar call sites. I think we need a more standard thing to return when we have exceptions. JSValue::encode(jsUndefined()) is so long and it shouldn’t matter what we return. Ok, I think I'll return JSValue::encode(jsUndefined()) for now, for consistency. >> Source/WebCore/bindings/scripts/CodeGeneratorGObject.pm:1113 >> + $paramName = "*$paramName" if ($codeGenerator->ShouldPassWrapperByReference($param, $parentNode)); > > perl coding style omits the parentheses after if when doing it like this, and I like that OK. >> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2867 >> + push(@implContent, " return false;\n"); > > Maybe we should return { } in all these places where there is an exception and it doesn’t matter what we return. Everything else in this function already returns false in case of error. The setters return a boolean.
Created attachment 277881 [details] Patch
Comment on attachment 277881 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=277881&action=review > Source/WebCore/bindings/js/JSDataCueCustom.cpp:90 > + if (!data) { UNLIKELY, I think
Created attachment 277889 [details] Patch
Comment on attachment 277889 [details] Patch Clearing flags on attachment: 277889 Committed r200316: <http://trac.webkit.org/changeset/200316>
All reviewed patches have been landed. Closing bug.