We have enabled the [Supplemental] IDL on Chromium, AppleWebKit, GTK and Qt, and are planning to enable it on all build systems (Meta bug 72138). In this bug, we enable it on Efl.
Created attachment 120784 [details] WIP patch to see if build passes
Comment on attachment 120784 [details] WIP patch to see if build passes Attachment 120784 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/11058120
Created attachment 120785 [details] WIP patch to see if build passes
Comment on attachment 120785 [details] WIP patch to see if build passes Attachment 120785 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/10899111
Created attachment 120793 [details] WIP patch to see if build passes
Comment on attachment 120793 [details] WIP patch to see if build passes Attachment 120793 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/10899167
Created attachment 120794 [details] WIP patch to see if build passes
Comment on attachment 120794 [details] WIP patch to see if build passes Attachment 120794 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/11056155
Created attachment 120796 [details] WIP patch to see if build passes
CC'ing dbates and paroga, as this is common infrastructure used by all ports which use CMake.
Comment on attachment 120796 [details] WIP patch to see if build passes View in context: https://bugs.webkit.org/attachment.cgi?id=120796&action=review > Source/WebCore/UseJSC.cmake:276 > + COMMAND echo ${IDL_FILES_LIST} | tr " " \\n > ${IDL_FILES_TMP} > + COMMAND cat ${IDL_FILES_TMP} the commands won't work on windows platform (e.g. ther is no cat) Maybe this can be written with the CMake FILE() and STRING() commands or better: add the required functionality into the perl scripts
Comment on attachment 120796 [details] WIP patch to see if build passes View in context: https://bugs.webkit.org/attachment.cgi?id=120796&action=review >> Source/WebCore/UseJSC.cmake:276 >> + COMMAND cat ${IDL_FILES_TMP} > > the commands won't work on windows platform (e.g. ther is no cat) > Maybe this can be written with the CMake FILE() and STRING() commands > or better: add the required functionality into the perl scripts Actually, this cat is just for debugging. I'll remove it in the following patch. Thanks!
Created attachment 120798 [details] patch for review
Comment on attachment 120798 [details] patch for review View in context: https://bugs.webkit.org/attachment.cgi?id=120798&action=review > Source/WebCore/ChangeLog:3 > + Enable the [Supplemental] IDL on Efl maybe you can change the efl to CMake? > Source/WebCore/UseJSC.cmake:275 > + COMMAND echo ${IDL_FILES_LIST} | tr " " \\n > ${IDL_FILES_TMP} same problem for echo and tr, they don't exist on windows :-/ > Source/WebCore/UseV8.cmake:263 > + COMMAND echo ${IDL_FILES_LIST} | tr " " \\n > ${IDL_FILES_TMP} here too
(In reply to comment #14) > (From update of attachment 120798 [details]) Thanks, Patrick! > > Source/WebCore/UseJSC.cmake:275 > > + COMMAND echo ${IDL_FILES_LIST} | tr " " \\n > ${IDL_FILES_TMP} > > same problem for echo and tr, they don't exist on windows :-/ > > > Source/WebCore/UseV8.cmake:263 > > + COMMAND echo ${IDL_FILES_LIST} | tr " " \\n > ${IDL_FILES_TMP} > > here too OK, I'll fix it in the next patch. By the way, maybe should we need to make a change on PlatformBlackBerry.cmake at the same time? In other words, if we make a change on CMakefile.list, the change will affect UseJSC.cmake, UseV8.cmake and PlatformBlackBerry.cmake, right? But I'm not sure how I can confirm that my change on PlatformBlackBerry.cmake would be correct or not (since I do not have a BlackBerry build environment around me)...
Created attachment 120800 [details] Patch
Comment on attachment 120800 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=120800&action=review > Source/WebCore/UseJSC.cmake:270 > +FILE(WRITE ${IDL_FILES_TMP} ${IDL_FILES_LIST}) does not seem like a problem, but only FYI: This way IDL_FILES_TMP will be written at _every_ CMake run, so nothing should depend on it to avoid unnecessary rebuilds. > Source/WebCore/UseJSC.cmake:275 > + MAIN_DEPENDENCY ${SCRIPTS_RESOLVE_SUPPLEMENTAL} ${WebCore_IDL_FILES} > + DEPENDS ${WEBCORE_DIR}/bindings/scripts/resolve-supplemental.pl ${SCRIPTS_RESOLVE_SUPPLEMENTAL} ${WebCore_IDL_FILES} sorry, missed in the last patch: MAIN_DEPENDENCY should contain only _ONE_ file. The "main dependency" will be used in IDE to provide a nicer interface. You can then right click on a IDL file and generate the corresponding h and cpp files. The MAIN_DEPENDENCY should be used only once in the project. All additional dependencies should be added to DEPENDS. The MAIN_DEPENDENCY does not need to be added to DEPENDS. > Source/WebCore/UseJSC.cmake:283 > + MAIN_DEPENDENCY ${_file} ${SUPPLEMENTAL_DEPENDENCY_FILE} too > Source/WebCore/UseV8.cmake:263 > + MAIN_DEPENDENCY ${SCRIPTS_RESOLVE_SUPPLEMENTAL} ${WebCore_IDL_FILES} > + DEPENDS ${WEBCORE_DIR}/bindings/scripts/resolve-supplemental.pl ${SCRIPTS_RESOLVE_SUPPLEMENTAL} ${WebCore_IDL_FILES} too > Source/WebCore/UseV8.cmake:271 > + MAIN_DEPENDENCY ${_file} ${SUPPLEMENTAL_DEPENDENCY_FILE} too
(In reply to comment #15) > (In reply to comment #14) > > (From update of attachment 120798 [details] [details]) > > By the way, maybe should we need to make a change on PlatformBlackBerry.cmake at the same time? In other words, if we make a change on CMakefile.list, the change will affect UseJSC.cmake, UseV8.cmake and PlatformBlackBerry.cmake, right? Correction: The change on CMakeLists.txt in this patch does not affect the current behavior of PlatformBlackBerry.cmake. Although we need to make a change to PlatformBlackBerry.cmake in the near future but do not have to do it in this patch.
(In reply to comment #17) > (From update of attachment 120800 [details]) > > Source/WebCore/UseJSC.cmake:270 > > +FILE(WRITE ${IDL_FILES_TMP} ${IDL_FILES_LIST}) > > does not seem like a problem, but only FYI: This way IDL_FILES_TMP will be written at _every_ CMake run, so nothing should depend on it to avoid unnecessary rebuilds. Yeah... To make it better, is there any way to write "FILE(WRITE ${IDL_FILES_TMP} ${IDL_FILES_LIST})" inside ADD_CUSTOM_COMMAND(...), like this? ADD_CUSTOM_COMMAND( OUTPUT ${IDL_FILES_TMP} COMMAND FILE(WRITE ${IDL_FILES_TMP} ${IDL_FILES_LIST}) # Syntax Error! ) ADD_CUSTOM_COMMAND( OUTPUT ${SUPPLEMENTAL_DEPENDENCY_FILE} DEPENDS ${IDL_FILES_TMP} COMMAND perl resolve-supplemental.pl ... )
Created attachment 120801 [details] Patch
(In reply to comment #19) > (In reply to comment #17) > > (From update of attachment 120800 [details] [details]) > > > Source/WebCore/UseJSC.cmake:270 > > > +FILE(WRITE ${IDL_FILES_TMP} ${IDL_FILES_LIST}) > > > > does not seem like a problem, but only FYI: This way IDL_FILES_TMP will be written at _every_ CMake run, so nothing should depend on it to avoid unnecessary rebuilds. > > Yeah... To make it better, is there any way to write "FILE(WRITE ${IDL_FILES_TMP} ${IDL_FILES_LIST})" inside ADD_CUSTOM_COMMAND(...), like this? > > ADD_CUSTOM_COMMAND( > OUTPUT ${IDL_FILES_TMP} > COMMAND FILE(WRITE ${IDL_FILES_TMP} ${IDL_FILES_LIST}) # Syntax Error! > ) > > ADD_CUSTOM_COMMAND( > OUTPUT ${SUPPLEMENTAL_DEPENDENCY_FILE} > DEPENDS ${IDL_FILES_TMP} > COMMAND perl resolve-supplemental.pl ... > ) Not really, you can write a CMake "script", but I don't think that we want this. IMHO it's not bad style to write this file as long as we know about possible problems.
Comment on attachment 120801 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=120801&action=review > Source/WebCore/UseJSC.cmake:269 > +SET(FIRST_IDL_FILE_FLAG 1) > +FOREACH (_idl ${WebCore_IDL_FILES}) > + IF (${FIRST_IDL_FILE_FLAG}) > + SET(FIRST_IDL_FILE_FLAG 0) > + SET(IDL_FILES_LIST "${WEBCORE_DIR}/${_idl}\n") > + ELSE () > + SET(IDL_FILES_LIST "${IDL_FILES_LIST}${WEBCORE_DIR}/${_idl}\n") > + ENDIF () > +ENDFOREACH () Isn't FOREACH (_idl ${WebCore_IDL_Files}) SET(IDL_FILES_LIST "${IDL_FILES_LIST}${WEBCORE_DIR}/${_idl}\n") ENDFOREACH () enough?
Created attachment 120802 [details] Patch
(In reply to comment #22) > (From update of attachment 120801 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=120801&action=review > > > Source/WebCore/UseJSC.cmake:269 > > +SET(FIRST_IDL_FILE_FLAG 1) > > +FOREACH (_idl ${WebCore_IDL_FILES}) > > + IF (${FIRST_IDL_FILE_FLAG}) > > + SET(FIRST_IDL_FILE_FLAG 0) > > + SET(IDL_FILES_LIST "${WEBCORE_DIR}/${_idl}\n") > > + ELSE () > > + SET(IDL_FILES_LIST "${IDL_FILES_LIST}${WEBCORE_DIR}/${_idl}\n") > > + ENDIF () > > +ENDFOREACH () > > Isn't > > FOREACH (_idl ${WebCore_IDL_Files}) > SET(IDL_FILES_LIST "${IDL_FILES_LIST}${WEBCORE_DIR}/${_idl}\n") > ENDFOREACH () > > enough? Absolutely. Fixed it.
Informal r+ from the EFL side.
CC'ing Antonio Gomes and Rob Buis so that they can follow this change with respect to PlatformBlackBerry.cmake.
Comment on attachment 120802 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=120802&action=review This looks good to me. > Source/WebCore/ChangeLog:35 > + * UseJSC.cmake: Described the above build. Nit: This remark doesn't read well (*). I would either remove this remark or revise it. One suggestion is: "Modified to reflect the new build flow as described above." (*) The remark doesn't read well because the word "describe" neither clarifies whether this patch modifies the file UseJSC.cmake nor what the modification is. (Although the presence of this line in the commit message indicates that either UseJSC.cmake was modified or newly added.) Moreover, the past tense usage of the word "describe" gives the impression that UseJSC.cmake either already implements or has a comment that describes the supplemental build flow ("new build flow"). Together these issues make it difficult to understand this remark. > Source/WebCore/ChangeLog:36 > + * UseV8.cmake: Exactly the same change as UseJSC.cmake, except for "JS" or "V8". Nit: Since the purpose of this remark is to indicate that a similar change to the build flow was made to file UseV8.cmake, I would just write "Ditto." > Source/WebCore/CMakeLists.txt:373 > webaudio/DelayNode.idl > + webaudio/DOMWindowWebAudio.idl Nit: These lines should be swapped such that this section of the list of IDL files is in sorted order according to the UNIX sort command.
Created attachment 120821 [details] patch for commit
Created attachment 120822 [details] patch for commit
(In reply to comment #27) > (From update of attachment 120802 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=120802&action=review > > This looks good to me. > > > Source/WebCore/ChangeLog:35 > > + * UseJSC.cmake: Described the above build. > > Nit: This remark doesn't read well (*). I would either remove this remark or revise it. One suggestion is: "Modified to reflect the new build flow as described above." > > (*) The remark doesn't read well because the word "describe" neither clarifies whether this patch modifies the file UseJSC.cmake nor what the modification is. (Although the presence of this line in the commit message indicates that either UseJSC.cmake was modified or newly added.) Moreover, the past tense usage of the word "describe" gives the impression that UseJSC.cmake either already implements or has a comment that describes the supplemental build flow ("new build flow"). Together these issues make it difficult to understand this remark. > > > Source/WebCore/ChangeLog:36 > > + * UseV8.cmake: Exactly the same change as UseJSC.cmake, except for "JS" or "V8". > > Nit: Since the purpose of this remark is to indicate that a similar change to the build flow was made to file UseV8.cmake, I would just write "Ditto." > > > Source/WebCore/CMakeLists.txt:373 > > webaudio/DelayNode.idl > > + webaudio/DOMWindowWebAudio.idl > > Nit: These lines should be swapped such that this section of the list of IDL files is in sorted order according to the UNIX sort command. dbates: Fixed them and committed. Thanks for the review.
Comment on attachment 120822 [details] patch for commit Clearing flags on attachment: 120822 Committed r103854: <http://trac.webkit.org/changeset/103854>
As Antonio suggested, I tried this patch with our BlackBerry port which also uses CMake. Unfortunately, our local repository hasn't synced up with upstream to have all the required IDLs, and our upstream BlackBerry build port is not yet functional. Therefore, we couldn't really try out this patch with our port at this moment, and we will re-try this later.
When is later?
(In reply to comment #33) > When is later? We are upstreaming, so lets not get this bug blocked on us. It was very nice of kentaro to try to build his patch for BlackBerry port. We have a bug for tracking BlackBerry specific bits of his work, so if there is nothing else left here, it is good to go.. :)
(In reply to comment #34) > (In reply to comment #33) > > When is later? > > We are upstreaming, so lets not get this bug blocked on us. > > It was very nice of kentaro to try to build his patch for BlackBerry port. We have a bug for tracking BlackBerry specific bits of his work, so if there is nothing else left here, it is good to go.. :) ming, tonikitoo: Thank you very much for the quick support. Do you mean that for the time being it is OK to commit the bug 75413 without confirming that the BlackBerry build passes? (The change of the bug 75413 is almost the same as the change that I've done for UseJSC.cmake and UseV8.cmake.) In fact, the bug 75413 is the last patch for enabling the [Supplemental] IDL on all build systems. In order to use the [Supplemental] IDL in WebKit, we need to enable it on *all* build systems. (If we start to use the [Supplemental] IDL in WebKit without committing the bug 75413, I am afraid that the BlackBerry build will be broken more because it cannot interpret the [Supplemental] IDL.)
I think he's saying to go ahead and use [Supplemental] without committing bug 75413. They're in the process of bringing up their port. When they're ready, they'll take a look at bug 75413 and provide feedback / tweak it as necessary.
(In reply to comment #36) > I think he's saying to go ahead and use [Supplemental] without committing bug 75413. They're in the process of bringing up their port. When they're ready, they'll take a look at bug 75413 and provide feedback / tweak it as necessary. Ah, I got it. Thanks.
I believe that cmake has had support for the supplemental IDL for a while now.