Bug 158830 – [WebIDL] Add support for Conditional extended attribute for implement statement.

• Component: Bindings
• Status: RESOLVED WONTFIX

[Rawinder Singh]

Bug #156096 relies on "Element implements Animatable" to be conditionally applied depending on whether ENABLE_WEB_ANIMATIONS is enabled.

This patch adds extra functionality to read extended attributes for the implements statement and processes the "Conditional" attribute checking the condition against the preprocessor definitions passed to the preprocess-idls.pl script.

See also http://www.w3.org/TR/WebIDL/#prod-ImplementsStatement.

[Rawinder Singh]

Created attachment 281444 [details]
Patch

[Chris Dumez]

Comment on attachment 281444 [details]
Patch

Why do we need this? Why can't we simply use [Conditional] on the interface that is on the right side of the 'implements' statement. This has been sufficient so far.

[Rawinder Singh]

(In reply to comment #2)
> Comment on attachment 281444 [details]
> Patch
> 
> Why do we need this? Why can't we simply use [Conditional] on the interface
> that is on the right side of the 'implements' statement. This has been
> sufficient so far.

See the "Build error" attachment for Bug #156096.

A quick overview:

- The Animatable interface does have Conditional=WEB_ANIMATIONS.
- In Element.idl, I defined Element implements Animatable

However, when building with the WEB_ANIMATIONS condition off, the following error is produced by preprocess-idls.pl:

Could not find a the IDL file where the following implemented interface is defined: Animatable at /home/mcatanzaro/src/WebKit/Source/WebCore/bindings/scripts/preprocess-idls.pl line 102.

[Chris Dumez]

(In reply to comment #3)
> (In reply to comment #2)
> > Comment on attachment 281444 [details]
> > Patch
> > 
> > Why do we need this? Why can't we simply use [Conditional] on the interface
> > that is on the right side of the 'implements' statement. This has been
> > sufficient so far.
> 
> See the "Build error" attachment for Bug #156096.
> 
> A quick overview:
> 
> - The Animatable interface does have Conditional=WEB_ANIMATIONS.
> - In Element.idl, I defined Element implements Animatable
> 
> However, when building with the WEB_ANIMATIONS condition off, the following
> error is produced by preprocess-idls.pl:
> 
> Could not find a the IDL file where the following implemented interface is
> defined: Animatable at
> /home/mcatanzaro/src/WebKit/Source/WebCore/bindings/scripts/preprocess-idls.
> pl line 102.

This is supposed to work and usually does AFAIK. I think you encountered a specific bug that needs fixing rather than adding support for [Conditional] on implements statements.

[Chris Dumez]

(In reply to comment #4)
> (In reply to comment #3)
> > (In reply to comment #2)
> > > Comment on attachment 281444 [details]
> > > Patch
> > > 
> > > Why do we need this? Why can't we simply use [Conditional] on the interface
> > > that is on the right side of the 'implements' statement. This has been
> > > sufficient so far.
> > 
> > See the "Build error" attachment for Bug #156096.
> > 
> > A quick overview:
> > 
> > - The Animatable interface does have Conditional=WEB_ANIMATIONS.
> > - In Element.idl, I defined Element implements Animatable
> > 
> > However, when building with the WEB_ANIMATIONS condition off, the following
> > error is produced by preprocess-idls.pl:
> > 
> > Could not find a the IDL file where the following implemented interface is
> > defined: Animatable at
> > /home/mcatanzaro/src/WebKit/Source/WebCore/bindings/scripts/preprocess-idls.
> > pl line 102.
> 
> This is supposed to work and usually does AFAIK. I think you encountered a
> specific bug that needs fixing rather than adding support for [Conditional]
> on implements statements.

The error you are giving seems to indicate that Animatable.idl was not passed to preprocess-idls.pl, which probably means Animatable.idl was not properly added to build fixes (e.g. CMakeList.txt).

[Chris Dumez]

(In reply to comment #5)
> (In reply to comment #4)
> > (In reply to comment #3)
> > > (In reply to comment #2)
> > > > Comment on attachment 281444 [details]
> > > > Patch
> > > > 
> > > > Why do we need this? Why can't we simply use [Conditional] on the interface
> > > > that is on the right side of the 'implements' statement. This has been
> > > > sufficient so far.
> > > 
> > > See the "Build error" attachment for Bug #156096.
> > > 
> > > A quick overview:
> > > 
> > > - The Animatable interface does have Conditional=WEB_ANIMATIONS.
> > > - In Element.idl, I defined Element implements Animatable
> > > 
> > > However, when building with the WEB_ANIMATIONS condition off, the following
> > > error is produced by preprocess-idls.pl:
> > > 
> > > Could not find a the IDL file where the following implemented interface is
> > > defined: Animatable at
> > > /home/mcatanzaro/src/WebKit/Source/WebCore/bindings/scripts/preprocess-idls.
> > > pl line 102.
> > 
> > This is supposed to work and usually does AFAIK. I think you encountered a
> > specific bug that needs fixing rather than adding support for [Conditional]
> > on implements statements.
> 
> The error you are giving seems to indicate that Animatable.idl was not
> passed to preprocess-idls.pl, which probably means Animatable.idl was not
> properly added to build fixes (e.g. CMakeList.txt).

s/fixes/files

[Rawinder Singh]

(In reply to comment #5)
> (In reply to comment #4)
> > (In reply to comment #3)
> > > (In reply to comment #2)
> > > > Comment on attachment 281444 [details]
> > > > Patch
> > > > 
> > > > Why do we need this? Why can't we simply use [Conditional] on the interface
> > > > that is on the right side of the 'implements' statement. This has been
> > > > sufficient so far.
> > > 
> > > See the "Build error" attachment for Bug #156096.
> > > 
> > > A quick overview:
> > > 
> > > - The Animatable interface does have Conditional=WEB_ANIMATIONS.
> > > - In Element.idl, I defined Element implements Animatable
> > > 
> > > However, when building with the WEB_ANIMATIONS condition off, the following
> > > error is produced by preprocess-idls.pl:
> > > 
> > > Could not find a the IDL file where the following implemented interface is
> > > defined: Animatable at
> > > /home/mcatanzaro/src/WebKit/Source/WebCore/bindings/scripts/preprocess-idls.
> > > pl line 102.
> > 
> > This is supposed to work and usually does AFAIK. I think you encountered a
> > specific bug that needs fixing rather than adding support for [Conditional]
> > on implements statements.
> 
> The error you are giving seems to indicate that Animatable.idl was not
> passed to preprocess-idls.pl, which probably means Animatable.idl was not
> properly added to build fixes (e.g. CMakeList.txt).

In Source/WebCore/CMakeLists.txt I only include Animatable.idl if ENABLE_WEB_ANIMATIONS is defined.  So, yes for the case that WEB_ANIMATIONS is disabled it will not be passed to preprocess-idls.pl (in which case this error occurs).

I understand that I could probably work around it and append Animatable.idl to WebCore_IDL_FILES in CMakeLists.txt even if WEB_ANIMATAIONS is disabled - but I feel like this solution is more consistent with the way conditions have been used in the WebKit IDL elsewhere (and also checking for [.*] before the implements statement is more correct in terms of Web IDL)

[Chris Dumez]

(In reply to comment #7)
> (In reply to comment #5)
> > (In reply to comment #4)
> > > (In reply to comment #3)
> > > > (In reply to comment #2)
> > > > > Comment on attachment 281444 [details]
> > > > > Patch
> > > > > 
> > > > > Why do we need this? Why can't we simply use [Conditional] on the interface
> > > > > that is on the right side of the 'implements' statement. This has been
> > > > > sufficient so far.
> > > > 
> > > > See the "Build error" attachment for Bug #156096.
> > > > 
> > > > A quick overview:
> > > > 
> > > > - The Animatable interface does have Conditional=WEB_ANIMATIONS.
> > > > - In Element.idl, I defined Element implements Animatable
> > > > 
> > > > However, when building with the WEB_ANIMATIONS condition off, the following
> > > > error is produced by preprocess-idls.pl:
> > > > 
> > > > Could not find a the IDL file where the following implemented interface is
> > > > defined: Animatable at
> > > > /home/mcatanzaro/src/WebKit/Source/WebCore/bindings/scripts/preprocess-idls.
> > > > pl line 102.
> > > 
> > > This is supposed to work and usually does AFAIK. I think you encountered a
> > > specific bug that needs fixing rather than adding support for [Conditional]
> > > on implements statements.
> > 
> > The error you are giving seems to indicate that Animatable.idl was not
> > passed to preprocess-idls.pl, which probably means Animatable.idl was not
> > properly added to build fixes (e.g. CMakeList.txt).
> 
> In Source/WebCore/CMakeLists.txt I only include Animatable.idl if
> ENABLE_WEB_ANIMATIONS is defined.  So, yes for the case that WEB_ANIMATIONS
> is disabled it will not be passed to preprocess-idls.pl (in which case this
> error occurs).
> 
> I understand that I could probably work around it and append Animatable.idl
> to WebCore_IDL_FILES in CMakeLists.txt even if WEB_ANIMATAIONS is disabled -
> but I feel like this solution is more consistent with the way conditions
> have been used in the WebKit IDL elsewhere (and also checking for [.*]
> before the implements statement is more correct in terms of Web IDL)

Really? Does WebIDL say it is expected for implements statements to have extended attributes?

It looks weird to me. I also don't think we want to add complexity to the bindings generator unless we really need to.

[Rawinder Singh]

(In reply to comment #8)
> (In reply to comment #7)
> > (In reply to comment #5)
> > > (In reply to comment #4)
> > > > (In reply to comment #3)
> > > > > (In reply to comment #2)
> > > > > > Comment on attachment 281444 [details]
> > > > > > Patch
> > > > > > 
> > > > > > Why do we need this? Why can't we simply use [Conditional] on the interface
> > > > > > that is on the right side of the 'implements' statement. This has been
> > > > > > sufficient so far.
> > > > > 
> > > > > See the "Build error" attachment for Bug #156096.
> > > > > 
> > > > > A quick overview:
> > > > > 
> > > > > - The Animatable interface does have Conditional=WEB_ANIMATIONS.
> > > > > - In Element.idl, I defined Element implements Animatable
> > > > > 
> > > > > However, when building with the WEB_ANIMATIONS condition off, the following
> > > > > error is produced by preprocess-idls.pl:
> > > > > 
> > > > > Could not find a the IDL file where the following implemented interface is
> > > > > defined: Animatable at
> > > > > /home/mcatanzaro/src/WebKit/Source/WebCore/bindings/scripts/preprocess-idls.
> > > > > pl line 102.
> > > > 
> > > > This is supposed to work and usually does AFAIK. I think you encountered a
> > > > specific bug that needs fixing rather than adding support for [Conditional]
> > > > on implements statements.
> > > 
> > > The error you are giving seems to indicate that Animatable.idl was not
> > > passed to preprocess-idls.pl, which probably means Animatable.idl was not
> > > properly added to build fixes (e.g. CMakeList.txt).
> > 
> > In Source/WebCore/CMakeLists.txt I only include Animatable.idl if
> > ENABLE_WEB_ANIMATIONS is defined.  So, yes for the case that WEB_ANIMATIONS
> > is disabled it will not be passed to preprocess-idls.pl (in which case this
> > error occurs).
> > 
> > I understand that I could probably work around it and append Animatable.idl
> > to WebCore_IDL_FILES in CMakeLists.txt even if WEB_ANIMATAIONS is disabled -
> > but I feel like this solution is more consistent with the way conditions
> > have been used in the WebKit IDL elsewhere (and also checking for [.*]
> > before the implements statement is more correct in terms of Web IDL)
> 
> Really? Does WebIDL say it is expected for implements statements to have
> extended attributes?

Yes. See https://www.w3.org/TR/WebIDL/#idl

[1]	Definitions	→	ExtendedAttributeList Definition Definitions
 | ε
[2]	Definition	→	CallbackOrInterface
 | Partial
 | Dictionary
 | Exception
 | Enum
 | Typedef
 | ImplementsStatement

> 
> It looks weird to me. I also don't think we want to add complexity to the
> bindings generator unless we really need to.

Okay. Let me know if you still think I should change the implementation of Bug #156096

[Rawinder Singh]

(In reply to comment #9)
> (In reply to comment #8)
> > (In reply to comment #7)
> > > (In reply to comment #5)
> > > > (In reply to comment #4)
> > > > > (In reply to comment #3)
> > > > > > (In reply to comment #2)
> > > > > > > Comment on attachment 281444 [details]
> > > > > > > Patch
> > > > > > > 
> > > > > > > Why do we need this? Why can't we simply use [Conditional] on the interface
> > > > > > > that is on the right side of the 'implements' statement. This has been
> > > > > > > sufficient so far.
> > > > > > 
> > > > > > See the "Build error" attachment for Bug #156096.
> > > > > > 
> > > > > > A quick overview:
> > > > > > 
> > > > > > - The Animatable interface does have Conditional=WEB_ANIMATIONS.
> > > > > > - In Element.idl, I defined Element implements Animatable
> > > > > > 
> > > > > > However, when building with the WEB_ANIMATIONS condition off, the following
> > > > > > error is produced by preprocess-idls.pl:
> > > > > > 
> > > > > > Could not find a the IDL file where the following implemented interface is
> > > > > > defined: Animatable at
> > > > > > /home/mcatanzaro/src/WebKit/Source/WebCore/bindings/scripts/preprocess-idls.
> > > > > > pl line 102.
> > > > > 
> > > > > This is supposed to work and usually does AFAIK. I think you encountered a
> > > > > specific bug that needs fixing rather than adding support for [Conditional]
> > > > > on implements statements.
> > > > 
> > > > The error you are giving seems to indicate that Animatable.idl was not
> > > > passed to preprocess-idls.pl, which probably means Animatable.idl was not
> > > > properly added to build fixes (e.g. CMakeList.txt).
> > > 
> > > In Source/WebCore/CMakeLists.txt I only include Animatable.idl if
> > > ENABLE_WEB_ANIMATIONS is defined.  So, yes for the case that WEB_ANIMATIONS
> > > is disabled it will not be passed to preprocess-idls.pl (in which case this
> > > error occurs).
> > > 
> > > I understand that I could probably work around it and append Animatable.idl
> > > to WebCore_IDL_FILES in CMakeLists.txt even if WEB_ANIMATAIONS is disabled -
> > > but I feel like this solution is more consistent with the way conditions
> > > have been used in the WebKit IDL elsewhere (and also checking for [.*]
> > > before the implements statement is more correct in terms of Web IDL)
> > 
> > Really? Does WebIDL say it is expected for implements statements to have
> > extended attributes?
> 
> Yes. See https://www.w3.org/TR/WebIDL/#idl
> 
> [1]	Definitions	→	ExtendedAttributeList Definition Definitions
>  | ε
> [2]	Definition	→	CallbackOrInterface
>  | Partial
>  | Dictionary
>  | Exception
>  | Enum
>  | Typedef
>  | ImplementsStatement
> 
> > 
> > It looks weird to me. I also don't think we want to add complexity to the
> > bindings generator unless we really need to.
> 
> Okay. Let me know if you still think I should change the implementation of
> Bug #156096

i.e. whether I should pursue this patch (or not)

[Chris Dumez]

(In reply to comment #10)
> (In reply to comment #9)
> > (In reply to comment #8)
> > > (In reply to comment #7)
> > > > (In reply to comment #5)
> > > > > (In reply to comment #4)
> > > > > > (In reply to comment #3)
> > > > > > > (In reply to comment #2)
> > > > > > > > Comment on attachment 281444 [details]
> > > > > > > > Patch
> > > > > > > > 
> > > > > > > > Why do we need this? Why can't we simply use [Conditional] on the interface
> > > > > > > > that is on the right side of the 'implements' statement. This has been
> > > > > > > > sufficient so far.
> > > > > > > 
> > > > > > > See the "Build error" attachment for Bug #156096.
> > > > > > > 
> > > > > > > A quick overview:
> > > > > > > 
> > > > > > > - The Animatable interface does have Conditional=WEB_ANIMATIONS.
> > > > > > > - In Element.idl, I defined Element implements Animatable
> > > > > > > 
> > > > > > > However, when building with the WEB_ANIMATIONS condition off, the following
> > > > > > > error is produced by preprocess-idls.pl:
> > > > > > > 
> > > > > > > Could not find a the IDL file where the following implemented interface is
> > > > > > > defined: Animatable at
> > > > > > > /home/mcatanzaro/src/WebKit/Source/WebCore/bindings/scripts/preprocess-idls.
> > > > > > > pl line 102.
> > > > > > 
> > > > > > This is supposed to work and usually does AFAIK. I think you encountered a
> > > > > > specific bug that needs fixing rather than adding support for [Conditional]
> > > > > > on implements statements.
> > > > > 
> > > > > The error you are giving seems to indicate that Animatable.idl was not
> > > > > passed to preprocess-idls.pl, which probably means Animatable.idl was not
> > > > > properly added to build fixes (e.g. CMakeList.txt).
> > > > 
> > > > In Source/WebCore/CMakeLists.txt I only include Animatable.idl if
> > > > ENABLE_WEB_ANIMATIONS is defined.  So, yes for the case that WEB_ANIMATIONS
> > > > is disabled it will not be passed to preprocess-idls.pl (in which case this
> > > > error occurs).
> > > > 
> > > > I understand that I could probably work around it and append Animatable.idl
> > > > to WebCore_IDL_FILES in CMakeLists.txt even if WEB_ANIMATAIONS is disabled -
> > > > but I feel like this solution is more consistent with the way conditions
> > > > have been used in the WebKit IDL elsewhere (and also checking for [.*]
> > > > before the implements statement is more correct in terms of Web IDL)
> > > 
> > > Really? Does WebIDL say it is expected for implements statements to have
> > > extended attributes?
> > 
> > Yes. See https://www.w3.org/TR/WebIDL/#idl
> > 
> > [1]	Definitions	→	ExtendedAttributeList Definition Definitions
> >  | ε
> > [2]	Definition	→	CallbackOrInterface
> >  | Partial
> >  | Dictionary
> >  | Exception
> >  | Enum
> >  | Typedef
> >  | ImplementsStatement
> > 
> > > 
> > > It looks weird to me. I also don't think we want to add complexity to the
> > > bindings generator unless we really need to.
> > 
> > Okay. Let me know if you still think I should change the implementation of
> > Bug #156096
> 
> i.e. whether I should pursue this patch (or not)

We haven't needed this so far and I don't think there is anything new that requires us to support extended attributes for implements statements. Unless there is some specific restriction (or problem I haven't thought of), I would personally prefer if we fixed CMakeLists.txt instead of adding code to the bindings generator to make CMake happy.

[Chris Dumez]

Maybe Darin or Sam have an opinion given that they hack the bindings a lot as well.

[Rawinder Singh]

(In reply to comment #12)
> Maybe Darin or Sam have an opinion given that they hack the bindings a lot
> as well.

Okay.  There are 3 methods I can try to sort out this issue.  However, all 3 require changes to the bindings generator code:

1. Use this method (i.e. Check for a conditional extended attribute)

2. Make changes to the CodeGeneratorGObject.pm and possibly the other bindings generators:
   - See attachment 281535 [details] in Bug 156096, comment 51
   - This error occurs when I move all IDL and cpp files that are currently guarded by the ENABLE_WEB_ANIMATAIONS condition in CMakeLists.txt into the main sections
   - Note: There may be other errors that also need to be addressed if I pursue this method.

3. See the patch for Bug #158975
   - I am able to do this when I only move the Animatable.idl file into the main body in CMakeLists.txt, rather than all the IDL and cpp files currently guarded by the WEB_ANIMATAIONS condition.

Let me know what you think.

[Chris Dumez]

(In reply to comment #13)
> (In reply to comment #12)
> > Maybe Darin or Sam have an opinion given that they hack the bindings a lot
> > as well.
> 
> Okay.  There are 3 methods I can try to sort out this issue.  However, all 3
> require changes to the bindings generator code:
> 
> 1. Use this method (i.e. Check for a conditional extended attribute)
> 
> 2. Make changes to the CodeGeneratorGObject.pm and possibly the other
> bindings generators:
>    - See attachment 281535 [details] in Bug 156096, comment 51
>    - This error occurs when I move all IDL and cpp files that are currently
> guarded by the ENABLE_WEB_ANIMATAIONS condition in CMakeLists.txt into the
> main sections
>    - Note: There may be other errors that also need to be addressed if I
> pursue this method.
> 
> 3. See the patch for Bug #158975
>    - I am able to do this when I only move the Animatable.idl file into the
> main body in CMakeLists.txt, rather than all the IDL and cpp files currently
> guarded by the WEB_ANIMATAIONS condition.
> 
> Let me know what you think.

I like option number 2. The build error seems to indicate that the GObject bindings generator does not obey [Conditional] in some cases which seems like a bug that needs fixing.

[Rawinder Singh]

(In reply to comment #14)
> (In reply to comment #13)
> > (In reply to comment #12)
> > > Maybe Darin or Sam have an opinion given that they hack the bindings a lot
> > > as well.
> > 
> > Okay.  There are 3 methods I can try to sort out this issue.  However, all 3
> > require changes to the bindings generator code:
> > 
> > 1. Use this method (i.e. Check for a conditional extended attribute)
> > 
> > 2. Make changes to the CodeGeneratorGObject.pm and possibly the other
> > bindings generators:
> >    - See attachment 281535 [details] in Bug 156096, comment 51
> >    - This error occurs when I move all IDL and cpp files that are currently
> > guarded by the ENABLE_WEB_ANIMATAIONS condition in CMakeLists.txt into the
> > main sections
> >    - Note: There may be other errors that also need to be addressed if I
> > pursue this method.
> > 
> > 3. See the patch for Bug #158975
> >    - I am able to do this when I only move the Animatable.idl file into the
> > main body in CMakeLists.txt, rather than all the IDL and cpp files currently
> > guarded by the WEB_ANIMATAIONS condition.
> > 
> > Let me know what you think.
> 
> I like option number 2. The build error seems to indicate that the GObject
> bindings generator does not obey [Conditional] in some cases which seems
> like a bug that needs fixing.

I think the build error for option number 3 also indicates that the JS bindings generator code doesn't obey [Conditional].  The reason I chose to just move the Animatable.idl in option 3, is so that unused generated code isn't unnecessarily generated.

Also, the reason I think it doesn't obey the [Conditional] is that the code is trying to include files for a return type in a function that has a [Conditional] - however the generated code doesn't try to put it within "#if ENABLE(CONDITION)" in this particular instance.  The existence of the function AddToImplIncludes (which I use) indicates that the header files are conditionally included elsewhere in the generator.

[Nikos Andronikos]

Hi Chris,

<snip>

(In reply to comment #14)
> 
> I like option number 2. The build error seems to indicate that the GObject
> bindings generator does not obey [Conditional] in some cases which seems
> like a bug that needs fixing.

What is it about option 2 that you like?

To us, it seems like the most clunky solution because it involves unnecessary work by the IDL preprocessor and generates unused bindings code. 

Personally, I think option 1 (add support for the Conditional extended attribute on the implements statement), which is implemented in this patch, sounds like the best solution. It's the most efficient and improves WebKit's IDL support. What's your reason for not liking it?

We really need to get one of these patches in so we can continue the Web Animations implementation. It's been waiting on this issue to be resolved for a few weeks now.

[youenn fablet]

I also prefer option 2.
It seems best to add compilation guards in header/source files and let the build system compile files even if feature is disabled.

Having each include file be guarded also allows including these files into other files without guarding the #include.

I don't know what is the build issue when moving all those files to the main section.
Maybe a patch could be dedicated to move AnimationTimeline.idl et al to the main section and make the necessary fixes for that to work.

[Brady Eidson]

Comment on attachment 281444 [details]
Patch

r-, as this has been pending review for over a year now. It is near-impossible that this patch still applies to trunk and unlikely to still be relevant in its current form.