Bug 157163 – First step in using "enum class" instead of "String" for enumerations in DOM

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

[Darin Adler]

First step in using "enum class" instead of "String" for enumerations in DOM

[Darin Adler]

Created attachment 277681 [details]
Patch

[Darin Adler]

Chris, my next patch also deals with some cases of optional enum values too. Started working on it and will rebase once this is landed.

[Chris Dumez]

Comment on attachment 277681 [details]
Patch

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

r=me with a few comments.

> Source/WebCore/bindings/scripts/CodeGenerator.pm:51
> +    "int" => 1,

Hmm. There is not such thing in Web IDL.

> Source/WebCore/bindings/scripts/CodeGenerator.pm:58
> +    "unsigned int" => 1,

Hmm. There is not such thing in Web IDL.

> Source/WebCore/bindings/scripts/CodeGenerator.pm:66
> +my %stringTypeHash = ( "DOMString" => 1 );

Seems like we may want to get rid of this hash at some point.

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:834
> +sub EnumerationClassName {

I think we usually use "Get" prefix for such routines in the bindings generator.

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:839
> +sub EnumerationValueName {

I think we usually use "Get" prefix for such routines in the bindings generator.

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:845
> +sub EnumerationImplementationContent

I think we usually use "Get" prefix for such routines in the bindings generator.

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:863
> +        $result .= "    static NeverDestroyed<String> values[] = {\n";

Just to confirm, is this really meant to be an array of NeverDestroyed<String> or a NeverDestroyed of an array of Strings (i.e. NeverDestroyed<String[]>) ?

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:874
> +                # Keep the style checker happy. Not sure I still love this style guideline.

I don't think we usually worry about style of the generated bindings. I would have kept the generator code simpler and ignored the style checker warning but it's your call.

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:879
> +        $result .= "    ASSERT(static_cast<size_t>(enumerationValue) < WTF_ARRAY_LENGTH(values));\n";

Should this be an ASSERT_WITH_SECURITY_IMPLICATIONS?

> Source/WebCore/bindings/scripts/test/JS/JSTestObj.cpp:97
> +    static NeverDestroyed<String> values[] = {

Shouldn't this use const String?

> Source/WebCore/bindings/scripts/test/JS/JSTestObj.cpp:2294
> +    TestEnumType nativeValue = enumerationValueTestEnumType(value.toWTFString(state)).value();

This seems wrong since the function returns an Optional<TestEnumType> but I am guessing you are planning to address this in a follow up?

> Source/WebCore/bindings/scripts/test/JS/JSTestObj.cpp:2297
>      if (nativeValue != "" && nativeValue != "EnumValue1" && nativeValue != "EnumValue2" && nativeValue != "EnumValue3")

Well, this is wrong too :)

> Source/WebCore/bindings/scripts/test/JS/JSTestObj.cpp:3361
> +    Optional nativeValue = enumerationValueOptional(value.toWTFString(state)).value();

Optional?

[Chris Dumez]

FYI, the expected Web IDL [1] behavior when the JS is passing a string that is not a valid enum value is:
* For non-readonly attribute assignment:
-> Ignore the assignment, do not throw

* For operation parameters:
-> Throw a TypeError

I believe our bindings already behave correctly.

[1] http://heycam.github.io/webidl/#dfn-enumeration-value

[Darin Adler]

Comment on attachment 277681 [details]
Patch

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

>> Source/WebCore/bindings/scripts/CodeGenerator.pm:51
>> +    "int" => 1,
> 
> Hmm. There is not such thing in Web IDL.

Yes, I plan to delete "int" and "unsigned int" after changing the small number of uses to "long" and "unsigned long".

>> Source/WebCore/bindings/scripts/CodeGenerator.pm:66
>> +my %stringTypeHash = ( "DOMString" => 1 );
> 
> Seems like we may want to get rid of this hash at some point.

I thought the same thing.

>> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:863
>> +        $result .= "    static NeverDestroyed<String> values[] = {\n";
> 
> Just to confirm, is this really meant to be an array of NeverDestroyed<String> or a NeverDestroyed of an array of Strings (i.e. NeverDestroyed<String[]>) ?

It’s an array of NeverDestroyed<String>, but that amounts to the same thing. Since the strings have no destructors, the array doesn’t have one either. I think either would work OK.

I probably would have made the entire array NeverDestroyed, but I don’t know how, and since this should work too, then I’m happy.

>> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:874
>> +                # Keep the style checker happy. Not sure I still love this style guideline.
> 
> I don't think we usually worry about style of the generated bindings. I would have kept the generator code simpler and ignored the style checker warning but it's your call.

For now I’d prefer to to leave it like this, just because I like the "style" bubble to be green rather than explaining to myself why it’s not. I can take this out if I teach the style checker to not look at the files in WebCore/bindings/scripts/test, so I should probably come back and do that.

>> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:879
>> +        $result .= "    ASSERT(static_cast<size_t>(enumerationValue) < WTF_ARRAY_LENGTH(values));\n";
> 
> Should this be an ASSERT_WITH_SECURITY_IMPLICATIONS?

Not sure. I am not an expert on when to use that. For what it’s worth, it’s an unlikely assertion to fail; illegal enumeration values are possible in theory but there’s no obvious way they’d be generated.

>> Source/WebCore/bindings/scripts/test/JS/JSTestObj.cpp:97
>> +    static NeverDestroyed<String> values[] = {
> 
> Shouldn't this use const String?

Yes, it could use const. I think we could either make the entire array const or make the array value type be "const String". Not sure which is better.

>> Source/WebCore/bindings/scripts/test/JS/JSTestObj.cpp:2294
>> +    TestEnumType nativeValue = enumerationValueTestEnumType(value.toWTFString(state)).value();
> 
> This seems wrong since the function returns an Optional<TestEnumType> but I am guessing you are planning to address this in a follow up?

Yes; I have already addressed it in my next patch.

>> Source/WebCore/bindings/scripts/test/JS/JSTestObj.cpp:2297
>>      if (nativeValue != "" && nativeValue != "EnumValue1" && nativeValue != "EnumValue2" && nativeValue != "EnumValue3")
> 
> Well, this is wrong too :)

Exactly, agreed. Fix coming soon.

>> Source/WebCore/bindings/scripts/test/JS/JSTestObj.cpp:3361
>> +    Optional nativeValue = enumerationValueOptional(value.toWTFString(state)).value();
> 
> Optional?

That’s the name of the test enum in TestObj.idl. I’d figure out how to deal with this if we actually needed an enum by this name, but we don’t yet, so I am not going to worry about it.

[Darin Adler]

Comment on attachment 277681 [details]
Patch

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

>> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:834
>> +sub EnumerationClassName {
> 
> I think we usually use "Get" prefix for such routines in the bindings generator.

Not sure I want to respect that, since it’s not how we do things elsewhere in WebKit. But I guess I will do it.

[Darin Adler]

(In reply to comment #4)
> FYI, the expected Web IDL [1] behavior when the JS is passing a string that
> is not a valid enum value is:
> * For non-readonly attribute assignment:
> -> Ignore the assignment, do not throw
> 
> * For operation parameters:
> -> Throw a TypeError
> 
> I believe our bindings already behave correctly.
> 
> [1] http://heycam.github.io/webidl/#dfn-enumeration-value

Yes, I noticed that when working on my next patch. I wrote a FIXME about it being wrong, and then looked it up and realized *I* was wrong.

[Darin Adler]

Committed r200288: <http://trac.webkit.org/changeset/200288>

[Csaba Osztrogonác]

(In reply to comment #8)
> Committed r200288: <http://trac.webkit.org/changeset/200288>

It broke the Apple Windows build, see build.webkit.org for details.

[Chris Dumez]

(In reply to comment #9)
> (In reply to comment #8)
> > Committed r200288: <http://trac.webkit.org/changeset/200288>
> 
> It broke the Apple Windows build, see build.webkit.org for details.

Landed a speculative build fix in <http://trac.webkit.org/changeset/200310>.

[Chris Dumez]

(In reply to comment #10)
> (In reply to comment #9)
> > (In reply to comment #8)
> > > Committed r200288: <http://trac.webkit.org/changeset/200288>
> > 
> > It broke the Apple Windows build, see build.webkit.org for details.
> 
> Landed a speculative build fix in <http://trac.webkit.org/changeset/200310>.

Fixed.

[Darin Adler]

Was the issue that Windows has a different version of perl?

[Chris Dumez]

(In reply to comment #12)
> Was the issue that Windows has a different version of perl?

Very likely since we were getting perl warnings on windows only.