Bug 157703 – [WebIDL] Add support for dictionary members of integer types

• Component: Bindings
• Status: RESOLVED FIXED

[Chris Dumez]

Add support for dictionary members of integer types, including support for the [Clamp] and [EnforceRange] IDL extended attributes on such members.

[Chris Dumez]

Created attachment 278927 [details]
Patch

[Darin Adler]

Comment on attachment 278927 [details]
Patch

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

I would like this patch even better if you used this in at least one place. I have been trying to only add things as they are actually used.

On the other hand, it’s great to keep building this out. Maybe eventually we can use this for function arguments too.

When trying to use dictionary more to replace Dictionary/JSDictionary, the main problem I have been running into is lack of support for union types.

> Source/WebCore/bindings/js/JSDOMConvert.h:39
> +template<typename T, typename U = T>
> +using enable_if_integral_t = typename std::enable_if<std::is_integral<T>::value, U>::type;
> +
> +template<typename T, typename U = T>
> +using enable_if_floating_point_t = typename std::enable_if<std::is_floating_point<T>::value, U>::type;

I understand that these are extensions of std::enable_if, but I’m not sure we should name them using the standard library style names rather than our style.

Also, I prefer to put things like this into one longer line rather than having template on one line and the body of the template on the next.

> Source/WebCore/bindings/js/JSDOMConvert.h:63
> -template<typename T> T convert(JSC::ExecState& state, JSC::JSValue value, IntegerConversionConfiguration configuration)
> +template<typename T>
> +inline enable_if_integral_t<T> convert(JSC::ExecState& state, JSC::JSValue value, IntegerConversionConfiguration configuration)

I intentionally did this on a single line and would prefer that it not be broken up into two like this.

> Source/WebCore/bindings/js/JSDOMConvert.h:69
> +template<typename T>
> +inline enable_if_floating_point_t<T> convert(JSC::ExecState& state, JSC::JSValue value, ShouldAllowNonFinite allow)

Ditto.

> Source/WebCore/bindings/js/JSDOMConvert.h:85
> +template<typename T>
> +inline enable_if_floating_point_t<T, Optional<T>> convertOptional(JSC::ExecState& state, JSC::JSValue value, ShouldAllowNonFinite allow)

Ditto.

> Source/WebCore/bindings/js/JSDOMConvert.h:91
> +template<typename T, typename U>
> +inline enable_if_floating_point_t<T> convertOptional(JSC::ExecState& state, JSC::JSValue value, ShouldAllowNonFinite allow, U&& defaultValue)

Ditto.

> Source/WebCore/bindings/js/JSDOMConvert.h:97
> +template<typename T>
> +inline enable_if_integral_t<T, Optional<T>> convertOptional(JSC::ExecState& state, JSC::JSValue value, IntegerConversionConfiguration configuration)

Ditto.

> Source/WebCore/bindings/js/JSDOMConvert.h:103
> +template<typename T, typename U>
> +inline enable_if_integral_t<T> convertOptional(JSC::ExecState& state, JSC::JSValue value, IntegerConversionConfiguration configuration, U&& defaultValue)

Ditto.

[Chris Dumez]

Comment on attachment 278927 [details]
Patch

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

>> Source/WebCore/bindings/js/JSDOMConvert.h:39
>> +using enable_if_floating_point_t = typename std::enable_if<std::is_floating_point<T>::value, U>::type;
> 
> I understand that these are extensions of std::enable_if, but I’m not sure we should name them using the standard library style names rather than our style.
> 
> Also, I prefer to put things like this into one longer line rather than having template on one line and the body of the template on the next.

Do you have a suggestion for the naming?

>> Source/WebCore/bindings/js/JSDOMConvert.h:63
>> +inline enable_if_integral_t<T> convert(JSC::ExecState& state, JSC::JSValue value, IntegerConversionConfiguration configuration)
> 
> I intentionally did this on a single line and would prefer that it not be broken up into two like this.

I know. I just thought the lines were getting really long but OK.

[Chris Dumez]

(In reply to comment #2)
> Comment on attachment 278927 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=278927&action=review
> 
> I would like this patch even better if you used this in at least one place.
> I have been trying to only add things as they are actually used.

I admit I have looked if I could use this new support in the existing code base yet. I will though. At the same time, supporting integers seems like basic support we want to have either way. If someone needs to add a dictionary with integers at some point, the cost of doing so will be much less. I know I added a new dictionary yesterday and had to hack the bindings generator because we did not support floating point members in dictionaries.

[Chris Dumez]

Created attachment 278933 [details]
Patch

[Chris Dumez]

Comment on attachment 278933 [details]
Patch

Clearing flags on attachment: 278933

Committed r200920: <http://trac.webkit.org/changeset/200920>

[Chris Dumez]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

Comment on attachment 278927 [details]
Patch

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

>>> Source/WebCore/bindings/js/JSDOMConvert.h:39
>>> +using enable_if_floating_point_t = typename std::enable_if<std::is_floating_point<T>::value, U>::type;
>> 
>> I understand that these are extensions of std::enable_if, but I’m not sure we should name them using the standard library style names rather than our style.
>> 
>> Also, I prefer to put things like this into one longer line rather than having template on one line and the body of the template on the next.
> 
> Do you have a suggestion for the naming?

My starting point would be the exact same name, but in our style:

    EnableIfIntegralType

But I find that “If” looks really bad in a name like this with our style, even though it looks good in lowercase-plus-underscores and I also don’t think we need a Type suffix just to indicate this is a type since that will be clear based on how it’s used. So maybe:

    EnableForIntegral
    EnableForFloatingPoint

Or we could find another naming scheme for the "enable_if" idiom. A few different ideas:

    OnlyIntegral
    OnlyFloatingPoint

    RestrictToIntegral
    RestrictToFloatingPoint

    SkipUnlessIntegral
    SkipUnlessFloatingPoint

I’m sure I could come up with more ideas with more time to think.