Bug 158975

Summary: [WebIDL] Add extra checking of conditional attributes when including header files for the function type.
Product: WebKit Reporter: Rawinder Singh <rawinder.webkit>
Component: BindingsAssignee: Rawinder Singh <rawinder.webkit>
Status: RESOLVED WONTFIX    
Severity: Normal CC: beidson, cdumez, cgarcia, commit-queue, youennf
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 156096    
Attachments:
Description Flags
Patch beidson: review-

Rawinder Singh
Reported 2016-06-20 22:24:23 PDT
Bug #156096 relies on "Element implements Animatable", where the Animatable interface is enabled/disabled by the ENABLE_WEB_ANIMATIONS condition. When ENABLE_WEB_ANIMATIONS is disabled the following error is produced: /WebCore/CMakeFiles/WebCoreDerivedSources.dir/_//DerivedSources/WebCore/JSElement.cpp.o.d" -o Source/WebCore/CMakeFiles/WebCoreDerivedSources.dir//_/DerivedSources/WebCore/JSElement.cpp.o -c DerivedSources/WebCore/JSElement.cpp DerivedSources/WebCore/JSElement.cpp:49:10: fatal error: 'JSWebAnimation.h' file not found #include "JSWebAnimation.h To reproduce the issue from Bug #156096, attachment #279339 [details]: 1. In the CMakeLists.txt file move Animatable.idl from inside the ENABLE_WEB_ANIMATIONS conditional statement to the main definition of WebCore_NON_SVG_IDL_FILES An overview of the issue: 1. GenerateImplementation iterates over the functions included in the Element.idl interface - This includes the getAnimations function in the Animatable interface (i.e. because of the "Element implements Animatable" statement). 2. The code generator looks at the return type of the getAnimations function (which is sequence<WebAnimations>) 3. AddIncludesForTypeInImpl tries to include the JSWebAnimation.h header file -> The problem occurs because the JSWebAnimation.h has not been generated by the code generator as ENABLE_WEB_ANIMATIONS is disabled.
Attachments
Patch (4.97 KB, patch)
2016-06-20 23:03 PDT, Rawinder Singh 		beidson: review-
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Rawinder Singh
Comment 1 2016-06-20 23:03:41 PDT
Created attachment 281717 [details] Patch
Chris Dumez
Comment 2 2016-06-21 09:38:50 PDT
I think JSWebAnimation.h should be generated and have the right #if ENABLE() protection. Why do we move only Animatable.idl out of the ENABLE_WEB_ANIMATIONS scope in CMakeLists.txt?
Rawinder Singh
Comment 3 2016-06-21 23:52:21 PDT
(In reply to comment #2) > I think JSWebAnimation.h should be generated and have the right #if ENABLE() > protection. > > Why do we move only Animatable.idl out of the ENABLE_WEB_ANIMATIONS scope in > CMakeLists.txt? See Bug 158830, comment 15. Additionally: I tried to model the code in a way that some of the other features are implemented (e.g. See ENABLE_VIDEO_TRACK) - this also has the benefit that code which is not used is not generated. Fair enough that Animatable.idl should be moved into the main section as Element (due to "Element implements Animatable") needs to at least see the description of the Animatable.idl interface (which is a NoInterfaceObject). However, I think it is better not to unnecessarily generate code which wont be used (hence, the other files remaining within the if(CONDITION) in the CMakeLists.txt file). Regardless of the above discussion, other parts of the code in the generator CodeGeneratorJS.pm also use AddToImplIncludes (which is used to conditionally include header files). Shouldn't this also be the case in this instance? i.e. if the function has a [Conditional], then conditionally include header files that are required by it?
youenn fablet
Comment 4 2016-07-20 00:56:59 PDT
As a specific comment to that patch, it would have been good to add a dedicated binding test to see the impact on generated source. As I commented in bug 158830, I agree with Chris here. It might be easier to have each header file have its own compilation guard.
Brady Eidson
Comment 5 2017-08-19 16:02:14 PDT
Comment on attachment 281717 [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.
Note You need to log in before you can comment on or make changes to this bug.