Bug 143832 – [CMake] Features list should print dots every other row

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Michael Catanzaro]

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.

[Michael Catanzaro]

Created attachment 250932 [details]
Patch

[Martin Robinson]

Do you want a review for this?

[Michael Catanzaro]

Yes, but this patch probably won't apply on master. I'll mark it r? after bug #143831 is resolved.

[Martin Robinson]

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.

[Michael Catanzaro]

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!

[Martin Robinson]

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.

[Martin Robinson]

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.

[Michael Catanzaro]

Created attachment 251319 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 251319 [details]
Patch

Clearing flags on attachment: 251319

Committed r183109: <http://trac.webkit.org/changeset/183109>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.