First step in using "enum class" instead of "String" for enumerations in DOM
Created attachment 277681 [details] Patch
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.
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?
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
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.
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.
(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.
Committed r200288: <http://trac.webkit.org/changeset/200288>
(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.
(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>.
(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.
Was the issue that Windows has a different version of perl?
(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.