Avoid requiring a recalc to update compositing state for iframes

Avoid requiring a recalc to update compositing state for iframes

r164013[1] first exposed this issue when it stopped issuing
StyleChangeFromRenderer in some cases.

The solution here is:
1. iframes always generate RenderLayers. This allows the
compositing update to consider the iframe in the same pass.
2. When the compositing update detects a change it requests
a requirements update in the enclosing frame. In the full
compositing update this will happen in the same pass.

[1] https://src.chromium.org/viewvc/blink?revision=164013&view=revision

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

[abarth-chromium, 2014-02-06 03:27:50]

https://codereview.chromium.org/149653003/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderLayerCompositor.cpp (right):

https://codereview.chromium.org/149653003/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderLayerCompositor.cpp:2474: }
I ran into this same function when working on putting the layout states into the
DocumentLifecycle state machine:

https://codereview.chromium.org/156413002

The problem with this function is that it schedules a style recalc while layout
is on the stack.  Now that we're more careful to only do layout when we're about
to put up a frame, the style recalc doesn't actually happen until the next
frame.  I'm not sure whether that's the cause of the bug you're seeing, but it's
one way that we might put up a frame while we're in an inconsistent state.

[dstockwell, 2014-02-06 03:36:19]

https://codereview.chromium.org/149653003/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderLayerCompositor.cpp (right):

https://codereview.chromium.org/149653003/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderLayerCompositor.cpp:2474: }
On 2014/02/06 03:27:50, abarth wrote:
> I ran into this same function when working on putting the layout states into
the
> DocumentLifecycle state machine:
> 
> https://codereview.chromium.org/156413002
> 
> The problem with this function is that it schedules a style recalc while
layout
> is on the stack.  Now that we're more careful to only do layout when we're
about
> to put up a frame, the style recalc doesn't actually happen until the next
> frame.  I'm not sure whether that's the cause of the bug you're seeing, but
it's
> one way that we might put up a frame while we're in an inconsistent state.

I think that's the main reason this is broken at head. I believe with these
changes it should be possible to remove the recalc request here.

[abarth-chromium, 2014-02-06 04:29:13]

On 2014/02/06 03:36:19, dstockwell wrote:
> I think that's the main reason this is broken at head. I believe with these
> changes it should be possible to remove the recalc request here.

Awesome.  One less DocumentLifecycle::DeprecatedTransition.  :)

[shawnsingh, 2014-02-06 13:32:15]

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

https://codereview.chromium.org/149653003/diff/90001/Source/core/rendering/Re...
Source/core/rendering/RenderLayerCompositor.cpp:2490:
ownerElement->scheduleLayerUpdate();
I'm curious what else does scheduleLayerUpdate do other than flag a
setNeedsRecalcStyle?

If it doesn't do anything meaningfully significant w.r.t. iframe compositing
changes (and if it does, it shouldn't), then I think we could just remove this
entire function, and replace it with a much simpler
setNeedsToRecoputeCompositingRequirements like you added here.

[dstockwell, 2014-02-07 03:22:01]

This is ready for review now, I think I have all of the rebaselines necessary,
they're probably worth a look though.

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

https://codereview.chromium.org/149653003/diff/90001/Source/core/rendering/Re...
Source/core/rendering/RenderLayerCompositor.cpp:2490:
ownerElement->scheduleLayerUpdate();
On 2014/02/06 13:32:16, shawnsingh wrote:
> I'm curious what else does scheduleLayerUpdate do other than flag a
> setNeedsRecalcStyle?
> 
> If it doesn't do anything meaningfully significant w.r.t. iframe compositing
> changes (and if it does, it shouldn't), then I think we could just remove this
> entire function, and replace it with a much simpler
> setNeedsToRecoputeCompositingRequirements like you added here.

That's all it does. I think we can remove it, but I think we should do it in a
follow up so that this more surgical change can be merged to M33.

[shawnsingh, 2014-02-07 08:04:22]

non-OWNER LGTM for RenderLayerCompositor.cpp

[abarth-chromium, 2014-02-07 18:39:00]

rslgtm

[abarth-chromium, 2014-02-07 18:39:11]

The CQ bit was checked by abarth@chromium.org

[commit-bot: I haz the power, 2014-02-07 18:39:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dstockwell@chromium.org/149653003/930001

[commit-bot: I haz the power, 2014-02-07 20:52:53]

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

[commit-bot: I haz the power, 2014-02-07 20:52:54]

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

[dstockwell, 2014-02-10 06:31:39]

The CQ bit was checked by dstockwell@chromium.org

[commit-bot: I haz the power, 2014-02-10 06:31:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dstockwell@chromium.org/149653003/3540006

[commit-bot: I haz the power, 2014-02-10 07:42:31]

Change committed as 166807

[abarth-chromium, 2014-02-10 08:02:54]

On 2014/02/07 03:22:01, dstockwell wrote:
> That's all it does. I think we can remove it, but I think we should do it in a
> follow up so that this more surgical change can be merged to M33.

@dstockwell: Are you planning to looking making that follow up change?  If not,
I can give it a try.

[dstockwell, 2014-02-10 09:42:29]

On 2014/02/10 08:02:54, abarth wrote:
> On 2014/02/07 03:22:01, dstockwell wrote:
> > That's all it does. I think we can remove it, but I think we should do it in
a
> > follow up so that this more surgical change can be merged to M33.
> 
> @dstockwell: Are you planning to looking making that follow up change?  If
not,
> I can give it a try.

I'll upload the patch shortly.