Bug 182600 – [Web Animations] Always expose "composite" in output of getKeyframes()

• Component: Animations
• Status: RESOLVED FIXED

[Antoine Quint]

[Web Animations] Always expose "composite" in output of getKeyframes()

[Antoine Quint]

Created attachment 333365 [details]
Patch

[EWS Watchlist]

Attachment 333365 [details] did not pass style-queue:


ERROR: Source/WebCore/animation/KeyframeEffect.cpp:454:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Total errors found: 1 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Chris Dumez]

Comment on attachment 333365 [details]
Patch

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

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2191
> +        return "std::nullopt" if $defaultValue eq "null";

and $type->isNullable ?

If the enum type is not nullable, then null should not be a valid default value. Maybe we could "die" if the default value is null and the enum type is not nullable.

Bindings change LGTM otherwise, although it'd be nice to have bindings test coverage. It is hard to review a bindings change without such coverage as I do not see the code it generates.

[Antoine Quint]

(In reply to Chris Dumez from comment #3)
> Comment on attachment 333365 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=333365&action=review
> 
> > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2191
> > +        return "std::nullopt" if $defaultValue eq "null";
> 
> and $type->isNullable ?
> 
> If the enum type is not nullable, then null should not be a valid default
> value. Maybe we could "die" if the default value is null and the enum type
> is not nullable.

I'll add this.

> Bindings change LGTM otherwise, although it'd be nice to have bindings test
> coverage. It is hard to review a bindings change without such coverage as I
> do not see the code it generates.

Can you point me to a recent commit that adds a bindings test for "inspiration"?

[Chris Dumez]

B(In reply to Antoine Quint from comment #4)
> (In reply to Chris Dumez from comment #3)
> > Comment on attachment 333365 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=333365&action=review
> > 
> > > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2191
> > > +        return "std::nullopt" if $defaultValue eq "null";
> > 
> > and $type->isNullable ?
> > 
> > If the enum type is not nullable, then null should not be a valid default
> > value. Maybe we could "die" if the default value is null and the enum type
> > is not nullable.
> 
> I'll add this.
> 
> > Bindings change LGTM otherwise, although it'd be nice to have bindings test
> > coverage. It is hard to review a bindings change without such coverage as I
> > do not see the code it generates.
> 
> Can you point me to a recent commit that adds a bindings test for
> "inspiration"?

Basically just add new IDL to Source/WebCore/bindings/scripts/test/TestObj.idl , then run:
Tools/Scripts/run-bindings-tests --reset-results

[Antoine Quint]

Created attachment 333393 [details]
Patch

[EWS Watchlist]

Attachment 333393 [details] did not pass style-queue:


ERROR: Source/WebCore/animation/KeyframeEffect.cpp:454:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Total errors found: 1 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Chris Dumez]

Comment on attachment 333393 [details]
Patch

bindings LGTM

[Antoine Quint]

Committed r228312: <https://trac.webkit.org/changeset/228312>

[Radar WebKit Bug Importer]

<rdar://problem/37381726>