Bug 158835 – Templatize JS bindings code generator of functions with variadic parameters

• Component: Bindings
• Status: RESOLVED FIXED

[Nael Ouedraogo]

Templatize JS bindings code generator of functions with variadic parameters.

[Nael Ouedraogo]

Created attachment 281454 [details]
Patch

[Nael Ouedraogo]

One possible improvement is to homogenize toNativeArgument() and toDOMArgumentsVariadic() functions. A second improvement is to process both native and DOM arguments with a single function in JS bindings code generator.  

This could be addressed in a follow-up bug.

[Nael Ouedraogo]

Created attachment 281905 [details]
Patch

[Nael Ouedraogo]

In the uploaded patch, variadic arguments (native or DOM) are handled by a single template function.

[youenn fablet]

Comment on attachment 281905 [details]
Patch

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

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3686
> +            }

Could it be done as a one liner?

[youenn fablet]

Comment on attachment 281905 [details]
Patch

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

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3686
> +            }

Could it be done as a one liner?

[Nael Ouedraogo]

(In reply to comment #6)
> https://bugs.webkit.org/attachment.cgi?id=281905&action=review
> 
> > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3686
> > +            }
> 
> Could it be done as a one liner?
Thanks for the review.

I will replace if/else block with a call to ternary operator.

[Nael Ouedraogo]

Created attachment 282128 [details]
Patch

[Nael Ouedraogo]

(In reply to comment #6)
> Comment on attachment 281905 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=281905&action=review
> 
> > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3686
> > +            }
> 
> Could it be done as a one liner?

Proposed modification is done in the uploaded patch.

[youenn fablet]

Comment on attachment 282128 [details]
Patch

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

Sounds good to me. Some proposed improvements below.

> Source/WebCore/bindings/js/JSDOMBinding.h:319
> +        if (!TraitsType::nativeValue(&state, jsValue, indexValue))

Would you be able to modify nativeValue to take an ExecState reference as a preliminary patch?

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3685
> +            my $domType = IsNativeType($type) ? GetNativeType($interface, $type) : "$type";

Why "$type" and not $type?

> Source/WebCore/bindings/scripts/test/JS/JSTestObj.cpp:5953
> +    auto tail = toArguments<VariadicHelper<JSdouble, double>>(*state, 1);

This code seems odd, since JSdouble probably does not exist.
Maybe replace it by JSC::JSValue for all native types, since it is not used anyway for them?

[Nael Ouedraogo]

I just ad(In reply to comment #10)
> > Source/WebCore/bindings/js/JSDOMBinding.h:319
> > +        if (!TraitsType::nativeValue(&state, jsValue, indexValue))
> 
> Would you be able to modify nativeValue to take an ExecState reference as a
> preliminary patch?

Ok. A preliminary patch is proposed in Bug 159298.

> > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3685
> > +            my $domType = IsNativeType($type) ? GetNativeType($interface, $type) : "$type";
> 
> Why "$type" and not $type?
> 
> > Source/WebCore/bindings/scripts/test/JS/JSTestObj.cpp:5953
> > +    auto tail = toArguments<VariadicHelper<JSdouble, double>>(*state, 1);
> 
> This code seems odd, since JSdouble probably does not exist.
> Maybe replace it by JSC::JSValue for all native types, since it is not used
> anyway for them?

I will fix this in a new patch once preliminary Bug 159298 is resolved.

[Nael Ouedraogo]

Created attachment 283325 [details]
Patch

[Nael Ouedraogo]

Preliminary patch is landed (modification of nativeValue to takes an ExecState reference). Uploaded patch fixed also the two remaining issues.

[Darin Adler]

Comment on attachment 283325 [details]
Patch

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

> Source/WebCore/bindings/js/JSDOMBinding.h:313
> +    using Result = typename std::pair<Optional<Container>, size_t>;

Annoying that this is repeated in both versions of VariadicHelper. Might be nice to use inheritance instead.

Since a pair or tuple here is a necessary evil, I suppose we should have the index first and then the value, rather than the value first and then the index. Think of it as an item in a numbered list.

> Source/WebCore/bindings/js/JSDOMBinding.h:315
> +    static Optional<DOMClass> argumentConverter(JSC::ExecState& state, JSC::JSValue jsValue)

Normally we use a verb phrase for a function or a noun phrase that describes the result. It’s not so common to use a noun phrase that describes the function itself. A better name for this would be convert or unwrap.

> Source/WebCore/bindings/js/JSDOMBinding.h:330
> +    using Class     = typename std::remove_reference<decltype(std::declval<JSClass>().wrapped())>::type;
> +    using Item      = std::reference_wrapper<Class>;
> +    using Container = Vector<Item>;
> +    using Result    = typename std::pair<Optional<Container>, size_t>;

We don’t do this kind of vertical alignment formatting in WebKit; style guide specifically forbids it.

> Source/WebCore/bindings/js/JSDOMBinding.h:791
> +        return {Nullopt, 0};

In WebKit coding style there is a space inside the { }.

> Source/WebCore/bindings/js/JSDOMBinding.h:799
> +            return {Nullopt, i};

Ditto.

> Source/WebCore/bindings/js/JSDOMBinding.h:800
> +        result.uncheckedAppend(*value);

We normally use the value() function instead of * with Optional in WebKit code, not entirely sure why.

This should be WTFMove(*value.value()) because in some cases, such as String, that is more efficient than copying.

> Source/WebCore/bindings/js/JSDOMBinding.h:802
> +    return {WTFMove(result), length};

Space here.

> Source/WebCore/bindings/js/JSDOMWrapper.h:88
> +    static constexpr bool isWrappingDOMObject = true;

Terminology here is not consistent. Above we say isSimpleWrapper and isComplexWrapper. This one should be isWrapper. But also this is redundant with isBuiltin; do we really need both?

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3709
> +            my $domType = IsNativeType($type) ? GetNativeType($interface, $type) : $type;
> +            my $jsType = IsNativeType($type) ? "JSC::JSValue" : "JS$domType";

I don’t think the names "domType" and "jsType" are good here even though that’s what’s used int he C++ code. I would probably call these "wrapped type" and "wrapper type". Also, since this is the only place in the entire file that uses IsNativeType, it seems like we could change it around to be better for this purpose. It would be more consistent with the other code in this file to have a function that implements this algorithm instead of writing it out like this. It seems likely that the algorithm here is only simple right now because of the many types that we don’t handle. For example, if we wanted this to work properly for enumerations or dictionaries this code would likely have to change. So I suggest coming up with some named functions for this instead of writing the code in here. Maybe a single function with two results.

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3718
> +            push(@$outputArray, "    if (!$name.first)\n");
> +            push(@$outputArray, "        return throwArgumentTypeError(*state, $name.second, \"$name\", \"$interfaceName\", $quotedFunctionName, \"$type\");\n");

Seems a shame that we now generate unreachable code to throw an error that can never happen in a case like this. Or is this reachable? If it is reachable, can we add a test case to show the change in behavior?

[Nael Ouedraogo]

Created attachment 287389 [details]
Patch

[Nael Ouedraogo]

Thanks for the review and sorry for the late response.
Coding style mistakes and terminology have been corrected in the uploaded patch. 

> > Source/WebCore/bindings/js/JSDOMBinding.h:313
> > +    using Result = typename std::pair<Optional<Container>, size_t>;
> 
> Annoying that this is repeated in both versions of VariadicHelper. Might be
> nice to use inheritance instead.
OK, repeated code has been factorized.

> > Source/WebCore/bindings/js/JSDOMBinding.h:800
> > +        result.uncheckedAppend(*value);
> 
> We normally use the value() function instead of * with Optional in WebKit
> code, not entirely sure why.
> 
> This should be WTFMove(*value.value()) because in some cases, such as
> String, that is more efficient than copying.
Actually, value was not necessarily an Optional. Return types of
convert methods have been changed to Optional in uploaded patch so that value() can be used. 

> > Source/WebCore/bindings/js/JSDOMWrapper.h:88
> > +    static constexpr bool isWrappingDOMObject = true;
> 
> Terminology here is not consistent. Above we say isSimpleWrapper and
> isComplexWrapper. This one should be isWrapper. But also this is redundant
> with isBuiltin; do we really need both?
Yes, it can be removed since not essential.

> > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3709
> > +            my $domType = IsNativeType($type) ? GetNativeType($interface, $type) : $type;
> > +            my $jsType = IsNativeType($type) ? "JSC::JSValue" : "JS$domType";
> 
> I don’t think the names "domType" and "jsType" are good here even though
> that’s what’s used int he C++ code. I would probably call these "wrapped
> type" and "wrapper type". Also, since this is the only place in the entire
> file that uses IsNativeType, it seems like we could change it around to be
> better for this purpose. It would be more consistent with the other code in
> this file to have a function that implements this algorithm instead of
> writing it out like this. It seems likely that the algorithm here is only
> simple right now because of the many types that we don’t handle. For
> example, if we wanted this to work properly for enumerations or dictionaries
> this code would likely have to change. So I suggest coming up with some
> named functions for this instead of writing the code in here. Maybe a single
> function with two results.
>
A function has been added in the uploaded patch with wrapper/wrapped terminology.

> > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3718
> > +            push(@$outputArray, "    if (!$name.first)\n");
> > +            push(@$outputArray, "        return throwArgumentTypeError(*state, $name.second, \"$name\", \"$interfaceName\", $quotedFunctionName, \"$type\");\n");
> 
> Seems a shame that we now generate unreachable code to throw an error that
> can never happen in a case like this. Or is this reachable? If it is
> reachable, can we add a test case to show the change in behavior?
I modified the code to avoid generating this unreachable code for native types.

[Darin Adler]

Comment on attachment 287389 [details]
Patch

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

> Source/WebCore/bindings/js/JSDOMBinding.h:321
> +template <typename JSClass, typename DOMClass, typename Enable = void>

We don’t leave a space between "template" and "<", although we don’t use the style consistently. Probably need to mention it in the style guideline.

> Source/WebCore/bindings/js/JSDOMBinding.h:335
> +template <typename JSClass, typename DOMClass>

We don’t leave a space between "template" and "<", although we don’t use the style consistently. Probably need to mention it in the style guideline.

> Source/WebCore/bindings/js/JSDOMBinding.h:353
> +    using Result = typename std::pair<size_t, Optional<Container>>;

Hard to read code that uses pair like this, since the names "first" and "second" aren’t so great. I suppose it wouldn’t be any better with std::tuple, but it could be better if we just used a struct instead because we could then name the members. We made this change a while for HashMap::AddResult and for HashMap::KeyValuePairType; it was a lot more readable to use a struct, and in modern C++ where we are doing anything generic with the pair, the code generated is nearly identical.

> Source/WebCore/bindings/js/JSDOMBinding.h:356
> +template <typename VariadicHelper> typename VariadicHelper::Result toArguments(JSC::ExecState&, size_t startIndex = 0);

We don’t leave a space between "template" and "<", although we don’t use the style consistently. Probably need to mention it in the style guideline.

> Source/WebCore/bindings/js/JSDOMBinding.h:801
> +template <typename VariadicHelper> typename VariadicHelper::Result toArguments(JSC::ExecState& state, size_t startIndex)

We don’t leave a space between "template" and "<", although we don’t use the style consistently. Probably need to mention it in the style guideline.

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:4582
> +sub GetVariadicType
> +{
> +    my ($interface, $type) = @_;
> +
> +    my $wrappedType = IsNativeType($type) ? GetNativeType($interface, $type) : $type;
> +    my $wrapperType = IsNativeType($type) ? "JSC::JSValue" : "JS$wrappedType";
> +
> +    return ($wrapperType, $wrappedType);
> +}

Really unfortunate that we have to add this helper. We are trying to do these things in the C++ code whenever possible rather than in the code generator. But I don’t have a good idea for how to avoid this.

[Nael Ouedraogo]

Created attachment 288122 [details]
Patch

[Nael Ouedraogo]

Code style errors have been fixed in uploaded patch.

> > Source/WebCore/bindings/js/JSDOMBinding.h:353
> > +    using Result = typename std::pair<size_t, Optional<Container>>;
> 
> Hard to read code that uses pair like this, since the names "first" and
> "second" aren’t so great. I suppose it wouldn’t be any better with
> std::tuple, but it could be better if we just used a struct instead because
> we could then name the members. We made this change a while for
> HashMap::AddResult and for HashMap::KeyValuePairType; it was a lot more
> readable to use a struct, and in modern C++ where we are doing anything
> generic with the pair, the code generated is nearly identical.
> 
Ok, I will fix this in a follow up bug.


Thanks for the review.

[WebKit Commit Bot]

Comment on attachment 288122 [details]
Patch

Clearing flags on attachment: 288122

Committed r205542: <http://trac.webkit.org/changeset/205542>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.