|Summary:||Move ENABLE macros for WebCore out from Platform.h and add all the macros from the FeatureFlags wiki|
|Product:||WebKit||Reporter:||Laszlo Gombos <laszlo.gombos>|
|Component:||Platform||Assignee:||Laszlo Gombos <laszlo.gombos>|
|Severity:||Normal||CC:||abarth, benjamin, cdumez, cmarcelo, dbarton, ddkilzer, dino, eric, gyuyoung.kim, jbriance, kadam, laszlo.gombos, mjs, ojan.autocc, rakuco, s.choi, tkent, vivekg, webkit.review.bot|
|Version:||528+ (Nightly build)|
|Bug Depends on:|
Description Laszlo Gombos 2012-12-24 21:52:09 PST
Move all ENABLE macros that are use in WebCore out from Platform.h and in the same file introduce all the other macros that are listed in the FeatureFlags wiki (http://trac.webkit.org/wiki/FeatureFlags). First step for meta bug 105734 - see the description there for the larger motivation.
Comment 1 Laszlo Gombos 2012-12-24 22:24:09 PST
Created attachment 180699 [details] 1st take Note that this is only a first step of some larger re-factoring. I set the macros with no defaults in Platform.h - to "TBD". The value for TBD is not important at the moment as most build systems already define these macros, so these new rules in FeatureDefines.h will not execute. I hope to get rid of all these TBDs in a follow-up patch (I think most of these features can be just enabled by default and let the port specific build system disable them if they need to). To get the list of all ENABLE macros one can just now run - grep -o "ENABLE_\(\w\+\)" wtf/FeatureDefines.h | sort | uniq. To get the list of features enabled by default for a PLATFORM(XXX) assuming that the build system does not define them: gcc -E -dM -I. -DWTF_PLATFORM_XXX "wtf/Platform.h" | grep "ENABLE_\w\+ 1" | cut -d' ' -f2 | sort
Comment 2 WebKit Review Bot 2012-12-24 22:27:16 PST
Attachment 180699 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WTF/ChangeLog', u'Source/WTF/GNUmak..." exit_code: 1 Source/WTF/wtf/FeatureDefines.h:283: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent]  Total errors found: 1 in 11 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Gyuyoung Kim 2013-01-02 16:37:52 PST
Comment on attachment 180718 [details] Fix style. Looks fine on EFL port side. But, as we talk, other reviewers need to take a look this.
Comment 5 Darin Adler 2013-01-16 15:24:05 PST
Comment on attachment 180718 [details] Fix style. View in context: https://bugs.webkit.org/attachment.cgi?id=180718&action=review Seems like a step in the right direction. We do want to have the feature definition system separate from the platform detection system. What I had imagined was that each platform would have its own file listing every feature that it wanted to be different than the default, rather than having one central file with #if statements in it. I don’t think that having a strict rule that every ENABLE has to be in FeatureDefines.h is quite right, though. It seems especially bad to move ENABLE_DEBUG_MATH_LAYOUT. Maybe we don’t want to use ENABLE() with that, but we should not pollute FeatureDefines.h with it. > Source/WTF/wtf/FeatureDefines.h:56 > +/* ENABLE macro defaults for WebCore */ Something should say that these are in alphabetical order. > Source/WTF/wtf/FeatureDefines.h:66 > +/* Add other platforms as they update their platfrom specific code to handle TextRun's with 8 bit data. */ Typo: platfrom
Comment 6 Laszlo Gombos 2013-01-16 17:06:09 PST
(In reply to comment #5) > (From update of attachment 180718 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=180718&action=review Thank a lot for the review Darin. > What I had imagined was that each platform would have its own file listing every feature that it wanted to be different than the default, rather than having one central file with #if statements in it. I fully agree. I was planning on doing this work as a follow-up step. > I don’t think that having a strict rule that every ENABLE has to be in FeatureDefines.h is quite right, though. It seems especially bad to move ENABLE_DEBUG_MATH_LAYOUT. Maybe we don’t want to use ENABLE() with that, but we should not pollute FeatureDefines.h with it. I had similar concerns writing the patch. I think it is best to not use ENABLE() for debug guards. I ended up including it just for completeness to match the list from the wiki page. I will remove this change from this patch before landing and will rename the macro instead as a separate patch. > > Source/WTF/wtf/FeatureDefines.h:56 > > +/* ENABLE macro defaults for WebCore */ > > Something should say that these are in alphabetical order. At the beginning of the FeatureDefines.h I put some languages that speak to this: "Note that the file is not necessary sorted by the name of the macros to allow inter-dependencies between the defaults." I will add some language to indicate to keep it sorted as much as possible.
Comment 7 Laszlo Gombos 2013-01-17 15:26:19 PST
Created attachment 183298 [details] patch for landing (review comments addressed)
Comment 8 Kent Tamura 2013-01-17 17:59:04 PST
Comment on attachment 183298 [details] patch for landing (review comments addressed) View in context: https://bugs.webkit.org/attachment.cgi?id=183298&action=review > Source/WTF/wtf/FeatureDefines.h:55 > +#define TBD 0 nit: The name 'TBD' is too short for the global namespace. We had better rename it or #undef it at the bottom of the file.
Comment 9 Laszlo Gombos 2013-01-26 11:36:23 PST
Created attachment 184875 [details] more cleanup I decided to make some more changes instead of landing the previously r+ patch from Darin as there was no urgency to land. I removed TBD and come up with sensible defaults for the defines (for most of the defines this is 0). I also structured the file so that each port has its own section that can be later moved out to a port specific file.
Comment 10 Build Bot 2013-01-26 14:14:39 PST
Comment on attachment 184875 [details] more cleanup Attachment 184875 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/16119944
Comment 11 Laszlo Gombos 2013-01-26 21:22:52 PST
Created attachment 184899 [details] disable progress element by default PROGRESS_ELEMENT is disabled for PLATFORM(WIN) && !OS(WINCE)
Comment 12 Benjamin Poulain 2013-02-08 16:03:29 PST
Comment on attachment 184899 [details] disable progress element by default Any chance to autogenerate FeatureDefines.h? It would be nice to split dependencies between features from platform configuration. What about, having 2 config file: 1) Defining the dependencies between features. 2) Give a list of each feature for each port. A little script reads the two, and output the FeatureDefines.h for the current port. The (only?) advantage would be readability.
Comment 13 Laszlo Gombos 2013-02-08 20:38:15 PST
(In reply to comment #12) > (From update of attachment 184899 [details]) Thanks for the review Benjamin ! > Any chance to autogenerate FeatureDefines.h? I think this is an idea worth exploring. I would like to do this work as a separate patch as this patch is already large in size and I think we can reasonably expect that auto generation would spark a longer discussions (which also ties into the "align build systems" discussion). > It would be nice to split dependencies between features from platform configuration. > > What about, having 2 config file: > 1) Defining the dependencies between features. > 2) Give a list of each feature for each port. As follow-up work I would like to separate the port specific rules into port specific files - but this would need buy in from each individual ports as they would have to maintain these files later on. > The (only?) advantage would be readability. My hope was that we do not hold up this work until we have consensus on how to improve the readability of the code further. I think the patch is an improvement over the current state in its current form.
Comment 14 Laszlo Gombos 2013-02-08 20:43:58 PST
The patch introduces wtf/FeatureDefines.h which is conceptually the same as default/Port.h as described on webkit-dev in a mail from Maciej - https://lists.webkit.org/pipermail/webkit-dev/2013-January/023542.html .
Comment 15 Benjamin Poulain 2013-02-12 14:05:23 PST
Comment on attachment 184899 [details] disable progress element by default I have nothing against this, but seriously this is putting lipstick on a pig. I think it would be easy and convenient to auto-generate the header from simple config files. I hope you are not gonna land this and go directly for a clean approach. If not, this patch is better than nothing.
Comment 16 Adam Barth 2013-02-12 14:10:27 PST
Comment on attachment 184899 [details] disable progress element by default Please don't land this patch. Comments to follow.
Comment 17 Adam Barth 2013-02-12 15:09:38 PST
Comment 18 Laszlo Gombos 2013-02-17 17:24:12 PST
Created attachment 188783 [details] for cq with Adam's feedback + rebase
Comment 19 WebKit Review Bot 2013-02-18 06:37:40 PST
Comment on attachment 188783 [details] for cq with Adam's feedback + rebase Clearing flags on attachment: 188783 Committed r143211: <http://trac.webkit.org/changeset/143211>
Comment 20 WebKit Review Bot 2013-02-18 06:37:46 PST
All reviewed patches have been landed. Closing bug.
Comment 21 Julien Brianceau 2013-02-18 08:24:11 PST
This patch breaks our sh4 build bot: http://build.webkit.org/builders/Qt%20Linux%20SH4%20Release/builds/20363 It appears that it also breaks the Qt Linux Release build too: http://build.webkit.org/builders/Qt%20Linux%20Release%20minimal/builds/72399
Comment 22 Ádám Kallai 2013-02-18 10:07:12 PST
It broke QtWebKit build on Mountain Lion: http://build.webkit.sed.hu/builders/x86-64%20Mountain%20Lion%20Qt%20Release%20%28WebGL%20Tester%29/builds/2732/steps/compile-webkit/logs/stdio
Comment 23 Laszlo Gombos 2013-02-18 11:11:35 PST
Created attachment 188920 [details] build fix when NETSCAPE_PLUGIN_API is disabled