Bug 62211 – [CMAKE] Replace ";" with space in FEATURE_DEFINES macro

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Gyuyoung Kim]

html.css file is using ENABLE_XXX macros for each feature. However, the html.css file doesn't recognize the macro. I have tried to find the reason.

Though I can't find correct solution, I think I find the reason at least.
CMakeLists.txt of WebCore is using "${FEATURE_DEFINES}" when building *.css files as below,

In Source/WebCore/CMakeList.txt

2099: COMMAND ${PERL_EXECUTABLE} -I${WEBCORE_DIR}/bindings/scripts ${WEBCORE_DIR}/css/make-css-file-arrays.pl --defines "${FEATURE_DEFINES}" --preprocessor "${CODE_GENERATOR_PREPROCE     SSOR}" ${DERIVED_SOURCES_WEBCORE_DIR}/UserAgentStyleSheets.h ${DERIVED_SOURCES_WEBCORE_DIR}/UserAgentStyleSheetsData.cpp ${WebCore_USER_AGENT_STYLE_SHEETS}

According to my debugging, the "${FEATURE_DEFINES}" prints below message with ";"

ENABLE_AS_IMAGE;ENABLE_CHANNEL_MESSAGING;ENABLE_DATABASE;ENABLE_DATALIST;ENABLE_DOM_STORAGE;ENABLE_EVENTSOURCE;ENABLE_FAST_MALLOC;ENABLE_FAST_MOBILE_SCROLLING;ENABLE_FILTERS;ENABLE_FTPDIR;ENABLE_GLIB_SUPPORT;ENABLE_ICONDATABASE;ENABLE_INSPECTOR;ENABLE_JAVASCRIPT_DEBUGGER;ENABLE_JIT;ENABLE_MATHML;ENABLE_OFFLINE_WEB_APPLICATIONS;ENABLE_SHARED_WORKERS;ENABLE_SVG;ENABLE_SVG_ANIMATION;ENABLE_SVG_FONTS;ENABLE_SVG_FOREIGN_OBJECT;ENABLE_SVG_USE;ENABLE_VIDEO;ENABLE_WORKERS;ENABLE_XPATH;ENABLE_XSLT

It seems the semicolon makes problems. When I remove the semicolon, there were no problems. So, for now, I make a patch for this problem.
However, I don't know which solution is correct yet. If anyone know correct solution, please let me know.

If we need to use the "{FEATURE_DEFINES}", we should replace ";" with space.

[Gyuyoung Kim]

Created attachment 96240 [details]
Proposed Patch

[Leandro Pereira]

(In reply to comment #1)
> Created an attachment (id=96240) [details]
> Proposed Patch

CMake doesn't really have lists or arrays -- they're actually strings where each element is separated by a semicolon. There are some other places in the build system where there is a need for a space-separated string of items; perhaps you could make that code into a function and refactor the build system?

[Gyuyoung Kim]

Created attachment 96370 [details]
Patch

[Gyuyoung Kim]

Leandro,

I replace ";" separator with space. How do you think this approach ?

[Lucas De Marchi]

Comment on attachment 96370 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=96370&action=review

> Source/cmake/WebKitFeatures.cmake:51
> -		LIST(APPEND FEATURE_DEFINES ${_feature})
> +		LIST(APPEND FEATURE_DEFINES ${_feature}${SPACES})
> +		STRING(REPLACE ";" ${SPACES} FEATURE_DEFINES ${FEATURE_DEFINES})

If you always need FEATURE_DEFINES separated by spaces instead of a list, it's much cleaner to do like this:
SET(FEATURE_DEFINES "${FEATURE_DEFINES} ${_feature})

[Gyuyoung Kim]

Created attachment 96378 [details]
Modified Patch

Lucas, thank you for your advice. I also think this is much cleaner than before.

[Gyuyoung Kim]

Progress Tag(Bug 62212) can be worked after landing this patch.

[Lucas De Marchi]

Comment on attachment 96378 [details]
Modified Patch

LGTM

[Gyuyoung Kim]

Paroga, I'd like to listen your comment.

[Patrick R. Gansterer]

(In reply to comment #9)
> Paroga, I'd like to listen your comment.

I don't think it's a good idea to store the features as string instead of a list.
IMO you should iterate over the list and create a string with space as separator at the place where you pass the features to the css generation.
If we compare this to C programming: We use too many global variables at the moment and should replace them with the correct local ones.
It doesn't make sense to my why the change in a "global file" fixes a "local" problem.

BTW: I'm not a big fan of the current state of our whole feature stuff. but unfortunately I don't have a good idea at the moment how we can improve it.

[Lucas De Marchi]

(In reply to comment #10)
> (In reply to comment #9)
> > Paroga, I'd like to listen your comment.
> 
> I don't think it's a good idea to store the features as string instead of a list.
> IMO you should iterate over the list and create a string with space as separator at the place where you pass the features to the css generation.
> If we compare this to C programming: We use too many global variables at the moment and should replace them with the correct local ones.
> It doesn't make sense to my why the change in a "global file" fixes a "local" problem.
> 
> BTW: I'm not a big fan of the current state of our whole feature stuff. but unfortunately I don't have a good idea at the moment how we can improve it.

I was thinking about this week, too. This is what I think would be a good idea:

Declare all the feature in a common place, common to all ports. We could be a macro DECLARE_FEATURE(ENABLE_XXXXX "Feature description" type default)

Ports would use something like:

USE_FEATURE(ENABLE_XXXXX) or USE_FEATURE(ENABLE_XXXXX "Feature description" type default) if they would like to override any option.


And as the last thing the top CMakeLists would generate the options based on the features list. This way we avoid having to duplicate the features descriptions, types and defaults for every port.

I can implement this if we agree to this solution. What do you think?

[Patrick R. Gansterer]

(In reply to comment #11)
> Ports would use something like:
> 
> USE_FEATURE(ENABLE_XXXXX) or USE_FEATURE(ENABLE_XXXXX "Feature description" type default) if they would like to override any option.

That's exatly what I want to do with the "general features" too. I think you can remove the "ENABLE_" part in the macros, since they are always the same.

> And as the last thing the top CMakeLists would generate the options based on the features list. This way we avoid having to duplicate the features descriptions, types and defaults for every port.

Maintaining the feature list for EFL and WinCE is already a headache.

> I can implement this if we agree to this solution. What do you think?

Go for it! :-)

[Gyuyoung Kim]

Patrick,(In reply to comment #12)
> (In reply to comment #11)
> > Ports would use something like:
> > 
> > USE_FEATURE(ENABLE_XXXXX) or USE_FEATURE(ENABLE_XXXXX "Feature description" type default) if they would like to override any option.
> 
> That's exatly what I want to do with the "general features" too. I think you can remove the "ENABLE_" part in the macros, since they are always the same.
> 
> > And as the last thing the top CMakeLists would generate the options based on the features list. This way we avoid having to duplicate the features descriptions, types and defaults for every port.
> 
> Maintaining the feature list for EFL and WinCE is already a headache.
> 
> > I can implement this if we agree to this solution. What do you think?
> 
> Go for it! :-)

Lucas and Patrick,

I am wondering when you can finish to implement your idea. If you need time for it, I think we need to add this patch for now. Then, we can remove this patch when you finish to implement your idea. How do you think ?

[Lucas De Marchi]

(In reply to comment #13)
> Lucas and Patrick,
> 
> I am wondering when you can finish to implement your idea. If you need time for it, I think we need to add this patch for now. Then, we can remove this patch when you finish to implement your idea. How do you think ?


What I've said is independent from what this patch is doing. IMO this patch can land as is. Or look at comment #10 to see Patrick's suggestion.

[Patrick R. Gansterer]

(In reply to comment #13)
> I am wondering when you can finish to implement your idea.

As Lucas said already, "our idea" is independent. The rest of comment #11 is still valid: I don't like the idea of mixing strings and lists in the "global namespace".

[Gyuyoung Kim]

Created attachment 97060 [details]
Prototyped Patch

I'm not sure if WebCore/CMakeList.txt file is best place for the replacement. I'd like to know which place is better ? 
(cmake/WebKitFeature.cmake or WebCore/CMakeLists.txt)

[Gyuyoung Kim]

Lucas and Patrick,

This patch influences on two features, progress tag and meter tag. So, I wanna land this patch. Please give your comment to me. :)

[Patrick R. Gansterer]

Comment on attachment 97060 [details]
Prototyped Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=97060&action=review

> Source/WebCore/CMakeLists.txt:2213
> +SET(${FEATURE_DEFINES_WITH_SPACE_SEPARATOR} "")

Did you try this? ${FEATURE_DEFINES_WITH_SPACE_SEPARATOR} evalueats to space, so you set the empty variable name. :-)
I'm not very happy with the variablename, but since I don't know a better one, it's ok for me.

> Source/WebCore/CMakeLists.txt:2214
> +FOREACH(f ${FEATURE_DEFINES})

A common way for "temporary" variables is the start with underscore in CMake. So "_feature" would be a better name instead of "f".

> Source/WebCore/ChangeLog:9
> +        as separator. So, replace ";" separator with space in new macro.

Not sure if you "replace" in the new patch. Maybe you can update the ChangeLog?

[Gyuyoung Kim]

Created attachment 97385 [details]
Modified Patch

Hello Patrick,

I update ChangeLog and make to use _feature instead of f.

[Patrick R. Gansterer]

Comment on attachment 97385 [details]
Modified Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=97385&action=review

> Source/WebCore/CMakeLists.txt:2215
> +SET(${FEATURE_DEFINES_WITH_SPACE_SEPARATOR} "")

This line is still wrong. The correct line would be >>SET(FEATURE_DEFINES_WITH_SPACE_SEPARATOR "")<<

> Source/WebCore/CMakeLists.txt:2218
> +ENDFOREACH(_feature)

Please don't insert sth. into the ENDFORECH() (that's old CMake style)

[Gyuyoung Kim]

Created attachment 97540 [details]
Patch

Ok, I fix it in new patch.

[Gyuyoung Kim]

Patrick, please review this patch. I need this patch for progress tag and meter tag.

[Patrick R. Gansterer]

(In reply to comment #22)
> Patrick, please review this patch. I need this patch for progress tag and meter tag.
Like in the other WebKit style i prefere to have a space between FOREACH and (, but it's ok without too.
since i'm not a reviever i can't r+, but patch LGTM.

[WebKit Review Bot]

Comment on attachment 97540 [details]
Patch

Clearing flags on attachment: 97540

Committed r89731: <http://trac.webkit.org/changeset/89731>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.