Stop using old-style string-based enums in Internals.idl
Created attachment 277823 [details] Patch
Comment on attachment 277823 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=277823&action=review > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:-3897 > - if (!$function->signature->extendedAttributes->{"Custom"} && GetNativeType($function->signature->type) ne "bool") { How about this check? I don't think we support generating callbacks whose return value is not bool. That said, the compile assert is not great. Maybe this should be a "die" statement in the script. I think it is important to let the developer know they need to use [Custom] in this case. > Source/WebCore/testing/Internals.cpp:2945 > +void Internals::sendMediaControlEvent(MediControlEvent event) Typo: Media
Comment on attachment 277823 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=277823&action=review I’m also noticing that the builds seem to be failing on EWS. Will have to fix that before landing. >> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:-3897 >> - if (!$function->signature->extendedAttributes->{"Custom"} && GetNativeType($function->signature->type) ne "bool") { > > How about this check? I don't think we support generating callbacks whose return value is not bool. That said, the compile assert is not great. Maybe this should be a "die" statement in the script. I think it is important to let the developer know they need to use [Custom] in this case. I even considered changing this to a die. Here’s why I didn’t (although I am willing to do it): Generally our bindings script gets a lot of things wrong; it supports the cases we have tested, but it doesn’t exhaustively check for all the many cases it doesn’t create correct code for. Here was a check for a specific case, but written so poorly that clearly it never did anyone any good. I assume there would be some kind of compile error anyway when we try to generate a callback with a return value other than bool. I don’t think it would make incorrect code that would happen to compile but do the wrong thing. The reason to write the "die" is to make an easier to understand error message. In my defense, the COMPILE_ASSERT wasn’t being correctly generated so it was definitely not an easy to understand message before! I don’t think my change made it worse. Once the topic becomes making good error messages that are easy to address, I realize that I don’t know how easy it is to understand messages that come from die. How easy it is to spot them in the build log, what other stuff gets dumped along with them to help pinpoint the source line of the IDL file with the error, etc. >> Source/WebCore/testing/Internals.cpp:2945 >> +void Internals::sendMediaControlEvent(MediControlEvent event) > > Typo: Media Oops.
Looks like I will have to implement [Conditional] for enumerations to fix the build.
Committed r200294: <http://trac.webkit.org/changeset/200294>
(In reply to comment #5) > Committed r200294: <http://trac.webkit.org/changeset/200294> Looks like it broke the GTK build: https://build.webkit.org/builders/GTK%20Linux%2032-bit%20Release/builds/60996/steps/compile-webkit/logs/stdio
Oh, I know how to fix that.