Code generation unconditionally adds supplemental interfaces to their parents even if the conditional attribute is not enabled/defined. I noticed this behaviour when building GTK port without geolocation enabled. The interface specified in NavigatorGeolocation.idl is always added to the Navigator interface (although the implementation is properly surrounded by guards).
Created attachment 128610 [details] Rebaseline bindings tests
Created attachment 128611 [details] Patch for the bug
Comment on attachment 128611 [details] Patch for the bug View in context: https://bugs.webkit.org/attachment.cgi?id=128611&action=review Thanks for working on the patch! > Source/WebCore/ChangeLog:19 > + * bindings/scripts/CodeGeneratorCPP.pm: > + (GenerateHeader): > + * bindings/scripts/CodeGeneratorJS.pm: > + (GenerateHeader): > + * bindings/scripts/CodeGeneratorObjC.pm: > + (GenerateHeader): > + * bindings/scripts/CodeGeneratorV8.pm: > + (GenerateHeader): Also we need to make similar changes in CodeGeneratorGObject.pm. > Source/WebCore/bindings/scripts/CodeGeneratorCPP.pm:497 > + my $conditionalString = GenerateConditionalString($function->signature); Maybe you need to support the conditional block for attributes in CPP? > Source/WebCore/bindings/scripts/CodeGeneratorObjC.pm:923 > + my $functionConditionalString = GenerateConditionalString($function->signature); Maybe you need to support the conditional block for attributes in ObjC?
(In reply to comment #3) > (From update of attachment 128611 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=128611&action=review > > Thanks for working on the patch! > > > Source/WebCore/ChangeLog:19 > > + * bindings/scripts/CodeGeneratorCPP.pm: > > + (GenerateHeader): > > + * bindings/scripts/CodeGeneratorJS.pm: > > + (GenerateHeader): > > + * bindings/scripts/CodeGeneratorObjC.pm: > > + (GenerateHeader): > > + * bindings/scripts/CodeGeneratorV8.pm: > > + (GenerateHeader): > > Also we need to make similar changes in CodeGeneratorGObject.pm. In the GObject bindings the conditional is inside the implementation of the function (meaning the functions are declared but they don't do anything if the condition is not enabled), so the header declarations shouldn't be guarded. Or it should be changed to align with the rest of the generators, i guess. > > Source/WebCore/bindings/scripts/CodeGeneratorCPP.pm:497 > > + my $conditionalString = GenerateConditionalString($function->signature); > > Maybe you need to support the conditional block for attributes in CPP? > > > Source/WebCore/bindings/scripts/CodeGeneratorObjC.pm:923 > > + my $functionConditionalString = GenerateConditionalString($function->signature); > > Maybe you need to support the conditional block for attributes in ObjC? These two already guard the attributes in the header, they were just missing the other cases.
Comment on attachment 128611 [details] Patch for the bug Thank you very much for the clarification. Great patch!
Have in mind that the second patch depends on the rebaseline one, unless you're rebaselining the bindings tests manually yourself. Cheers!
(In reply to comment #6) > Have in mind that the second patch depends on the rebaseline one, unless you're rebaselining the bindings tests manually yourself. At the moment the run-bindings-tests results on the WebKit trunk appear correct, and thus I guess your patch will be applied successfully. (Maybe your working copy is a bit old?) Anyway, I'll rebaseline it if it causes inconsistency. Thanks.
(In reply to comment #4) > In the GObject bindings the conditional is inside the implementation of the function (meaning the functions are declared but they don't do anything if the condition is not enabled), so the header declarations shouldn't be guarded. Or it should be changed to align with the rest of the generators, i guess. Pablo then the original bug I reported is not fixed for the GTK port. As I said if you just guard the implementation the build will fail because the function declaration refers to non existent objects.
(In reply to comment #8) > (In reply to comment #4) > > In the GObject bindings the conditional is inside the implementation of the function (meaning the functions are declared but they don't do anything if the condition is not enabled), so the header declarations shouldn't be guarded. Or it should be changed to align with the rest of the generators, i guess. > > Pablo then the original bug I reported is not fixed for the GTK port. As I said if you just guard the implementation the build will fail because the function declaration refers to non existent objects. Ah, indeed, I'll make the gobject generator match the other ones then.
(In reply to comment #9) > (In reply to comment #8) > > (In reply to comment #4) > > > In the GObject bindings the conditional is inside the implementation of the function (meaning the functions are declared but they don't do anything if the condition is not enabled), so the header declarations shouldn't be guarded. Or it should be changed to align with the rest of the generators, i guess. > > > > Pablo then the original bug I reported is not fixed for the GTK port. As I said if you just guard the implementation the build will fail because the function declaration refers to non existent objects. > > Ah, indeed, I'll make the gobject generator match the other ones then. Great, thx for taking care. BTW I guess you should set cq- to prevent the previous patch from landing.
Created attachment 128763 [details] Patch
Comment on attachment 128763 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=128763&action=review It appears that your patch conflicts with the latest WebKit trunk (ews-bots look purple). Please rebase your patch with the latest WebKit trunk and make ews-bots green. > Source/WebCore/bindings/scripts/CodeGeneratorGObject.pm:-1018 > - if ($returnType ne "void") { > - push(@cBody, "#else\n"); > - if ($codeGenerator->IsNonPointerType($functionSigType)) { > - push(@cBody, " return static_cast<${returnType}>(0);\n"); > - } else { > - push(@cBody, " return NULL;\n"); > - } > - } Is it OK to remove this part?
(In reply to comment #12) > (From update of attachment 128763 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=128763&action=review > > Source/WebCore/bindings/scripts/CodeGeneratorGObject.pm:-1018 > > - if ($returnType ne "void") { > > - push(@cBody, "#else\n"); > > - if ($codeGenerator->IsNonPointerType($functionSigType)) { > > - push(@cBody, " return static_cast<${returnType}>(0);\n"); > > - } else { > > - push(@cBody, " return NULL;\n"); > > - } > > - } > > Is it OK to remove this part? I think so, since it was ifdeffing the inside of the function it needed to provide an empty return value for functions that declared one. Now the whole function is ifdeffed so the problem goes away.
Comment on attachment 128763 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=128763&action=review Please just re-upload a patch after rebasing your patch with the latest WebKit trunk. Let me cq+ it after seeing green faces of ews-bots. >>> Source/WebCore/bindings/scripts/CodeGeneratorGObject.pm:-1018 >>> - } >> >> Is it OK to remove this part? > > I think so, since it was ifdeffing the inside of the function it needed to provide an empty return value for functions that declared one. Now the whole function is ifdeffed so the problem goes away. Thanks for the clarification.
Created attachment 128824 [details] Rebased patch
Comment on attachment 128824 [details] Rebased patch Attachment 128824 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/11631190
Comment on attachment 128824 [details] Rebased patch Attachment 128824 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/11628253
gtk build failed complaining that ENABLE is not defined in the headers, i guess config.h should be included somewhere. Sergio, any suggestions on how to fix this nicely? I'm thoroughly unfamiliar with gtk.
(In reply to comment #18) > gtk build failed complaining that ENABLE is not defined in the headers, i guess config.h should be included somewhere. Yeah, I guess you just need to add the following line to WriteData() in CodeGeneratorGObject.pm: print HEADER "#include \"config.h\"\n";
(In reply to comment #19) > (In reply to comment #18) > > gtk build failed complaining that ENABLE is not defined in the headers, i guess config.h should be included somewhere. > > Yeah, I guess you just need to add the following line to WriteData() in CodeGeneratorGObject.pm: > > print HEADER "#include \"config.h\"\n"; Yeah, i tried that but webkit-check-style balked: "Header file should not contain WebCore config.h." Should i ignore the warning?
(In reply to comment #20) > > > gtk build failed complaining that ENABLE is not defined in the headers, i guess config.h should be included somewhere. > > > > Yeah, I guess you just need to add the following line to WriteData() in CodeGeneratorGObject.pm: > > > > print HEADER "#include \"config.h\"\n"; > > Yeah, i tried that but webkit-check-style balked: "Header file should not contain WebCore config.h." Basically, we do not need to include config.h into a header file, since config.h should be included before the header file is included. For example, WebKitDOMMouseEvent.cpp includes WebKitDOMMouseEvent.h after including config.h, like this: #include "config.h" ... #include "webkit/WebKitDOMMouseEvent.h" So we do not need to include config.h into WebKitDOMMouseEvent.h. However, the problem happens when WebKitDOMMouseEvent.h is included before config.h is included. This should not happen, but it is actually happening (at least) in webkitdom.h. I guess that this is the cause of the build failure. If so, you can fix WebCore/bindings/scripts/gobject-generate-headers.pl so that webkitdom.h includes config.h. This will cause that warning again, but I guess we can ignore it.
Created attachment 129077 [details] Possibly fix gtk build
Created attachment 129091 [details] Possibly fix gtk build - rebased
Comment on attachment 129091 [details] Possibly fix gtk build - rebased Attachment 129091 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/11645167
(In reply to comment #24) > (From update of attachment 129091 [details]) > Attachment 129091 [details] did not pass gtk-ews (gtk): > Output: http://queues.webkit.org/results/11645167 That didn't work, any other ideas? There's always #if defined(ENABLE_BLAH) && ENABLE_BLAH if everything fails...
(In reply to comment #21) > (In reply to comment #20) > > > > gtk build failed complaining that ENABLE is not defined in the headers, i guess config.h should be included somewhere. > > > > > > Yeah, I guess you just need to add the following line to WriteData() in CodeGeneratorGObject.pm: > > > > > > print HEADER "#include \"config.h\"\n"; > > > > Yeah, i tried that but webkit-check-style balked: "Header file should not contain WebCore config.h." > > Basically, we do not need to include config.h into a header file, since config.h should be included before the header file is included. For example, WebKitDOMMouseEvent.cpp includes WebKitDOMMouseEvent.h after including config.h, like this: > > #include "config.h" > ... > #include "webkit/WebKitDOMMouseEvent.h" > > So we do not need to include config.h into WebKitDOMMouseEvent.h. > > However, the problem happens when WebKitDOMMouseEvent.h is included before config.h is included. This should not happen, but it is actually happening (at least) in webkitdom.h. I guess that this is the cause of the build failure. > > If so, you can fix WebCore/bindings/scripts/gobject-generate-headers.pl so that webkitdom.h includes config.h. This will cause that warning again, but I guess we can ignore it. webkitdom.h is part of the public API, so it must not include WebCore's config.h, because that header simply won't be present. IMO the solution to this is to simply not generate functions that are under conditional attribute.
(In reply to comment #26) > > IMO the solution to this is to simply not generate functions that are under conditional attribute. ... for which the feature flag is not present, of course.
(In reply to comment #26) > webkitdom.h is part of the public API, so it must not include WebCore's config.h, because that header simply won't be present. OK. Thanks. > IMO the solution to this is to simply not generate functions that are under conditional attribute. I do not think the approach is good. Current code generators are working without any knowledge about what flags are enabled/disabled. Maybe can we include config.h into the files that are including wekitdom.h? But anyway as far as I see the comment #25, it appears that the problem is not only webkitdom.h. We need to first clarify where config.h should be included.
(In reply to comment #28) > (In reply to comment #26) > > webkitdom.h is part of the public API, so it must not include WebCore's config.h, because that header simply won't be present. > > OK. Thanks. > > > IMO the solution to this is to simply not generate functions that are under conditional attribute. > > I do not think the approach is good. Current code generators are working without any knowledge about what flags are enabled/disabled. > > Maybe can we include config.h into the files that are including wekitdom.h? > > But anyway as far as I see the comment #25, it appears that the problem is not only webkitdom.h. We need to first clarify where config.h should be included. ENABLE macros aren't used anywhere in Gtk's public-facing API because these macros' logic is not being exported in API. I think this should and will stay the case, meaning DOM bindings public-facing API (or rather headers) should not contain ENABLE macros as well. Because of that including config.h in files that include webkitdom.h is not a viable solution.
(In reply to comment #18) > gtk build failed complaining that ENABLE is not defined in the headers, i guess config.h should be included somewhere. > > Sergio, any suggestions on how to fix this nicely? I'm thoroughly unfamiliar with gtk. Pablo why the other ports do not have this problem? How do they solve this?
One workaround is to generate the WebKitDOMGeolocation typedef in webkitdomdefines.h unconditionally and to not include WebKitDOMGeolocation* headers in WebKitDOMNavigator.cpp (or putting that inside #ifdef - it is possible here). The build then succeeds. The problem is that currently the webkitdomdefines.h header is generated with a class list of only classes that are added to the list of built sources, which does not inlude geolocation if the geolocation feature is not enabled.
(In reply to comment #28) > (In reply to comment #26) > > IMO the solution to this is to simply not generate functions that are under conditional attribute. > > I do not think the approach is good. Current code generators are working without any knowledge about what flags are enabled/disabled. > After some thoughts on this, I believe not generating the function would be the best. If the future is not present, there's absolutely no reason why a function that is conditioned with the presence of that future should be available in public-facing API. Currently it's as if the geolocation attribute on navigator would be always present but would be null if the feature was not enabled at compile time. While this can be completely avoided in JS bindings by just putting an #ifdef around the attribute definition, it's not possible with the GObject bindings because, reiterating, ENABLE macros are not usable in public headers.
(In reply to comment #32) > (In reply to comment #28) > > (In reply to comment #26) > > > IMO the solution to this is to simply not generate functions that are under conditional attribute. > > > > I do not think the approach is good. Current code generators are working without any knowledge about what flags are enabled/disabled. > > > > After some thoughts on this, I believe not generating the function would be the best. > > If the future is not present, there's absolutely no reason why a function that is conditioned with the presence of that future should be available in public-facing API. Currently it's as if the geolocation attribute on navigator would be always present but would be null if the feature was not enabled at compile time. > > While this can be completely avoided in JS bindings by just putting an #ifdef around the attribute definition, it's not possible with the GObject bindings because, reiterating, ENABLE macros are not usable in public headers. As it stands all those functions are declared in the public API, without any flags, i don't see why it would be any worse to have them ifdeffed out (in the sense that guarding is trivial in comparison to the changes that would be required to make the generators aware of which features are enabled/disabled). If including config.h somewhere along the way is not feasible or practical, maybe we can do an exception for GObject and just check #if defined(ENABLE_XXXX) && ENABLE_XXXX directly or define just the ENABLE macro somewhere appropriate?
(In reply to comment #33) > If including config.h somewhere along the way is not feasible or practical, maybe we can do an exception for GObject and just check #if defined(ENABLE_XXXX) && ENABLE_XXXX directly or define just the ENABLE macro somewhere appropriate? One important consideration, which I hope does not complicate the matter terribly, is that the API should change as little as possible with different configuration flags. For instance, some distributions may compile WebKit with --disable-geolcation. Ideally, the GObject binding API should be the same in either case, even if the geolocation methods are stubs when disabled. The reason for this is that downstream applications need to be able to rely on certain API points being present to use them effectively.
(In reply to comment #33) > (In reply to comment #32) > > (In reply to comment #28) > > > (In reply to comment #26) > > > > IMO the solution to this is to simply not generate functions that are under conditional attribute. > > > > > > I do not think the approach is good. Current code generators are working without any knowledge about what flags are enabled/disabled. > > > > > > > After some thoughts on this, I believe not generating the function would be the best. > > > > If the future is not present, there's absolutely no reason why a function that is conditioned with the presence of that future should be available in public-facing API. Currently it's as if the geolocation attribute on navigator would be always present but would be null if the feature was not enabled at compile time. > > > > While this can be completely avoided in JS bindings by just putting an #ifdef around the attribute definition, it's not possible with the GObject bindings because, reiterating, ENABLE macros are not usable in public headers. > > As it stands all those functions are declared in the public API, without any flags, i don't see why it would be any worse to have them ifdeffed out (in the sense that guarding is trivial in comparison to the changes that would be required to make the generators aware of which features are enabled/disabled). > #ifdeffing functions out would not be any worse, the problem is that is is currently not possible. If we went down that way, I guess new API would have to be developed for that. > If including config.h somewhere along the way is not feasible or practical, maybe we can do an exception for GObject and just check #if defined(ENABLE_XXXX) && ENABLE_XXXX directly or define just the ENABLE macro somewhere appropriate? Let's keep that as a last resort. (In reply to comment #34) > (In reply to comment #33) > > > If including config.h somewhere along the way is not feasible or practical, maybe we can do an exception for GObject and just check #if defined(ENABLE_XXXX) && ENABLE_XXXX directly or define just the ENABLE macro somewhere appropriate? > > One important consideration, which I hope does not complicate the matter terribly, is that the API should change as little as possible with different configuration flags. For instance, some distributions may compile WebKit with --disable-geolcation. Ideally, the GObject binding API should be the same in either case, even if the geolocation methods are stubs when disabled. The reason for this is that downstream applications need to be able to rely on certain API points being present to use them effectively. One way of doing this would be to generate all the bindings API and use stubs for functions under features that are not enabled. Perhaps throwing in a g_warning, explaining that the function is useless because of the future not being present.
(In reply to comment #35) > (In reply to comment #34) > > (In reply to comment #33) > > One important consideration, which I hope does not complicate the matter terribly, is that the API should change as little as possible with different configuration flags. For instance, some distributions may compile WebKit with --disable-geolcation. Ideally, the GObject binding API should be the same in either case, even if the geolocation methods are stubs when disabled. The reason for this is that downstream applications need to be able to rely on certain API points being present to use them effectively. > > One way of doing this would be to generate all the bindings API and use stubs for functions under features that are not enabled. Perhaps throwing in a g_warning, explaining that the function is useless because of the future not being present. Isn't that going back to the original problem? (i.e. comment #8, copied below): > > In the GObject bindings the conditional is inside the implementation of the function (meaning the functions are declared but they don't do anything if the condition is not enabled), so the header declarations shouldn't be guarded. Or it should be changed to align with the rest of the generators, i guess. > > Pablo then the original bug I reported is not fixed for the GTK port. As I said if you just guard the implementation the build will fail because the function declaration refers to non existent objects.
Created attachment 129620 [details] Ignore
Comment on attachment 129620 [details] Ignore Patch for wrong bug, ignore.
(In reply to comment #36) > (In reply to comment #35) > > (In reply to comment #34) > > > (In reply to comment #33) > > > One important consideration, which I hope does not complicate the matter terribly, is that the API should change as little as possible with different configuration flags. For instance, some distributions may compile WebKit with --disable-geolcation. Ideally, the GObject binding API should be the same in either case, even if the geolocation methods are stubs when disabled. The reason for this is that downstream applications need to be able to rely on certain API points being present to use them effectively. > > > > One way of doing this would be to generate all the bindings API and use stubs for functions under features that are not enabled. Perhaps throwing in a g_warning, explaining that the function is useless because of the future not being present. > > Isn't that going back to the original problem? (i.e. comment #8, copied below): > I imagine that when using stubs, the GObject interfaces would stay the same regardless of the future being enabled in WebCore or not, but, when the future is not enabled, the functions would simply not interact with WebCore but rather throw a warning that nothing is being done. I'd recommend creating a new bug for the troubles the GObject bindings are facing rather than blocking fix for all the other bindings.
(In reply to comment #39) > I'd recommend creating a new bug for the troubles the GObject bindings are facing rather than blocking fix for all the other bindings. +1. If you want, you can commit the change for other code generators first.
(In reply to comment #34) > (In reply to comment #33) > > > If including config.h somewhere along the way is not feasible or practical, maybe we can do an exception for GObject and just check #if defined(ENABLE_XXXX) && ENABLE_XXXX directly or define just the ENABLE macro somewhere appropriate? > > One important consideration, which I hope does not complicate the matter terribly, is that the API should change as little as possible with different configuration flags. For instance, some distributions may compile WebKit with --disable-geolcation. Ideally, the GObject binding API should be the same in either case, even if the geolocation methods are stubs when disabled. The reason for this is that downstream applications need to be able to rely on certain API points being present to use them effectively. Thing is, as I said in comment #8, that those API might have references to objects that are not generated. That's the case of geolocation for example. Maybe we could create empty objects but I'm sure that people will start to complain about non working stuff.
(In reply to comment #40) > (In reply to comment #39) > > I'd recommend creating a new bug for the troubles the GObject bindings are facing rather than blocking fix for all the other bindings. > > +1. If you want, you can commit the change for other code generators first. I've created bug #80030 and cc-ed everyone that commented in this bug regarding the problem. Unfortunately I can't commit the partial patch since I don't have commit access, so someone else should take that up. (In reply to comment #41) > (In reply to comment #34) > > (In reply to comment #33) > > > > > If including config.h somewhere along the way is not feasible or practical, maybe we can do an exception for GObject and just check #if defined(ENABLE_XXXX) && ENABLE_XXXX directly or define just the ENABLE macro somewhere appropriate? > > > > One important consideration, which I hope does not complicate the matter terribly, is that the API should change as little as possible with different configuration flags. For instance, some distributions may compile WebKit with --disable-geolcation. Ideally, the GObject binding API should be the same in either case, even if the geolocation methods are stubs when disabled. The reason for this is that downstream applications need to be able to rely on certain API points being present to use them effectively. > > Thing is, as I said in comment #8, that those API might have references to objects that are not generated. That's the case of geolocation for example. Maybe we could create empty objects but I'm sure that people will start to complain about non working stuff. I'll reply to this in the new bug.
Created attachment 129729 [details] Patch minus the gobject stuff
Comment on attachment 129729 [details] Patch minus the gobject stuff Clearing flags on attachment: 129729 Committed r109515: <http://trac.webkit.org/changeset/109515>
All reviewed patches have been landed. Closing bug.