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.
Created attachment 96240 [details] Proposed Patch
(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?
Created attachment 96370 [details] Patch
Leandro, I replace ";" separator with space. How do you think this approach ?
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})
Created attachment 96378 [details] Modified Patch Lucas, thank you for your advice. I also think this is much cleaner than before.
Progress Tag(Bug 62212) can be worked after landing this patch.
Comment on attachment 96378 [details] Modified Patch LGTM
Paroga, I'd like to listen your comment.
(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.
(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?
(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! :-)
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 ?
(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.
(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".
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)
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. :)
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?
Created attachment 97385 [details] Modified Patch Hello Patrick, I update ChangeLog and make to use _feature instead of f.
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)
Created attachment 97540 [details] Patch Ok, I fix it in new patch.
Patrick, please review this patch. I need this patch for progress tag and meter tag.
(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.
Comment on attachment 97540 [details] Patch Clearing flags on attachment: 97540 Committed r89731: <http://trac.webkit.org/changeset/89731>
All reviewed patches have been landed. Closing bug.