Bug 143831 - [CMake] Require specifying visibility of WebKit options
Summary: [CMake] Require specifying visibility of WebKit options
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Enhancement
Assignee: Michael Catanzaro
URL:
Keywords:
Depends on:
Blocks: 143832 143839 143998
  Show dependency treegraph
 
Reported: 2015-04-16 10:46 PDT by Michael Catanzaro
Modified: 2015-04-22 08:26 PDT (History)
5 users (show)

See Also:


Attachments
Patch (78.72 KB, patch)
2015-04-16 10:52 PDT, Michael Catanzaro
no flags Details | Formatted Diff | Diff
Patch (78.95 KB, patch)
2015-04-22 08:14 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 10:46:21 PDT
It's not clear that WEBKIT_OPTION_DEFINE defines a private option or that WEBKIT_OPTION_DEFAULT_PORT_VALUE makes the option public. This was probably a mistake: let's require the user to choose PUBLIC or PRIVATE when defining or overriding the option. This also removes the short-lived WEBKIT_OPTION_DEFINE_PUBLIC and WEBKIT_OPTION_PRIVATE_PORT_VALUE macros.
Comment 1 Michael Catanzaro 2015-04-16 10:52:36 PDT
Created attachment 250929 [details]
Patch
Comment 2 Michael Catanzaro 2015-04-16 10:56:18 PDT
Warning for Alex Christensen: this patch hides all your options (so they are not shown with 'cmake -L' except when -A is also passed, and so that they are not printed when running cmake). I figure that is probably what you want, so you can choose a small subset of them to make public later rather than choosing a large subset to make private. (The idea is that you only make the option public if you actually support both building with it on and building with it off.) Let me know if you want these to be public instead.

EFL and GTK options all remain public in this patch, so as not to change the current behavior. For GTK we will make most of these private in bug #143558.
Comment 3 Alex Christensen 2015-04-16 11:24:48 PDT
(In reply to comment #2)
Thanks for the warning, but this is not a problem right now.  We can publicize options later once it actually works on Mac and Windows and has more than just one user.
Comment 4 Gyuyoung Kim 2015-04-22 01:11:26 PDT
Comment on attachment 250929 [details]
Patch

It seems there is no critical issue for EFL side.
Comment 5 WebKit Commit Bot 2015-04-22 06:52:12 PDT
Comment on attachment 250929 [details]
Patch

Rejecting attachment 250929 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-02', 'apply-attachment', '--no-update', '--non-interactive', 250929, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
e Source/cmake/OptionsMac.cmake
patching file Source/cmake/OptionsWindows.cmake
patching file Source/cmake/WebKitFeatures.cmake
Hunk #2 FAILED at 20.
Hunk #3 succeeded at 216 (offset 1 line).
1 out of 3 hunks FAILED -- saving rejects to file Source/cmake/WebKitFeatures.cmake.rej
patching file ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.

Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force', '--reviewer', u'Alex Christensen']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Full output: http://webkit-queues.appspot.com/results/6076748026347520
Comment 6 Alex Christensen 2015-04-22 07:08:57 PDT
Feel free to update and land.  I'm not surprised something changed underneath you.
Comment 7 Michael Catanzaro 2015-04-22 08:14:39 PDT
Created attachment 251316 [details]
Patch
Comment 8 WebKit Commit Bot 2015-04-22 08:26:46 PDT
Comment on attachment 251316 [details]
Patch

Clearing flags on attachment: 251316

Committed r183104: <http://trac.webkit.org/changeset/183104>
Comment 9 WebKit Commit Bot 2015-04-22 08:26:50 PDT
All reviewed patches have been landed.  Closing bug.