Bug 92553 - Add ENABLE_CSS_COMPOSITING flag
Summary: Add ENABLE_CSS_COMPOSITING flag
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL: https://dvcs.w3.org/hg/FXTF/rawfile/t...
Keywords:
Depends on:
Blocks: 91908
  Show dependency treegraph
 
Reported: 2012-07-27 15:04 PDT by Rik Cabanier
Modified: 2012-07-29 15:00 PDT (History)
9 users (show)

See Also:


Attachments
patch (26.47 KB, text/plain)
2012-07-27 15:14 PDT, Rik Cabanier
no flags Details
patch with OOPS removed (26.44 KB, text/plain)
2012-07-27 15:48 PDT, Rik Cabanier
krit: review-
krit: commit-queue-
Details
remove all OOPS + set flag to 0 (25.81 KB, text/plain)
2012-07-27 20:10 PDT, Rik Cabanier
krit: review-
Details
added descriptions (26.65 KB, text/plain)
2012-07-27 20:54 PDT, Rik Cabanier
krit: review-
Details
reordered description (26.66 KB, text/plain)
2012-07-27 21:19 PDT, Rik Cabanier
dino: review-
Details
patch (28.48 KB, text/plain)
2012-07-28 17:27 PDT, Rik Cabanier
no flags Details
patch (69.34 KB, text/plain)
2012-07-28 18:30 PDT, Rik Cabanier
no flags Details
patch (68.94 KB, text/plain)
2012-07-28 18:31 PDT, Rik Cabanier
no flags Details
enable ENABLE_CSS_COMPOSITING flag on all platforms (30.00 KB, text/plain)
2012-07-28 18:43 PDT, Rik Cabanier
no flags Details
enable ENABLE_CSS_COMPOSITING flag on all platforms (30.00 KB, text/plain)
2012-07-28 18:54 PDT, Rik Cabanier
no flags Details
enable ENABLE_CSS_COMPOSITING flag on all platforms (29.96 KB, patch)
2012-07-28 18:58 PDT, Rik Cabanier
no flags Details | Formatted Diff | Diff
enable ENABLE_CSS_COMPOSITING flag on all platforms (29.98 KB, patch)
2012-07-28 19:24 PDT, Rik Cabanier
no flags Details | Formatted Diff | Diff
enable ENABLE_CSS_COMPOSITING flag on all platforms (28.63 KB, patch)
2012-07-28 20:54 PDT, Rik Cabanier
no flags Details | Formatted Diff | Diff
prepare the ENABLE_CSS_COMPOSITING flag for all platforms (28.56 KB, patch)
2012-07-28 21:19 PDT, Rik Cabanier
krit: review-
Details | Formatted Diff | Diff
Added missing target (31.72 KB, patch)
2012-07-29 08:51 PDT, Rik Cabanier
no flags Details | Formatted Diff | Diff
Added missing Source/cmakeconfig.h.cmake setting (32.28 KB, patch)
2012-07-29 12:42 PDT, Rik Cabanier
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Rik Cabanier 2012-07-27 15:04:04 PDT
preliminary work for landing of the CSS compositing spec
Comment 1 Rik Cabanier 2012-07-27 15:14:05 PDT
Created attachment 155062 [details]
patch
Comment 2 Rik Cabanier 2012-07-27 15:48:24 PDT
Created attachment 155073 [details]
patch with OOPS removed
Comment 3 Dirk Schulze 2012-07-27 16:39:03 PDT
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.
Comment 4 Dean Jackson 2012-07-27 18:28:46 PDT
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.
Comment 5 Dirk Schulze 2012-07-27 20:04:57 PDT
(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.
Comment 6 Rik Cabanier 2012-07-27 20:10:20 PDT
Created attachment 155103 [details]
remove all OOPS + set flag to 0
Comment 7 Dirk Schulze 2012-07-27 20:15:08 PDT
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.
Comment 8 Rik Cabanier 2012-07-27 20:28:19 PDT
(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.
Comment 9 Rik Cabanier 2012-07-27 20:45:19 PDT
(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.
Comment 10 Rik Cabanier 2012-07-27 20:54:05 PDT
Created attachment 155104 [details]
added descriptions
Comment 11 Dirk Schulze 2012-07-27 21:04:09 PDT
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.
Comment 12 Rik Cabanier 2012-07-27 21:19:29 PDT
Created attachment 155106 [details]
reordered description
Comment 13 Dirk Schulze 2012-07-27 21:23:29 PDT
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 14 Dean Jackson 2012-07-28 14:37:17 PDT
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.
Comment 15 Rik Cabanier 2012-07-28 17:27:36 PDT
Created attachment 155145 [details]
patch
Comment 16 Rik Cabanier 2012-07-28 17:30:35 PDT
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.
Comment 17 Simon Fraser (smfr) 2012-07-28 17:36:36 PDT
(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.
Comment 18 Rik Cabanier 2012-07-28 17:44:21 PDT
(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?
Comment 19 Rik Cabanier 2012-07-28 18:30:20 PDT
Created attachment 155147 [details]
patch
Comment 20 Rik Cabanier 2012-07-28 18:31:59 PDT
Created attachment 155148 [details]
patch
Comment 21 Rik Cabanier 2012-07-28 18:43:46 PDT
Created attachment 155149 [details]
enable ENABLE_CSS_COMPOSITING flag on all platforms
Comment 22 WebKit Review Bot 2012-07-28 18:51:43 PDT
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.
Comment 23 Rik Cabanier 2012-07-28 18:54:09 PDT
Created attachment 155150 [details]
enable ENABLE_CSS_COMPOSITING flag on all platforms
Comment 24 WebKit Review Bot 2012-07-28 18:56:56 PDT
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.
Comment 25 Rik Cabanier 2012-07-28 18:58:19 PDT
Created attachment 155151 [details]
enable ENABLE_CSS_COMPOSITING flag on all platforms 

forgot to update the patch file
Comment 26 Rik Cabanier 2012-07-28 19:24:14 PDT
Created attachment 155152 [details]
enable ENABLE_CSS_COMPOSITING flag on all platforms
Comment 27 Raphael Kubo da Costa (:rakuco) 2012-07-28 20:18:17 PDT
For the CMake-based ports you also need to edit Source/cmakeconfig.h.cmake.
Comment 28 Rik Cabanier 2012-07-28 20:54:45 PDT
Created attachment 155155 [details]
enable ENABLE_CSS_COMPOSITING flag on all platforms
Comment 29 Rik Cabanier 2012-07-28 21:19:37 PDT
Created attachment 155157 [details]
prepare the ENABLE_CSS_COMPOSITING flag for all platforms
Comment 30 Dirk Schulze 2012-07-28 22:42:25 PDT
Comment on attachment 155157 [details]
prepare the ENABLE_CSS_COMPOSITING flag for all platforms 

Is there no change in the GNUmakefile.am necessary?
Comment 31 Dirk Schulze 2012-07-28 22:52:39 PDT
(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 32 Dirk Schulze 2012-07-28 22:54:22 PDT
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!
Comment 33 Rik Cabanier 2012-07-29 08:51:48 PDT
Created attachment 155173 [details]
Added missing target
Comment 34 Dirk Schulze 2012-07-29 10:58:15 PDT
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 35 Raphael Kubo da Costa (:rakuco) 2012-07-29 11:48:02 PDT
Comment on attachment 155173 [details]
Added missing target

Please add the proper entry to Source/cmakeconfig.h.cmake as I mentioned in comment #27.
Comment 36 Rik Cabanier 2012-07-29 12:42:22 PDT
Created attachment 155177 [details]
Added missing Source/cmakeconfig.h.cmake setting
Comment 37 Dirk Schulze 2012-07-29 13:05:25 PDT
Comment on attachment 155177 [details]
Added missing Source/cmakeconfig.h.cmake setting

Thanks rakuco, missed your comment as well, sorry. r=me.
Comment 38 WebKit Review Bot 2012-07-29 13:07:55 PDT
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 39 WebKit Review Bot 2012-07-29 15:00:09 PDT
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>
Comment 40 WebKit Review Bot 2012-07-29 15:00:18 PDT
All reviewed patches have been landed.  Closing bug.