Bug 156955 – [WebIDL] Drop [Default] WebKit-IDL extended attribute

• Status: RESOLVED
• Resolution: FIXED
• Component: Bindings

[Chris Dumez]

Drop [Default] WebKit-IDL extended attribute in favor of the standard WebIDL syntax for parameters' default value.

[Chris Dumez]

return-type identifier(type identifier, optional type identifier = value);

[Chris Dumez]

Created attachment 277177 [details]
Patch

[Chris Dumez]

Created attachment 277178 [details]
Patch

[Chris Dumez]

Created attachment 277179 [details]
Patch

[Chris Dumez]

Working with EWS to get all ports building.

[Build Bot]

Comment on attachment 277179 [details]
Patch

Attachment 277179 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/1210734

New failing tests:
imported/w3c/web-platform-tests/html/dom/interfaces.html

[Build Bot]

Created attachment 277183 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.4

[Chris Dumez]

Created attachment 277184 [details]
Patch

[Darin Adler]

Comment on attachment 277184 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=277184&action=review

Nice work.

> Source/WebCore/ChangeLog:30
> +        These do not cause any Web-Exposed behavior change. However, when getting
> +        rid of [Default] started passing more parameters by derefence instead of
> +        pointer (as is expected for parameters not marked as nullable). As a result,
> +        I had to mark a few parameters as nullable in the IDL to maintain the
> +        previous behavior. In some cases, the implementation was already throwing
> +        when passing null. In such cases, I have not marked the type as nullable
> +        so that the generated bindings now throw the exception instead of our
> +        implementation code. In some cases, the exception being thrown is now
> +        different. This is why some of the layout tests had to be rebaselined.

I would have expected this patch to include removal of the code in ShouldPassWrapperByReference in CodeGenerator.pm that looks at the Default extended attribute.

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3582
> +                    $defaultValue = "nullptr" if $defaultValue eq "null";
> +                    $defaultValue = "PNaN" if $defaultValue eq "NaN";

I suggest putting these after the string related optimizations to avoid the peculiar use of nullptr in the string code.

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3583
> +                    $defaultValue = GetNativeTypeFromSignature($parameter) . "()" if $defaultValue eq "[]";

Could we just use "{}" instead of doing this?

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3592
> +                            $defaultValue = $useAtomicString ? "AtomicString($defaultValue, AtomicString::ConstructFromLiteral)" : "ASCIILiteral($defaultValue)";

Should talk to Ben about these to be sure they are what we want. For example, if there is something performance critical going on it would be good to avoid allocating and then deleting every time.

> Source/WebCore/dom/NodeFilter.idl:50
>  #if defined(LANGUAGE_OBJECTIVE_C) && LANGUAGE_OBJECTIVE_C
> -    short acceptNode([Default=Undefined] optional Node n);
> +    short acceptNode(optional Node n);
>  #else
> -    [Custom] unsigned short acceptNode([Default=Undefined] optional Node n);
> +    [Custom] unsigned short acceptNode(optional Node n);
>  #endif

We have to be able to do better than the name "n".

[Chris Dumez]

Comment on attachment 277184 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=277184&action=review

>> Source/WebCore/ChangeLog:30
>> +        different. This is why some of the layout tests had to be rebaselined.
> 
> I would have expected this patch to include removal of the code in ShouldPassWrapperByReference in CodeGenerator.pm that looks at the Default extended attribute.

Will do.

>> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3582
>> +                    $defaultValue = "PNaN" if $defaultValue eq "NaN";
> 
> I suggest putting these after the string related optimizations to avoid the peculiar use of nullptr in the string code.

Ok

>> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3583
>> +                    $defaultValue = GetNativeTypeFromSignature($parameter) . "()" if $defaultValue eq "[]";
> 
> Could we just use "{}" instead of doing this?

That was my first attempt but it did not build. It complained about not being allowed to use an initializer list on the right side of a '?'.

>> Source/WebCore/dom/NodeFilter.idl:50
>>  #endif
> 
> We have to be able to do better than the name "n".

Ok.

[Chris Dumez]

Created attachment 277188 [details]
Patch

[Chris Dumez]

Committed r199969: <http://trac.webkit.org/changeset/199969>