Don't access compositing state for blending in RenderLayer add/remove child

Don't access compositing state for blending in RenderLayer add/remove child

Code to update the blending status in the RenderLayer hierarchy when a child is added or removed current invokes paintsWithBlendMode which calls compositingState() when it should not. This patch removes the compositing state check in this case, which means we may do more work.

All existing tests pass.

R=senorblanco@chromium.org
BUG=343759
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=170220

[Stephen Chennney, 2014-03-25 16:06:29]

Adobe folks, could you take a look and comment on how sensible this is. We need
to remove the check on compositing state during layout (which is accessing
potentially invalid state anyway). This approach is the most direct but I am
unsure of the consequences.

Senorblanco, I'm adding you as reviewer but please no commit until we have
feedback from the people who implemented this.

[mitica, 2014-03-25 16:16:17]

On 2014/03/25 16:06:29, Stephen Chenney wrote:
> Adobe folks, could you take a look and comment on how sensible this is. We
need
> to remove the check on compositing state during layout (which is accessing
> potentially invalid state anyway). This approach is the most direct but I am
> unsure of the consequences.
> 
> Senorblanco, I'm adding you as reviewer but please no commit until we have
> feedback from the people who implemented this.

Could you please add rosca@adobe in CC?
He did work in this area, I guess he could help on this issue.

[Stephen Chennney, 2014-03-25 16:45:50]

+rosca

[rosca, 2014-03-25 18:06:42]

On 2014/03/25 16:45:50, Stephen Chenney wrote:
> +rosca

I think we should remove paintsWithBlendMode from
RenderLayer::updateDescendantDependentFlags as well.

It should be ok with these changes, because we need blending flags up to date in
RenderLayerBlendInfo regardless of the compositor state. The compositingState is
required only in order to avoid creating new transparencyLayers and to avoid
using blending operators on layers that have their own backings.

[Stephen White, 2014-03-25 18:32:27]

LGTM (but I won't commit).

[Stephen Chennney, 2014-03-26 14:37:51]

More compositing checks removed. I wonder why the assertion code never asserted
on those.

Anyway, all tests pass but I have a question ont he logic around stacking
context.

https://codereview.chromium.org/211103007/diff/110001/Source/core/rendering/R...
File Source/core/rendering/RenderLayer.cpp (left):

https://codereview.chromium.org/211103007/diff/110001/Source/core/rendering/R...
Source/core/rendering/RenderLayer.cpp:862: || (childLayerHasBlendMode &&
!child->stackingNode()->isStackingContext());
Note that this logic is broken, and I believe I have modified it to have the
same effect without the confusion.

In particular, the && clause involving stacking context can only be true if
childLayerHasBlendMode is true, but we will already set the result true because
we || with that sooner. In other words, there's no way the && clause can be true
without the entire expression being true anyway.

It makes me wonder, however, whether this original code is a bug.

[rosca, 2014-03-27 14:52:32]

https://codereview.chromium.org/211103007/diff/110001/Source/core/rendering/R...
File Source/core/rendering/RenderLayer.cpp (left):

https://codereview.chromium.org/211103007/diff/110001/Source/core/rendering/R...
Source/core/rendering/RenderLayer.cpp:862: || (childLayerHasBlendMode &&
!child->stackingNode()->isStackingContext());
On 2014/03/26 14:37:52, Stephen Chenney wrote:
> Note that this logic is broken, and I believe I have modified it to have the
> same effect without the confusion.
> 
> In particular, the && clause involving stacking context can only be true if
> childLayerHasBlendMode is true, but we will already set the result true
because
> we || with that sooner. In other words, there's no way the && clause can be
true
> without the entire expression being true anyway.
> 
> It makes me wonder, however, whether this original code is a bug.

I agree that your change will not change the old logic, which doesn't look to be
right.

I think the correct condition would be:
bool childLayerHasBlendMode = child->blendInfo().hasBlendMode() ||
(!child->stackingNode()->isStackingContext() &&
child->blendInfo().childLayerHasBlendMode());

Why: we don't care if a stacking context child has blending children, we need
this info only for non-stacking context children. For non-stacking context
children we know that the flag is not dirty, as we call
updateDescendantDependentFlags() right above.

I've run the layout tests with all these changes and they pass. If you are ok
with this line, you can insert it to your patch. Tell me if you want me to add
this change with another review along with a test that makes sure this code is
touched.

[Stephen Chennney, 2014-03-27 16:55:59]

I'll commit this and, rosca, you can come on in and fix the logic and add a
test.

[Stephen Chennney, 2014-03-27 16:56:17]

The CQ bit was checked by schenney@chromium.org

[commit-bot: I haz the power, 2014-03-27 16:56:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/schenney@chromium.org/211103007/110001

[commit-bot: I haz the power, 2014-03-27 17:01:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-27 17:01:35]

Try jobs failed on following builders:
  tryserver.blink on linux_blink_rel

[Stephen Chennney, 2014-03-27 18:16:43]

The CQ bit was checked by schenney@chromium.org

[commit-bot: I haz the power, 2014-03-27 18:16:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/schenney@chromium.org/211103007/110001

[commit-bot: I haz the power, 2014-03-27 19:59:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-27 19:59:54]

Try jobs failed on following builders:
  tryserver.blink on linux_blink_dbg

[Stephen Chennney, 2014-03-27 20:53:26]

The CQ bit was checked by schenney@chromium.org

[commit-bot: I haz the power, 2014-03-27 20:53:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/schenney@chromium.org/211103007/110001

[commit-bot: I haz the power, 2014-03-27 22:32:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-27 22:32:40]

Commit queue rejected this change because the description was changed
between the time the change entered the commit queue and the time it
was ready to commit. You can safely check the commit box again.

[Stephen Chennney, 2014-03-27 23:37:59]

Committed patchset #2 manually as r170220 (presubmit successful).