158975 2016-06-20 22:24:23 -0700 [WebIDL] Add extra checking of conditional attributes when including header files for the function type. 2022-02-27 23:10:27 -0800 1 1 1 Unclassified WebKit Bindings WebKit Nightly Build Unspecified Unspecified RESOLVED WONTFIX P2 Normal --- 156096 1 rawinder.webkit rawinder.webkit beidson cdumez cgarcia commit-queue youennf oldest_to_newest 1204050 0 rawinder.webkit 2016-06-20 22:24:23 -0700 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: 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. 1204058 1 281717 rawinder.webkit 2016-06-20 23:03:41 -0700 Created attachment 281717 Patch 1204171 2 cdumez 2016-06-21 09:38:50 -0700 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? 1204451 3 rawinder.webkit 2016-06-21 23:52:21 -0700 (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? 1212683 4 youennf 2016-07-20 00:56:59 -0700 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. 1340440 5 281717 beidson 2017-08-19 16:02:14 -0700 Comment on attachment 281717 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. 281717 2016-06-20 23:03:41 -0700 2022-02-27 23:10:27 -0800 Patch bug-158975-20160621160248.patch text/plain 5093 rawinder.webkit U3VidmVyc2lvbiBSZXZpc2lvbjogMjAyMjY4CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggZGIwMTlhMDhiN2NjMzZj YTJlN2JhNTdjNTBjNjBiNzZmMDU2MmQ2Zi4uZGI1MDQ4ZjRhZjk0ZDBmYWZlOTZjZDY5MmI0YzMx NWFhOTg3OWUwOSAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE5IEBACisyMDE2LTA2LTIwICBSYXdp bmRlciBTaW5naCAgPHJhd2luZGVyLnNpbmdoLXdlYmtpdEBjaXNyYS5jYW5vbi5jb20uYXU+CisK KyAgICAgICAgW1dlYklETF0gQWRkIGV4dHJhIGNoZWNraW5nIG9mIGNvbmRpdGlvbmFsIGF0dHJp YnV0ZXMgd2hlbiBpbmNsdWRpbmcgaGVhZGVyIGZpbGVzIGZvciB0aGUgZnVuY3Rpb24gdHlwZS4K KyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTE1ODk3NQor CisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIFVwZGF0ZSBB ZGRJbmNsdWRlc0ZvclR5cGUgdG8gaW5jbHVkZSBjb25kaXRpb25hbCBpbmZvcm1hdGlvbiB2aWEg QWRkVG9JbXBsSW5jbHVkZXMgd2hlbiBnZW5lcmF0aW5nIGNvZGUgdG8gaW5jbHVkaW5nIGhlYWRl ciBmaWxlcy4KKworICAgICAgICAqIGJpbmRpbmdzL3NjcmlwdHMvQ29kZUdlbmVyYXRvckpTLnBt OgorICAgICAgICAoQWRkSW5jbHVkZXNGb3JUeXBlSW5JbXBsKToKKyAgICAgICAgKEFkZEluY2x1 ZGVzRm9yVHlwZUluSGVhZGVyKToKKyAgICAgICAgKEFkZEluY2x1ZGVzRm9yVHlwZSk6CisgICAg ICAgIChBZGRUb0ltcGxJbmNsdWRlcyk6CisgICAgICAgIChHZW5lcmF0ZUltcGxlbWVudGF0aW9u KToKKwogMjAxNi0wNi0xOSAgR2F2aW4gJiBFbGxpZSBCYXJyYWNsb3VnaCAgPGJhcnJhY2xvdWdo QGFwcGxlLmNvbT4KIAogICAgICAgICBEb24ndCBlYWdlcmx5IHJlaWZ5IERPTSBQcm90b3R5cGUg cHJvcGVydGllcwpkaWZmIC0tZ2l0IGEvU291cmNlL1dlYkNvcmUvYmluZGluZ3Mvc2NyaXB0cy9D b2RlR2VuZXJhdG9ySlMucG0gYi9Tb3VyY2UvV2ViQ29yZS9iaW5kaW5ncy9zY3JpcHRzL0NvZGVH ZW5lcmF0b3JKUy5wbQppbmRleCA5MTAzMDI4ODY2NjRmY2IzMjhhODE4ZjA1ZWY0OTQyMzAwNTI0 ZDQ5Li5hMmRjOWJkZDE5YzkwMWZlYjhjNDY5ZWE2ZjQwZTk1YTc2ZGQ2Zjg2IDEwMDY0NAotLS0g YS9Tb3VyY2UvV2ViQ29yZS9iaW5kaW5ncy9zY3JpcHRzL0NvZGVHZW5lcmF0b3JKUy5wbQorKysg Yi9Tb3VyY2UvV2ViQ29yZS9iaW5kaW5ncy9zY3JpcHRzL0NvZGVHZW5lcmF0b3JKUy5wbQpAQCAt MTcxLDE2ICsxNzEsMTggQEAgc3ViIEFkZEluY2x1ZGVzRm9yVHlwZUluSW1wbAogewogICAgIG15 ICR0eXBlID0gc2hpZnQ7CiAgICAgbXkgJGlzQ2FsbGJhY2sgPSBAXyA/IHNoaWZ0IDogMDsKKyAg ICBteSAkY29uZGl0aW9uYWwgPSBAXyA/IHNoaWZ0IDogMDsKICAgICAKLSAgICBBZGRJbmNsdWRl c0ZvclR5cGUoJHR5cGUsICRpc0NhbGxiYWNrLCBcJWltcGxJbmNsdWRlcyk7CisgICAgQWRkSW5j bHVkZXNGb3JUeXBlKCR0eXBlLCAkaXNDYWxsYmFjaywgJGNvbmRpdGlvbmFsLCBcJWltcGxJbmNs dWRlcyk7CiB9CiAKIHN1YiBBZGRJbmNsdWRlc0ZvclR5cGVJbkhlYWRlcgogewogICAgIG15ICR0 eXBlID0gc2hpZnQ7CiAgICAgbXkgJGlzQ2FsbGJhY2sgPSBAXyA/IHNoaWZ0IDogMDsKKyAgICBt eSAkY29uZGl0aW9uYWwgPSBAXyA/IHNoaWZ0IDogMDsKICAgICAKLSAgICBBZGRJbmNsdWRlc0Zv clR5cGUoJHR5cGUsICRpc0NhbGxiYWNrLCBcJWhlYWRlckluY2x1ZGVzKTsKKyAgICBBZGRJbmNs dWRlc0ZvclR5cGUoJHR5cGUsICRpc0NhbGxiYWNrLCAkY29uZGl0aW9uYWwsIFwlaGVhZGVySW5j bHVkZXMpOwogfQogCiBzdWIgR2V0RXhwb3J0TWFjcm9Gb3JKU0NsYXNzCkBAIC0xOTUsMTMgKzE5 NywxNCBAQCBzdWIgQWRkSW5jbHVkZXNGb3JUeXBlCiB7CiAgICAgbXkgJHR5cGUgPSBzaGlmdDsK ICAgICBteSAkaXNDYWxsYmFjayA9IHNoaWZ0OworICAgIG15ICRjb25kaXRpb25hbCA9IHNoaWZ0 OwogICAgIG15ICRpbmNsdWRlc1JlZiA9IHNoaWZ0OwogCiAgICAgcmV0dXJuIGlmICRjb2RlR2Vu ZXJhdG9yLT5Ta2lwSW5jbHVkZUhlYWRlcigkdHlwZSk7CiAgICAgCiAgICAgIyBXaGVuIHdlJ3Jl IGZpbmlzaGVkIHdpdGggdGhlIG9uZS1maWxlLXBlci1jbGFzcyByZW9yZ2FuaXphdGlvbiwgd2Ug d29uJ3QgbmVlZCB0aGVzZSBzcGVjaWFsIGNhc2VzLgogICAgIGlmICgkaXNDYWxsYmFjayAmJiAk Y29kZUdlbmVyYXRvci0+SXNXcmFwcGVyVHlwZSgkdHlwZSkpIHsKLSAgICAgICAgJGluY2x1ZGVz UmVmLT57IkpTJHt0eXBlfS5oIn0gPSAxOworICAgICAgICBBZGRUb0ltcGxJbmNsdWRlcygiSlMk e3R5cGV9LmgiLCAkY29uZGl0aW9uYWwsICRpbmNsdWRlc1JlZik7CiAgICAgfSBlbHNpZiAoJGNv ZGVHZW5lcmF0b3ItPkdldEFycmF5T3JTZXF1ZW5jZVR5cGUoJHR5cGUpKSB7CiAgICAgICAgIG15 ICRhcnJheVR5cGUgPSAkY29kZUdlbmVyYXRvci0+R2V0QXJyYXlUeXBlKCR0eXBlKTsKICAgICAg ICAgbXkgJGFycmF5T3JTZXF1ZW5jZVR5cGUgPSAkYXJyYXlUeXBlIHx8ICRjb2RlR2VuZXJhdG9y LT5HZXRTZXF1ZW5jZVR5cGUoJHR5cGUpOwpAQCAtMjA5LDEzICsyMTIsMTMgQEAgc3ViIEFkZElu Y2x1ZGVzRm9yVHlwZQogICAgICAgICAgICAgJGluY2x1ZGVzUmVmLT57IkpTRE9NU3RyaW5nTGlz dC5oIn0gPSAxOwogICAgICAgICAgICAgJGluY2x1ZGVzUmVmLT57IkRPTVN0cmluZ0xpc3QuaCJ9 ID0gMTsKICAgICAgICAgfSBlbHNpZiAoJGNvZGVHZW5lcmF0b3ItPklzUmVmUHRyVHlwZSgkYXJy YXlPclNlcXVlbmNlVHlwZSkpIHsKLSAgICAgICAgICAgICRpbmNsdWRlc1JlZi0+eyJKUyR7YXJy YXlPclNlcXVlbmNlVHlwZX0uaCJ9ID0gMTsKLSAgICAgICAgICAgICRpbmNsdWRlc1JlZi0+eyIk e2FycmF5T3JTZXF1ZW5jZVR5cGV9LmgifSA9IDE7CisgICAgICAgICAgICBBZGRUb0ltcGxJbmNs dWRlcygiSlMke2FycmF5T3JTZXF1ZW5jZVR5cGV9LmgiLCAkY29uZGl0aW9uYWwsICRpbmNsdWRl c1JlZik7CisgICAgICAgICAgICBBZGRUb0ltcGxJbmNsdWRlcygiJHthcnJheU9yU2VxdWVuY2VU eXBlfS5oIiwgJGNvbmRpdGlvbmFsLCAkaW5jbHVkZXNSZWYpOwogICAgICAgICB9CiAgICAgICAg ICRpbmNsdWRlc1JlZi0+eyI8cnVudGltZS9KU0FycmF5Lmg+In0gPSAxOwogICAgIH0gZWxzZSB7 CiAgICAgICAgICMgZGVmYXVsdCwgaW5jbHVkZSB0aGUgc2FtZSBuYW1lZCBmaWxlCi0gICAgICAg ICRpbmNsdWRlc1JlZi0+eyIke3R5cGV9LmgifSA9IDE7CisgICAgICAgIEFkZFRvSW1wbEluY2x1 ZGVzKCIke3R5cGV9LmgiLCAkY29uZGl0aW9uYWwsICRpbmNsdWRlc1JlZik7CiAgICAgfQogfQog CkBAIC0yMjMsMTQgKzIyNiwxNSBAQCBzdWIgQWRkVG9JbXBsSW5jbHVkZXMKIHsKICAgICBteSAk aGVhZGVyID0gc2hpZnQ7CiAgICAgbXkgJGNvbmRpdGlvbmFsID0gc2hpZnQ7CisgICAgbXkgJGlu Y2x1ZGVzUmVmID0gQF8gPyBzaGlmdCA6IFwlaW1wbEluY2x1ZGVzOwogCiAgICAgaWYgKG5vdCAk Y29uZGl0aW9uYWwpIHsKLSAgICAgICAgJGltcGxJbmNsdWRlc3skaGVhZGVyfSA9IDE7Ci0gICAg fSBlbHNpZiAobm90IGV4aXN0cygkaW1wbEluY2x1ZGVzeyRoZWFkZXJ9KSkgewotICAgICAgICAk aW1wbEluY2x1ZGVzeyRoZWFkZXJ9ID0gJGNvbmRpdGlvbmFsOworICAgICAgICAkaW5jbHVkZXNS ZWYtPnskaGVhZGVyfSA9IDE7CisgICAgfSBlbHNpZiAobm90IGV4aXN0cygkaW5jbHVkZXNSZWYt PnskaGVhZGVyfSkpIHsKKyAgICAgICAgJGluY2x1ZGVzUmVmLT57JGhlYWRlcn0gPSAkY29uZGl0 aW9uYWw7CiAgICAgfSBlbHNlIHsKLSAgICAgICAgbXkgJG9sZFZhbHVlID0gJGltcGxJbmNsdWRl c3skaGVhZGVyfTsKLSAgICAgICAgJGltcGxJbmNsdWRlc3skaGVhZGVyfSA9ICIkb2xkVmFsdWV8 JGNvbmRpdGlvbmFsIiBpZiAkb2xkVmFsdWUgbmUgMTsKKyAgICAgICAgbXkgJG9sZFZhbHVlID0g JGluY2x1ZGVzUmVmLT57JGhlYWRlcn07CisgICAgICAgICRpbmNsdWRlc1JlZi0+eyRoZWFkZXJ9 ID0gIiRvbGRWYWx1ZXwkY29uZGl0aW9uYWwiIGlmICRvbGRWYWx1ZSBuZSAxOwogICAgIH0KIH0K IApAQCAtMzA5OCwxMSArMzEwMiwxMSBAQCBzdWIgR2VuZXJhdGVJbXBsZW1lbnRhdGlvbgogCiAg ICAgICAgICAgICBuZXh0IGlmICRpc0N1c3RvbSAmJiAkaXNPdmVybG9hZGVkICYmICRmdW5jdGlv bi0+e292ZXJsb2FkSW5kZXh9ID4gMTsKIAotICAgICAgICAgICAgQWRkSW5jbHVkZXNGb3JUeXBl SW5JbXBsKCRmdW5jdGlvbi0+c2lnbmF0dXJlLT50eXBlKSB1bmxlc3MgJGlzQ3VzdG9tIG9yIElz UmV0dXJuaW5nUHJvbWlzZSgkZnVuY3Rpb24pOworICAgICAgICAgICAgbXkgJGNvbmRpdGlvbmFs ID0gJGZ1bmN0aW9uLT5zaWduYXR1cmUtPmV4dGVuZGVkQXR0cmlidXRlcy0+eyJDb25kaXRpb25h bCJ9OworICAgICAgICAgICAgQWRkSW5jbHVkZXNGb3JUeXBlSW5JbXBsKCRmdW5jdGlvbi0+c2ln bmF0dXJlLT50eXBlLCAwLCAkY29uZGl0aW9uYWwpIHVubGVzcyAkaXNDdXN0b20gb3IgSXNSZXR1 cm5pbmdQcm9taXNlKCRmdW5jdGlvbik7CiAKICAgICAgICAgICAgIG15ICRmdW5jdGlvbk5hbWUg PSBHZXRGdW5jdGlvbk5hbWUoJGludGVyZmFjZSwgJGNsYXNzTmFtZSwgJGZ1bmN0aW9uKTsKIAot ICAgICAgICAgICAgbXkgJGNvbmRpdGlvbmFsID0gJGZ1bmN0aW9uLT5zaWduYXR1cmUtPmV4dGVu ZGVkQXR0cmlidXRlcy0+eyJDb25kaXRpb25hbCJ9OwogICAgICAgICAgICAgaWYgKCRjb25kaXRp b25hbCkgewogICAgICAgICAgICAgICAgIG15ICRjb25kaXRpb25hbFN0cmluZyA9ICRjb2RlR2Vu ZXJhdG9yLT5HZW5lcmF0ZUNvbmRpdGlvbmFsU3RyaW5nRnJvbUF0dHJpYnV0ZVZhbHVlKCRjb25k aXRpb25hbCk7CiAgICAgICAgICAgICAgICAgcHVzaChAaW1wbENvbnRlbnQsICIjaWYgJHtjb25k aXRpb25hbFN0cmluZ31cbiIpOwo=