Bug 79375

Summary: Supplemental interfaces are not disabled with the "Conditional" attribute
Product: WebKit Reporter: Sergio Villar Senin <svillar>
Component: WebCore Misc.Assignee: Pablo Flouret <pf>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, cgarcia, donggwan.kim, gns, gustavo.noronha, haraken, japhet, mrobinson, pf, svillar, webkit.review.bot, xan.lopez, zan
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Rebaseline bindings tests
none
Patch for the bug
none
Patch
none
Rebased patch
none
Possibly fix gtk build
none
Possibly fix gtk build - rebased
none
Ignore
none
Patch minus the gobject stuff none

Description Sergio Villar Senin 2012-02-23 10:17:48 PST
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).
Comment 1 Pablo Flouret 2012-02-23 17:09:19 PST
Created attachment 128610 [details]
Rebaseline bindings tests
Comment 2 Pablo Flouret 2012-02-23 17:09:26 PST
Created attachment 128611 [details]
Patch for the bug
Comment 3 Kentaro Hara 2012-02-23 17:17:23 PST
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?
Comment 4 Pablo Flouret 2012-02-23 17:26:42 PST
(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 5 Kentaro Hara 2012-02-23 17:29:05 PST
Comment on attachment 128611 [details]
Patch for the bug

Thank you very much for the clarification. Great patch!
Comment 6 Pablo Flouret 2012-02-23 17:31:25 PST
Have in mind that the second patch depends on the rebaseline one, unless you're rebaselining the bindings tests manually yourself.

Cheers!
Comment 7 Kentaro Hara 2012-02-23 17:38:47 PST
(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.
Comment 8 Sergio Villar Senin 2012-02-23 23:49:30 PST
(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.
Comment 9 Pablo Flouret 2012-02-24 00:06:54 PST
(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.
Comment 10 Sergio Villar Senin 2012-02-24 00:14:46 PST
(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.
Comment 11 Pablo Flouret 2012-02-24 11:00:58 PST
Created attachment 128763 [details]
Patch
Comment 12 Kentaro Hara 2012-02-24 15:12:20 PST
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?
Comment 13 Pablo Flouret 2012-02-24 15:15:56 PST
(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 14 Kentaro Hara 2012-02-24 15:27:00 PST
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.
Comment 15 Pablo Flouret 2012-02-24 15:59:57 PST
Created attachment 128824 [details]
Rebased patch
Comment 16 Gustavo Noronha (kov) 2012-02-24 17:28:16 PST
Comment on attachment 128824 [details]
Rebased patch

Attachment 128824 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11631190
Comment 17 Collabora GTK+ EWS bot 2012-02-24 18:49:21 PST
Comment on attachment 128824 [details]
Rebased patch

Attachment 128824 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11628253
Comment 18 Pablo Flouret 2012-02-24 19:28:07 PST
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.
Comment 19 Kentaro Hara 2012-02-24 20:25:02 PST
(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";
Comment 20 Pablo Flouret 2012-02-24 20:31:36 PST
(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?
Comment 21 Kentaro Hara 2012-02-24 21:20:16 PST
(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.
Comment 22 Pablo Flouret 2012-02-27 11:35:19 PST
Created attachment 129077 [details]
Possibly fix gtk build
Comment 23 Pablo Flouret 2012-02-27 13:30:39 PST
Created attachment 129091 [details]
Possibly fix gtk build - rebased
Comment 24 Gustavo Noronha (kov) 2012-02-27 13:57:29 PST
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
Comment 25 Pablo Flouret 2012-02-27 14:44:08 PST
(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...
Comment 26 Zan Dobersek 2012-02-29 03:24:41 PST
(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.
Comment 27 Zan Dobersek 2012-02-29 03:48:08 PST
(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.
Comment 28 Kentaro Hara 2012-02-29 03:58:29 PST
(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.
Comment 29 Zan Dobersek 2012-02-29 08:04:34 PST
(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.
Comment 30 Sergio Villar Senin 2012-02-29 08:10:38 PST
(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?
Comment 31 Zan Dobersek 2012-02-29 09:14:15 PST
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.
Comment 32 Zan Dobersek 2012-02-29 10:24:07 PST
(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.
Comment 33 Pablo Flouret 2012-02-29 12:21:25 PST
(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?
Comment 34 Martin Robinson 2012-02-29 12:26:33 PST
(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.
Comment 35 Zan Dobersek 2012-02-29 12:32:24 PST
(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.
Comment 36 Pablo Flouret 2012-02-29 15:24:17 PST
(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.
Comment 37 Pablo Flouret 2012-02-29 20:10:19 PST
Created attachment 129620 [details]
Ignore
Comment 38 Pablo Flouret 2012-02-29 20:11:16 PST
Comment on attachment 129620 [details]
Ignore

Patch for wrong bug, ignore.
Comment 39 Zan Dobersek 2012-03-01 02:07:02 PST
(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.
Comment 40 Kentaro Hara 2012-03-01 02:33:44 PST
(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.
Comment 41 Sergio Villar Senin 2012-03-01 02:57:46 PST
(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.
Comment 42 Zan Dobersek 2012-03-01 08:47:58 PST
(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.
Comment 43 Pablo Flouret 2012-03-01 11:56:36 PST
Created attachment 129729 [details]
Patch minus the gobject stuff
Comment 44 WebKit Review Bot 2012-03-01 22:50:27 PST
Comment on attachment 129729 [details]
Patch minus the gobject stuff

Clearing flags on attachment: 129729

Committed r109515: <http://trac.webkit.org/changeset/109515>
Comment 45 WebKit Review Bot 2012-03-01 22:50:35 PST
All reviewed patches have been landed.  Closing bug.