Bug 144394 – [CMake] Automatically expose WTF_USE_FOO to the build when USE_FOO is exposed

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Martin Robinson]

When options are presented to the user they are presented in the form USE_FOO, but the WTF USE(...) macro expects WTF_USE_FOO. We can automatically expose WTF_USE_FOO to make some things simpler.

[Martin Robinson]

Created attachment 251964 [details]
Patch

[Michael Catanzaro]

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.

[Martin Robinson]

Created attachment 251966 [details]
Patch

[Martin Robinson]

(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.

[Carlos Garcia Campos]

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.

[Martin Robinson]

(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.

[Martin Robinson]

(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.

[Martin Robinson]

Created attachment 252060 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 252060 [details]
Patch

Hopefully USE won't require the WTF prefix eventually.

[Martin Robinson]

Comment on attachment 252060 [details]
Patch

Clearing flags on attachment: 252060

Committed r183626: <http://trac.webkit.org/changeset/183626>

[Martin Robinson]

All reviewed patches have been landed.  Closing bug.