Bug 157235 – Stop using old-style string-based enums in Internals.idl

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

[Darin Adler]

Stop using old-style string-based enums in Internals.idl

[Darin Adler]

Created attachment 277823 [details]
Patch

[Chris Dumez]

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

[Darin Adler]

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.

[Darin Adler]

Looks like I will have to implement [Conditional] for enumerations to fix the build.

[Darin Adler]

Committed r200294: <http://trac.webkit.org/changeset/200294>

[Chris Dumez]

(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

[Darin Adler]

Oh, I know how to fix that.