Bug 161793 – JSDOMBindings' toArguments() should return a more descriptive object

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

[Nael Ouedraogo]

JSDOMBindings' toArguments() returns a std::pair of argumentIndex and Optional. In JS bindings code generator, use of ".first" and ".second" to access to members is hard to read. 

We should use a struct instead to be able to name the members.

[Nael Ouedraogo]

Created attachment 288400 [details]
Patch

[Chris Dumez]

Comment on attachment 288400 [details]
Patch

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

> Source/WebCore/bindings/js/JSDOMBinding.h:357
> +struct VariadicHelperResult {

Would something simpler work? E.g.

struct Result {
    size_t argumentIndex;
    Optional<Container> arguments;
}

Also, could we declare this struct inside the VariadicHelper struct?

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:-1107
> -    

In the future, please try to avoid this kind of unrelated spacing changes.

[Chris Dumez]

Comment on attachment 288400 [details]
Patch

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

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3993
> +            $value = "WTFMove($name.arguments.value())";

Why is this change needed? wouldn't this work?
$value = "WTFMove(*$name.arguments)";

[Nael Ouedraogo]

Created attachment 288681 [details]
Patch

[Nael Ouedraogo]

Thanks for the review.

> > Source/WebCore/bindings/js/JSDOMBinding.h:357
> > +struct VariadicHelperResult {
> 
> Would something simpler work? E.g.
> 
> struct Result {
>     size_t argumentIndex;
>     Optional<Container> arguments;
> }
> 
> Also, could we declare this struct inside the VariadicHelper struct?
Ok, done in the uploaded patch. 

> > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:-1107
> > -    
> 
> In the future, please try to avoid this kind of unrelated spacing changes.
Ok, I thought it was good to remove trailing whitespaces. 

> > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3993
> > +            $value = "WTFMove($name.arguments.value())";
> 
> Why is this change needed? wouldn't this work?
> $value = "WTFMove(*$name.arguments)";
Yes, it works also but it seems that value() is generally used in WebKit  instead of * with Optional value (c.f. Bug 158835#c14).

[WebKit Commit Bot]

Comment on attachment 288681 [details]
Patch

Clearing flags on attachment: 288681

Committed r206090: <http://trac.webkit.org/changeset/206090>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

Comment on attachment 288681 [details]
Patch

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

> Source/WebCore/bindings/js/JSDOMBinding.h:363
> +        Result(size_t argumentIndex, Optional<Container>&& arguments)
> +            : argumentIndex(argumentIndex)
> +            , arguments(WTFMove(arguments))
> +        {
> +        }

We should not include this constructor. Member-wise initialization should accomplish the same thing.

[Nael Ouedraogo]

(In reply to comment #8)
> Comment on attachment 288681 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=288681&action=review
> 
> > Source/WebCore/bindings/js/JSDOMBinding.h:363
> > +        Result(size_t argumentIndex, Optional<Container>&& arguments)
> > +            : argumentIndex(argumentIndex)
> > +            , arguments(WTFMove(arguments))
> > +        {
> > +        }
> 
> We should not include this constructor. Member-wise initialization should
> accomplish the same thing.

Ok, I filed bug 162298 for this.