mix-blend-mode implementation for accelerated layers - blink part

Added setters and getters in WebLayer.h / GraphicsLayer.h for:
* blendMode - specifying the formula for blending the layer with its backdrop - http://dev.w3.org/fxtf/compositing-1/#blending
* isIsolatedGroupRoot - this layer should make sure that all its blending child layers in the layer tree and the layer itself will use a transparent black initial backdrop. http://dev.w3.org/fxtf/compositing-1/#isolatedgroups

Added WebBlendMode.h defining the WebBlendMode enum and convertion functions.

Added a new compositing reason - CompositingReasonIsolateCompositedDescendants. A RenderLayer might require compositing in order to perform isolation of its composited blending children

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

[jamesr, 2013-08-28 22:24:49]

I don't understand why blend mode requires a different isolation method from
filters or any other functionality that requires a separate render surface. 
Could you explain?

https://codereview.chromium.org/23511004/diff/4001/public/platform/WebBlendMo...
File public/platform/WebBlendMode.h (right):

https://codereview.chromium.org/23511004/diff/4001/public/platform/WebBlendMo...
public/platform/WebBlendMode.h:39: #include
"core/platform/graphics/GraphicsTypes.h"
don't do this

https://codereview.chromium.org/23511004/diff/4001/public/platform/WebBlendMo...
public/platform/WebBlendMode.h:66: WBM_COMPILE_ASSERT(WebBlendModeNormal,    
WebCore::BlendModeNormal);
put these assertions in AssertMatchingEnums.cpp, not in the interface header

https://codereview.chromium.org/23511004/diff/4001/public/platform/WebBlendMo...
public/platform/WebBlendMode.h:83: inline WebCore::BlendMode
toWebCoreBlendMode(WebBlendMode blendMode)
these don't belong in the public header - they're only for WebCore's consumption
and should go in WebCore.  Do you even need to have separate WebBlendMode and
WebCore::BlendMode types at all?

https://codereview.chromium.org/23511004/diff/4001/public/platform/WebBlendMo...
public/platform/WebBlendMode.h:95: inline SkXfermode::Mode
toSkiaBlendMode(WebBlendMode blendMode)
I don't see why this is part of the public interface either.  Who's going to
call these functions?

https://codereview.chromium.org/23511004/diff/4001/public/platform/WebComposi...
File public/platform/WebCompositingReasons.h (right):

https://codereview.chromium.org/23511004/diff/4001/public/platform/WebComposi...
public/platform/WebCompositingReasons.h:75: const uint64_t
CompositingReasonIsolateCompositedDescendants           =
static_cast<uint64_t>(1) << 35;
why does this go from << 33 to << 35?

[rosca, 2013-08-29 15:04:41]

A blending layer will require a separate render surface for itself (like filters
or masking do), but it might also require a render surface for the root layer of
the stacking context it belongs to. This is because a blending layer needs to
read pixels from the underneath, limited to the current! stacking context[1], it
should not have access to the pixels outside this stacking context. The subtree
starting from the root layer of this stacking context will become an isolated
group [2].

So, the layers with isRootForIsolatedGroup-true will create a render surface
without having a direct reason like blending, transform or filter. Also, the
"isolate" css property[3] might set isRootForIsolatedGroup to true in order to
enforce isolation (render surface) to any element.

[1] http://dev.w3.org/fxtf/compositing-1/#csscompositingrules_CSS
[2] http://dev.w3.org/fxtf/compositing-1/#isolatedgroups
[3] http://dev.w3.org/fxtf/compositing-1/#isolation

https://codereview.chromium.org/23511004/diff/4001/public/platform/WebBlendMo...
File public/platform/WebBlendMode.h (right):

https://codereview.chromium.org/23511004/diff/4001/public/platform/WebBlendMo...
public/platform/WebBlendMode.h:39: #include
"core/platform/graphics/GraphicsTypes.h"
On 2013/08/28 22:24:49, jamesr wrote:
> don't do this

Done.

https://codereview.chromium.org/23511004/diff/4001/public/platform/WebBlendMo...
public/platform/WebBlendMode.h:66: WBM_COMPILE_ASSERT(WebBlendModeNormal,    
WebCore::BlendModeNormal);
On 2013/08/28 22:24:49, jamesr wrote:
> put these assertions in AssertMatchingEnums.cpp, not in the interface header

Done.

https://codereview.chromium.org/23511004/diff/4001/public/platform/WebBlendMo...
public/platform/WebBlendMode.h:83: inline WebCore::BlendMode
toWebCoreBlendMode(WebBlendMode blendMode)
On 2013/08/28 22:24:49, jamesr wrote:
> these don't belong in the public header - they're only for WebCore's
consumption
> and should go in WebCore.
I removed to/fromWebCoreBlendMode from public interface and did conversion in
GraphicsLayer.

> Do you even need to have separate WebBlendMode and WebCore::BlendMode types at
all?
WebCore::BlendMode is already there in GraphicsTypes.h. Do you mean we should
use WebKit::WebBlendMode instead?

https://codereview.chromium.org/23511004/diff/4001/public/platform/WebBlendMo...
public/platform/WebBlendMode.h:95: inline SkXfermode::Mode
toSkiaBlendMode(WebBlendMode blendMode)
On 2013/08/28 22:24:49, jamesr wrote:
> I don't see why this is part of the public interface either.  Who's going to
> call these functions?
compositor_bindings/web_layer_impl.cc used to convert WebBlendMode to SkXfermode
and to pass it to cc::Layer. I moved these functions in the compositor_bindings.

https://codereview.chromium.org/23511004/diff/4001/public/platform/WebComposi...
File public/platform/WebCompositingReasons.h (right):

https://codereview.chromium.org/23511004/diff/4001/public/platform/WebComposi...
public/platform/WebCompositingReasons.h:75: const uint64_t
CompositingReasonIsolateCompositedDescendants           =
static_cast<uint64_t>(1) << 35;
On 2013/08/28 22:24:49, jamesr wrote:
> why does this go from << 33 to << 35?
<< 34 is used by CompositingReasonLayerForVideoOverlay in
Source/core/rendering/CompositingReasons.h

[jamesr, 2013-09-05 22:06:19]

I'm not aware of anything else in the web platform that has rendering
requirements on everything within an element's stacking context.  That seems
like a pretty strange restriction and one I doubt web authors will find easy to
work with.  Where is this feature being discussed?

[Rik, 2013-09-05 23:58:53]

On 2013/09/05 22:06:19, jamesr wrote:
> I'm not aware of anything else in the web platform that has rendering
> requirements on everything within an element's stacking context.  That seems
> like a pretty strange restriction and one I doubt web authors will find easy
to
> work with.  Where is this feature being discussed?

www-style and public-fx.
I'd like to remove that restriction too, but unfortunately, performance would
suffer and certain browsers would not be able to implement the feature,

Other features in the platform such as access to the backdrop for filter
operations, will have the same limitation. (Isn't z-index also per stacking
context?)

[Rik, 2013-09-06 00:07:00]

You can find me on irc if you want to chat...

[shawnsingh, 2013-09-13 10:22:59]

So...I was asked to review this, so here it is.

First, nice patch.  It really makes it look simpler than it really is.  I'm sure
you had far more challenge getting this uploaded than the patch would indicate
=)

Has the compositor cc/ side of this been uploaded and presented yet?  If so, can
we please get a link?

I think we need to defer this patch until the compositor side lands.  The
details of how we choose compositing layers in this patch depend on how things
are finally impemented on the compositor side.  It would be an unnecessary
iterative process to land this first, only to find that we may need to modify it
due to unforeseen implementation details.

Also I'm worried that complicated usage of blend modes will kill performance. 
It seems like it will require an explosion of render surfaces.  Are there any
ideas for how to deal with that?  Or do we already have some indication that it
won't be a problem?  Keeping in mind, we have to consider all platforms
including smaller ones that may be bandwidth or GPU limited.

Rest of my comments are below.  Cheers!

https://codereview.chromium.org/23511004/diff/10001/Source/core/platform/grap...
File Source/core/platform/graphics/GraphicsLayer.cpp (right):

https://codereview.chromium.org/23511004/diff/10001/Source/core/platform/grap...
Source/core/platform/graphics/GraphicsLayer.cpp:736: ts << "(blendMode " <<
m_blendMode << ")\n";
Does this mean that various blend modes will be identified as integers in the
output of layout tests?  I think we should stringify this somehow, so that there
is some sanity to looking at the output manually.  Could be a major timesaver
for anyone who needs to debug blend mode layout tests in the future.

https://codereview.chromium.org/23511004/diff/10001/Source/core/platform/grap...
Source/core/platform/graphics/GraphicsLayer.cpp:1036:
platformLayer()->setBlendMode(WebKit::WebBlendMode(blendMode));
using platformLayer() here seems correct.  I just want to make sure that we
consider the case where GraphicsLayer has a contentsLayer in addition to
m_layer, and which of those layers would we want to set the blend mode.

https://codereview.chromium.org/23511004/diff/10001/Source/core/platform/grap...
Source/core/platform/graphics/GraphicsLayer.cpp:1044:
platformLayer()->setIsRootForIsolatedGroup(isolated);
Same issue for platformLayer() accessor here.

https://codereview.chromium.org/23511004/diff/10001/Source/core/platform/grap...
File Source/core/platform/graphics/GraphicsLayer.h (right):

https://codereview.chromium.org/23511004/diff/10001/Source/core/platform/grap...
Source/core/platform/graphics/GraphicsLayer.h:504: bool m_isRootForIsolatedGroup
: 1;
As far as I know, declaring a bitfield here, in-between two other data types,
will be a no-op.  It seems like you would want to put this boolean with the rest
of them a few lines below, so that the new boolean will fall into the existing
bitfield being declared.

https://codereview.chromium.org/23511004/diff/10001/Source/core/rendering/Ren...
File Source/core/rendering/RenderLayerBacking.h (right):

https://codereview.chromium.org/23511004/diff/10001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayerBacking.h:217: void
updateRootForIsolatedGroup();
In my opinion this should be called updateIsRootForIsolatedGroup()  (i.e. insert
"Is").

https://codereview.chromium.org/23511004/diff/10001/Source/core/rendering/Ren...
File Source/core/rendering/RenderLayerCompositor.cpp (right):

https://codereview.chromium.org/23511004/diff/10001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayerCompositor.cpp:183: ,
m_hasCompositedBlendingDescendents(false)
Let's spell it "Descendants" instead... that way the spelling is consistent with
what already exists in the code.

https://codereview.chromium.org/23511004/diff/10001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayerCompositor.cpp:951: // an element that has
blending applied, must blend with all the underlying
nit - remove the comma after "applied"

https://codereview.chromium.org/23511004/diff/10001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayerCompositor.cpp:955: if
(layer->isStackingContext())
OK - please bear with me =)

(1) This part of the patch seems to depend on exactly how you intend to
implement blend modes on the back-end in cc/  Can you please give some details
about that?

(2) I would feel much more comfortable if we redundantly and explicitly set all
values in both cases.  The efficiency cost is negligible, but it removes the
need to rely on assumptions about recursive state that come from distant places.
 It's becoming increasingly important to avoid unnecessary assumptions in this
part of the code.

if (layer->isStackingContext()) {
   
layer->setShouldIsolateCompositedBlendingDescendants(childState.m_hasCompositedBlendingDescdendents)
    compositingState.m_hasCompositedBlendingDescendents = false;
} else {
    layer->setShouldIsolateCompositedBlendingDescendants(false);
    compositingState.m_hasCompositedBlendingDescendents =
childState.m_hasCompositedBlendingDescdendents;
}


(3) Also, another issue, what happens when there is a composited layer
in-between the stacking context and the composited blending descendant?  Or is
that case guaranteed not to happen for some reason?

https://codereview.chromium.org/23511004/diff/10001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayerCompositor.cpp:1952: if
(layer->shouldIsolateCompositedBlendingDescendants())
So it seems like this boolean value,
shouldIsolateCompositedBlendingDescendants() is really only used here.   It is
used in one ASSERT statement in RenderLayerBacking, but perhaps there's
something else that could be ASSERTed instead?

If that's true, then I think we should remove that accessor,
shouldIsolateCompositedBlendingDescendants(), and put the conditions of logic
directly in here.  Maybe instead of adding another arg to
subtreeReasonsForCompositing(), we could just pass compositingState itself to
this function.

Doing this would also simplify the if-statement that is being added to
computeCompositingRequirements().

[rosca, 2013-09-19 14:26:53]

Thanks for the review!

> Has the compositor cc/ side of this been uploaded and presented yet?  If so,
can
> we please get a link?

> I think we need to defer this patch until the compositor side lands.  The
> details of how we choose compositing layers in this patch depend on how things
> are finally impemented on the compositor side.  It would be an unnecessary
> iterative process to land this first, only to find that we may need to modify
it
> due to unforeseen implementation details.

I didn't upload the compositor part yet. I will submit it soon and I'll send you
the link.

> Also I'm worried that complicated usage of blend modes will kill performance. 
> It seems like it will require an explosion of render surfaces.  Are there any
> ideas for how to deal with that?  Or do we already have some indication that
it
> won't be a problem?  Keeping in mind, we have to consider all platforms
> including smaller ones that may be bandwidth or GPU limited.

I didn't notice any performance decrease (looking at the fps counter) when
blending some animations or video elements. I'm not sure how it would behave for
more complex scenarios with many render surfaces, we don't have any indicators
for this.
After we'll make it work, we can optimize things, for example we can skip
creating separate render surfaces for stacking contexts that are fully opaque
and elements inside don't overflow.

> Rest of my comments are below.  Cheers!
Thanks again and please review my new changes.

https://codereview.chromium.org/23511004/diff/10001/Source/core/platform/grap...
File Source/core/platform/graphics/GraphicsLayer.cpp (right):

https://codereview.chromium.org/23511004/diff/10001/Source/core/platform/grap...
Source/core/platform/graphics/GraphicsLayer.cpp:736: ts << "(blendMode " <<
m_blendMode << ")\n";
On 2013/09/13 10:22:59, shawnsingh wrote:
> Does this mean that various blend modes will be identified as integers in the
> output of layout tests?  I think we should stringify this somehow, so that
there
> is some sanity to looking at the output manually.  Could be a major timesaver
> for anyone who needs to debug blend mode layout tests in the future.

Done.

https://codereview.chromium.org/23511004/diff/10001/Source/core/platform/grap...
Source/core/platform/graphics/GraphicsLayer.cpp:1036:
platformLayer()->setBlendMode(WebKit::WebBlendMode(blendMode));
We set the blend mode to the outermost layer so it can take its subtree and
blend it with the backdrop. contentsLayer is set as a child of the
platformLayer, so it is fine to set the blend mode to the platformLayer. I 
tried to blend elements with contensLayer (accelerated canvas) and it seems to
work.

https://codereview.chromium.org/23511004/diff/10001/Source/core/platform/grap...
Source/core/platform/graphics/GraphicsLayer.cpp:1044:
platformLayer()->setIsRootForIsolatedGroup(isolated);
On 2013/09/13 10:22:59, shawnsingh wrote:
> Same issue for platformLayer() accessor here.

Done.

https://codereview.chromium.org/23511004/diff/10001/Source/core/platform/grap...
File Source/core/platform/graphics/GraphicsLayer.h (right):

https://codereview.chromium.org/23511004/diff/10001/Source/core/platform/grap...
Source/core/platform/graphics/GraphicsLayer.h:504: bool m_isRootForIsolatedGroup
: 1;
On 2013/09/13 10:22:59, shawnsingh wrote:
> As far as I know, declaring a bitfield here, in-between two other data types,
> will be a no-op.  It seems like you would want to put this boolean with the
rest
> of them a few lines below, so that the new boolean will fall into the existing
> bitfield being declared.

Done.

https://codereview.chromium.org/23511004/diff/10001/Source/core/rendering/Ren...
File Source/core/rendering/RenderLayerBacking.h (right):

https://codereview.chromium.org/23511004/diff/10001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayerBacking.h:217: void
updateRootForIsolatedGroup();
On 2013/09/13 10:22:59, shawnsingh wrote:
> In my opinion this should be called updateIsRootForIsolatedGroup()  (i.e.
insert
> "Is").

Done.

https://codereview.chromium.org/23511004/diff/10001/Source/core/rendering/Ren...
File Source/core/rendering/RenderLayerCompositor.cpp (right):

https://codereview.chromium.org/23511004/diff/10001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayerCompositor.cpp:183: ,
m_hasCompositedBlendingDescendents(false)
On 2013/09/13 10:22:59, shawnsingh wrote:
> Let's spell it "Descendants" instead... that way the spelling is consistent
with
> what already exists in the code.

Done.

https://codereview.chromium.org/23511004/diff/10001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayerCompositor.cpp:951: // an element that has
blending applied, must blend with all the underlying
On 2013/09/13 10:22:59, shawnsingh wrote:
> nit - remove the comma after "applied"

Done.

https://codereview.chromium.org/23511004/diff/10001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayerCompositor.cpp:955: if
(layer->isStackingContext())
On 2013/09/13 10:22:59, shawnsingh wrote:
> OK - please bear with me =)
> 
> (1) This part of the patch seems to depend on exactly how you intend to
> implement blend modes on the back-end in cc/  Can you please give some details
> about that?
This part of the code should make sure that a stacking contexts, containing at
least one accelerated blending descendant (not entering other descending
stacking contexts), will be accelerated too. This will help the compositor to
"isolate" the layer being a stacking context so that the blending descendant
will not be able to access the color information outside this stacking context
for the backdrop.

rendererLayerCompositor just accelerates root layers for isolated groups and
tells the compositor that they are root layers for isolated groups
(updateIsRootForIsolatedGroup). 

The compositor will create create a separate rendering surface for each isolated
group.

> 
> (2) I would feel much more comfortable if we redundantly and explicitly set
all
> values in both cases.  The efficiency cost is negligible, but it removes the
> need to rely on assumptions about recursive state that come from distant
places.
>  It's becoming increasingly important to avoid unnecessary assumptions in this
> part of the code.

Done. I made some changes here and things should be more clear now.

> 
> if (layer->isStackingContext()) {
>    
>
layer->setShouldIsolateCompositedBlendingDescendants(childState.m_hasCompositedBlendingDescdendents)
>     compositingState.m_hasCompositedBlendingDescendents = false;
> } else {
>     layer->setShouldIsolateCompositedBlendingDescendants(false);
>     compositingState.m_hasCompositedBlendingDescendents =
> childState.m_hasCompositedBlendingDescdendents;
> }
> 
> 
> (3) Also, another issue, what happens when there is a composited layer
> in-between the stacking context and the composited blending descendant?  Or is
> that case guaranteed not to happen for some reason?

It is possible to have accelerated layers behind the blending layer in the same
stacking context. The backdrop is the composition of all the accelerated layers
which go behind the blending layer in the current stacking context.
In computeCompositingRequirements - if the current layer has blending
descendants, is not a stacking context and it will be accelerated, the
m_hasCompositedBlendingDescdendents property is passed to its parent, up to the
first layer being a stacking context.

https://codereview.chromium.org/23511004/diff/10001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayerCompositor.cpp:1952: if
(layer->shouldIsolateCompositedBlendingDescendants())
On 2013/09/13 10:22:59, shawnsingh wrote:
> So it seems like this boolean value,
> shouldIsolateCompositedBlendingDescendants() is really only used here.   It is
> used in one ASSERT statement in RenderLayerBacking, but perhaps there's
> something else that could be ASSERTed instead?
This property is used in RenderLayerBacking::updateRootForIsolatedGroup, not
only in the ASSERT. shouldIsolateBlendingDescendants==true means that the
accelerated layer is going to isolate its layer subtree with a new rendering
surface and the software layer is going to create a new skia bitmap to render
the subtree separately (https://codereview.chromium.org/23503046/). I removed
the "Composited" part from and it became shouldIsolateBlendingDescendants.
> 
> If that's true, then I think we should remove that accessor,
> shouldIsolateCompositedBlendingDescendants(), and put the conditions of logic
> directly in here.  Maybe instead of adding another arg to
> subtreeReasonsForCompositing(), we could just pass compositingState itself to
> this function.
> 
> Doing this would also simplify the if-statement that is being added to
> computeCompositingRequirements().

I simplified the if-statement from computeCompositingRequirements

[enne (OOO), 2013-11-07 22:59:06]

https://codereview.chromium.org/23511004/diff/47001/LayoutTests/css3/composit...
File LayoutTests/css3/compositing/mix-blend-mode-isolation-layer-expected.txt
(right):

https://codereview.chromium.org/23511004/diff/47001/LayoutTests/css3/composit...
LayoutTests/css3/compositing/mix-blend-mode-isolation-layer-expected.txt:1: 
Where does this blank line come from?

https://codereview.chromium.org/23511004/diff/47001/Source/core/rendering/Com...
File Source/core/rendering/CompositedLayerMapping.cpp (right):

https://codereview.chromium.org/23511004/diff/47001/Source/core/rendering/Com...
Source/core/rendering/CompositedLayerMapping.cpp:220: if
(RuntimeEnabledFeatures::cssCompositingEnabled())
Put these under the same conditional.

https://codereview.chromium.org/23511004/diff/47001/Source/core/rendering/Com...
Source/core/rendering/CompositedLayerMapping.cpp:285:
m_graphicsLayer->setIsRootForIsolatedGroup(m_owningLayer->hasCompositedBlendingDescendants()
Whoa there.  If you have a tree of RenderLayers creating stacking contexts with
a blending child at the bottom, then all of them will be set as roots for
isolated groups and needlessly become composited layers?  And therefore also in
Chromium all of them will needlessly become render surfaces?

https://codereview.chromium.org/23511004/diff/47001/public/platform/WebLayer.h
File public/platform/WebLayer.h (right):

https://codereview.chromium.org/23511004/diff/47001/public/platform/WebLayer....
public/platform/WebLayer.h:97: // surfece when required.
http://dev.w3.org/fxtf/compositing-1/
Typo, although I think saying that it will create a surface is an implementation
detail.

[rosca, 2013-11-08 15:36:32]

I uploaded a new patch with addressing your comments.
Thanks.

https://codereview.chromium.org/23511004/diff/47001/LayoutTests/css3/composit...
File LayoutTests/css3/compositing/mix-blend-mode-isolation-layer-expected.txt
(right):

https://codereview.chromium.org/23511004/diff/47001/LayoutTests/css3/composit...
LayoutTests/css3/compositing/mix-blend-mode-isolation-layer-expected.txt:1: 
On 2013/11/07 22:59:07, enne wrote:
> Where does this blank line come from?

I fixed this by hiding the other content in body.

https://codereview.chromium.org/23511004/diff/47001/Source/core/rendering/Com...
File Source/core/rendering/CompositedLayerMapping.cpp (right):

https://codereview.chromium.org/23511004/diff/47001/Source/core/rendering/Com...
Source/core/rendering/CompositedLayerMapping.cpp:220: if
(RuntimeEnabledFeatures::cssCompositingEnabled())
On 2013/11/07 22:59:07, enne wrote:
> Put these under the same conditional.

Done.

https://codereview.chromium.org/23511004/diff/47001/Source/core/rendering/Com...
Source/core/rendering/CompositedLayerMapping.cpp:285:
m_graphicsLayer->setIsRootForIsolatedGroup(m_owningLayer->hasCompositedBlendingDescendants()
On 2013/11/07 22:59:07, enne wrote:
> Whoa there.  If you have a tree of RenderLayers creating stacking contexts
with
> a blending child at the bottom, then all of them will be set as roots for
> isolated groups and needlessly become composited layers?  And therefore also
in
> Chromium all of them will needlessly become render surfaces?
No. hasCompositedBlendingDescendants is true only if the layer has a descendant
in the current stacking context, descendants that are enclosed in a descendant
stacking contexts are not taken into account.
I agree that the function name doesn't suggest the same thing. I renamed it to
requiresIsolationForCompositedStackingContext. It will be set to true for all
the layers nodes between the blending layer up to its stacking context layer.
Only the stacking contexts having this flag set will request an isolated group.

https://codereview.chromium.org/23511004/diff/47001/public/platform/WebLayer.h
File public/platform/WebLayer.h (right):

https://codereview.chromium.org/23511004/diff/47001/public/platform/WebLayer....
public/platform/WebLayer.h:97: // surfece when required.
http://dev.w3.org/fxtf/compositing-1/
On 2013/11/07 22:59:07, enne wrote:
> Typo, although I think saying that it will create a surface is an
implementation
> detail.
I removed the comments, I think the functions are self-explanatory and other
setters/getters here don't have comments as well.

[enne (OOO), 2013-11-11 21:52:45]

A few clarification and testing requests.

shawnsingh: can you take another look at this?

https://codereview.chromium.org/23511004/diff/147001/LayoutTests/css3/composi...
File LayoutTests/css3/compositing/mix-blend-mode-isolation-layer.html (right):

https://codereview.chromium.org/23511004/diff/147001/LayoutTests/css3/composi...
LayoutTests/css3/compositing/mix-blend-mode-isolation-layer.html:23: <div
id="stacking-context">
Can you add another test with an additional stacking context parent to verify
that it does not get (isolate 1) in its layer tree dump? I think your code does
the right thing, but I'd like to regression test that.  (This is where
RenderLayerCompositor could really use a unit test framework.)

Also, can you add a test that has a tree like:

  Stacking context A (composited)
    Stacking context B (not composited)
      Blending element C

To verify that neither B nor C get composited for blending or isolate reasons?

https://codereview.chromium.org/23511004/diff/147001/Source/core/rendering/Re...
File Source/core/rendering/RenderLayerCompositor.cpp (right):

https://codereview.chromium.org/23511004/diff/147001/Source/core/rendering/Re...
Source/core/rendering/RenderLayerCompositor.cpp:180: ,
m_hasCompositedBlendingDescendants(false)
Can you rename this to make it more clear? At first blush I would expect this to
be true if there was a composited blending descendant anywhere.  It's really
"hasUnisolatedCompositedBlendingDescendant"? I'm not sure if there's a better
way to make it clear that this is a more localized setting.  I could also
suggest something like "promoteAncestorStackingLayerDueToIsolation"?

https://codereview.chromium.org/23511004/diff/147001/Source/core/rendering/Re...
Source/core/rendering/RenderLayerCompositor.cpp:1726: if
(layer->requiresIsolationForCompositedStackingContext() &&
layer->stackingNode()->isStackingContext())
It's a little awkward to set a "requires ... for stacking context" flag on a
layer that doesn't create a stacking context and then ignore that later.  Could
you only set this flag in cases where the layer actually is isolated and is a
stacking context?

[rosca, 2013-11-12 20:27:07]

Thanks.
Comments inline.

https://codereview.chromium.org/23511004/diff/147001/LayoutTests/css3/composi...
File LayoutTests/css3/compositing/mix-blend-mode-isolation-layer.html (right):

https://codereview.chromium.org/23511004/diff/147001/LayoutTests/css3/composi...
LayoutTests/css3/compositing/mix-blend-mode-isolation-layer.html:23: <div
id="stacking-context">
On 2013/11/11 21:52:46, enne wrote:
> Can you add another test with an additional stacking context parent to verify
> that it does not get (isolate 1) in its layer tree dump? I think your code
does
> the right thing, but I'd like to regression test that.  (This is where
> RenderLayerCompositor could really use a unit test framework.)
> 
> Also, can you add a test that has a tree like:
> 
>   Stacking context A (composited)
>     Stacking context B (not composited)
>       Blending element C
> 
> To verify that neither B nor C get composited for blending or isolate reasons?

Done for the two tests you suggested.

I added one more test with a tree like:

  Stacking context A (not composited)
    Stacking context B (not composited)
      Blending element C (not composited)
        Composited element D

It will verify that B and C will be accelerated and A will not.

https://codereview.chromium.org/23511004/diff/147001/Source/core/rendering/Re...
File Source/core/rendering/RenderLayerCompositor.cpp (right):

https://codereview.chromium.org/23511004/diff/147001/Source/core/rendering/Re...
Source/core/rendering/RenderLayerCompositor.cpp:180: ,
m_hasCompositedBlendingDescendants(false)
I think hasUnisolatedCompositedBlendingDescendant looks good. Thanks.

https://codereview.chromium.org/23511004/diff/147001/Source/core/rendering/Re...
Source/core/rendering/RenderLayerCompositor.cpp:1726: if
(layer->requiresIsolationForCompositedStackingContext() &&
layer->stackingNode()->isStackingContext())
On 2013/11/11 21:52:46, enne wrote:
> It's a little awkward to set a "requires ... for stacking context" flag on a
> layer that doesn't create a stacking context and then ignore that later. 
Could
> you only set this flag in cases where the layer actually is isolated and is a
> stacking context?

Yes, done. In this case I think it's more appropriate to call it
shouldIsolateCompositedDescendants and ASSERT everywhere to make sure it's only
set for stacking contexts.

[rosca, 2013-11-19 23:42:11]

Ping.
shawnsingh: can you take another look at this patch please?

[shawnsingh, 2013-11-23 00:38:13]

Looks great - a few minor comments, and one possible correctness issue due to
other changes that have been happening recently in this code.

https://codereview.chromium.org/23511004/diff/207001/Source/core/rendering/Co...
File Source/core/rendering/CompositedLayerMapping.cpp (right):

https://codereview.chromium.org/23511004/diff/207001/Source/core/rendering/Co...
Source/core/rendering/CompositedLayerMapping.cpp:538: if
(RuntimeEnabledFeatures::cssCompositingEnabled()) {
I think it would be equivalent to move these down to the end of this function
along with all the other updateXXX functions, right?  If that's true, let's do
it that way.

https://codereview.chromium.org/23511004/diff/207001/Source/core/rendering/Co...
Source/core/rendering/CompositedLayerMapping.cpp:1606: if
(m_ancestorClippingLayer) {
lately we are adding more layers in the CompositedLayerMapping, each of which
may be the childForSuperlayers() under different scenarios.  So it's probably
best not to rely on childForSuperLayers() unless you intend to reset the blend
mode to BlendModeNormal for all these other layers, too.

Would it work to simply use m_graphicsLayer to house the blend mode always?  Why
does it need to be the root-most layer of this cluster?

https://codereview.chromium.org/23511004/diff/207001/Source/core/rendering/Re...
File Source/core/rendering/RenderLayer.h (right):

https://codereview.chromium.org/23511004/diff/207001/Source/core/rendering/Re...
Source/core/rendering/RenderLayer.h:662: unsigned
m_shouldIsolateCompositedDescendants : 1;
I think this belongs in the m_compositingProperties struct instead.

[enne (OOO), 2013-11-25 20:21:03]

Thanks for all the test updates.  This lgtm, modulo shawnsingh's approval.

[rosca, 2013-11-25 21:11:41]

Thanks you for comments!

https://codereview.chromium.org/23511004/diff/207001/Source/core/rendering/Co...
File Source/core/rendering/CompositedLayerMapping.cpp (right):

https://codereview.chromium.org/23511004/diff/207001/Source/core/rendering/Co...
Source/core/rendering/CompositedLayerMapping.cpp:538: if
(RuntimeEnabledFeatures::cssCompositingEnabled()) {
On 2013/11/23 00:38:13, shawnsingh wrote:
> I think it would be equivalent to move these down to the end of this function
> along with all the other updateXXX functions, right?  If that's true, let's do
> it that way.

It should be the same. Done.

https://codereview.chromium.org/23511004/diff/207001/Source/core/rendering/Co...
Source/core/rendering/CompositedLayerMapping.cpp:1606: if
(m_ancestorClippingLayer) {
On 2013/11/23 00:38:13, shawnsingh wrote:
> lately we are adding more layers in the CompositedLayerMapping, each of which
> may be the childForSuperlayers() under different scenarios.  So it's probably
> best not to rely on childForSuperLayers() unless you intend to reset the blend
> mode to BlendModeNormal for all these other layers, too.
The same approach is used by scrolling coordinator (updateScrollParent). We
should keep this in sync when other childForSuperlayers are added.
> 
> Would it work to simply use m_graphicsLayer to house the blend mode always? 
Why
> does it need to be the root-most layer of this cluster?
Blending doesn't work if the blend mode is set on m_graphicsLayer being clipped
by an ancestorClippingLayer. It seems like such a structure doesn't allow cc
renderer to copy the correct backdrop. I should look more into this. I'd prefer
to address this with a follow-up patch if you are ok with this.
Added a layout test with this scenario.

https://codereview.chromium.org/23511004/diff/207001/Source/core/rendering/Re...
File Source/core/rendering/RenderLayer.h (right):

https://codereview.chromium.org/23511004/diff/207001/Source/core/rendering/Re...
Source/core/rendering/RenderLayer.h:662: unsigned
m_shouldIsolateCompositedDescendants : 1;
On 2013/11/23 00:38:13, shawnsingh wrote:
> I think this belongs in the m_compositingProperties struct instead.

Done.

https://codereview.chromium.org/23511004/diff/277001/Source/core/rendering/Re...
File Source/core/rendering/RenderLayer.cpp (right):

https://codereview.chromium.org/23511004/diff/277001/Source/core/rendering/Re...
Source/core/rendering/RenderLayer.cpp:3960: ||
needsCompositingLayersRebuiltForBlending(oldStyle, newStyle))
I added this new check. The tree must be rebuilt if blending is turned on/off or
if the isolating stacking context doesn't need to be stacking context any
longer.
Added a new LayoutTest for this as well.

[shawnsingh, 2013-11-26 21:02:17]

> Blending doesn't work if the blend mode is set on m_graphicsLayer being
clipped
> by an ancestorClippingLayer. It seems like such a structure doesn't allow cc
> renderer to copy the correct backdrop. I should look more into this. I'd
prefer
> to address this with a follow-up patch if you are ok with this.
> Added a layout test with this scenario.

That's OK with me - LGTM for landing =)

[rosca, 2013-11-27 13:53:59]

jamesr: I rebased this patch on top of http://crrev.com/90313002/ (Moving
WebCore::BlendMode to blink::WebBlendMode).
Do you have any other comments on Source/web/ and public/ files or you can
approve them for landing?

[jamesr, 2013-11-27 18:19:07]

/web/ and public/ lgtm

[commit-bot: I haz the power, 2013-12-03 11:39:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rosca@adobe.com/23511004/377001

[commit-bot: I haz the power, 2013-12-03 12:36:33]

Change committed as 163079