Refactor BlendMode into a separate class.

Refactor BlendMode into a separate class.

This moves the BlendMode code out of RenderLayer and into RenderLayerBlendInfo.

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

[dsinclair, 2013-12-02 19:20:50]

jchaffraix PTAL.

I didn't make the m_blendInfo a OwnPtr because the
RenderLayerBlendInfo::dirtyAncestorChainBlendedDescendantStatus() and
RenderLayerBlendInfo::setAncestorChainBlendedDescendant() would need to be able
to create the blendInfo in the RenderLayer objects. It can be done, just seems
like it won't be pretty.

[dsinclair, 2013-12-05 15:37:11]

ping

[Julien - ping for review, 2013-12-10 00:55:09]

Really really sorry for the delay in reviewing.

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

https://codereview.chromium.org/99883002/diff/20001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayer.cpp:815: ||
(child->blendInfo().childLayerHasBlendModeWhileDirty() &&
!child->stackingNode()->isStackingContext());
We should *never* allow the flag to be queried dirty. Is this really needed in
the current code?

https://codereview.chromium.org/99883002/diff/20001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayer.cpp:817:
m_blendInfo.updateChildLayerHasBlendMode(childLayerHasBlendMode);
A better way to handle that would be to store this in a local variable and set
it at the end. This would remove the need for updateChildLayerHasBlendMode.

https://codereview.chromium.org/99883002/diff/20001/Source/core/rendering/Ren...
File Source/core/rendering/RenderLayerBlendInfo.h (right):

https://codereview.chromium.org/99883002/diff/20001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayerBlendInfo.h:59: explicit
RenderLayerBlendInfo(RenderLayerModelObject*);
Isn't this object limited to RenderBox?

https://codereview.chromium.org/99883002/diff/20001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayerBlendInfo.h:76: void
ChildLayerHasBlendModeStatusDirty(bool b) { m_childLayerHasBlendModeStatusDirty
= b; }
This function should be removed.

https://codereview.chromium.org/99883002/diff/20001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayerBlendInfo.h:88: unsigned
m_childLayerHasBlendModeStatusDirty : 1;
Because of these bits, we have to always allocate a RenderLayerBlendInfo.

I would keep them on RenderLayer for now as we need to figure out what to do
with them, especially since we will want to have different trees layered on top
of RenderObject and it's unclear how we would keep track of these bits in this
new environment.

[Julien - ping for review, 2013-12-10 01:46:12]

https://codereview.chromium.org/99883002/diff/20001/Source/core/rendering/Ren...
File Source/core/rendering/RenderLayerBlendInfo.h (right):

https://codereview.chromium.org/99883002/diff/20001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayerBlendInfo.h:88: unsigned
m_childLayerHasBlendModeStatusDirty : 1;
On 2013/12/10 00:55:09, Julien Chaffraix - PST wrote:
> Because of these bits, we have to always allocate a RenderLayerBlendInfo.
> 
> I would keep them on RenderLayer for now as we need to figure out what to do
> with them, especially since we will want to have different trees layered on
top
> of RenderObject and it's unclear how we would keep track of these bits in this
> new environment.

This is a stale comment, actually moving them is fine. That's because once we
link these objects together in a sub-tree of the render tree independent of
RenderLayer, the bits will be updated based on the sub-tree so it makes sense to
move them.

[dsinclair, 2013-12-10 16:59:11]

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

https://codereview.chromium.org/99883002/diff/20001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayer.cpp:815: ||
(child->blendInfo().childLayerHasBlendModeWhileDirty() &&
!child->stackingNode()->isStackingContext());
On 2013/12/10 00:55:09, Julien Chaffraix - PST wrote:
> We should *never* allow the flag to be queried dirty. Is this really needed in
> the current code?


The current code does require this at the moment.  If you look in
RenderLayer::styleChanged on line ~3932 we do: 

    if (RuntimeEnabledFeatures::cssCompositingEnabled())
        updateBlendMode();

updateBlendMode() will call to dirtyAncestorChainBlendDescendantStatus() which
sets the dirty flag to true.

Then on line ~3934 (the next call) we call updateDescendentDependentFlags()
which has this chunk of code which, on line ~808 above, specifically checks if
the status is dirty before doing any work. So, currently, I need to be able to
get the blend mode while it's dirty as otherwise it will ASSERT and blowup.

The current code works because it's all operating on internal m_* values. After
splitting it into it's own class is where it causes the weirdness.

[dsinclair, 2013-12-13 16:53:58]

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

https://codereview.chromium.org/99883002/diff/20001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayer.cpp:817:
m_blendInfo.updateChildLayerHasBlendMode(childLayerHasBlendMode);
On 2013/12/10 00:55:09, Julien Chaffraix - PST wrote:
> A better way to handle that would be to store this in a local variable and set
> it at the end. This would remove the need for updateChildLayerHasBlendMode.

Done.

https://codereview.chromium.org/99883002/diff/20001/Source/core/rendering/Ren...
File Source/core/rendering/RenderLayerBlendInfo.h (right):

https://codereview.chromium.org/99883002/diff/20001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayerBlendInfo.h:59: explicit
RenderLayerBlendInfo(RenderLayerModelObject*);
On 2013/12/10 00:55:09, Julien Chaffraix - PST wrote:
> Isn't this object limited to RenderBox?

Added a FIXME.

https://codereview.chromium.org/99883002/diff/20001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayerBlendInfo.h:76: void
ChildLayerHasBlendModeStatusDirty(bool b) { m_childLayerHasBlendModeStatusDirty
= b; }
On 2013/12/10 00:55:09, Julien Chaffraix - PST wrote:
> This function should be removed.

Done.

[Julien - ping for review, 2013-12-16 11:52:14]

lgtm but let's not land with the FIXME about RenderBox.

https://codereview.chromium.org/99883002/diff/20001/Source/core/rendering/Ren...
File Source/core/rendering/RenderLayerBlendInfo.h (right):

https://codereview.chromium.org/99883002/diff/20001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayerBlendInfo.h:59: explicit
RenderLayerBlendInfo(RenderLayerModelObject*);
On 2013/12/13 16:53:59, dsinclair wrote:
> On 2013/12/10 00:55:09, Julien Chaffraix - PST wrote:
> > Isn't this object limited to RenderBox?
> 
> Added a FIXME.

I don't think a FIXME is the right solution here. If blend mode is limited to
RenderBoxes (which I am pretty sure it is), we should just make the object takes
a RenderBox. If it doesn't, no need for a FIXME anyway.

https://codereview.chromium.org/99883002/diff/80001/Source/core/rendering/Ren...
File Source/core/rendering/RenderLayerBlendInfo.h (right):

https://codereview.chromium.org/99883002/diff/80001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayerBlendInfo.h:75: void
updateChildLayerHasBlendMode(bool b) { m_childLayerHasBlendMode |= b; }
This function is unused now.

[dsinclair, 2014-01-06 19:12:07]

https://codereview.chromium.org/99883002/diff/20001/Source/core/rendering/Ren...
File Source/core/rendering/RenderLayerBlendInfo.h (right):

https://codereview.chromium.org/99883002/diff/20001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayerBlendInfo.h:59: explicit
RenderLayerBlendInfo(RenderLayerModelObject*);
On 2013/12/16 11:52:15, Julien Chaffraix - CET wrote:
> On 2013/12/13 16:53:59, dsinclair wrote:
> > On 2013/12/10 00:55:09, Julien Chaffraix - PST wrote:
> > > Isn't this object limited to RenderBox?
> > 
> > Added a FIXME.
> 
> I don't think a FIXME is the right solution here. If blend mode is limited to
> RenderBoxes (which I am pretty sure it is), we should just make the object
takes
> a RenderBox. If it doesn't, no need for a FIXME anyway.


I took a longer look at this, and I'm not sure that blend mode is restricted to
RenderBox. From reading http://dev.w3.org/fxtf/compositing-1/#mix-blend-mode it
mentions that the mix-blend-mode property applies to all elements. Doesn't that
mean that it could be set on anything, not just RenderBox and children?

https://codereview.chromium.org/99883002/diff/80001/Source/core/rendering/Ren...
File Source/core/rendering/RenderLayerBlendInfo.h (right):

https://codereview.chromium.org/99883002/diff/80001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayerBlendInfo.h:75: void
updateChildLayerHasBlendMode(bool b) { m_childLayerHasBlendMode |= b; }
On 2013/12/16 11:52:15, Julien Chaffraix - CET wrote:
> This function is unused now.

Done.

[dsinclair, 2014-01-08 16:17:56]

Ping. I've removed the FIXME as it seems like the blend mode is related to all
elements, not just RenderBox elements. Just want to make sure I didn't mis-read
the spec.

[Julien - ping for review, 2014-01-09 14:13:50]

https://codereview.chromium.org/99883002/diff/20001/Source/core/rendering/Ren...
File Source/core/rendering/RenderLayerBlendInfo.h (right):

https://codereview.chromium.org/99883002/diff/20001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayerBlendInfo.h:59: explicit
RenderLayerBlendInfo(RenderLayerModelObject*);
On 2014/01/06 19:12:07, dsinclair wrote:
> On 2013/12/16 11:52:15, Julien Chaffraix - CET wrote:
> > On 2013/12/13 16:53:59, dsinclair wrote:
> > > On 2013/12/10 00:55:09, Julien Chaffraix - PST wrote:
> > > > Isn't this object limited to RenderBox?
> > > 
> > > Added a FIXME.
> > 
> > I don't think a FIXME is the right solution here. If blend mode is limited
to
> > RenderBoxes (which I am pretty sure it is), we should just make the object
> takes
> > a RenderBox. If it doesn't, no need for a FIXME anyway.
> 
> 
> I took a longer look at this, and I'm not sure that blend mode is restricted
to
> RenderBox. From reading http://dev.w3.org/fxtf/compositing-1/#mix-blend-mode
it
> mentions that the mix-blend-mode property applies to all elements. Doesn't
that
> mean that it could be set on anything, not just RenderBox and children?

My understanding is that the "applies to" section is for the authors'
convenience more than implementers'. Considering that it references SVG elements
(which are not RenderBoxModelObjects) is fishy. I would have just tried to
restrict the type and see if anything breaks. That said this detail shouldn't
block this change.

[commit-bot: I haz the power, 2014-01-09 15:50:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dsinclair@chromium.org/99883002/220001

[commit-bot: I haz the power, 2014-01-09 16:50:05]

Change committed as 164790