Bug 157250 – Pass reference instead of pointer to IDL attribute setters if not nullable

• Component: Bindings
• Status: RESOLVED FIXED

[Chris Dumez]

Pass reference instead of pointer to IDL attribute setters if not nullable.

[Chris Dumez]

Created attachment 277874 [details]
Patch

[Chris Dumez]

Created attachment 277877 [details]
Patch

[Chris Dumez]

Created attachment 277878 [details]
Patch

[Chris Dumez]

Another attempt to make the GObject bindings happy :/

[Darin Adler]

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&.

[Chris Dumez]

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.

[Chris Dumez]

Created attachment 277881 [details]
Patch

[Darin Adler]

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

[Chris Dumez]

Created attachment 277889 [details]
Patch

[Chris Dumez]

Comment on attachment 277889 [details]
Patch

Clearing flags on attachment: 277889

Committed r200316: <http://trac.webkit.org/changeset/200316>

[Chris Dumez]

All reviewed patches have been landed.  Closing bug.