Bug 143832 - [CMake] Features list should print dots every other row
Summary: [CMake] Features list should print dots every other row
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Trivial
Assignee: Michael Catanzaro
Depends on: 143831
  Show dependency treegraph
Reported: 2015-04-16 11:20 PDT by Michael Catanzaro
Modified: 2015-04-22 09:41 PDT (History)
3 users (show)

See Also:

Patch (2.59 KB, patch)
2015-04-16 11:28 PDT, Michael Catanzaro
no flags Details | Formatted Diff | Diff
Patch (2.82 KB, patch)
2015-04-22 08:46 PDT, Michael Catanzaro
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Catanzaro 2015-04-16 11:20:47 PDT
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_ACCESSIBILITY .................... ON
--  ENABLE_API_TESTS                          ON
--  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.
Comment 1 Michael Catanzaro 2015-04-16 11:28:20 PDT
Created attachment 250932 [details]
Comment 2 Martin Robinson 2015-04-16 11:33:23 PDT
Do you want a review for this?
Comment 3 Michael Catanzaro 2015-04-16 11:55:21 PDT
Yes, but this patch probably won't apply on master. I'll mark it r? after bug #143831 is resolved.
Comment 4 Martin Robinson 2015-04-22 08:30:24 PDT
Comment on attachment 250932 [details]

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.
Comment 5 Michael Catanzaro 2015-04-22 08:36:16 PDT
I considered moving it up to line 199. My first instinct was to do this:

foreach (_name ${_WEBKIT_AVAILABLE_OPTIONS})
        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})
        # ...
    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!
Comment 6 Martin Robinson 2015-04-22 08:38:35 PDT
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 7 Martin Robinson 2015-04-22 08:40:54 PDT
Comment on attachment 250932 [details]

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.
Comment 8 Michael Catanzaro 2015-04-22 08:46:41 PDT
Created attachment 251319 [details]
Comment 9 WebKit Commit Bot 2015-04-22 09:41:35 PDT
Comment on attachment 251319 [details]

Clearing flags on attachment: 251319

Committed r183109: <http://trac.webkit.org/changeset/183109>
Comment 10 WebKit Commit Bot 2015-04-22 09:41:39 PDT
All reviewed patches have been landed.  Closing bug.