Bug 160487 – Add support for wrapper types in dictionaries

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

[Chris Dumez]

Add support for wrapper types in dictionaries

[Chris Dumez]

Created attachment 285184 [details]
WIP patch

[Chris Dumez]

Created attachment 285186 [details]
Patch

[Chris Dumez]

@Darin / @Sam: What do you think of this approach?
@Simon: Is this sufficient to support your use cases?

I have verified that the generated code builds.

[Simon Fraser (smfr)]

What I need to work is:
https://wicg.github.io/IntersectionObserver/#intersection-observer-init
and
https://wicg.github.io/IntersectionObserver/#intersection-observer-entry

[Chris Dumez]

(In reply to comment #4)
> What I need to work is:
> https://wicg.github.io/IntersectionObserver/#intersection-observer-init
> and
> https://wicg.github.io/IntersectionObserver/#intersection-observer-entry

Ok, this patch gets us closer but you will still have trouble with the union type because we don't support unions at all in the bindings generator (not just in dictionaries).

Honestly, I would suggest using the "Dictionary" type in the IDL and deal with the JSDictionary type manually in the implementation for now.

[Chris Dumez]

(In reply to comment #5)
> (In reply to comment #4)
> > What I need to work is:
> > https://wicg.github.io/IntersectionObserver/#intersection-observer-init
> > and
> > https://wicg.github.io/IntersectionObserver/#intersection-observer-entry
> 
> Ok, this patch gets us closer but you will still have trouble with the union
> type because we don't support unions at all in the bindings generator (not
> just in dictionaries).
> 
> Honestly, I would suggest using the "Dictionary" type in the IDL and deal
> with the JSDictionary type manually in the implementation for now.

Or we could maybe use 'any' type for the ´threshold' dictionary member for now. If it does not work yet it would be trivial to add support for ´any' type in dictionaries. It would pass a JSValue to the implementation and the implementation would need to take care of converting this value into either a double or a sequence of double.

[Darin Adler]

Comment on attachment 285186 [details]
Patch

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

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:1022
> +            # FIXME: Ideally we would pass non-nullable wrapper types by reference. However, we currently
> +            # cannot have references in the struct on native side because this function returns { } in case
> +            # of error.

I suppose this could be fixed by making the return type of convert be an Optional just like the type of convertOptional and use Nullopt when we are throwing an exception. If that doesn’t hurt performance too badly it might be worth doing. Could even make the code calling this more optimal because checking for null is probably more efficient than checking state.hadException().

[Chris Dumez]

(In reply to comment #7)
> Comment on attachment 285186 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=285186&action=review
> 
> > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:1022
> > +            # FIXME: Ideally we would pass non-nullable wrapper types by reference. However, we currently
> > +            # cannot have references in the struct on native side because this function returns { } in case
> > +            # of error.
> 
> I suppose this could be fixed by making the return type of convert be an
> Optional just like the type of convertOptional and use Nullopt when we are
> throwing an exception. If that doesn’t hurt performance too badly it might
> be worth doing. Could even make the code calling this more optimal because
> checking for null is probably more efficient than checking
> state.hadException().

Yes, I had the same thought but it seemed like a bigger change, especially considering we do not have users that need support for non billable wrapper types in dictionaries yet (in Simon's case the member is nullable. That said, I am happy to incorporate this into my patch tomorrow if you think I should.

[Chris Dumez]

(In reply to comment #8)
> (In reply to comment #7)
> > Comment on attachment 285186 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=285186&action=review
> > 
> > > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:1022
> > > +            # FIXME: Ideally we would pass non-nullable wrapper types by reference. However, we currently
> > > +            # cannot have references in the struct on native side because this function returns { } in case
> > > +            # of error.
> > 
> > I suppose this could be fixed by making the return type of convert be an
> > Optional just like the type of convertOptional and use Nullopt when we are
> > throwing an exception. If that doesn’t hurt performance too badly it might
> > be worth doing. Could even make the code calling this more optimal because
> > checking for null is probably more efficient than checking
> > state.hadException().
> 
> Yes, I had the same thought but it seemed like a bigger change, especially
> considering we do not have users that need support for non billable wrapper
> types in dictionaries yet (in Simon's case the member is nullable. That
> said, I am happy to incorporate this into my patch tomorrow if you think I
> should.

s/billable/nullable (darn autocorrect)

[Darin Adler]

Comment on attachment 285186 [details]
Patch

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

>>>> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:1022
>>>> +            # of error.
>>> 
>>> I suppose this could be fixed by making the return type of convert be an Optional just like the type of convertOptional and use Nullopt when we are throwing an exception. If that doesn’t hurt performance too badly it might be worth doing. Could even make the code calling this more optimal because checking for null is probably more efficient than checking state.hadException().
>> 
>> Yes, I had the same thought but it seemed like a bigger change, especially considering we do not have users that need support for non billable wrapper types in dictionaries yet (in Simon's case the member is nullable. That said, I am happy to incorporate this into my patch tomorrow if you think I should.
> 
> s/billable/nullable (darn autocorrect)

You could make it an error to ask for a non-nullable wrapper type in a dictionary rather than implementing it with the wrong type. If nobody needs it right now.

[Chris Dumez]

Created attachment 285233 [details]
Patch

[Chris Dumez]

(In reply to comment #10)
> Comment on attachment 285186 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=285186&action=review
> 
> >>>> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:1022
> >>>> +            # of error.
> >>> 
> >>> I suppose this could be fixed by making the return type of convert be an Optional just like the type of convertOptional and use Nullopt when we are throwing an exception. If that doesn’t hurt performance too badly it might be worth doing. Could even make the code calling this more optimal because checking for null is probably more efficient than checking state.hadException().
> >> 
> >> Yes, I had the same thought but it seemed like a bigger change, especially considering we do not have users that need support for non billable wrapper types in dictionaries yet (in Simon's case the member is nullable. That said, I am happy to incorporate this into my patch tomorrow if you think I should.
> > 
> > s/billable/nullable (darn autocorrect)
> 
> You could make it an error to ask for a non-nullable wrapper type in a
> dictionary rather than implementing it with the wrong type. If nobody needs
> it right now.

Ok, I chose this approach.

[Chris Dumez]

Comment on attachment 285233 [details]
Patch

Clearing flags on attachment: 285233

Committed r204143: <http://trac.webkit.org/changeset/204143>

[Chris Dumez]

All reviewed patches have been landed.  Closing bug.