Bug 162919 – Generate passing union types as part of a functions variadic arguments

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Sam Weinig]

Autogenerate passing union types as part of a functions variadic arguments

[Sam Weinig]

Created attachment 290620 [details]
Patch

[Sam Weinig]

Created attachment 290627 [details]
Patch

[Sam Weinig]

Created attachment 290628 [details]
Patch

[Sam Weinig]

Created attachment 290636 [details]
Patch

[Sam Weinig]

Created attachment 290639 [details]
Patch

[Sam Weinig]

Created attachment 290655 [details]
Patch

[Chris Dumez]

Comment on attachment 290655 [details]
Patch

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

> Source/WebCore/ChangeLog:12
> +        * bindings/js/JSCharacterDataCustom.cpp: Removed.

Fails to build on Windows:
C:\cygwin\home\buildbot\WebKit\Source\WebCore\bindings\js\JSBindingsAllInOne.cpp(44): fatal error C1083: Cannot open include file: 'JSCharacterDataCustom.cpp': No such file or directory [C:\cygwin\home\buildbot\WebKit\WebKitBuild\Release\Source\WebCore\WebCore.vcxproj]

[Chris Dumez]

Comment on attachment 290655 [details]
Patch

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

> Source/WebCore/bindings/js/JSDOMConvert.h:79
> +    static Optional<T> convertFailable(JSC::ExecState& state, JSC::JSValue value)

I am not sure we need this, all convert() functions can already fail.

> Source/WebCore/bindings/js/JSDOMConvert.h:129
> +    static Optional<Ref<T>> convertFailable(JSC::ExecState& state, JSC::JSValue value)

Why do we need the concept of 'Failable"?

We previously converted interface types using convertWrapperType(). The way we check for failure is usually by checking if an exception was thrown via "state".

[Chris Dumez]

Comment on attachment 290655 [details]
Patch

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

Looks great otherwise.

> Source/WebCore/bindings/scripts/CodeGenerator.pm:637
> +            push @flattenedMemberTypes, $object->GetFlattenedMemberTypes($memberType)

I think we usually use the version with the parentheses in this script: push().

> Source/WebCore/bindings/scripts/CodeGenerator.pm:639
> +            push @flattenedMemberTypes, $memberType;

ditto.

[Sam Weinig]

(In reply to comment #8)
> Comment on attachment 290655 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=290655&action=review
> 
> > Source/WebCore/bindings/js/JSDOMConvert.h:79
> > +    static Optional<T> convertFailable(JSC::ExecState& state, JSC::JSValue value)
> 
> I am not sure we need this, all convert() functions can already fail.
> 
> > Source/WebCore/bindings/js/JSDOMConvert.h:129
> > +    static Optional<Ref<T>> convertFailable(JSC::ExecState& state, JSC::JSValue value)
> 
> Why do we need the concept of 'Failable"?
> 
> We previously converted interface types using convertWrapperType(). The way
> we check for failure is usually by checking if an exception was thrown via
> "state".

The issue is that I need to be able to return a Ref<T> from the convert function, since that is what the implementation files want in this case, but in the case that they fail, I cannot return a Ref<T>. So, returning an Optional<Ref<T>> is the way to go, and if I am doing that, I need to make it generic, so all converts must return an Optional<Foo>. 

Another way to go would be to have the convert function return a RefPtr<T> and have another static function on Converter that can transform the RefPtr<T> into a Ref<T> if there was no exception (and for non interface types, just pass the result through). Do you have a preference for either?

[Chris Dumez]

(In reply to comment #10)
> (In reply to comment #8)
> > Comment on attachment 290655 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=290655&action=review
> > 
> > > Source/WebCore/bindings/js/JSDOMConvert.h:79
> > > +    static Optional<T> convertFailable(JSC::ExecState& state, JSC::JSValue value)
> > 
> > I am not sure we need this, all convert() functions can already fail.
> > 
> > > Source/WebCore/bindings/js/JSDOMConvert.h:129
> > > +    static Optional<Ref<T>> convertFailable(JSC::ExecState& state, JSC::JSValue value)
> > 
> > Why do we need the concept of 'Failable"?
> > 
> > We previously converted interface types using convertWrapperType(). The way
> > we check for failure is usually by checking if an exception was thrown via
> > "state".
> 
> The issue is that I need to be able to return a Ref<T> from the convert
> function, since that is what the implementation files want in this case, but
> in the case that they fail, I cannot return a Ref<T>. So, returning an
> Optional<Ref<T>> is the way to go, and if I am doing that, I need to make it
> generic, so all converts must return an Optional<Foo>. 
> 
> Another way to go would be to have the convert function return a RefPtr<T>
> and have another static function on Converter that can transform the
> RefPtr<T> into a Ref<T> if there was no exception (and for non interface
> types, just pass the result through). Do you have a preference for either?

My preference would be to rely on convertWrapperType() which returns a raw pointer.

I don't think the implementation really needs a Ref<>. We can call convertWrapperType() with the right IsNullable parameter.
Then the calling code just needs to check if it threw an exception.

Why would the convert<> function ref the object? I understand that your call site currently eventually wants to ref it but in general we pass raw pointers (or C++ references when not nullable) to the implementation to avoid ref counting churn. We then let the implementation decide if it wants to ref it or not.

[Sam Weinig]

Created attachment 290739 [details]
Patch

[Chris Dumez]

Comment on attachment 290739 [details]
Patch

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

> Source/WebCore/bindings/js/JSDOMConvert.h:132
> +    static Optional<Ref<T>> convertFailable(JSC::ExecState& state, JSC::JSValue value)

Still using convertFailable? All convert functions can already fail and there is already a convert function for wrapper types.

[Sam Weinig]

Created attachment 290852 [details]
Patch

[Darin Adler]

Comment on attachment 290739 [details]
Patch

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

r=me but I think you should consider the comment Chris made about convertFailable.

> Source/WebCore/bindings/scripts/CodeGenerator.pm:666
> +        if ($memberType->isNullable) {
> +            $count++;
> +        }

I like one-liner if for this kind of thing in perl.

> Source/WebCore/bindings/scripts/CodeGenerator.pm:670
> +        if ($memberType->isUnion) {
> +            $count += $object->GetNumberOfNullableMemberTypes($memberType);
> +        }

Ditto.

> Source/WebCore/bindings/scripts/CodeGenerator.pm:683
> +    if ($numberOfNullableMembers > 1) {
> +        assert("Union types must only have 0 or 1 nullable types.");
> +    }

Ditto.

> Source/WebCore/bindings/scripts/CodeGenerator.pm:689
> +    if ($numberOfNullableMembers == 1) {
> +        push(@idlUnionMemberTypes, "IDLNull");
> +    }

Ditto.

[Darin Adler]

Sorry, my review comments (trivial tiny things) showed up just as you posted your new patch.

[Sam Weinig]

Committed r206956: <http://trac.webkit.org/changeset/206956>