Bug 79375 – Supplemental interfaces are not disabled with the "Conditional" attribute

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Sergio Villar Senin]

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).

[Pablo Flouret]

Created attachment 128610 [details]
Rebaseline bindings tests

[Pablo Flouret]

Created attachment 128611 [details]
Patch for the bug

[Kentaro Hara]

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?

[Pablo Flouret]

(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.

[Kentaro Hara]

Comment on attachment 128611 [details]
Patch for the bug

Thank you very much for the clarification. Great patch!

[Pablo Flouret]

Have in mind that the second patch depends on the rebaseline one, unless you're rebaselining the bindings tests manually yourself.

Cheers!

[Kentaro Hara]

(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.

[Sergio Villar Senin]

(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.

[Pablo Flouret]

(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.

[Sergio Villar Senin]

(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.

[Pablo Flouret]

Created attachment 128763 [details]
Patch

[Kentaro Hara]

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?

[Pablo Flouret]

(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.

[Kentaro Hara]

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.

[Pablo Flouret]

Created attachment 128824 [details]
Rebased patch

[Gustavo Noronha (kov)]

Comment on attachment 128824 [details]
Rebased patch

Attachment 128824 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11631190

[Collabora GTK+ EWS bot]

Comment on attachment 128824 [details]
Rebased patch

Attachment 128824 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11628253

[Pablo Flouret]

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.

[Kentaro Hara]

(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";

[Pablo Flouret]

(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?

[Kentaro Hara]

(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.

[Pablo Flouret]

Created attachment 129077 [details]
Possibly fix gtk build

[Pablo Flouret]

Created attachment 129091 [details]
Possibly fix gtk build - rebased

[Gustavo Noronha (kov)]

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

[Pablo Flouret]

(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...

[Zan Dobersek]

(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.

[Zan Dobersek]

(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.

[Kentaro Hara]

(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.

[Zan Dobersek]

(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.

[Sergio Villar Senin]

(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?

[Zan Dobersek]

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.

[Zan Dobersek]

(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.

[Pablo Flouret]

(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?

[Martin Robinson]

(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.

[Zan Dobersek]

(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.

[Pablo Flouret]

(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.

[Pablo Flouret]

Created attachment 129620 [details]
Ignore

[Pablo Flouret]

Comment on attachment 129620 [details]
Ignore

Patch for wrong bug, ignore.

[Zan Dobersek]

(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.

[Kentaro Hara]

(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.

[Sergio Villar Senin]

(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.

[Zan Dobersek]

(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.

[Pablo Flouret]

Created attachment 129729 [details]
Patch minus the gobject stuff

[WebKit Review Bot]

Comment on attachment 129729 [details]
Patch minus the gobject stuff

Clearing flags on attachment: 129729

Committed r109515: <http://trac.webkit.org/changeset/109515>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.