[CSS Blending] Implement mix-blend-mode in software.

[CSS Blending] Implement mix-blend-mode in software.
The implementation follows the CSS Blending and compositing
spec at http://dev.w3.org/fxtf/compositing-1/

It seems that there might be slightly better performance if we use the software path for CSS blending. Basically, on the hardware path, there are some bitmap copy operations involved, and we'd like to avoid this whenever possible.

The hardware implementation, currently in the works will only be followed for elements that force the creation of a composited layer.

BUG=288655
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=159061

[mitica, 2013-09-10 20:08:18]

Hi, can you please review?
Thanks!

[jamesr, 2013-09-10 20:17:34]

I'm not a good reviewer for this patch.  Perhaps some of the skia/graphics folks
(pdr/schenney/fmalita) could take a look.

[shawnsingh, 2013-09-10 21:23:40]

I have some thoughts on this.

https://codereview.chromium.org/23503046/diff/1/Source/core/rendering/Composi...
File Source/core/rendering/CompositingReasons.h (right):

https://codereview.chromium.org/23503046/diff/1/Source/core/rendering/Composi...
Source/core/rendering/CompositingReasons.h:28: // FIXME: remove the following
compositing reason since it's no longer used.
I think it's best to address this FIXME in this patch.   It should be possible
to remove it on the chromium side first to make this possible.

https://codereview.chromium.org/23503046/diff/1/Source/core/rendering/RenderL...
File Source/core/rendering/RenderLayer.cpp (right):

https://codereview.chromium.org/23503046/diff/1/Source/core/rendering/RenderL...
Source/core/rendering/RenderLayer.cpp:428: if (child->isStackingContext())
The ordering of this logic here is a little bit nuanced, and could come back to
bite us.   I think it would be better to rewrite this boolean logic equivalently
so that we don't have continue statements interlaced with return statements.

for example:

if (!child->isStackingContext() &&
child->hasNonAcceleratedBlendedChildInCurrentStackingContext())
  return true

in my personal opinion will be better =)

https://codereview.chromium.org/23503046/diff/1/Source/core/rendering/RenderL...
Source/core/rendering/RenderLayer.cpp:3573:
context->beginTransparencyLayer(renderer()->opacity());
Would it be possible to re-use the existing beginTransparencyLayer/endLayer
logic?  I think we could put hasBlendMode() inside paintsWithTransparency()  
and also inside bool haveTransparency.

There might be subtlety that I'm not seeing though... but if it could work, I
think it's a good idea to have only one point int he painting code that
creates/ends transparency layers, instead of adding a second one here.

[mitica, 2013-09-11 16:04:14]

Thanks for the review, uploading a revisited version of this patch.

https://codereview.chromium.org/23503046/diff/1/Source/core/rendering/Composi...
File Source/core/rendering/CompositingReasons.h (right):

https://codereview.chromium.org/23503046/diff/1/Source/core/rendering/Composi...
Source/core/rendering/CompositingReasons.h:28: // FIXME: remove the following
compositing reason since it's no longer used.
On 2013/09/10 21:23:40, shawnsingh wrote:
> I think it's best to address this FIXME in this patch.   It should be possible
> to remove it on the chromium side first to make this possible.

The only problem with this would be that removing the blending flag from
https://code.google.com/p/chromium/codesearch#chromium/src/cc/layers/composit...
would create a 1 bit gap in the reasons list.
This cannot be avoided due to the compile asserts in blink in
https://code.google.com/p/chromium/codesearch#chromium/src/webkit/renderer/co...

Anyhow, could you please have a look at https://codereview.chromium.org/23530026
first? After this is finished, I will be able to completely remove the blending
compositing reason on the blink side :)

https://codereview.chromium.org/23503046/diff/1/Source/core/rendering/RenderL...
File Source/core/rendering/RenderLayer.cpp (right):

https://codereview.chromium.org/23503046/diff/1/Source/core/rendering/RenderL...
Source/core/rendering/RenderLayer.cpp:428: if (child->isStackingContext())
On 2013/09/10 21:23:40, shawnsingh wrote:
> The ordering of this logic here is a little bit nuanced, and could come back
to
> bite us.   I think it would be better to rewrite this boolean logic
equivalently
> so that we don't have continue statements interlaced with return statements.
> 
> for example:
> 
> if (!child->isStackingContext() &&
> child->hasNonAcceleratedBlendedChildInCurrentStackingContext())
>   return true
> 
> in my personal opinion will be better =)

Sounds good, I'll rewrite this method.

https://codereview.chromium.org/23503046/diff/1/Source/core/rendering/RenderL...
Source/core/rendering/RenderLayer.cpp:3573:
context->beginTransparencyLayer(renderer()->opacity());
On 2013/09/10 21:23:40, shawnsingh wrote:
> Would it be possible to re-use the existing beginTransparencyLayer/endLayer
> logic?  I think we could put hasBlendMode() inside paintsWithTransparency()  
> and also inside bool haveTransparency.
> 
> There might be subtlety that I'm not seeing though... but if it could work, I
> think it's a good idea to have only one point int he painting code that
> creates/ends transparency layers, instead of adding a second one here.

I've refactored my code to use this logic, good suggestion!

[mitica, 2013-09-12 16:24:31]

Can you please have another look at this? Thanks!

[shawnsingh, 2013-09-13 10:26:31]

On 2013/09/12 16:24:31, mitica wrote:
> Can you please have another look at this? Thanks!

Will try to have a look tomorrow (Friday).

[mitica, 2013-09-16 08:46:42]

Completely remove CompositingReasonBlending

[mitica, 2013-09-18 14:37:21]

Can you please have another look? Thanks!

[shawnsingh, 2013-09-18 23:58:38]

On 2013/09/18 14:37:21, mitica wrote:
> Can you please have another look? Thanks!

Sorry for the delay! I will make sure to do it tonight.

[shawnsingh, 2013-09-19 11:05:54]

This looks solid as far as I can tell =)

There is only one issue about the isComposited() condition.  Other comments are
just nitpicks which I think make things easier to understand.

I'm pretty sure I would approve of the next patch, and then we can ask an OWNER
to take a look.

https://codereview.chromium.org/23503046/diff/13001/Source/core/rendering/Ren...
File Source/core/rendering/RenderLayer.cpp (right):

https://codereview.chromium.org/23503046/diff/13001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayer.cpp:424: if (isComposited())

So I've just recently debugged two issues about this code pattern.   I found
that it is incorrect to use "isComposited()" to mean "will this layer paint into
its own composited backing".   There are situations where a layer has a
RenderLayerBacking pointer, but it actually still paints into its ancestor
compositing layer.

It sounds like you intend "return false if this layer does not paint into its
own backing".  Then the correct condition here would be if (isComposited() &&
!backing()->paintsIntoCompositedAncestor())

I am working on a patch right now that will prevent this type of
misunderstanding.

https://codereview.chromium.org/23503046/diff/13001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayer.cpp:430: if (!childIsBlended &&
child->isStackingContext())

Yeah this is much nicer - but I still think we can make these conditions even
better.  Here's specifically what I imagine:

if (childIsBlended)
    return true;

// Recursion should not continue across stacking context boundaries.
if (!child->isStackingContext &&
child->hasNonAcceleratedBlendedChildInCurrentStackingContext())
    return true;

I prefer this because each statement is now one intuitive condition that answers
the question asked in this function.

https://codereview.chromium.org/23503046/diff/13001/Source/core/rendering/Ren...
File Source/core/rendering/RenderLayer.h (right):

https://codereview.chromium.org/23503046/diff/13001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayer.h:1153: // only be performed with the contents
of the underlying
Some nitpicks here - 

(1) Is it correct to say "A layer with a mix-blend-mode should only be blended
with content underneath this layer, within the same stacking context" ?  I think
that would be more clear.   The word "underlying" here (and actually, also in
the spec) feels a bit unclear to me.

(2) In my opinion, the name "m_isolateForDescendantBlendMode" might be clearer
here.

[mitica, 2013-09-19 11:48:59]

https://codereview.chromium.org/23503046/diff/13001/Source/core/rendering/Ren...
File Source/core/rendering/RenderLayer.cpp (right):

https://codereview.chromium.org/23503046/diff/13001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayer.cpp:430: if (!childIsBlended &&
child->isStackingContext())
On 2013/09/19 11:05:56, shawnsingh wrote:
> 
> Yeah this is much nicer - but I still think we can make these conditions even
> better.  Here's specifically what I imagine:
> 
> if (childIsBlended)
>     return true;
> 
> // Recursion should not continue across stacking context boundaries.
> if (!child->isStackingContext &&
> child->hasNonAcceleratedBlendedChildInCurrentStackingContext())
>     return true;
> 
> I prefer this because each statement is now one intuitive condition that
answers
> the question asked in this function.

Indeed, that looks better, I'll change the method according to your suggestion
:)

https://codereview.chromium.org/23503046/diff/13001/Source/core/rendering/Ren...
File Source/core/rendering/RenderLayer.h (right):

https://codereview.chromium.org/23503046/diff/13001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayer.h:1153: // only be performed with the contents
of the underlying
On 2013/09/19 11:05:56, shawnsingh wrote:
> Some nitpicks here - 
> 
> (1) Is it correct to say "A layer with a mix-blend-mode should only be blended
> with content underneath this layer, within the same stacking context" ?  I
think
> that would be more clear.   The word "underlying" here (and actually, also in
> the spec) feels a bit unclear to me.
> 
> (2) In my opinion, the name "m_isolateForDescendantBlendMode" might be clearer
> here. 

Sounds good, I'll change it.

[mitica, 2013-09-19 12:23:17]

Can you please have another look? Thank you!

[shawnsingh, 2013-09-21 10:38:53]

non-OWNER LGTM.  Thanks for your patience while addressing my feedback.

I think Julien is appropriate to request an OWNER review.

[Julien - ping for review, 2013-09-23 18:55:38]

https://codereview.chromium.org/23503046/diff/2/Source/core/rendering/RenderL...
File Source/core/rendering/RenderLayer.cpp (right):

https://codereview.chromium.org/23503046/diff/2/Source/core/rendering/RenderL...
Source/core/rendering/RenderLayer.cpp:422: if (!child->isStackingContext() &&
child->hasNonAcceleratedBlendedChildInCurrentStackingContext())
Doesn't this means we will walk the whole subtree for every paint as most
renderers are not stacking contexts and don't have blend-mode set?

This really looks like a descendant dependent flag, which would prevent the
potential tree walk explosion.

https://codereview.chromium.org/23503046/diff/2/Source/core/rendering/RenderL...
Source/core/rendering/RenderLayer.cpp:3296: m_isolateForDescendantBlendMode =
isStackingContext() && hasNonAcceleratedBlendedChildInCurrentStackingContext();
Trying to understand the need to have this stored as a member of RenderLayer: it
seems that it could be an argument [actually more
m_isolateForDescendantBlendMode || hasBlendMode()] to beginTransparencyLayers
but are you concerned about recursive calls into this code?

[mitica, 2013-09-23 20:12:19]

https://codereview.chromium.org/23503046/diff/2/Source/core/rendering/RenderL...
File Source/core/rendering/RenderLayer.cpp (right):

https://codereview.chromium.org/23503046/diff/2/Source/core/rendering/RenderL...
Source/core/rendering/RenderLayer.cpp:422: if (!child->isStackingContext() &&
child->hasNonAcceleratedBlendedChildInCurrentStackingContext())
On 2013/09/23 18:55:38, Julien Chaffraix wrote:
> Doesn't this means we will walk the whole subtree for every paint as most
> renderers are not stacking contexts and don't have blend-mode set?
> 
> This really looks like a descendant dependent flag, which would prevent the
> potential tree walk explosion.

If, for each time a blend mode would be set on a RenderLayer, we'd call a method
that sets a flag (call it |m_childIsBlended|, for example) for each parent
recursively, it would be more efficient. Is this the approach you are
suggesting? However, we'd have to call this recursive setter even for more odd
cases, for example, when the RenderLayer is removed.

https://codereview.chromium.org/23503046/diff/2/Source/core/rendering/RenderL...
Source/core/rendering/RenderLayer.cpp:3296: m_isolateForDescendantBlendMode =
isStackingContext() && hasNonAcceleratedBlendedChildInCurrentStackingContext();
On 2013/09/23 18:55:38, Julien Chaffraix wrote:
> Trying to understand the need to have this stored as a member of RenderLayer:
it
> seems that it could be an argument [actually more
> m_isolateForDescendantBlendMode || hasBlendMode()] to beginTransparencyLayers
> but are you concerned about recursive calls into this code?

This member is computed using the recursive layer traversing method above
(hasNonAcceleratedBlendedChildInCurrentStackingContext), I'm using it just to
avoid running this method twice: once for deciding wether to call
beginTransparencyLayers, and the other one to avoid recursive calls.

[Julien - ping for review, 2013-09-23 22:15:08]

https://codereview.chromium.org/23503046/diff/2/Source/core/rendering/RenderL...
File Source/core/rendering/RenderLayer.cpp (right):

https://codereview.chromium.org/23503046/diff/2/Source/core/rendering/RenderL...
Source/core/rendering/RenderLayer.cpp:422: if (!child->isStackingContext() &&
child->hasNonAcceleratedBlendedChildInCurrentStackingContext())
On 2013/09/23 20:12:20, mitica wrote:
> On 2013/09/23 18:55:38, Julien Chaffraix wrote:
> > Doesn't this means we will walk the whole subtree for every paint as most
> > renderers are not stacking contexts and don't have blend-mode set?
> > 
> > This really looks like a descendant dependent flag, which would prevent the
> > potential tree walk explosion.
> 
> If, for each time a blend mode would be set on a RenderLayer, we'd call a
method
> that sets a flag (call it |m_childIsBlended|, for example) for each parent
> recursively, it would be more efficient. Is this the approach you are
> suggesting? However, we'd have to call this recursive setter even for more odd
> cases, for example, when the RenderLayer is removed.

Yes and this is a fairly common paradigm in RenderLayer, look at what
updateDescendantDependentFlags does and the flags in there. Adding a potential
full tree walk for each paint is a definite no from my side.

https://codereview.chromium.org/23503046/diff/2/Source/core/rendering/RenderL...
Source/core/rendering/RenderLayer.cpp:3296: m_isolateForDescendantBlendMode =
isStackingContext() && hasNonAcceleratedBlendedChildInCurrentStackingContext();
On 2013/09/23 20:12:20, mitica wrote:
> On 2013/09/23 18:55:38, Julien Chaffraix wrote:
> > Trying to understand the need to have this stored as a member of
RenderLayer:
> it
> > seems that it could be an argument [actually more
> > m_isolateForDescendantBlendMode || hasBlendMode()] to
beginTransparencyLayers
> > but are you concerned about recursive calls into this code?
> 
> This member is computed using the recursive layer traversing method above
> (hasNonAcceleratedBlendedChildInCurrentStackingContext), I'm using it just to
> avoid running this method twice: once for deciding wether to call
> beginTransparencyLayers, and the other one to avoid recursive calls.

That's not what I meant: it is possible for this code (paintLayerContent) to be
called recursively inside a reflection. This would justify the need to have a
member variable (it's the reason we have a member called
m_paintingInsideReflection).

If you are only avoiding the cost of recomputing it, I would rather avoid adding
an unneeded member to RenderLayer.

[mitica, 2013-09-23 23:01:06]

On 2013/09/23 22:15:08, Julien Chaffraix wrote:
>
https://codereview.chromium.org/23503046/diff/2/Source/core/rendering/RenderL...
> File Source/core/rendering/RenderLayer.cpp (right):
> 
>
https://codereview.chromium.org/23503046/diff/2/Source/core/rendering/RenderL...
> Source/core/rendering/RenderLayer.cpp:422: if (!child->isStackingContext() &&
> child->hasNonAcceleratedBlendedChildInCurrentStackingContext())
> On 2013/09/23 20:12:20, mitica wrote:
> > On 2013/09/23 18:55:38, Julien Chaffraix wrote:
> > > Doesn't this means we will walk the whole subtree for every paint as most
> > > renderers are not stacking contexts and don't have blend-mode set?
> > > 
> > > This really looks like a descendant dependent flag, which would prevent
the
> > > potential tree walk explosion.
> > 
> > If, for each time a blend mode would be set on a RenderLayer, we'd call a
> method
> > that sets a flag (call it |m_childIsBlended|, for example) for each parent
> > recursively, it would be more efficient. Is this the approach you are
> > suggesting? However, we'd have to call this recursive setter even for more
odd
> > cases, for example, when the RenderLayer is removed.
> 
> Yes and this is a fairly common paradigm in RenderLayer, look at what
> updateDescendantDependentFlags does and the flags in there. Adding a potential
> full tree walk for each paint is a definite no from my side.
> 
>
https://codereview.chromium.org/23503046/diff/2/Source/core/rendering/RenderL...
> Source/core/rendering/RenderLayer.cpp:3296: m_isolateForDescendantBlendMode =
> isStackingContext() &&
hasNonAcceleratedBlendedChildInCurrentStackingContext();
> On 2013/09/23 20:12:20, mitica wrote:
> > On 2013/09/23 18:55:38, Julien Chaffraix wrote:
> > > Trying to understand the need to have this stored as a member of
> RenderLayer:
> > it
> > > seems that it could be an argument [actually more
> > > m_isolateForDescendantBlendMode || hasBlendMode()] to
> beginTransparencyLayers
> > > but are you concerned about recursive calls into this code?
> > 
> > This member is computed using the recursive layer traversing method above
> > (hasNonAcceleratedBlendedChildInCurrentStackingContext), I'm using it just
to
> > avoid running this method twice: once for deciding wether to call
> > beginTransparencyLayers, and the other one to avoid recursive calls.
> 
> That's not what I meant: it is possible for this code (paintLayerContent) to
be
> called recursively inside a reflection. This would justify the need to have a
> member variable (it's the reason we have a member called
> m_paintingInsideReflection).
> 
> If you are only avoiding the cost of recomputing it, I would rather avoid
adding
> an unneeded member to RenderLayer.

Thanks for the review, I'll start work on addressing your comments.

[mitica, 2013-09-30 15:19:38]

On 2013/09/23 23:01:06, mitica wrote:
> On 2013/09/23 22:15:08, Julien Chaffraix wrote:
> >
>
https://codereview.chromium.org/23503046/diff/2/Source/core/rendering/RenderL...
> > File Source/core/rendering/RenderLayer.cpp (right):
> > 
> >
>
https://codereview.chromium.org/23503046/diff/2/Source/core/rendering/RenderL...
> > Source/core/rendering/RenderLayer.cpp:422: if (!child->isStackingContext()
&&
> > child->hasNonAcceleratedBlendedChildInCurrentStackingContext())
> > On 2013/09/23 20:12:20, mitica wrote:
> > > On 2013/09/23 18:55:38, Julien Chaffraix wrote:
> > > > Doesn't this means we will walk the whole subtree for every paint as
most
> > > > renderers are not stacking contexts and don't have blend-mode set?
> > > > 
> > > > This really looks like a descendant dependent flag, which would prevent
> the
> > > > potential tree walk explosion.
> > > 
> > > If, for each time a blend mode would be set on a RenderLayer, we'd call a
> > method
> > > that sets a flag (call it |m_childIsBlended|, for example) for each parent
> > > recursively, it would be more efficient. Is this the approach you are
> > > suggesting? However, we'd have to call this recursive setter even for more
> odd
> > > cases, for example, when the RenderLayer is removed.
> > 
> > Yes and this is a fairly common paradigm in RenderLayer, look at what
> > updateDescendantDependentFlags does and the flags in there. Adding a
potential
> > full tree walk for each paint is a definite no from my side.
> > 
> >
>
https://codereview.chromium.org/23503046/diff/2/Source/core/rendering/RenderL...
> > Source/core/rendering/RenderLayer.cpp:3296: m_isolateForDescendantBlendMode
=
> > isStackingContext() &&
> hasNonAcceleratedBlendedChildInCurrentStackingContext();
> > On 2013/09/23 20:12:20, mitica wrote:
> > > On 2013/09/23 18:55:38, Julien Chaffraix wrote:
> > > > Trying to understand the need to have this stored as a member of
> > RenderLayer:
> > > it
> > > > seems that it could be an argument [actually more
> > > > m_isolateForDescendantBlendMode || hasBlendMode()] to
> > beginTransparencyLayers
> > > > but are you concerned about recursive calls into this code?
> > > 
> > > This member is computed using the recursive layer traversing method above
> > > (hasNonAcceleratedBlendedChildInCurrentStackingContext), I'm using it just
> to
> > > avoid running this method twice: once for deciding wether to call
> > > beginTransparencyLayers, and the other one to avoid recursive calls.
> > 
> > That's not what I meant: it is possible for this code (paintLayerContent) to
> be
> > called recursively inside a reflection. This would justify the need to have
a
> > member variable (it's the reason we have a member called
> > m_paintingInsideReflection).
> > 
> > If you are only avoiding the cost of recomputing it, I would rather avoid
> adding
> > an unneeded member to RenderLayer.
> 
> Thanks for the review, I'll start work on addressing your comments.

All done, can you please have another look?

[Julien - ping for review, 2013-09-30 17:59:11]

https://codereview.chromium.org/23503046/diff/33001/Source/core/rendering/Ren...
File Source/core/rendering/RenderLayer.cpp (right):

https://codereview.chromium.org/23503046/diff/33001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayer.cpp:828: bool hadBlendMode = m_blendMode !=
BlendModeNormal;
Unused variable.

https://codereview.chromium.org/23503046/diff/33001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayer.cpp:834: setAncestorChainBlendedDescendant();
I don't think this logic is right: you should only call it if you *do* have a
blend mode, which is not guaranteed if you come here (the only condition you
check is whether you have changed).

Btw, this should be tested.

https://codereview.chromium.org/23503046/diff/33001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayer.cpp:1811: if (child->hasBlendMode() ||
child->m_childLayerHasBlendMode)
You can potentially check a dirty value here. You should expose a helper
function that ASSERT that we don't query a dirtied value and transition all
checks to use it.

If you can't have the ASSERT, it's still important to document it (see
hasSelfPaintingLayerDescendant).

https://codereview.chromium.org/23503046/diff/33001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayer.cpp:1875: if (oldChild->hasBlendMode() ||
oldChild->m_childLayerHasBlendMode)
Ditto.

[mitica, 2013-10-01 13:14:37]

https://codereview.chromium.org/23503046/diff/33001/Source/core/rendering/Ren...
File Source/core/rendering/RenderLayer.cpp (right):

https://codereview.chromium.org/23503046/diff/33001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayer.cpp:834: setAncestorChainBlendedDescendant();
On 2013/09/30 17:59:11, Julien Chaffraix wrote:
> I don't think this logic is right: you should only call it if you *do* have a
> blend mode, which is not guaranteed if you come here (the only condition you
> check is whether you have changed).
> 
> Btw, this should be tested.

You are right, I missed this. The new approach will imply modifying the flags
only if a blend mode is set or unset. In other words, if the style already has
blendMode set to Multiply, and this changes to Hue, the flag doesn't need to be
updated. I'll also add a repaint test that validates the mechanism

https://codereview.chromium.org/23503046/diff/33001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayer.cpp:1811: if (child->hasBlendMode() ||
child->m_childLayerHasBlendMode)
On 2013/09/30 17:59:11, Julien Chaffraix wrote:
> You can potentially check a dirty value here. You should expose a helper
> function that ASSERT that we don't query a dirtied value and transition all
> checks to use it.
> 
> If you can't have the ASSERT, it's still important to document it (see
> hasSelfPaintingLayerDescendant).

Indeed, thanks for pointing this out, I'll add the assert :)

[mitica, 2013-10-01 20:04:12]

Can you please have another look? Thanks

[Julien - ping for review, 2013-10-03 21:38:28]

https://codereview.chromium.org/23503046/diff/39001/LayoutTests/css3/composit...
File LayoutTests/css3/compositing/mix-blend-mode-simple-text.html (right):

https://codereview.chromium.org/23503046/diff/39001/LayoutTests/css3/composit...
LayoutTests/css3/compositing/mix-blend-mode-simple-text.html:15: <div
style="mix-blend-mode: normal; background-color: blue;">The quick brown fox
jumps over the lazy dog</div>
Can we at least use Ahem with a line-height to have a cross-platform output?

https://codereview.chromium.org/23503046/diff/39001/LayoutTests/css3/composit...
File LayoutTests/css3/compositing/mix-blend-mode-simple.html (right):

https://codereview.chromium.org/23503046/diff/39001/LayoutTests/css3/composit...
LayoutTests/css3/compositing/mix-blend-mode-simple.html:8: height: 130px;
DRT only dumps about 800px of height so I would expect most of the divs to be
ignored during testing.

https://codereview.chromium.org/23503046/diff/39001/LayoutTests/fast/repaint/...
File LayoutTests/fast/repaint/mix-blend-mode-separate-stacking-context.html
(right):

https://codereview.chromium.org/23503046/diff/39001/LayoutTests/fast/repaint/...
LayoutTests/fast/repaint/mix-blend-mode-separate-stacking-context.html:15: <div
id="first" style="mix-blend-mode: normal; background-color: blue;"></div>
I don't understand this test: mix-blend-mode has an initial value of 'normal' so
both the reference and the reftest are identical which means that we are testing
nothing.

https://codereview.chromium.org/23503046/diff/39001/Source/core/rendering/Ren...
File Source/core/rendering/RenderLayer.cpp (right):

https://codereview.chromium.org/23503046/diff/39001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayer.cpp:1162: m_childLayerHasBlendMode |=
childLayerHasBlendMode;
Normally we bail out if m_childLayerHasBlendMode but considering it's a rare
property, it probably wouldn't buy us much.

[mitica, 2013-10-07 10:56:02]

Please have another look

https://codereview.chromium.org/23503046/diff/39001/LayoutTests/fast/repaint/...
File LayoutTests/fast/repaint/mix-blend-mode-separate-stacking-context.html
(right):

https://codereview.chromium.org/23503046/diff/39001/LayoutTests/fast/repaint/...
LayoutTests/fast/repaint/mix-blend-mode-separate-stacking-context.html:15: <div
id="first" style="mix-blend-mode: normal; background-color: blue;"></div>
On 2013/10/03 21:38:28, Julien Chaffraix wrote:
> I don't understand this test: mix-blend-mode has an initial value of 'normal'
so
> both the reference and the reftest are identical which means that we are
testing
> nothing.

This was incorrectly uploaded, sorry for the mistake. I've added the proper
version of this test in the latests patch.

https://codereview.chromium.org/23503046/diff/39001/Source/core/rendering/Ren...
File Source/core/rendering/RenderLayer.cpp (right):

https://codereview.chromium.org/23503046/diff/39001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayer.cpp:1162: m_childLayerHasBlendMode |=
childLayerHasBlendMode;
On 2013/10/03 21:38:28, Julien Chaffraix wrote:
> Normally we bail out if m_childLayerHasBlendMode but considering it's a rare
> property, it probably wouldn't buy us much.

Good point, I've modified this function to break after reaching the first
blended child.

[Julien - ping for review, 2013-10-07 16:40:55]

lgtm

https://codereview.chromium.org/23503046/diff/49001/LayoutTests/css3/composit...
File LayoutTests/css3/compositing/mix-blend-mode-isolated-group-2.html (right):

https://codereview.chromium.org/23503046/diff/49001/LayoutTests/css3/composit...
LayoutTests/css3/compositing/mix-blend-mode-isolated-group-2.html:28: <body>
Can you add a description of what is expected and what are the conditions for
passing the test? It doesn't have to be dumped in the output.

[Julien - ping for review, 2013-10-07 16:41:35]

https://codereview.chromium.org/23503046/diff/49001/LayoutTests/css3/composit...
File LayoutTests/css3/compositing/mix-blend-mode-isolated-group-2.html (right):

https://codereview.chromium.org/23503046/diff/49001/LayoutTests/css3/composit...
LayoutTests/css3/compositing/mix-blend-mode-isolated-group-2.html:28: <body>
On 2013/10/07 16:40:55, Julien Chaffraix wrote:
> Can you add a description of what is expected and what are the conditions for
> passing the test? It doesn't have to be dumped in the output.

This applies to all tests btw but I didn't repeat this comment.

[commit-bot: I haz the power, 2013-10-07 17:12:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mitica@adobe.com/23503046/55001

[commit-bot: I haz the power, 2013-10-07 17:13:02]

Failed to apply patch for Source/core/rendering/RenderLayer.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/core/rendering/RenderLayer.cpp
  Hunk #1 succeeded at 121 with fuzz 2 (offset -11 lines).
  Hunk #2 FAILED at 825.
  Hunk #3 succeeded at 962 (offset -98 lines).
  Hunk #4 succeeded at 1048 (offset -98 lines).
  Hunk #5 succeeded at 1590 (offset -145 lines).
  Hunk #6 succeeded at 1660 (offset -145 lines).
  Hunk #7 succeeded at 1724 (offset -145 lines).
  Hunk #8 succeeded at 2742 (offset -529 lines).
  Hunk #9 succeeded at 2869 (offset -529 lines).
  Hunk #10 succeeded at 3054 (offset -529 lines).
  Hunk #11 succeeded at 3078 (offset -529 lines).
  Hunk #12 succeeded at 5133 with fuzz 2 (offset -556 lines).
  1 out of 12 hunks FAILED -- saving rejects to file
Source/core/rendering/RenderLayer.cpp.rej

Patch:       Source/core/rendering/RenderLayer.cpp
Index: Source/core/rendering/RenderLayer.cpp
diff --git a/Source/core/rendering/RenderLayer.cpp
b/Source/core/rendering/RenderLayer.cpp
index
6ccee938b4b7e369a31ece346e302db2bdbe3592..b2d250529845cf1ee63b2354a47b5e325573faa7
100644
--- a/Source/core/rendering/RenderLayer.cpp
+++ b/Source/core/rendering/RenderLayer.cpp
@@ -132,6 +132,8 @@ RenderLayer::RenderLayer(RenderLayerModelObject* renderer)
     , m_canBePromotedToStackingContainerDirty(true)
     , m_isRootLayer(renderer->isRenderView())
     , m_usedTransparency(false)
+    , m_childLayerHasBlendMode(false)
+    , m_childLayerHasBlendModeStatusDirty(false)
     , m_paintingInsideReflection(false)
     , m_repaintStatus(NeedsNormalRepaint)
     , m_visibleContentStatusDirty(true)
@@ -823,9 +825,15 @@ void RenderLayer::updateBlendMode()
     if (!RuntimeEnabledFeatures::cssCompositingEnabled())
         return;
 
+    bool hadBlendMode = m_blendMode != BlendModeNormal;
     BlendMode newBlendMode = renderer()->style()->blendMode();
     if (newBlendMode != m_blendMode) {
         m_blendMode = newBlendMode;
+
+        // Only update the flag if a blend mode is set or unset.
+        if (!hadBlendMode || !hasBlendMode())
+            dirtyAncestorChainBlendedDescendantStatus();
+
         if (backing())
             backing()->setBlendMode(newBlendMode);
     }
@@ -1058,6 +1066,33 @@ void RenderLayer::setAncestorChainHasVisibleDescendant()
     }
 }
 
+void RenderLayer::dirtyAncestorChainBlendedDescendantStatus()
+{
+    for (RenderLayer* layer = this; layer; layer = layer->parent()) {
+        if (layer->m_childLayerHasBlendModeStatusDirty)
+            break;
+
+        layer->m_childLayerHasBlendModeStatusDirty = true;
+
+        if (layer->isStackingContext())
+            break;
+    }
+}
+
+void RenderLayer::setAncestorChainBlendedDescendant()
+{
+    for (RenderLayer* layer = this; layer; layer = layer->parent()) {
+        if (!layer->m_childLayerHasBlendModeStatusDirty &&
layer->childLayerHasBlendMode())
+            break;
+
+        layer->m_childLayerHasBlendMode = true;
+        layer->m_childLayerHasBlendModeStatusDirty = false;
+
+        if (layer->isStackingContext())
+            break;
+    }
+}
+
 void RenderLayer::updateHasUnclippedDescendant()
 {
     TRACE_EVENT0("blink_rendering",
"RenderLayer::updateHasUnclippedDescendant");
@@ -1117,6 +1152,20 @@ void RenderLayer::updateDescendantDependentFlags()
         m_hasOutOfFlowPositionedDescendantDirty = false;
     }
 
+    if (m_childLayerHasBlendModeStatusDirty) {
+        for (RenderLayer* child = firstChild(); child; child =
child->nextSibling()) {
+            if (!child->isStackingContext())
+                child->updateDescendantDependentFlags();
+
+            bool childLayerHasBlendMode = child->hasBlendMode() ||
(child->m_childLayerHasBlendMode && !child->isStackingContext());
+            m_childLayerHasBlendMode |= childLayerHasBlendMode;
+
+            if (m_childLayerHasBlendMode)
+                break;
+        }
+        m_childLayerHasBlendModeStatusDirty = false;
+    }
+
     if (m_visibleContentStatusDirty) {
         if (renderer()->style()->visibility() == VISIBLE)
             m_hasVisibleContent = true;
@@ -1692,18 +1741,22 @@ LayoutRect RenderLayer::paintingExtent(const
RenderLayer* rootLayer, const Layou
 
 void RenderLayer::beginTransparencyLayers(GraphicsContext* context, const
RenderLayer* rootLayer, const LayoutRect& paintDirtyRect, PaintBehavior
paintBehavior)
 {
-    if (context->paintingDisabled() || (paintsWithTransparency(paintBehavior)
&& m_usedTransparency))
+    bool createTransparencyLayerForBlendMode = isStackingContext() &&
m_childLayerHasBlendMode;
+    if (context->paintingDisabled() || ((paintsWithTransparency(paintBehavior)
|| hasBlendMode() || createTransparencyLayerForBlendMode) &&
m_usedTransparency))
         return;
 
     RenderLayer* ancestor = transparentPaintingAncestor();
     if (ancestor)
         ancestor->beginTransparencyLayers(context, rootLayer, paintDirtyRect,
paintBehavior);
 
-    if (paintsWithTransparency(paintBehavior)) {
+    if (paintsWithTransparency(paintBehavior) || hasBlendMode() ||
createTransparencyLayerForBlendMode) {
         m_usedTransparency = true;
         context->save();
         LayoutRect clipRect = paintingExtent(rootLayer, paintDirtyRect,
paintBehavior);
         context->clip(clipRect);
+        if (hasBlendMode())
+            context->setCompositeOperation(context->compositeOperation(),
m_blendMode);
+
         context->beginTransparencyLayer(renderer()->opacity());
 #ifdef REVEAL_TRANSPARENCY_LAYERS
         context->setFillColor(Color(0.0f, 0.0f, 0.5f, 0.2f));
@@ -1758,6 +1811,9 @@ void RenderLayer::addChild(RenderLayer* child,
RenderLayer* beforeChild)
     if (child->isSelfPaintingLayer() ||
child->hasSelfPaintingLayerDescendant())
         setAncestorChainHasSelfPaintingLayerDescendant();
 
+    if (child->hasBlendMode() || child->childLayerHasBlendMode())
+        setAncestorChainBlendedDescendant();
+
     if (subtreeContainsOutOfFlowPositionedLayer(child)) {
         // Now that the out of flow positioned descendant is in the tree, we
         // need to tell the compositor to reevaluate the compositing
@@ -1819,6 +1875,9 @@ RenderLayer* RenderLayer::removeChild(RenderLayer*
oldChild)
     if (oldChild->isSelfPaintingLayer() ||
oldChild->hasSelfPaintingLayerDescendant())
         dirtyAncestorChainHasSelfPaintingLayerDescendantStatus();
 
+    if (oldChild->hasBlendMode() || oldChild->childLayerHasBlendMode())
+        dirtyAncestorChainBlendedDescendantStatus();
+
     return oldChild;
 }
 
@@ -3218,6 +3277,12 @@ void RenderLayer::paintLayerContents(GraphicsContext*
context, const LayerPainti
         }
     }
 
+    // Blending operations must be performed only with the nearest ancestor
stacking context.
+    bool createTransparencyLayerForBlendMode = isStackingContext() &&
m_childLayerHasBlendMode;
+
+    if (createTransparencyLayerForBlendMode)
+        beginTransparencyLayers(context, paintingInfo.rootLayer,
paintingInfo.paintDirtyRect, paintingInfo.paintBehavior);
+
     LayerPaintingInfo localPaintingInfo(paintingInfo);
     FilterEffectRendererHelper filterPainter(filterRenderer() &&
paintsWithFilters());
     if (filterPainter.haveFilterEffect() && !context->paintingDisabled()) {
@@ -3339,7 +3404,7 @@ void RenderLayer::paintLayerContents(GraphicsContext*
context, const LayerPainti
     }
 
     // End our transparency layer
-    if (haveTransparency && m_usedTransparency && !m_paintingInsideReflection)
{
+    if ((haveTransparency || hasBlendMode() ||
createTransparencyLayerForBlendMode) && m_usedTransparency &&
!m_paintingInsideReflection) {
         context->endLayer();
         context->restore();
         m_usedTransparency = false;
@@ -3524,7 +3589,7 @@ void RenderLayer::paintBackgroundForFragments(const
LayerFragments& layerFragmen
             continue;
 
         // Begin transparency layers lazily now that we know we have to paint
something.
-        if (haveTransparency)
+        if (haveTransparency || hasBlendMode())
             beginTransparencyLayers(transparencyLayerContext,
localPaintingInfo.rootLayer, transparencyPaintDirtyRect,
localPaintingInfo.paintBehavior);
 
         if (localPaintingInfo.clipToDirtyRect) {
@@ -3548,7 +3613,7 @@ void RenderLayer::paintForegroundForFragments(const
LayerFragments& layerFragmen
     RenderObject* paintingRootForRenderer, bool selectionOnly, bool
forceBlackText)
 {
     // Begin transparency if we have something to paint.
-    if (haveTransparency) {
+    if (haveTransparency || hasBlendMode()) {
         for (size_t i = 0; i < layerFragments.size(); ++i) {
             const LayerFragment& fragment = layerFragments.at(i);
             if (fragment.shouldPaintContent &&
!fragment.foregroundRect.isEmpty()) {
@@ -5630,12 +5695,12 @@ void RenderLayer::styleChanged(StyleDifference, const
RenderStyle* oldStyle)
 
     updateResizerStyle();
 
-    updateDescendantDependentFlags();
-    updateTransform();
-
     if (RuntimeEnabledFeatures::cssCompositingEnabled())
         updateBlendMode();
 
+    updateDescendantDependentFlags();
+    updateTransform();
+
     bool didPaintWithFilters = false;
 
     if (paintsWithFilters())

[commit-bot: I haz the power, 2013-10-07 19:32:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mitica@adobe.com/23503046/59001

[commit-bot: I haz the power, 2013-10-07 19:55:50]

Retried try job too often on blink_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=blink_pres...

[mitica, 2013-10-07 19:59:49]

Hi abarth, can you please stamp this chageset with your l g t m? Thanks!

[abarth-chromium, 2013-10-07 21:08:27]

public <--- LGTM

[commit-bot: I haz the power, 2013-10-07 21:12:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mitica@adobe.com/23503046/59001

[commit-bot: I haz the power, 2013-10-08 00:00:25]

Change committed as 159061