Bug 99200 - Implement compositing path for blending
Summary: Implement compositing path for blending
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mihai Tica
URL:
Keywords:
Depends on: 126159
Blocks: 95614
  Show dependency treegraph
 
Reported: 2012-10-12 13:05 PDT by Rik Cabanier
Modified: 2014-03-04 00:18 PST (History)
26 users (show)

See Also:


Attachments
Patch (483.84 KB, text/plain)
2012-10-12 19:20 PDT, Rik Cabanier
no flags Details
Patch (482.92 KB, patch)
2012-10-12 19:25 PDT, Rik Cabanier
no flags Details | Formatted Diff | Diff
Patch (804.47 KB, patch)
2012-10-12 21:07 PDT, Rik Cabanier
no flags Details | Formatted Diff | Diff
strange failure. Try again for EWS output (804.47 KB, patch)
2012-10-12 21:15 PDT, Rik Cabanier
no flags Details | Formatted Diff | Diff
Patch (804.37 KB, patch)
2012-10-13 21:03 PDT, Rik Cabanier
simon.fraser: review-
simon.fraser: commit-queue-
Details | Formatted Diff | Diff
Try again to get linux chromium bot feedback (804.37 KB, text/plain)
2012-10-14 11:20 PDT, Rik Cabanier
no flags Details
Fixes per smfr (800.08 KB, patch)
2012-10-15 11:29 PDT, Rik Cabanier
no flags Details | Formatted Diff | Diff
try again. bad style bot (800.08 KB, text/plain)
2012-10-15 15:33 PDT, Rik Cabanier
no flags Details
I set up my own bot (800.08 KB, text/plain)
2012-10-15 21:26 PDT, Rik Cabanier
no flags Details
cleaned my tree for my bot (800.08 KB, text/plain)
2012-10-15 21:59 PDT, Rik Cabanier
no flags Details
set the mime type (800.08 KB, text/plain)
2012-10-15 22:11 PDT, Rik Cabanier
no flags Details
Finally figured it out. Sorry for the noise! (800.14 KB, patch)
2012-10-16 10:27 PDT, Rik Cabanier
no flags Details | Formatted Diff | Diff
incorporated Alex' comments (800.29 KB, patch)
2012-10-17 10:12 PDT, Rik Cabanier
no flags Details | Formatted Diff | Diff
Patch (667.25 KB, patch)
2012-10-17 15:26 PDT, Rik Cabanier
no flags Details | Formatted Diff | Diff
removed retainptr (667.90 KB, patch)
2012-10-17 16:20 PDT, Rik Cabanier
no flags Details | Formatted Diff | Diff
Patch (667.24 KB, patch)
2012-10-17 17:18 PDT, Rik Cabanier
no flags Details | Formatted Diff | Diff
Updated patch because it became out-of-date (667.07 KB, patch)
2012-11-08 21:01 PST, Rik Cabanier
no flags Details | Formatted Diff | Diff
Patch (1017.72 KB, patch)
2012-11-14 13:31 PST, Rik Cabanier
no flags Details | Formatted Diff | Diff
Added test files + fixed type (1017.72 KB, patch)
2012-11-14 13:45 PST, Rik Cabanier
no flags Details | Formatted Diff | Diff
Made testfile more clear (967.20 KB, patch)
2012-11-14 16:54 PST, Rik Cabanier
no flags Details | Formatted Diff | Diff
refreshed patch (640.52 KB, patch)
2013-02-21 12:33 PST, Rik Cabanier
no flags Details | Formatted Diff | Diff
Patch (639.35 KB, patch)
2013-05-06 11:14 PDT, Rik Cabanier
no flags Details | Formatted Diff | Diff
Patch (559.42 KB, patch)
2013-05-30 00:08 PDT, Mihai Tica
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-03 for mac-mountainlion (699.93 KB, application/zip)
2013-05-30 01:59 PDT, Build Bot
no flags Details
Patch (560.31 KB, patch)
2013-05-30 04:26 PDT, Mihai Tica
no flags Details | Formatted Diff | Diff
Patch (534.60 KB, patch)
2013-10-11 15:33 PDT, Dean Jackson
no flags Details | Formatted Diff | Diff
Test case showing problem (1.22 KB, text/html)
2013-10-11 17:34 PDT, Dean Jackson
no flags Details
Screenshot of test case (69.19 KB, image/png)
2013-10-11 17:38 PDT, Dean Jackson
no flags Details
Patch (790.20 KB, patch)
2013-10-14 13:38 PDT, Dean Jackson
no flags Details | Formatted Diff | Diff
Test case with preserve-3d (595 bytes, text/html)
2013-10-17 05:58 PDT, Mihai Tica
no flags Details
Screenshot of blending with preserve-3d (24.71 KB, image/png)
2013-10-17 06:00 PDT, Mihai Tica
no flags Details
Patch (765.77 KB, patch)
2013-10-21 06:51 PDT, Mihai Tica
no flags Details | Formatted Diff | Diff
Patch (775.02 KB, patch)
2013-12-23 04:52 PST, Mihai Tica
no flags Details | Formatted Diff | Diff
Patch for review (773.12 KB, patch)
2013-12-23 05:51 PST, Mihai Tica
no flags Details | Formatted Diff | Diff
Patch (753.85 KB, patch)
2014-01-10 01:16 PST, Mihai Tica
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-10-12 13:05:44 PDT
As a first pass, always assume that blending creates a layer.
Apply blending to this layer.
Comment 1 Rik Cabanier 2012-10-12 19:20:22 PDT
Created attachment 168530 [details]
Patch
Comment 2 Rik Cabanier 2012-10-12 19:25:21 PDT
Created attachment 168532 [details]
Patch
Comment 3 Build Bot 2012-10-12 20:08:26 PDT
Comment on attachment 168532 [details]
Patch

Attachment 168532 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14297099

New failing tests:
css3/compositing/should-have-compositing-layer.html
css3/compositing/effect-blend-mode.html
Comment 4 Rik Cabanier 2012-10-12 21:07:46 PDT
Created attachment 168536 [details]
Patch
Comment 5 WebKit Review Bot 2012-10-12 21:09:50 PDT
Attachment 168536 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/css3..." exit_code: 1
LayoutTests/platform/chromium/TestExpectations:676:  Test lacks BUG modifier.  [test/expectations] [5]
LayoutTests/platform/chromium/TestExpectations:677:  Test lacks BUG modifier.  [test/expectations] [5]
Total errors found: 2 in 18 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 6 Rik Cabanier 2012-10-12 21:15:15 PDT
Created attachment 168537 [details]
strange failure. Try again for EWS output
Comment 7 WebKit Review Bot 2012-10-12 21:17:12 PDT
Attachment 168537 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/css3..." exit_code: 1
LayoutTests/platform/chromium/TestExpectations:676:  Test lacks BUG modifier.  [test/expectations] [5]
LayoutTests/platform/chromium/TestExpectations:677:  Test lacks BUG modifier.  [test/expectations] [5]
Total errors found: 2 in 18 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 8 Rik Cabanier 2012-10-13 21:03:39 PDT
Created attachment 168574 [details]
Patch
Comment 9 Rik Cabanier 2012-10-14 11:20:42 PDT
Created attachment 168585 [details]
Try again to get linux chromium bot feedback
Comment 10 Simon Fraser (smfr) 2012-10-15 11:01:43 PDT
Comment on attachment 168574 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=168574&action=review

> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:1126
> +    if (m_uncommittedChanges & CompositingChanged)
> +        updateBlending();

Confusing that you called the flag CompositingChanged but then call updateBlending().

> Source/WebCore/rendering/RenderLayerBacking.cpp:1389
> +    if (m_graphicsLayer)
> +        m_graphicsLayer->setBlendMode(blendMode);

No need to null-check m_graphicsLayer.

> Source/WebCore/rendering/RenderLayerCompositor.cpp:845
> +    if (childState.m_subtreeHasBlending || layer->hasBlendMode())
> +        compositingState.m_subtreeHasBlending = true;

I don't think you need to track this at all.

> Source/WebCore/rendering/RenderLayerCompositor.cpp:1805
> +    bool HasBlendedDescendants, bool has3DTransformedDescendants, RenderLayer::IndirectCompositingReason& reason) const

HasBlendedDescendants -> hasBlendedDescendants

> Source/WebCore/rendering/RenderLayerCompositor.cpp:1812
> -    if (hasCompositedDescendants && (layer->transform() || renderer->createsGroup() || renderer->hasReflection())) {
> +    if (hasCompositedDescendants
> +        && (HasBlendedDescendants || layer->transform() || renderer->createsGroup() || renderer->hasReflection())) {

You have this backwards. A layer with blending needs to be composited if has composited descendants, but not vice versa. It should be just like opacity and filters, which are consulted in createsGroup().

> LayoutTests/css3/compositing/effect-blend-mode-expected.txt:38
> +layer at (0,0) size 800x600
> +  RenderView at (0,0) size 800x600
> +layer at (0,0) size 800x600
> +  RenderBlock {HTML} at (0,0) size 800x600
> +    RenderBody {BODY} at (8,8) size 784x584
> +      RenderBlock {UL} at (0,0) size 784x0
> +        RenderBlock (floating) {LI} at (45,5) size 130x130
> +          RenderImage {IMG} at (0,0) size 130x130
> +        RenderBlock (floating) {LI} at (185,5) size 130x130
> +          RenderImage {IMG} at (0,0) size 130x130
> +        RenderBlock (floating) {LI} at (325,5) size 130x130
> +          RenderImage {IMG} at (0,0) size 130x130
> +        RenderBlock (floating) {LI} at (465,5) size 130x130
> +          RenderImage {IMG} at (0,0) size 130x130
> +        RenderBlock (floating) {LI} at (605,5) size 130x130
> +          RenderImage {IMG} at (0,0) size 130x130
> +        RenderBlock (floating) {LI} at (45,145) size 130x130
> +          RenderImage {IMG} at (0,0) size 130x130
> +        RenderBlock (floating) {LI} at (185,145) size 130x130
> +          RenderImage {IMG} at (0,0) size 130x130
> +        RenderBlock (floating) {LI} at (325,145) size 130x130
> +          RenderImage {IMG} at (0,0) size 130x130
> +        RenderBlock (floating) {LI} at (465,145) size 130x130
> +          RenderImage {IMG} at (0,0) size 130x130
> +        RenderBlock (floating) {LI} at (605,145) size 130x130
> +          RenderImage {IMG} at (0,0) size 130x130
> +        RenderBlock (floating) {LI} at (45,285) size 130x130
> +          RenderImage {IMG} at (0,0) size 130x130
> +        RenderBlock (floating) {LI} at (185,285) size 130x130
> +          RenderImage {IMG} at (0,0) size 130x130
> +        RenderBlock (floating) {LI} at (325,285) size 130x130
> +          RenderImage {IMG} at (0,0) size 130x130
> +        RenderBlock (floating) {LI} at (465,285) size 130x130
> +          RenderImage {IMG} at (0,0) size 130x130
> +        RenderBlock (floating) {LI} at (605,285) size 130x130
> +          RenderImage {IMG} at (0,0) size 130x130
> +        RenderBlock (floating) {LI} at (45,425) size 130x130
> +          RenderImage {IMG} at (0,0) size 130x130

This output is not useful. It should be a dumpAsText(true) test.

> LayoutTests/platform/mac/css3/compositing/effect-blend-mode-expected.txt:53
> +layer at (0,0) size 800x600
> +  RenderView at (0,0) size 800x600
> +layer at (0,0) size 800x576
> +  RenderBlock {HTML} at (0,0) size 800x576
> +    RenderBody {BODY} at (8,16) size 784x0
> +      RenderBlock {UL} at (0,0) size 784x0
> +        RenderBlock (floating) {LI} at (45,5) size 130x130
> +          RenderImage {IMG} at (0,0) size 130x130
> +        RenderBlock (floating) {LI} at (185,5) size 130x130
> +        RenderBlock (floating) {LI} at (325,5) size 130x130
> +        RenderBlock (floating) {LI} at (465,5) size 130x130
> +        RenderBlock (floating) {LI} at (605,5) size 130x130
> +        RenderBlock (floating) {LI} at (45,145) size 130x130
> +        RenderBlock (floating) {LI} at (185,145) size 130x130
> +        RenderBlock (floating) {LI} at (325,145) size 130x130
> +        RenderBlock (floating) {LI} at (465,145) size 130x130
> +        RenderBlock (floating) {LI} at (605,145) size 130x130
> +        RenderBlock (floating) {LI} at (45,285) size 130x130
> +        RenderBlock (floating) {LI} at (185,285) size 130x130
> +        RenderBlock (floating) {LI} at (325,285) size 130x130
> +        RenderBlock (floating) {LI} at (465,285) size 130x130
> +        RenderBlock (floating) {LI} at (605,285) size 130x130
> +        RenderBlock (floating) {LI} at (45,425) size 130x130
> +layer at (193,21) size 130x130
> +  RenderImage {IMG} at (0,0) size 130x130
> +layer at (333,21) size 130x130
> +  RenderImage {IMG} at (0,0) size 130x130
> +layer at (473,21) size 130x130
> +  RenderImage {IMG} at (0,0) size 130x130
> +layer at (613,21) size 130x130
> +  RenderImage {IMG} at (0,0) size 130x130
> +layer at (53,161) size 130x130
> +  RenderImage {IMG} at (0,0) size 130x130
> +layer at (193,161) size 130x130
> +  RenderImage {IMG} at (0,0) size 130x130
> +layer at (333,161) size 130x130
> +  RenderImage {IMG} at (0,0) size 130x130
> +layer at (473,161) size 130x130
> +  RenderImage {IMG} at (0,0) size 130x130
> +layer at (613,161) size 130x130
> +  RenderImage {IMG} at (0,0) size 130x130
> +layer at (53,301) size 130x130
> +  RenderImage {IMG} at (0,0) size 130x130
> +layer at (193,301) size 130x130
> +  RenderImage {IMG} at (0,0) size 130x130
> +layer at (333,301) size 130x130
> +  RenderImage {IMG} at (0,0) size 130x130
> +layer at (473,301) size 130x130
> +  RenderImage {IMG} at (0,0) size 130x130
> +layer at (613,301) size 130x130
> +  RenderImage {IMG} at (0,0) size 130x130
> +layer at (53,441) size 130x130
> +  RenderImage {IMG} at (0,0) size 130x130

Ditto.

> LayoutTests/platform/mac/css3/compositing/should-have-compositing-layer-expected.txt:1
> +Test to make sure a blend mode creates a compositing layer. Test is successful of render tree shows compositing

You need a test that has blending on an element with composited children, and dumps the layer tree too.
Comment 11 Rik Cabanier 2012-10-15 11:15:24 PDT
(In reply to comment #10)
> (From update of attachment 168574 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=168574&action=review
> 
> > Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:1126
> > +    if (m_uncommittedChanges & CompositingChanged)
> > +        updateBlending();
> 
> Confusing that you called the flag CompositingChanged but then call updateBlending().

the idea is that this will be extended later for compositing as well.
I can rename the flag for now if you want to

> 
> > Source/WebCore/rendering/RenderLayerBacking.cpp:1389
> > +    if (m_graphicsLayer)
> > +        m_graphicsLayer->setBlendMode(blendMode);
> 
> No need to null-check m_graphicsLayer.

OK

> 
> > Source/WebCore/rendering/RenderLayerCompositor.cpp:845
> > +    if (childState.m_subtreeHasBlending || layer->hasBlendMode())
> > +        compositingState.m_subtreeHasBlending = true;
> 
> I don't think you need to track this at all.

Unfortunately, it does seem like it's needed.
If there's no backing for the root layer, the top layer will not blend with it.

> 
> > Source/WebCore/rendering/RenderLayerCompositor.cpp:1805
> > +    bool HasBlendedDescendants, bool has3DTransformedDescendants, RenderLayer::IndirectCompositingReason& reason) const
> 
> HasBlendedDescendants -> hasBlendedDescendants
> 
> > Source/WebCore/rendering/RenderLayerCompositor.cpp:1812
> > -    if (hasCompositedDescendants && (layer->transform() || renderer->createsGroup() || renderer->hasReflection())) {
> > +    if (hasCompositedDescendants
> > +        && (HasBlendedDescendants || layer->transform() || renderer->createsGroup() || renderer->hasReflection())) {
> 
> You have this backwards. A layer with blending needs to be composited if has composited descendants, but not vice versa. It should be just like opacity and filters, which are consulted in createsGroup().

See above

> 
> > LayoutTests/css3/compositing/effect-blend-mode-expected.txt:38
> > +layer at (0,0) size 800x600
> > +  RenderView at (0,0) size 800x600
> > +layer at (0,0) size 800x600
> > +  RenderBlock {HTML} at (0,0) size 800x600
> > +    RenderBody {BODY} at (8,8) size 784x584
> > +      RenderBlock {UL} at (0,0) size 784x0
> > +        RenderBlock (floating) {LI} at (45,5) size 130x130
> > +          RenderImage {IMG} at (0,0) size 130x130
> > +        RenderBlock (floating) {LI} at (185,5) size 130x130
> > +          RenderImage {IMG} at (0,0) size 130x130
> > +        RenderBlock (floating) {LI} at (325,5) size 130x130
> > +          RenderImage {IMG} at (0,0) size 130x130
> > +        RenderBlock (floating) {LI} at (465,5) size 130x130
> > +          RenderImage {IMG} at (0,0) size 130x130
> > +        RenderBlock (floating) {LI} at (605,5) size 130x130
> > +          RenderImage {IMG} at (0,0) size 130x130
> > +        RenderBlock (floating) {LI} at (45,145) size 130x130
> > +          RenderImage {IMG} at (0,0) size 130x130
> > +        RenderBlock (floating) {LI} at (185,145) size 130x130
> > +          RenderImage {IMG} at (0,0) size 130x130
> > +        RenderBlock (floating) {LI} at (325,145) size 130x130
> > +          RenderImage {IMG} at (0,0) size 130x130
> > +        RenderBlock (floating) {LI} at (465,145) size 130x130
> > +          RenderImage {IMG} at (0,0) size 130x130
> > +        RenderBlock (floating) {LI} at (605,145) size 130x130
> > +          RenderImage {IMG} at (0,0) size 130x130
> > +        RenderBlock (floating) {LI} at (45,285) size 130x130
> > +          RenderImage {IMG} at (0,0) size 130x130
> > +        RenderBlock (floating) {LI} at (185,285) size 130x130
> > +          RenderImage {IMG} at (0,0) size 130x130
> > +        RenderBlock (floating) {LI} at (325,285) size 130x130
> > +          RenderImage {IMG} at (0,0) size 130x130
> > +        RenderBlock (floating) {LI} at (465,285) size 130x130
> > +          RenderImage {IMG} at (0,0) size 130x130
> > +        RenderBlock (floating) {LI} at (605,285) size 130x130
> > +          RenderImage {IMG} at (0,0) size 130x130
> > +        RenderBlock (floating) {LI} at (45,425) size 130x130
> > +          RenderImage {IMG} at (0,0) size 130x130
> 
> This output is not useful. It should be a dumpAsText(true) test.

I copied the test from the filters folder.
Will update.

> 
> > LayoutTests/platform/mac/css3/compositing/effect-blend-mode-expected.txt:53
> > +layer at (0,0) size 800x600
> > +  RenderView at (0,0) size 800x600
> > +layer at (0,0) size 800x576
> > +  RenderBlock {HTML} at (0,0) size 800x576
> > +    RenderBody {BODY} at (8,16) size 784x0
> > +      RenderBlock {UL} at (0,0) size 784x0
> > +        RenderBlock (floating) {LI} at (45,5) size 130x130
> > +          RenderImage {IMG} at (0,0) size 130x130
> > +        RenderBlock (floating) {LI} at (185,5) size 130x130
> > +        RenderBlock (floating) {LI} at (325,5) size 130x130
> > +        RenderBlock (floating) {LI} at (465,5) size 130x130
> > +        RenderBlock (floating) {LI} at (605,5) size 130x130
> > +        RenderBlock (floating) {LI} at (45,145) size 130x130
> > +        RenderBlock (floating) {LI} at (185,145) size 130x130
> > +        RenderBlock (floating) {LI} at (325,145) size 130x130
> > +        RenderBlock (floating) {LI} at (465,145) size 130x130
> > +        RenderBlock (floating) {LI} at (605,145) size 130x130
> > +        RenderBlock (floating) {LI} at (45,285) size 130x130
> > +        RenderBlock (floating) {LI} at (185,285) size 130x130
> > +        RenderBlock (floating) {LI} at (325,285) size 130x130
> > +        RenderBlock (floating) {LI} at (465,285) size 130x130
> > +        RenderBlock (floating) {LI} at (605,285) size 130x130
> > +        RenderBlock (floating) {LI} at (45,425) size 130x130
> > +layer at (193,21) size 130x130
> > +  RenderImage {IMG} at (0,0) size 130x130
> > +layer at (333,21) size 130x130
> > +  RenderImage {IMG} at (0,0) size 130x130
> > +layer at (473,21) size 130x130
> > +  RenderImage {IMG} at (0,0) size 130x130
> > +layer at (613,21) size 130x130
> > +  RenderImage {IMG} at (0,0) size 130x130
> > +layer at (53,161) size 130x130
> > +  RenderImage {IMG} at (0,0) size 130x130
> > +layer at (193,161) size 130x130
> > +  RenderImage {IMG} at (0,0) size 130x130
> > +layer at (333,161) size 130x130
> > +  RenderImage {IMG} at (0,0) size 130x130
> > +layer at (473,161) size 130x130
> > +  RenderImage {IMG} at (0,0) size 130x130
> > +layer at (613,161) size 130x130
> > +  RenderImage {IMG} at (0,0) size 130x130
> > +layer at (53,301) size 130x130
> > +  RenderImage {IMG} at (0,0) size 130x130
> > +layer at (193,301) size 130x130
> > +  RenderImage {IMG} at (0,0) size 130x130
> > +layer at (333,301) size 130x130
> > +  RenderImage {IMG} at (0,0) size 130x130
> > +layer at (473,301) size 130x130
> > +  RenderImage {IMG} at (0,0) size 130x130
> > +layer at (613,301) size 130x130
> > +  RenderImage {IMG} at (0,0) size 130x130
> > +layer at (53,441) size 130x130
> > +  RenderImage {IMG} at (0,0) size 130x130
> 
> Ditto.
> 
> > LayoutTests/platform/mac/css3/compositing/should-have-compositing-layer-expected.txt:1
> > +Test to make sure a blend mode creates a compositing layer. Test is successful of render tree shows compositing
> 
> You need a test that has blending on an element with composited children, and dumps the layer tree too.

This will be updated when integrating CG. For now everything with blending creates a compositing layer (as we agreed last week)
Comment 12 Rik Cabanier 2012-10-15 11:29:38 PDT
Created attachment 168746 [details]
Fixes per smfr
Comment 13 Eric Seidel (no email) 2012-10-15 14:36:01 PDT
Attachment 168746 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/css3..." exit_code: 1
LayoutTests/platform/mac/css3/compositing/effect-blend-mode-expected.png:0:  Have to enable auto props in the subversion config file (/Users/eseidel/.subversion/config "enable-auto-props = yes"). Have to set the svn:mime-type in the subversion config file (/Users/eseidel/.subversion/config "*.png = svn:mime-type=image/png").  [image/png] [5]
Total errors found: 1 in 18 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 14 Rik Cabanier 2012-10-15 15:33:27 PDT
Created attachment 168793 [details]
try again. bad style bot
Comment 15 Eric Seidel (no email) 2012-10-15 15:35:45 PDT
Attachment 168793 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/css3..." exit_code: 1
LayoutTests/platform/mac/css3/compositing/effect-blend-mode-expected.png:0:  Have to enable auto props in the subversion config file (/Users/eseidel/.subversion/config "enable-auto-props = yes"). Have to set the svn:mime-type in the subversion config file (/Users/eseidel/.subversion/config "*.png = svn:mime-type=image/png").  [image/png] [5]
Total errors found: 1 in 18 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 16 Rik Cabanier 2012-10-15 21:26:02 PDT
Created attachment 168847 [details]
I set up my own bot
Comment 17 Rik Cabanier 2012-10-15 21:59:11 PDT
Created attachment 168849 [details]
cleaned my tree for my bot
Comment 18 Rik Cabanier 2012-10-15 22:02:41 PDT
Attachment 168849 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/platform/chromium/TestExpectat..." exit_code: 1
WARNING: Patch's size is 23 kbytes.
Patches 20k or smaller are more likely to be reviewed. Larger patches may sit unreviewed for a long time.
LayoutTests/platform/mac/css3/compositing/effect-blend-mode-expected.png:0:  Set the svn:mime-type property (svn propset svn:mime-type image/png LayoutTests/platform/mac/css3/compositing/effect-blend-mode-expected.png).  [image/png] [5]
LayoutTests/css3/compositing/effect-blend-mode-expected.png:0:  Set the svn:mime-type property (svn propset svn:mime-type image/png LayoutTests/css3/compositing/effect-blend-mode-expected.png).  [image/png] [5]
LayoutTests/css3/compositing/resources/ducky.png:0:  Set the svn:mime-type property (svn propset svn:mime-type image/png LayoutTests/css3/compositing/resources/ducky.png).  [image/png] [5]
Total errors found: 3 in 20 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 19 Rik Cabanier 2012-10-15 22:11:42 PDT
Created attachment 168853 [details]
set the mime type
Comment 20 Rik Cabanier 2012-10-15 22:21:39 PDT
Attachment 168853 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/platform/chromium/TestExpectat..." exit_code: 1
WARNING: Patch's size is 23 kbytes.
Patches 20k or smaller are more likely to be reviewed. Larger patches may sit unreviewed for a long time.
LayoutTests/platform/mac/css3/compositing/effect-blend-mode-expected.png:0:  Set the svn:mime-type property (svn propset svn:mime-type image/png LayoutTests/platform/mac/css3/compositing/effect-blend-mode-expected.png).  [image/png] [5]
LayoutTests/css3/compositing/effect-blend-mode-expected.png:0:  Set the svn:mime-type property (svn propset svn:mime-type image/png LayoutTests/css3/compositing/effect-blend-mode-expected.png).  [image/png] [5]
LayoutTests/css3/compositing/resources/ducky.png:0:  Set the svn:mime-type property (svn propset svn:mime-type image/png LayoutTests/css3/compositing/resources/ducky.png).  [image/png] [5]
Total errors found: 3 in 20 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 21 Simon Fraser (smfr) 2012-10-15 22:25:25 PDT
So much noise.....
Comment 22 Rik Cabanier 2012-10-16 10:27:12 PDT
Created attachment 168965 [details]
Finally figured it out. Sorry for the noise!
Comment 23 Alexandru Chiculita 2012-10-17 08:45:57 PDT
Comment on attachment 168965 [details]
Finally figured it out. Sorry for the noise!

View in context: https://bugs.webkit.org/attachment.cgi?id=168965&action=review

Looks good overall! Some minor comments below.

> Source/WebCore/platform/graphics/GraphicsLayer.h:316
> +

nit: Remove the empty line here.

> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:647
> +    noteLayerPropertyChanged(CompositingChanged);

I think CompositingChanged should actually be BlendingModeChanged.

> Source/WebCore/platform/graphics/ca/mac/PlatformCALayerMac.mm:678
> +    CIFilter* filter = nil;

Do you need a RetainPtr here? Looking at the filters function below it looks like only the array has a RetainPtr and the filters don't, but that might be a bug too.

> Source/WebCore/rendering/RenderLayerCompositor.cpp:845
> +    if (childState.m_subtreeHasBlending || layer->hasBlendMode())
> +        compositingState.m_subtreeHasBlending = true;

Why do you need to composite all the layers in the parent chain? It might be worth explaining that in a comment. I would argue that only the first ancestor that is opaque and covers the whole child needs to be composited as well.

> Source/WebCore/rendering/RenderLayerCompositor.h:303
> +        bool HasBlendedDescendants, bool has3DTransformedDescendants, RenderLayer::IndirectCompositingReason&) const;

nit: Use lower case first letter in parameter names.

> LayoutTests/css3/compositing/effect-blend-mode.html:1
> +<!DOCTYPE HTML>

I think you need a little bit more testing. If these use-cases are not working with your patch, then either add a bug and reference it from the implementation or fix it in this patch.
1. Check that the filter changes from 'multiply' back to 'normal'. For example, this was an issue with drop-shadow filter that could not be removed from a layer.
2. Check that a layers correctly multiplies with a different ancestor and not just the first one. This is because you are creating composited layers for all the parents (is that really required?).
3. Check that the blend mode is set correctly on an element that has composited children. Try different combinations of transform styling (preserve 3d or not).
4. You added some code related to the reflection layer. Maybe add a reflection layer and see if that is blended correctly?

> LayoutTests/css3/compositing/effect-blend-mode.html:24
> +<ul>

You need to add an explanation about what happens and what you expect on screen (ie, There should be n ducks blended on a ... ) . It could be an HTML comment.

Looks like LayoutTests/css3/compositing/effect-blend-mode-expected.png is wrong. I think you need to remove that file from the patch.

> LayoutTests/platform/chromium/TestExpectations:112
> +webkit.org/b/99200 css3/compositing/effect-blend-mode.html [ Skip ]

What about other platforms? Are they skipping the whole compositing folder already?
Comment 24 Rik Cabanier 2012-10-17 08:55:10 PDT
(In reply to comment #23)
> (From update of attachment 168965 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=168965&action=review
> 
> Looks good overall! Some minor comments below.
> 
> > Source/WebCore/platform/graphics/GraphicsLayer.h:316
> > +
> 
> nit: Remove the empty line here.

Will do

> 
> > Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:647
> > +    noteLayerPropertyChanged(CompositingChanged);
> 
> I think CompositingChanged should actually be BlendingModeChanged.

This is in preparation for compositing since it will use the same codepath.
I can change if needed though, but that should be another patch.

> 
> > Source/WebCore/platform/graphics/ca/mac/PlatformCALayerMac.mm:678
> > +    CIFilter* filter = nil;
> 
> Do you need a RetainPtr here? Looking at the filters function below it looks like only the array has a RetainPtr and the filters don't, but that might be a bug too.

I don't know. As you say, this code is in spirit of how filters are implemented. I will add a retainptr and see what happens.

> 
> > Source/WebCore/rendering/RenderLayerCompositor.cpp:845
> > +    if (childState.m_subtreeHasBlending || layer->hasBlendMode())
> > +        compositingState.m_subtreeHasBlending = true;
> 
> Why do you need to composite all the layers in the parent chain? It might be worth explaining that in a comment. I would argue that only the first ancestor that is opaque and covers the whole child needs to be composited as well.

I don't think so. You want to blend with everything that came before so it all needs to be in a layer with backing.
The spec is unclear if this blending should happen with the page's background color, but for now it does.

> 
> > Source/WebCore/rendering/RenderLayerCompositor.h:303
> > +        bool HasBlendedDescendants, bool has3DTransformedDescendants, RenderLayer::IndirectCompositingReason&) const;
> 
> nit: Use lower case first letter in parameter names.

Will do.

> 
> > LayoutTests/css3/compositing/effect-blend-mode.html:1
> > +<!DOCTYPE HTML>
> 
> I think you need a little bit more testing. If these use-cases are not working with your patch, then either add a bug and reference it from the implementation or fix it in this patch.
> 1. Check that the filter changes from 'multiply' back to 'normal'. For example, this was an issue with drop-shadow filter that could not be removed from a layer.
> 2. Check that a layers correctly multiplies with a different ancestor and not just the first one. This is because you are creating composited layers for all the parents (is that really required?).
> 3. Check that the blend mode is set correctly on an element that has composited children. Try different combinations of transform styling (preserve 3d or not).
> 4. You added some code related to the reflection layer. Maybe add a reflection layer and see if that is blended correctly?

The next step is to add support for CG blending which will introduce a bunch of new test cases.
I'm unsure if adding a bunch of tests that will need to change again new is a good use of time.

> 
> > LayoutTests/css3/compositing/effect-blend-mode.html:24
> > +<ul>
> 
> You need to add an explanation about what happens and what you expect on screen (ie, There should be n ducks blended on a ... ) . It could be an HTML comment.

Will do

> 
> Looks like LayoutTests/css3/compositing/effect-blend-mode-expected.png is wrong. I think you need to remove that file from the patch.

No. This is how the file appears on platforms that don't have blending (such as the QT ports)

> 
> > LayoutTests/platform/chromium/TestExpectations:112
> > +webkit.org/b/99200 css3/compositing/effect-blend-mode.html [ Skip ]
> 
> What about other platforms? Are they skipping the whole compositing folder already?

Chromium skips the folder but all the others don't
Comment 25 Rik Cabanier 2012-10-17 10:12:18 PDT
Created attachment 169204 [details]
incorporated Alex' comments
Comment 26 Alexandru Chiculita 2012-10-17 11:01:02 PDT
Comment on attachment 168965 [details]
Finally figured it out. Sorry for the noise!

View in context: https://bugs.webkit.org/attachment.cgi?id=168965&action=review

>>> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:647
>>> +    noteLayerPropertyChanged(CompositingChanged);
>> 
>> I think CompositingChanged should actually be BlendingModeChanged.
> 
> This is in preparation for compositing since it will use the same codepath.
> I can change if needed though, but that should be another patch.

Then maybe you need to call it that way. Anyway, why would it require a different patch? You're adding CompositingChanged in this patch :)

> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:1126
> +    if (m_uncommittedChanges & CompositingChanged)
> +        updateBlending();

Here's why I don't like the naming of 'CompositingChanged'. There's a compositing change, but you update the blending instead.

>>> Source/WebCore/rendering/RenderLayerCompositor.cpp:845
>>> +        compositingState.m_subtreeHasBlending = true;
>> 
>> Why do you need to composite all the layers in the parent chain? It might be worth explaining that in a comment. I would argue that only the first ancestor that is opaque and covers the whole child needs to be composited as well.
> 
> I don't think so. You want to blend with everything that came before so it all needs to be in a layer with backing.
> The spec is unclear if this blending should happen with the page's background color, but for now it does.

I got that, but I still think you don't need to create compositing layers out of every single parent. If you think about the ancestors that need compositing, only a single one needs to create a layer too. That's either the root, or if you want to optimize it you could find the parent that covers the bounding box of the child and is known to be opaque.

>>> LayoutTests/css3/compositing/effect-blend-mode.html:1
>>> +<!DOCTYPE HTML>
>> 
>> I think you need a little bit more testing. If these use-cases are not working with your patch, then either add a bug and reference it from the implementation or fix it in this patch.
>> 1. Check that the filter changes from 'multiply' back to 'normal'. For example, this was an issue with drop-shadow filter that could not be removed from a layer.
>> 2. Check that a layers correctly multiplies with a different ancestor and not just the first one. This is because you are creating composited layers for all the parents (is that really required?).
>> 3. Check that the blend mode is set correctly on an element that has composited children. Try different combinations of transform styling (preserve 3d or not).
>> 4. You added some code related to the reflection layer. Maybe add a reflection layer and see if that is blended correctly?
> 
> The next step is to add support for CG blending which will introduce a bunch of new test cases.
> I'm unsure if adding a bunch of tests that will need to change again new is a good use of time.

What do you expect to change? 

If you add CG blending, most probably you will have a switch forcing the related tests to use CG, so the CA part will not be covered by those tests anyway. You might need to duplicate tests for the different implementation paths (software path vs. hardware path).

>>> LayoutTests/css3/compositing/effect-blend-mode.html:24
>>> +<ul>
>> 
>> You need to add an explanation about what happens and what you expect on screen (ie, There should be n ducks blended on a ... ) . It could be an HTML comment.
>> 
>> Looks like LayoutTests/css3/compositing/effect-blend-mode-expected.png is wrong. I think you need to remove that file from the patch.
> 
> Will do

> No. This is how the file appears on platforms that don't have blending (such as the QT ports)

In those cases you skip the test instead. Do not integrate bogus expected results. If a platform passes a test it means that the tested functionality is implemented.

>>> LayoutTests/platform/chromium/TestExpectations:112
>>> +webkit.org/b/99200 css3/compositing/effect-blend-mode.html [ Skip ]
>> 
>> What about other platforms? Are they skipping the whole compositing folder already?
> 
> Chromium skips the folder but all the others don't

Are the other platforms passing the test? If they don't pass, then those platforms should skip the test/folder, too.

BTW, I think the skipped line should reference a bug that will track and fix the issue. You can add a master bug "Implement CSS3 compositing" that will track this issue globally on all the platforms.
Comment 27 Rik Cabanier 2012-10-17 15:26:32 PDT
Created attachment 169274 [details]
Patch
Comment 28 Rik Cabanier 2012-10-17 16:20:15 PDT
Created attachment 169287 [details]
removed retainptr
Comment 29 Rik Cabanier 2012-10-17 17:18:26 PDT
Created attachment 169308 [details]
Patch
Comment 30 Rik Cabanier 2012-11-08 21:01:56 PST
Created attachment 173191 [details]
Updated patch because it became out-of-date
Comment 31 Simon Fraser (smfr) 2012-11-13 19:14:58 PST
Comment on attachment 173191 [details]
Updated patch because it became out-of-date

View in context: https://bugs.webkit.org/attachment.cgi?id=173191&action=review

This is pretty close. I'm not convinced the m_subtreeHasBlending = true;  works without seeing some more tests, and wonder if it will cause every subsequent layer in the tree traversal to be composited.

> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:2935
> +    primaryLayer()->setBlendMode(m_blendMode);

I recently fixed an issue with reflections where primary() layer was the wrong layer to use. Please check reflected, blended content.

> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.h:395
> +        BlendingModeChanged = 1 << 26,

You're re-using 26.

> Source/WebCore/rendering/RenderLayerCompositor.cpp:900
> +    if (childState.m_subtreeHasBlending || layer->hasBlendMode())
> +        compositingState.m_subtreeHasBlending = true;
> +
>      // Now check for reasons to become composited that depend on the state of descendant layers.
>      RenderLayer::IndirectCompositingReason indirectCompositingReason;
>      if (!willBeComposited && canBeComposited(layer)
> -        && requiresCompositingForIndirectReason(layer->renderer(), childState.m_subtreeIsCompositing, anyDescendantHas3DTransform, indirectCompositingReason)) {
> +        && requiresCompositingForIndirectReason(layer->renderer(), childState.m_subtreeIsCompositing, compositingState.m_subtreeHasBlending, anyDescendantHas3DTransform, indirectCompositingReason)) {
>          layer->setIndirectCompositingReason(indirectCompositingReason);
>          childState.m_compositingAncestor = layer;

You should add some tests that exercise this code.

> Source/WebCore/rendering/RenderLayerCompositor.h:318
> +    bool requiresCompositingForIndirectReason(RenderObject*, bool , 
> +        bool , bool , RenderLayer::IndirectCompositingReason&) const;

Those bool parameter names are useful; I think you should keep them.
Comment 32 Rik Cabanier 2012-11-14 13:31:57 PST
Created attachment 174241 [details]
Patch
Comment 33 Rik Cabanier 2012-11-14 13:45:55 PST
Created attachment 174247 [details]
Added test files + fixed type
Comment 34 Rik Cabanier 2012-11-14 13:50:05 PST
(In reply to comment #31)
> (From update of attachment 173191 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=173191&action=review
> 
> This is pretty close. I'm not convinced the m_subtreeHasBlending = true;  works without seeing some more tests, and wonder if it will cause every subsequent layer in the tree traversal to be composited.

I added anotther test that verifies that blending is also happening correctly if it is in a stacking context or if it contains a stacking context

I also looked into your other concern that all parent layers are composited. That was correct.
I added some code to RenderLayerCompositor.cpp that catches this and will turn off the tracking of blending when it's no longer needed.

> 
> > Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:2935
> > +    primaryLayer()->setBlendMode(m_blendMode);
> 
> I recently fixed an issue with reflections where primary() layer was the wrong layer to use. Please check reflected, blended content.

I added a testfile that blends reflected content and it seems to work OK.

> 
> > Source/WebCore/platform/graphics/ca/GraphicsLayerCA.h:395
> > +        BlendingModeChanged = 1 << 26,
> 
> You're re-using 26.

Oops. Yes, things changed and I didn't catch that merge

> 
> > Source/WebCore/rendering/RenderLayerCompositor.cpp:900
> > +    if (childState.m_subtreeHasBlending || layer->hasBlendMode())
> > +        compositingState.m_subtreeHasBlending = true;
> > +
> >      // Now check for reasons to become composited that depend on the state of descendant layers.
> >      RenderLayer::IndirectCompositingReason indirectCompositingReason;
> >      if (!willBeComposited && canBeComposited(layer)
> > -        && requiresCompositingForIndirectReason(layer->renderer(), childState.m_subtreeIsCompositing, anyDescendantHas3DTransform, indirectCompositingReason)) {
> > +        && requiresCompositingForIndirectReason(layer->renderer(), childState.m_subtreeIsCompositing, compositingState.m_subtreeHasBlending, anyDescendantHas3DTransform, indirectCompositingReason)) {
> >          layer->setIndirectCompositingReason(indirectCompositingReason);
> >          childState.m_compositingAncestor = layer;
> 
> You should add some tests that exercise this code.

All blending goes through this code.

> 
> > Source/WebCore/rendering/RenderLayerCompositor.h:318
> > +    bool requiresCompositingForIndirectReason(RenderObject*, bool , 
> > +        bool , bool , RenderLayer::IndirectCompositingReason&) const;
> 
> Those bool parameter names are useful; I think you should keep them.

I put them back in.
Comment 35 Rik Cabanier 2012-11-14 16:54:00 PST
Created attachment 174288 [details]
Made testfile more clear
Comment 36 Rik Cabanier 2013-02-21 12:33:46 PST
Created attachment 189576 [details]
refreshed patch
Comment 37 Simon Fraser (smfr) 2013-02-21 13:39:37 PST
CA has no support for non-separable modes, btw.
Comment 38 Rik Cabanier 2013-02-21 14:05:29 PST
(In reply to comment #37)
> CA has no support for non-separable modes, btw.

I believe it does (see expected png file). What is not supported is if you create a cgcontext with an iosurface.
Comment 39 Rik Cabanier 2013-05-06 11:14:09 PDT
Created attachment 200723 [details]
Patch
Comment 40 Mihai Tica 2013-05-30 00:08:17 PDT
Created attachment 203313 [details]
Patch
Comment 41 Build Bot 2013-05-30 01:59:25 PDT
Comment on attachment 203313 [details]
Patch

Attachment 203313 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/759061

New failing tests:
css3/compositing/should-have-compositing-layer.html
Comment 42 Build Bot 2013-05-30 01:59:28 PDT
Created attachment 203326 [details]
Archive of layout-test-results from webkit-ews-03 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-03  Port: mac-mountainlion  Platform: Mac OS X 10.8.3
Comment 43 Mihai Tica 2013-05-30 04:26:20 PDT
Created attachment 203342 [details]
Patch
Comment 44 Mihai Tica 2013-06-11 10:56:48 PDT
Simon, can you please have another look at this? Thanks!
Comment 45 Simon Fraser (smfr) 2013-10-07 11:55:37 PDT
Comment on attachment 203342 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=203342&action=review

r-. We have to be able to do this without CIFilters.

> Source/WebCore/ChangeLog:3
> +        Rebasing patch.

Not a useful change log comment.

> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.h:104
> +    virtual void setBlendMode(BlendMode);

OVERRIDE

> Source/WebCore/platform/graphics/ca/mac/PlatformCALayerMac.mm:792
> +    [m_layer.get() setCompositingFilter:filter];

We can't use CI filters without extra work, because on Mavericks it will kick the WebProcess out of WindowServer-hosted compositing, and we need to add code to deal with the switch to fix plug-ins etc.
Comment 46 Dean Jackson 2013-10-07 12:01:43 PDT
We *might* be able to do some of the effects by exporting some private CoreAnimation API, but we'd have to make sure it plays nicely with the existing filter code.

CA_EXTERN NSString * const kCAFilterNormalBlendMode;
CA_EXTERN NSString * const kCAFilterMultiplyBlendMode;
CA_EXTERN NSString * const kCAFilterScreenBlendMode;
CA_EXTERN NSString * const kCAFilterOverlayBlendMode;
CA_EXTERN NSString * const kCAFilterDarkenBlendMode;
CA_EXTERN NSString * const kCAFilterLightenBlendMode;
CA_EXTERN NSString * const kCAFilterColorDodgeBlendMode;
CA_EXTERN NSString * const kCAFilterColorBurnBlendMode;
CA_EXTERN NSString * const kCAFilterSoftLightBlendMode;
CA_EXTERN NSString * const kCAFilterHardLightBlendMode;
CA_EXTERN NSString * const kCAFilterDifferenceBlendMode;
CA_EXTERN NSString * const kCAFilterExclusionBlendMode;

CA_EXTERN NSString * const kCAFilterSubtractBlendMode;
CA_EXTERN NSString * const kCAFilterLinearBurnBlendMode;
CA_EXTERN NSString * const kCAFilterLinearDodgeBlendMode;
CA_EXTERN NSString * const kCAFilterLinearLightBlendMode;
CA_EXTERN NSString * const kCAFilterPinLightBlendMode;
Comment 47 Mihai Tica 2013-10-09 01:30:06 PDT
(In reply to comment #46)
> We *might* be able to do some of the effects by exporting some private CoreAnimation API, but we'd have to make sure it plays nicely with the existing filter code.
> 
> CA_EXTERN NSString * const kCAFilterNormalBlendMode;
> CA_EXTERN NSString * const kCAFilterMultiplyBlendMode;
> CA_EXTERN NSString * const kCAFilterScreenBlendMode;
> CA_EXTERN NSString * const kCAFilterOverlayBlendMode;
> CA_EXTERN NSString * const kCAFilterDarkenBlendMode;
> CA_EXTERN NSString * const kCAFilterLightenBlendMode;
> CA_EXTERN NSString * const kCAFilterColorDodgeBlendMode;
> CA_EXTERN NSString * const kCAFilterColorBurnBlendMode;
> CA_EXTERN NSString * const kCAFilterSoftLightBlendMode;
> CA_EXTERN NSString * const kCAFilterHardLightBlendMode;
> CA_EXTERN NSString * const kCAFilterDifferenceBlendMode;
> CA_EXTERN NSString * const kCAFilterExclusionBlendMode;
> 
> CA_EXTERN NSString * const kCAFilterSubtractBlendMode;
> CA_EXTERN NSString * const kCAFilterLinearBurnBlendMode;
> CA_EXTERN NSString * const kCAFilterLinearDodgeBlendMode;
> CA_EXTERN NSString * const kCAFilterLinearLightBlendMode;
> CA_EXTERN NSString * const kCAFilterPinLightBlendMode;

I've tried using these filters without success, could you please provide some pointers in using them? Are there some specific properties that have to be set in order to make them work? I'm thinking of the setValue: forKey: selector
Comment 48 Dean Jackson 2013-10-09 15:19:01 PDT
Yeah, we'll have to expose the SPI for you to have access. I'll take a look.
Comment 49 Dean Jackson 2013-10-11 15:33:43 PDT
Created attachment 214027 [details]
Patch
Comment 50 Dean Jackson 2013-10-11 15:37:35 PDT
Here's a patch that works on Mac (Mountain Lion and Mavericks, but not Lion).

We can only support the separable blend modes in compositing. I've left the four non-separable modes unimplemented.

I'd like to do some testing on more complex content before actually deciding to land this or not. We still don't have any idea how to solve the problem of an intermediary compositing layer getting added to the tree, breaking our blending into our parent. A good example of this is <video>, which could be a bare compositing layer, or could be a hierarchy that contains sublayers for the controls.
Comment 51 Dean Jackson 2013-10-11 17:34:37 PDT
Created attachment 214039 [details]
Test case showing problem

Here's a test case that shows the major issue.
Notice the difference between example 3 and 4. In this case the blending element's parent has been composited. The blending still works fine into the parent, but does not blend above that. I think you want 3 in all cases (I think trying to describe why 4 happened would be difficult, and it could happen just because an animation is running on a sibling.
Comment 52 Dean Jackson 2013-10-11 17:38:00 PDT
Created attachment 214040 [details]
Screenshot of test case
Comment 53 Mihai Tica 2013-10-11 23:32:39 PDT
Great work!

(In reply to comment #50)
> Here's a patch that works on Mac (Mountain Lion and Mavericks, but not Lion).
Would falling back to CIFilters when detecting Lion be feasible?
> 
> We can only support the separable blend modes in compositing. I've left the four non-separable modes unimplemented.
> 
> I'd like to do some testing on more complex content before actually deciding to land this or not. We still don't have any idea how to solve the problem of an intermediary compositing layer getting added to the tree, breaking our blending into our parent. A good example of this is <video>, which could be a bare compositing layer, or could be a hierarchy that contains sublayers for the controls.
Comment 54 Mihai Tica 2013-10-14 08:47:22 PDT
(In reply to comment #51)
> Created an attachment (id=214039) [details]
> Test case showing problem
> 
> Here's a test case that shows the major issue.
> Notice the difference between example 3 and 4. In this case the blending element's parent has been composited. The blending still works fine into the parent, but does not blend above that. I think you want 3 in all cases (I think trying to describe why 4 happened would be difficult, and it could happen just because an animation is running on a sibling.

According to the spec, an element with mix-blend-mode (-webkit-blend-mode) specified should only blend with the stacking context right below it.
In the testcase attached, in example 4, the transform property creates a new stacking context for the background element, so it's actually correct for the topmost element to only blend with the background, and not the body of the document.

You can browse the spec (http://dev.w3.org/fxtf/compositing-1/#mix-blend-mode) at example 6, where this behaviour is described.

Also, we should probably make sure that this rule is followed before landing the patch and probably validate this with a few more tests. Please let me know if I can help with anything
Comment 55 Simon Fraser (smfr) 2013-10-14 10:35:25 PDT
Things to test/fix:
* parent stacking context is "paints into ancestor"
* compositing layer (e.g. for overflow:clip) between blended thing and its stacking context
* compositing layer for parent stacking context is not the size of the element because of other positioned elements, shadows, outline etc.

If a blended element is animated with left/transform, is it expected that it stops blending when it moves outside of the stacking context ancestor?
Comment 56 Rik Cabanier 2013-10-14 11:23:55 PDT
(In reply to comment #55)
> Things to test/fix:
> * parent stacking context is "paints into ancestor"
> * compositing layer (e.g. for overflow:clip) between blended thing and its stacking context
> * compositing layer for parent stacking context is not the size of the element because of other positioned elements, shadows, outline etc.

Firefox does some tricks with layers for "overflow:scroll" (which is not a stacking context). Would that apply for WK too?

> 
> If a blended element is animated with left/transform, is it expected that it stops blending when it moves outside of the stacking context ancestor?

Yes. It should only blend with the stacking context ancestor.
Comment 57 Dean Jackson 2013-10-14 11:55:04 PDT
I think we should land this now and add extra test cases in a follow-up (which I'm preparing now).
Comment 58 Simon Fraser (smfr) 2013-10-14 13:29:33 PDT
(In reply to comment #56)
> (In reply to comment #55)
> > Things to test/fix:
> > * parent stacking context is "paints into ancestor"
> > * compositing layer (e.g. for overflow:clip) between blended thing and its stacking context
> > * compositing layer for parent stacking context is not the size of the element because of other positioned elements, shadows, outline etc.
> 
> Firefox does some tricks with layers for "overflow:scroll" (which is not a stacking context). Would that apply for WK too?

Depends what "some tricks" means :)

> > If a blended element is animated with left/transform, is it expected that it stops blending when it moves outside of the stacking context ancestor?
> 
> Yes. It should only blend with the stacking context ancestor.

Authors are not going to like this.
Comment 59 Dean Jackson 2013-10-14 13:38:35 PDT
Created attachment 214184 [details]
Patch
Comment 60 Dean Jackson 2013-10-14 13:40:25 PDT
(In reply to comment #55)
> * compositing layer (e.g. for overflow:clip) between blended thing and its stacking context

New patch has a test that shows we fail this.

> If a blended element is animated with left/transform, is it expected that it stops blending when it moves outside of the stacking context ancestor?

According to www-style, the answer is yes. Or more precisely, it's still blending, just with nothing.
Comment 61 WebKit Commit Bot 2013-10-14 13:41:19 PDT
Attachment 214184 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/css3/compositing/blend-mode-layers-expected.txt', u'LayoutTests/css3/compositing/blend-mode-layers.html', u'LayoutTests/css3/compositing/blend-mode-overflow-expected.txt', u'LayoutTests/css3/compositing/blend-mode-overflow.html', u'LayoutTests/css3/compositing/effect-blend-mode-expected.txt', u'LayoutTests/css3/compositing/effect-blend-mode.html', u'LayoutTests/css3/compositing/reflected-blend-mode-expected.txt', u'LayoutTests/css3/compositing/reflected-blend-mode.html', u'LayoutTests/css3/compositing/should-have-compositing-layer-expected.txt', u'LayoutTests/css3/compositing/should-not-have-compositing-layer-expected.txt', u'LayoutTests/css3/compositing/should-not-have-compositing-layer.html', u'LayoutTests/platform/efl/TestExpectations', u'LayoutTests/platform/mac/css3/compositing/blend-mode-layers-expected.png', u'LayoutTests/platform/mac/css3/compositing/blend-mode-overflow-expected.png', u'LayoutTests/platform/mac/css3/compositing/effect-blend-mode-expected.png', u'LayoutTests/platform/mac/css3/compositing/reflected-blend-mode-expected.png', u'Source/WebCore/ChangeLog', u'Source/WebCore/WebCore.xcodeproj/project.pbxproj', u'Source/WebCore/platform/graphics/GraphicsLayer.cpp', u'Source/WebCore/platform/graphics/GraphicsLayer.h', u'Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp', u'Source/WebCore/platform/graphics/ca/GraphicsLayerCA.h', u'Source/WebCore/platform/graphics/ca/PlatformCAFilters.h', u'Source/WebCore/platform/graphics/ca/PlatformCALayer.h', u'Source/WebCore/platform/graphics/ca/mac/PlatformCAFiltersMac.h', u'Source/WebCore/platform/graphics/ca/mac/PlatformCAFiltersMac.mm', u'Source/WebCore/platform/graphics/ca/mac/PlatformCALayerMac.mm', u'Source/WebCore/rendering/RenderLayerBacking.cpp', u'Source/WebCore/rendering/RenderLayerBacking.h', u'Source/WebCore/rendering/RenderLayerCompositor.cpp', u'Source/WebCore/rendering/RenderLayerCompositor.h']" exit_code: 1
Source/WebCore/platform/graphics/ca/mac/PlatformCAFiltersMac.h:39:  Extra space before ( in function call  [whitespace/parens] [4]
Source/WebCore/platform/graphics/ca/mac/PlatformCAFiltersMac.h:46:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 2 in 29 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 62 Rik Cabanier 2013-10-14 14:33:47 PDT
(In reply to comment #58)
> (In reply to comment #56)
> > (In reply to comment #55)
> > > Things to test/fix:
> > > * parent stacking context is "paints into ancestor"
> > > * compositing layer (e.g. for overflow:clip) between blended thing and its stacking context
> > > * compositing layer for parent stacking context is not the size of the element because of other positioned elements, shadows, outline etc.
> > 
> > Firefox does some tricks with layers for "overflow:scroll" (which is not a stacking context). Would that apply for WK too?
> 
> Depends what "some tricks" means :)
> 
> > > If a blended element is animated with left/transform, is it expected that it stops blending when it moves outside of the stacking context ancestor?
> > 
> > Yes. It should only blend with the stacking context ancestor.
> 
> Authors are not going to like this.

I agree that this will generate some surprises but I'm pretty sure that they will get used to it, especially if the developer tools keep improving (so authors can see the stacking contexts).

Can you see a way to make non-isolated grouping workable?
Comment 63 Mihai Tica 2013-10-17 05:58:59 PDT
Created attachment 214448 [details]
Test case with preserve-3d

Blending currently fails when setting preserve-3d on an element.
Comment 64 Mihai Tica 2013-10-17 06:00:01 PDT
Created attachment 214449 [details]
Screenshot of blending with preserve-3d
Comment 65 Mihai Tica 2013-10-17 06:13:06 PDT
I've tested the patch with several accelerated elements such as canvas, video, plugin, 3d transform or iframe. Besides preserve-3d, in all other cases blending was performed.
Comment 66 Simon Fraser (smfr) 2013-10-17 08:52:10 PDT
Blending needs to force transform-style: flat like filters do.

What I'd like to see is a solution for:

<div style="position:absolute; z-index: 0;">
   <div style="overflow:hidden">
      <div style="blend-mode: difference; transform: translateZ(0)"></div>
   </div>
</div>
Comment 67 Mihai Tica 2013-10-17 11:17:28 PDT
Ok, I'll have have a look at this problem
(In reply to comment #66)
> Blending needs to force transform-style: flat like filters do.
> 
> What I'd like to see is a solution for:
> 
> <div style="position:absolute; z-index: 0;">
>    <div style="overflow:hidden">
>       <div style="blend-mode: difference; transform: translateZ(0)"></div>
>    </div>
> </div>
Comment 68 Mihai Tica 2013-10-21 06:51:08 PDT
Created attachment 214735 [details]
Patch
Comment 69 Mihai Tica 2013-10-21 06:55:12 PDT
(In reply to comment #68)
> Created an attachment (id=214735) [details]
> Patch

I've added a solution to the overflow:hidden problem with this patch, could you please have a look?
Comment 70 Simon Fraser (smfr) 2013-10-21 10:31:42 PDT
(In reply to comment #69)
> (In reply to comment #68)
> > Created an attachment (id=214735) [details] [details]
> > Patch
> 
> I've added a solution to the overflow:hidden problem with this patch, could you please have a look?

Could you explain it?
Comment 71 Mihai Tica 2013-10-21 23:55:50 PDT
(In reply to comment #70)
> (In reply to comment #69)
> > (In reply to comment #68)
> > > Created an attachment (id=214735) [details] [details] [details]
> > > Patch
> > 
> > I've added a solution to the overflow:hidden problem with this patch, could you please have a look?
> 
> Could you explain it?

When having a parent with "overflow:hidden", an ancestor clipping layer is additionally created for the element. When detecting the presence of this layer, I set the blendMode for this layer, causing the filter operation to be executed with the underlying content, which in our case is the backdrop needed for blending.

The change can be found in Source/WebCore/rendering/RenderLayerBacking.cpp:379
Comment 72 Mihai Tica 2013-10-31 09:08:14 PDT
The previous patch didn't provide functionality for accelerated parents with overflow:hidden, for example:

<div class="parent" style="overflow:hidden; -webkit-transform: translateZ(0)">
    <div class="blendedChild" style="-webkit-blend-mode: difference"></div>
    <div class="nonBlendedChild"></div>
</div>

For this particular case, the parent element creates a clipping layer (m_childContainmentLayer), meaning that the blended child won't get an m_ancestorClippingLayer, breaking the previous functionality.
Setting the blend mode on the parent clipping layer is wrong because it would cause all of the parent's children to blend, instead of just the ones that have blending set.

To address this problem, I've found the following solution, which I've managed to validate with a prototype:

When detecting an element that should create a clipping layer, while also having blended children, we disallow the clipping layer creation and mark the element.

For each child layer, we check whether it has a blend mode or a subtree with a blended child.
If so, we create an ancestor clipping layer (m_ancestorClippingLayer) and set its size/location according the what the clipping layer for the parent should have been.
Otherwise, we have detected a child that doesn't have blending (neither the current element, nor its children), so we create a clipping layer.

When setting the blend mode for the layer, we check for the existence of an ancestor clipping layer.
If it exists, we set the CAFilter on it, otherwise we set the filter on the main graphics layer.

Would this approach be acceptable? Are there any other things that we missed or that we should take into account? Do you see any other viable options?
Comment 73 Simon Fraser (smfr) 2013-10-31 09:24:35 PDT
Does putting the filter on the clipping layer do the right thing, e.g. with blur? Does it mask then blur (which would be wrong), or blur then mask?
Comment 74 Rik Cabanier 2013-10-31 11:20:37 PDT
(In reply to comment #73)
> Does putting the filter on the clipping layer do the right thing, e.g. with blur? Does it mask then blur (which would be wrong), or blur then mask?

If you put a filter on the element with clipping, that element becomes a stacking context.
This means that we don't need access to the backdrop of the element with the clip so we don't need this workaround.
Comment 75 Simon Fraser (smfr) 2013-10-31 11:24:59 PDT
(In reply to comment #74)
> (In reply to comment #73)
> > Does putting the filter on the clipping layer do the right thing, e.g. with blur? Does it mask then blur (which would be wrong), or blur then mask?
> 
> If you put a filter on the element with clipping, that element becomes a stacking context.
> This means that we don't need access to the backdrop of the element with the clip so we don't need this workaround.

 I think you misunderstood. I was referring to:

> When setting the blend mode for the layer, we check for the existence of an ancestor clipping layer.
> If it exists, we set the CAFilter on it, otherwise we set the filter on the main graphics layer.

So you put the CAFilter on the ancestor clipping layer (which has -masksToBounds). Does this combination of masking and filtering give the correct result with blur?
Comment 76 Mihai Tica 2013-11-01 08:46:55 PDT
(In reply to comment #75)
> (In reply to comment #74)
> > (In reply to comment #73)
> > > Does putting the filter on the clipping layer do the right thing, e.g. with blur? Does it mask then blur (which would be wrong), or blur then mask?
> > 
> > If you put a filter on the element with clipping, that element becomes a stacking context.
> > This means that we don't need access to the backdrop of the element with the clip so we don't need this workaround.
> 
>  I think you misunderstood. I was referring to:
> 
> > When setting the blend mode for the layer, we check for the existence of an ancestor clipping layer.
> > If it exists, we set the CAFilter on it, otherwise we set the filter on the main graphics layer.
> 
> So you put the CAFilter on the ancestor clipping layer (which has -masksToBounds). Does this combination of masking and filtering give the correct result with blur?

I'm not sure if blur is the best example here since it's not used as a filter for any blending operation, so it won't be set on the ancestor clipping layer.

Suppose an element has a mask, blending, and creates a clipping layer according to the model in #72.
In this case, the content would draw, masking would then be performed, followed in the end by the compositing filter specified by the blend mode.

If this element also has a filter, such as blur, the content is first drawn and then blurred. Next, the mask is applied and the results gets blended with the underlying content.
This behavior is stated as correct in the blending and compositing spec (see http://dev.w3.org/fxtf/compositing-1/#compositingandblendingorder), so I don't think this might be a problem.
Comment 77 Mihai Tica 2013-11-18 10:05:46 PST
Is this approach acceptable or should we investigate an alternative?
Simon, Dean, could you please comment on this?
Comment 78 Simon Fraser (smfr) 2013-11-21 11:16:34 PST
My main concern here is that we'll be hampered from making future changes to the CALayer tree by constraints imposed by blending; we might be able to special-case layers for overflow, .but in general, any CALayer tree config change that results in the blending layer not having the stacking context layers as its direct parent will break the rendering.
Comment 79 Mihai Tica 2013-12-06 07:18:22 PST
I did an investigation to find use cases similar to overflow:hidden and haven't found any additional ones.
I checked for accelerated elements that don't create a stacking context (found these in the list from WebCore/rendering/CompositionReasons.h) and parented them to an element that creates a stacking context, while also adding a blended child element. In all of these cases, the blending operation is performed with the topmost element (the stacking context), as stated in the spec.

Since we haven't found any additional issues, would it perhaps be acceptable to starting work on this?
Comment 80 Mihai Tica 2013-12-17 10:02:17 PST
Any updates? Could you please comment on this?
Comment 81 Simon Fraser (smfr) 2013-12-17 10:45:28 PST
Comment on attachment 214735 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=214735&action=review

> Source/WebCore/ChangeLog:12
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Tests: css3/compositing/blend-mode-layers.html

This needs some text summarizing the changes in the patch.

> Source/WebCore/rendering/RenderLayerBacking.cpp:384
> +    if (hasAncestorClippingLayer())
> +        ancestorClippingLayer()->setBlendMode(style->blendMode());
> +
> +    m_graphicsLayer->setBlendMode(style->blendMode());

Do you really want to set it on both here?

> Source/WebCore/rendering/RenderLayerCompositor.cpp:1061
> +        // If the layer composited for other reasons than blending, it is no longer needed to keep track if a child was blended

"keep track of whether a child"

> Source/WebCore/rendering/RenderLayerCompositor.h:85
> +    CompositingReasonRoot                                   = 1 << 23,
> +    CompositingReasonBlending                               = 1 << 24

The web inspector exposes these to authors. You should file a follow-up to ensure that the inspector shows blending as a layer creation reason.
Comment 82 Mihai Tica 2013-12-23 04:52:46 PST
Created attachment 219908 [details]
Patch
Comment 83 WebKit Commit Bot 2013-12-23 04:55:55 PST
Attachment 219908 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/css3/compositing/blend-mode-layers.html', u'LayoutTests/css3/compositing/blend-mode-overflow-expected.txt', u'LayoutTests/css3/compositing/blend-mode-overflow.html', u'LayoutTests/css3/compositing/blend-mode-reflection.html', u'LayoutTests/css3/compositing/blend-mode-should-not-have-compositing-layer-expected.txt', u'LayoutTests/css3/compositing/blend-mode-should-not-have-compositing-layer.html', u'LayoutTests/css3/compositing/blend-mode-simple.html', u'LayoutTests/css3/compositing/should-have-compositing-layer-expected.txt', u'LayoutTests/platform/efl/TestExpectations', u'LayoutTests/platform/mac/css3/compositing/blend-mode-layers-expected.png', u'LayoutTests/platform/mac/css3/compositing/blend-mode-layers-expected.txt', u'LayoutTests/platform/mac/css3/compositing/blend-mode-overflow-expected.png', u'LayoutTests/platform/mac/css3/compositing/blend-mode-reflection-expected.png', u'LayoutTests/platform/mac/css3/compositing/blend-mode-reflection-expected.txt', u'LayoutTests/platform/mac/css3/compositing/blend-mode-simple-expected.png', u'LayoutTests/platform/mac/css3/compositing/blend-mode-simple-expected.txt', u'Source/WebCore/ChangeLog', u'Source/WebCore/WebCore.exp.in', u'Source/WebCore/WebCore.xcodeproj/project.pbxproj', u'Source/WebCore/platform/graphics/GraphicsLayer.cpp', u'Source/WebCore/platform/graphics/GraphicsLayer.h', u'Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp', u'Source/WebCore/platform/graphics/ca/GraphicsLayerCA.h', u'Source/WebCore/platform/graphics/ca/PlatformCAFilters.h', u'Source/WebCore/platform/graphics/ca/PlatformCALayer.h', u'Source/WebCore/platform/graphics/ca/mac/PlatformCAFiltersMac.h', u'Source/WebCore/platform/graphics/ca/mac/PlatformCAFiltersMac.mm', u'Source/WebCore/platform/graphics/ca/mac/PlatformCALayerMac.mm', u'Source/WebCore/rendering/RenderLayerBacking.cpp', u'Source/WebCore/rendering/RenderLayerBacking.h', u'Source/WebCore/rendering/RenderLayerCompositor.cpp', u'Source/WebCore/rendering/RenderLayerCompositor.h', '--commit-queue']" exit_code: 1
Traceback (most recent call last):
  File "Tools/Scripts/check-webkit-style", line 48, in <module>
    sys.exit(CheckWebKitStyle().main())
  File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/style/main.py", line 154, in main
    patch_checker.check(patch)
  File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/style/patchreader.py", line 71, in check
    self._text_file_reader.process_file(file_path=path, line_numbers=None)
  File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/style/filereader.py", line 118, in process_file
    lines = self._read_lines(file_path)
  File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/style/filereader.py", line 86, in _read_lines
    contents = file.read()
  File "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/codecs.py", line 671, in read
    return self.reader.read(size)
  File "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/codecs.py", line 477, in read
    newchars, decodedbytes = self.decode(data, self.errors)
UnicodeDecodeError: 'utf8' codec can't decode byte 0x89 in position 0: invalid start byte


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 84 Mihai Tica 2013-12-23 05:51:02 PST
Created attachment 219911 [details]
Patch for review
Comment 85 WebKit Commit Bot 2013-12-23 05:54:07 PST
Attachment 219911 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/css3/compositing/blend-mode-layers.html', u'LayoutTests/css3/compositing/blend-mode-overflow-expected.txt', u'LayoutTests/css3/compositing/blend-mode-overflow.html', u'LayoutTests/css3/compositing/blend-mode-reflection.html', u'LayoutTests/css3/compositing/blend-mode-should-not-have-compositing-layer-expected.txt', u'LayoutTests/css3/compositing/blend-mode-should-not-have-compositing-layer.html', u'LayoutTests/css3/compositing/blend-mode-simple.html', u'LayoutTests/css3/compositing/should-have-compositing-layer-expected.txt', u'LayoutTests/platform/efl/TestExpectations', u'LayoutTests/platform/mac/css3/compositing/blend-mode-layers-expected.png', u'LayoutTests/platform/mac/css3/compositing/blend-mode-layers-expected.txt', u'LayoutTests/platform/mac/css3/compositing/blend-mode-overflow-expected.png', u'LayoutTests/platform/mac/css3/compositing/blend-mode-reflection-expected.png', u'LayoutTests/platform/mac/css3/compositing/blend-mode-reflection-expected.txt', u'LayoutTests/platform/mac/css3/compositing/blend-mode-simple-expected.png', u'LayoutTests/platform/mac/css3/compositing/blend-mode-simple-expected.txt', u'Source/WebCore/ChangeLog', u'Source/WebCore/WebCore.exp.in', u'Source/WebCore/WebCore.xcodeproj/project.pbxproj', u'Source/WebCore/platform/graphics/GraphicsLayer.cpp', u'Source/WebCore/platform/graphics/GraphicsLayer.h', u'Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp', u'Source/WebCore/platform/graphics/ca/GraphicsLayerCA.h', u'Source/WebCore/platform/graphics/ca/PlatformCAFilters.h', u'Source/WebCore/platform/graphics/ca/PlatformCALayer.h', u'Source/WebCore/platform/graphics/ca/mac/PlatformCAFiltersMac.h', u'Source/WebCore/platform/graphics/ca/mac/PlatformCAFiltersMac.mm', u'Source/WebCore/platform/graphics/ca/mac/PlatformCALayerMac.mm', u'Source/WebCore/rendering/RenderLayerBacking.cpp', u'Source/WebCore/rendering/RenderLayerBacking.h', u'Source/WebCore/rendering/RenderLayerCompositor.cpp', u'Source/WebCore/rendering/RenderLayerCompositor.h', '--commit-queue']" exit_code: 1
Traceback (most recent call last):
  File "Tools/Scripts/check-webkit-style", line 48, in <module>
    sys.exit(CheckWebKitStyle().main())
  File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/style/main.py", line 154, in main
    patch_checker.check(patch)
  File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/style/patchreader.py", line 71, in check
    self._text_file_reader.process_file(file_path=path, line_numbers=None)
  File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/style/filereader.py", line 118, in process_file
    lines = self._read_lines(file_path)
  File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/style/filereader.py", line 86, in _read_lines
    contents = file.read()
  File "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/codecs.py", line 671, in read
    return self.reader.read(size)
  File "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/codecs.py", line 477, in read
    newchars, decodedbytes = self.decode(data, self.errors)
UnicodeDecodeError: 'utf8' codec can't decode byte 0x89 in position 0: invalid start byte


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 86 Mihai Tica 2014-01-06 08:26:43 PST
Please take another look
Comment 87 Dirk Schulze 2014-01-09 09:00:08 PST
Comment on attachment 219911 [details]
Patch for review

rs=me
Comment 88 WebKit Commit Bot 2014-01-09 09:26:54 PST
Comment on attachment 219911 [details]
Patch for review

Rejecting attachment 219911 [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', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 219911, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
c8f
r161556 = cb8504d4c7ab896253bbf8c7e1eb58ef5c4634c2
r161557 = 4a2c6f6ea09968b0ab42260c19fee2966b2eb915
r161558 = 8fb8a5c7a2a39884cf5dc9a050fc498578c62114
Done rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc
First, rewinding head to replay your work on top of it...
Fast-forwarded master to refs/remotes/origin/master.
ERROR: LayoutTests/platform/efl/TestExpectations:499:  Path does not exist.  [test/expectations] [5]
Total errors found: 1 in 1 files

Full output: http://webkit-queues.appspot.com/results/5068903893958656
Comment 89 Joseph Pecoraro 2014-01-09 10:39:46 PST
Comment on attachment 219911 [details]
Patch for review

View in context: https://bugs.webkit.org/attachment.cgi?id=219911&action=review

> Source/WebCore/rendering/RenderLayerCompositor.h:86
> +    CompositingReasonBlending                               = 1 << 24

We should have the inspector list this reason:

  - Source/WebCore/inspector/prototype/LayerTree.json => update CompositingReasons to include "blending"
  - Source/WebCore/inspector/InspectorLayerTreeAgent.cpp => update InspectorLayerTreeAgent::reasonsForCompositingLayer to set blending
  - Source/WebInspectorUI/UserInterface/LayerTreeSidebarPanel.js => update _populateListOfCompositingReasons to add a user readable string when blending is a reason

Seeing as this landed I'll whip up an inspector fix:
<https://webkit.org/b/126706> Web Inspector: New Reason for Layers: CompositingReasonBlending
Comment 90 Mihai Tica 2014-01-10 01:16:03 PST
Created attachment 220819 [details]
Patch
Comment 91 WebKit Commit Bot 2014-01-10 01:18:34 PST
Attachment 220819 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/css3/compositing/blend-mode-layers.html', u'LayoutTests/css3/compositing/blend-mode-overflow-expected.txt', u'LayoutTests/css3/compositing/blend-mode-overflow.html', u'LayoutTests/css3/compositing/blend-mode-reflection.html', u'LayoutTests/css3/compositing/blend-mode-should-not-have-compositing-layer-expected.txt', u'LayoutTests/css3/compositing/blend-mode-should-not-have-compositing-layer.html', u'LayoutTests/css3/compositing/blend-mode-simple.html', u'LayoutTests/css3/compositing/should-have-compositing-layer-expected.txt', u'LayoutTests/platform/efl/TestExpectations', u'LayoutTests/platform/mac/css3/compositing/blend-mode-layers-expected.png', u'LayoutTests/platform/mac/css3/compositing/blend-mode-layers-expected.txt', u'LayoutTests/platform/mac/css3/compositing/blend-mode-overflow-expected.png', u'LayoutTests/platform/mac/css3/compositing/blend-mode-reflection-expected.png', u'LayoutTests/platform/mac/css3/compositing/blend-mode-reflection-expected.txt', u'LayoutTests/platform/mac/css3/compositing/blend-mode-simple-expected.png', u'LayoutTests/platform/mac/css3/compositing/blend-mode-simple-expected.txt', u'Source/WebCore/ChangeLog', u'Source/WebCore/WebCore.exp.in', u'Source/WebCore/WebCore.xcodeproj/project.pbxproj', u'Source/WebCore/platform/graphics/GraphicsLayer.cpp', u'Source/WebCore/platform/graphics/GraphicsLayer.h', u'Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp', u'Source/WebCore/platform/graphics/ca/GraphicsLayerCA.h', u'Source/WebCore/platform/graphics/ca/PlatformCAFilters.h', u'Source/WebCore/platform/graphics/ca/PlatformCALayer.h', u'Source/WebCore/platform/graphics/ca/mac/PlatformCAFiltersMac.h', u'Source/WebCore/platform/graphics/ca/mac/PlatformCAFiltersMac.mm', u'Source/WebCore/platform/graphics/ca/mac/PlatformCALayerMac.mm', u'Source/WebCore/rendering/RenderLayerBacking.cpp', u'Source/WebCore/rendering/RenderLayerBacking.h', u'Source/WebCore/rendering/RenderLayerCompositor.cpp', u'Source/WebCore/rendering/RenderLayerCompositor.h', '--commit-queue']" exit_code: 1
Traceback (most recent call last):
  File "Tools/Scripts/check-webkit-style", line 48, in <module>
    sys.exit(CheckWebKitStyle().main())
  File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/style/main.py", line 154, in main
    patch_checker.check(patch)
  File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/style/patchreader.py", line 71, in check
    self._text_file_reader.process_file(file_path=path, line_numbers=None)
  File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/style/filereader.py", line 118, in process_file
    lines = self._read_lines(file_path)
  File "/Volumes/Data/StyleQueue/WebKit/Tools/Scripts/webkitpy/style/filereader.py", line 86, in _read_lines
    contents = file.read()
  File "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/codecs.py", line 671, in read
    return self.reader.read(size)
  File "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/codecs.py", line 477, in read
    newchars, decodedbytes = self.decode(data, self.errors)
UnicodeDecodeError: 'utf8' codec can't decode byte 0x89 in position 0: invalid start byte


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 92 WebKit Commit Bot 2014-01-10 03:26:16 PST
Comment on attachment 220819 [details]
Patch

Clearing flags on attachment: 220819

Committed r161628: <http://trac.webkit.org/changeset/161628>
Comment 93 Brent Fulgham 2014-01-30 11:29:01 PST
Comment on attachment 214184 [details]
Patch

Clearing review request flag since this is closed.