When we print the list of features, dots should be printed every other row, for readability. Like this: -- Enabled features: -- ENABLE_3D_RENDERING ..................... ON -- ENABLE_ACCELERATED_2D_CANVAS ON -- ENABLE_ACCESSIBILITY .................... ON -- ENABLE_API_TESTS ON -- ENABLE_ATTACHMENT_ELEMENT ............... OFF -- ENABLE_BATTERY_STATUS OFF -- ENABLE_CANVAS_PATH ...................... OFF -- ENABLE_CSS3_TEXT OFF I broke this in r182658; the dots are being printed on seemingly-random rows based on the list of all options including hidden options, rather than the list of only printed options.
Created attachment 250932 [details] Patch
Do you want a review for this?
Yes, but this patch probably won't apply on master. I'll mark it r? after bug #143831 is resolved.
Comment on attachment 250932 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=250932&action=review > Source/cmake/WebKitFeatures.cmake:214 > if (${_WEBKIT_AVAILABLE_OPTIONS_IS_PUBLIC_${_name}}) Is there some reason this conditional cannot move up to line 199? I don't quite understand why we would build the message if we aren't going to show it.
I considered moving it up to line 199. My first instinct was to do this: foreach (_name ${_WEBKIT_AVAILABLE_OPTIONS}) if (NOT ${_WEBKIT_AVAILABLE_OPTIONS_IS_PUBLIC_${_name}}) continue () endif () # ... endforeach () But the continue statement would require some newer version of cmake, which doesn't seem worth it for such a small convenience. The alternative is to do this: foreach (_name ${_WEBKIT_AVAILABLE_OPTIONS}) if (${_WEBKIT_AVAILABLE_OPTIONS_IS_PUBLIC_${_name}}) # ... endif () endforeach () But then that adds an extra layer of indentation to the entire outer loop. I had a slight preference to doing the extra work in order to avoid that, since I figure this will add almost no time, but maybe that was silly. Whichever you prefer is fine by me!
I understand what you mean about the indent. I have a slight preference for keeping everything inside the indented loop, simply because I found it a bit confusing at first.
Comment on attachment 250932 [details] Patch Maybe just modify the loop while landing. I think it's probably fine either way, but I believe the version where all of the logic is guarded by the conditional is a bit less confusing.
Created attachment 251319 [details] Patch
Comment on attachment 251319 [details] Patch Clearing flags on attachment: 251319 Committed r183109: <http://trac.webkit.org/changeset/183109>
All reviewed patches have been landed. Closing bug.