Summary: | [CMake] Automatically expose WTF_USE_FOO to the build when USE_FOO is exposed | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Martin Robinson <mrobinson> | ||||||||
Component: | Tools / Tests | Assignee: | Martin Robinson <mrobinson> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | cgarcia, gyuyoung.kim, mcatanzaro, ossy | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
Martin Robinson
2015-04-29 09:58:35 PDT
Created attachment 251964 [details]
Patch
Nits: You should mention in the changelog that this fixes USE_REDIRECTED_XCOMPOSITE_WINDOW after yesterday's commit, since that's a significant consequence of the change. Also, the comment "when we forget" implies that exposing WTF_USE_WHATEVER is best practice, but after this change I don't see any reason to do that anymore. Created attachment 251966 [details]
Patch
(In reply to comment #2) > Nits: > > You should mention in the changelog that this fixes > USE_REDIRECTED_XCOMPOSITE_WINDOW after yesterday's commit, since that's a > significant consequence of the change. > > Also, the comment "when we forget" implies that exposing WTF_USE_WHATEVER is > best practice, but after this change I don't see any reason to do that > anymore. Thanks for the comments. I have updated the patch with your suggestions. Comment on attachment 251966 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=251966&action=review > Source/cmake/OptionsGTK.cmake:378 > set(DERIVED_SOURCES_GOBJECT_DOM_BINDINGS_DIR ${DERIVED_SOURCES_DIR}/webkitdom) I guess you can remove SET_AND_EXPOSE_TO_BUILD(WTF_USE_LIBHYPHEN 1) > Source/cmake/WebKitFeatures.cmake:314 > + _ADD_CONFIGURATION_LINE_TO_HEADER_STRING(_file_contents ${_variable_name} ${_variable_name}) > + > + # WebKit looks for WTF_USE_FOO when calling the USE(FOO) macro. Automatically exposing > + # this definition when USE_FOO is exposed, prevents us from having to expose both (or > + # forgetting to do so). > + string(FIND ${_variable_name} "USE_" _use_string_location) > + if (${_use_string_location} EQUAL 0) > + _ADD_CONFIGURATION_LINE_TO_HEADER_STRING(_file_contents ${_variable_name} "WTF_${_variable_name}") This duplicates the USE macros in the cmakeconfig.h, with and without the WTF prefix, but I guess only the prefixed one is useful. I guess we could move the previous _ADD_CONFIGURATION_LINE_TO_HEADER_STRING to and else() here. (In reply to comment #5) > Comment on attachment 251966 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=251966&action=review > > > Source/cmake/OptionsGTK.cmake:378 > > set(DERIVED_SOURCES_GOBJECT_DOM_BINDINGS_DIR ${DERIVED_SOURCES_DIR}/webkitdom) > > I guess you can remove SET_AND_EXPOSE_TO_BUILD(WTF_USE_LIBHYPHEN 1) > Yes, this patch was written before libhyphen integration was complete. :) > > Source/cmake/WebKitFeatures.cmake:314 > > + _ADD_CONFIGURATION_LINE_TO_HEADER_STRING(_file_contents ${_variable_name} ${_variable_name}) > > + > > + # WebKit looks for WTF_USE_FOO when calling the USE(FOO) macro. Automatically exposing > > + # this definition when USE_FOO is exposed, prevents us from having to expose both (or > > + # forgetting to do so). > > + string(FIND ${_variable_name} "USE_" _use_string_location) > > + if (${_use_string_location} EQUAL 0) > > + _ADD_CONFIGURATION_LINE_TO_HEADER_STRING(_file_contents ${_variable_name} "WTF_${_variable_name}") > > This duplicates the USE macros in the cmakeconfig.h, with and without the > WTF prefix, but I guess only the prefixed one is useful. I guess we could > move the previous _ADD_CONFIGURATION_LINE_TO_HEADER_STRING to and else() > here. I waffled a bit on this, so your approach is probably fine too. (In reply to comment #6) > > This duplicates the USE macros in the cmakeconfig.h, with and without the > > WTF prefix, but I guess only the prefixed one is useful. I guess we could > > move the previous _ADD_CONFIGURATION_LINE_TO_HEADER_STRING to and else() > > here. > > I waffled a bit on this, so your approach is probably fine too. Actually, this isn't possible because there is at least one exception. USE_SYSTEM_MALLOC shouldn't have the prefix. I think it's safer to expose both for now. Created attachment 252060 [details]
Patch
Comment on attachment 252060 [details]
Patch
Hopefully USE won't require the WTF prefix eventually.
Comment on attachment 252060 [details] Patch Clearing flags on attachment: 252060 Committed r183626: <http://trac.webkit.org/changeset/183626> All reviewed patches have been landed. Closing bug. |