| Summary: | Add ENABLE_CSS_COMPOSITING flag | ||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Rik Cabanier <cabanier> | ||||||||||||||||||||||||||||||||||
| Component: | CSS | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||||||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||||||||
| Severity: | Normal | CC: | dino, gustavo, gyuyoung.kim, krit, philn, rakuco, simon.fraser, webkit.review.bot, xan.lopez | ||||||||||||||||||||||||||||||||||
| Priority: | P2 | ||||||||||||||||||||||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||||||||||||||||||||||
| Hardware: | All | ||||||||||||||||||||||||||||||||||||
| OS: | All | ||||||||||||||||||||||||||||||||||||
| URL: | https://dvcs.w3.org/hg/FXTF/rawfile/tip/compositing/index.html | ||||||||||||||||||||||||||||||||||||
| Bug Depends on: | |||||||||||||||||||||||||||||||||||||
| Bug Blocks: | 91908 | ||||||||||||||||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||||||||||||||||
|
Description
Rik Cabanier
2012-07-27 15:04:04 PDT
Created attachment 155062 [details]
patch
Created attachment 155073 [details]
patch with OOPS removed
Comment on attachment 155073 [details] patch with OOPS removed View in context: https://bugs.webkit.org/attachment.cgi?id=155073&action=review It looks like you are missing a bunch of build systems like CMAKE and GNUmakefile, aren't you? Also, please fill the lines that ask for content (mainly the lines with OOPS). Something like "Adds compiler flag CSS_COMPOSITING to build systems to enable CSS blending and compositing. See spec ...." Only exception is "Reviewed by". Keep them as they are. > Tools/Scripts/webkitperl/FeatureList.pm:186 > + define => "ENABLE_CSS_COMPOSITING", default => 1, value => \$cssCompositingSupport }, It should be 0 by default. We did not decide to activate it in trunk builds. I don't think we need to add it to the other build systems. They can choose to turn it on (or have ways to do that as they build). We should not (yet) turn it on by default for systems that use build-webkit though. (In reply to comment #4) > I don't think we need to add it to the other build systems. They can choose to turn it on (or have ways to do that as they build). > I disagree. We had this discussion multiple times before. This is a platform wide feature that does not just affect one port. Therefore developers should of course take care of all platforms. It is really interesting that most developers take care of other ports. The most of the time it does not happen is when Apple implements new functionalities ;). That said, I don't think that it is a hard burden to modify the other makefiles as well. > We should not (yet) turn it on by default for systems that use build-webkit though. That for sure. But that is independent of the discussion. Created attachment 155103 [details]
remove all OOPS + set flag to 0
Comment on attachment 155103 [details] remove all OOPS + set flag to 0 You should not only remove the OOPS lines, but also add a comment what the change is about. I gave an example in my previous review: "Adds compiler flag CSS_COMPOSITING to build systems to enable CSS blending and compositing. See spec ...." See also http://www.webkit.org/coding/contributing.html for guidelines how to contribute patches. I still would like to see the flag added in other make files as well. Later it might be required to describe on every function listed in the ChangeLog what the change is about. (In reply to comment #7) > (From update of attachment 155103 [details]) > You should not only remove the OOPS lines, but also add a comment what the change is about. I gave an example in my previous review: "Adds compiler flag CSS_COMPOSITING to build systems to enable CSS blending and compositing. See spec ...." See also http://www.webkit.org/coding/contributing.html for guidelines how to contribute patches. I still would like to see the flag added in other make files as well. Later it might be required to describe on every function listed in the ChangeLog what the change is about. I copied Dean's change for CSS-Shaders: https://bugs.webkit.org/show_bug.cgi?id=71394 Since it's just config files and no code, it's obvious what is happening. (In reply to comment #7) > (From update of attachment 155103 [details]) > You should not only remove the OOPS lines, but also add a comment what the change is about. I gave an example in my previous review: "Adds compiler flag CSS_COMPOSITING to build systems to enable CSS blending and compositing. See spec ...." See also http://www.webkit.org/coding/contributing.html for guidelines how to contribute patches. I still would like to see the flag added in other make files as well. Later it might be required to describe on every function listed in the ChangeLog what the change is about. I will make the change. However, http://www.webkit.org/coding/contributing.html does not say that you have to do this. Maybe someone should update that. Created attachment 155104 [details]
added descriptions
Comment on attachment 155104 [details] added descriptions View in context: https://bugs.webkit.org/attachment.cgi?id=155104&action=review I would still like to see more makefiles edited. At least CMAKE GNUmakefile and the chromium make file (together with the already edited files, these should cover all build systems anyway). > Source/JavaScriptCore/ChangeLog:9 > + Add ENABLE_CSS_COMPOSITING flag > + https://bugs.webkit.org/show_bug.cgi?id=92553 > + > + Reviewed by NOBODY (OOPS!). > + > + * Configurations/FeatureDefines.xcconfig: > + Adds compiler flag CSS_COMPOSITING to build systems to enable CSS blending and compositing. See spec https://dvcs.w3.org/hg/FXTF/rawfile/tip/compositing/index.html The change log should look like this: 2012-07-27 Rik Cabanier <cabanier@adobe.com> Add ENABLE_CSS_COMPOSITING flag https://bugs.webkit.org/show_bug.cgi?id=92553 Reviewed by NOBODY (OOPS!). Adds compiler flag CSS_COMPOSITING to build systems to enable CSS blending and compositing. See spec https://dvcs.w3.org/hg/FXTF/rawfile/tip/compositing/index.html * Configurations/FeatureDefines.xcconfig: The link that I posted before say how a ChangeLog should look like. And they also say "Use this to write up a brief summary of the changes you've made". You can also find a link with an example of a ChangeLog entry. The lines that you removed usually guide you as well and say what you should do. Created attachment 155106 [details]
reordered description
Comment on attachment 155106 [details]
reordered description
The patch looks great now, but still lacks the flags on the other build systems that I asked for on the last two reviews.
Comment on attachment 155106 [details]
reordered description
I think you should do what Dirk suggests. He rightfully has called us (me!) out on not covering all the ports.
Created attachment 155145 [details]
patch
i(In reply to comment #14) > (From update of attachment 155106 [details]) > I think you should do what Dirk suggests. He rightfully has called us (me!) out on not covering all the ports. It woud be great to have a patch that does this. I've look at how a couple of define's were set and they just did it for a platform. In other words, I have no way to know what files I need to change. Since it's behind a define, the other platforms should not be affected anyway. (In reply to comment #16) > It woud be great to have a patch that does this. I've look at how a couple of define's were set and they just did it for a platform. You can easily find the right files by grepping for a commonly used flag. Look at my recent sticky position checkin for an example. (In reply to comment #17) > (In reply to comment #16) > > It woud be great to have a patch that does this. I've look at how a couple of define's were set and they just did it for a platform. > > You can easily find the right files by grepping for a commonly used flag. Look at my recent sticky position checkin for an example. What flag is that? Created attachment 155147 [details]
patch
Created attachment 155148 [details]
patch
Created attachment 155149 [details]
enable ENABLE_CSS_COMPOSITING flag on all platforms
Attachment 155149 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'Source/JavaScriptCore/Change..." exit_code: 1
Source/WebCore/ChangeLog:3: Need whitespace between colon and description [changelog/filechangedescriptionwhitespace] [5]
Total errors found: 1 in 16 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 155150 [details]
enable ENABLE_CSS_COMPOSITING flag on all platforms
Attachment 155150 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'Source/JavaScriptCore/Change..." exit_code: 1
Source/WebCore/ChangeLog:3: Need whitespace between colon and description [changelog/filechangedescriptionwhitespace] [5]
Total errors found: 1 in 16 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 155151 [details]
enable ENABLE_CSS_COMPOSITING flag on all platforms
forgot to update the patch file
Created attachment 155152 [details]
enable ENABLE_CSS_COMPOSITING flag on all platforms
For the CMake-based ports you also need to edit Source/cmakeconfig.h.cmake. Created attachment 155155 [details]
enable ENABLE_CSS_COMPOSITING flag on all platforms
Created attachment 155157 [details]
prepare the ENABLE_CSS_COMPOSITING flag for all platforms
Comment on attachment 155157 [details]
prepare the ENABLE_CSS_COMPOSITING flag for all platforms
Is there no change in the GNUmakefile.am necessary?
(In reply to comment #30) > (From update of attachment 155157 [details]) > Is there no change in the GNUmakefile.am necessary? Ah, you had the changes two patches before but the build broke with: Source/WebCore/GNUmakefile.am:504: ENABLE_CSS_COMPOSITING does not appear in AM_CONDITIONAL It looks like you have to do some changes in configure.ac in the root directory of webkit as well. Mainly just copy paste of what filters or regions are doing. Comment on attachment 155157 [details]
prepare the ENABLE_CSS_COMPOSITING flag for all platforms
You are nearly done. Just the changes from the last comment and the patch is ready to go!
Created attachment 155173 [details]
Added missing target
Comment on attachment 155173 [details] Added missing target View in context: https://bugs.webkit.org/attachment.cgi?id=155173&action=review Patch looks great. Thanks for your patience! R= me. > Source/WebCore/ChangeLog:8 > + Adds compiler flag CSS_COMPOSITING to build systems to enable CSS blending and compositing. See spec https://dvcs.w3.org/hg/FXTF/rawfile/tip/compositing/index.html Make sure that you use line breaks the next time. Comment on attachment 155173 [details] Added missing target Please add the proper entry to Source/cmakeconfig.h.cmake as I mentioned in comment #27. Created attachment 155177 [details]
Added missing Source/cmakeconfig.h.cmake setting
Comment on attachment 155177 [details]
Added missing Source/cmakeconfig.h.cmake setting
Thanks rakuco, missed your comment as well, sorry. r=me.
Comment on attachment 155173 [details] Added missing target Cleared Dirk Schulze's review+ from obsolete attachment 155173 [details] so that this bug does not appear in http://webkit.org/pending-commit. Comment on attachment 155177 [details] Added missing Source/cmakeconfig.h.cmake setting Clearing flags on attachment: 155177 Committed r123984: <http://trac.webkit.org/changeset/123984> All reviewed patches have been landed. Closing bug. |