NOT FOR REVIEW - Update comp-scrolling state at a well defined point in the pipeline.

Update comp-scrolling state at a well defined point in the pipeline.

NOTE - this is an aggregate patch. It will hopefully be landed in pieces.

BUG=

[hartmanng, 2013-05-01 20:25:06]

-CCs (blink-reviews@chromium.org, jchaffraix+rendering@chromium.org) to avoid
spamming them with a not-for-review patch.

In general, looks great, there are a few little things I'm not understanding
completely though.

I sort of expected fewer calls to updateDescendantDependentFlags(). Maybe it's
just my own misunderstanding, but why do we still call it throughout, but we're
able to refactor the out-of-flow stuff to RLC?

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

https://codereview.chromium.org/14741004/diff/1/Source/core/rendering/RenderL...
Source/core/rendering/RenderLayerCompositor.cpp:334: void
RenderLayerCompositor::updateCompositingRequirementsState(RenderLayer*
updateRoot)
I feel like updateDescendantDependentFlags should be called in here, don't we
need it?

https://codereview.chromium.org/14741004/diff/1/Source/core/rendering/RenderL...
Source/core/rendering/RenderLayerCompositor.cpp:343:
updateRoot->updateHasOutOfFlowPositionedDescendant();
why do we only update this inside the if block?

[Ian Vollick, 2013-05-02 03:24:50]

Thanks for the review. All tests pass now. I've brought back the
can-be-promoted-to-stacking-container dirty bit.

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

https://codereview.chromium.org/14741004/diff/1/Source/core/rendering/RenderL...
Source/core/rendering/RenderLayerCompositor.cpp:334: void
RenderLayerCompositor::updateCompositingRequirementsState(RenderLayer*
updateRoot)
On 2013/05/01 20:25:07, hartmanng wrote:
> I feel like updateDescendantDependentFlags should be called in here, don't we
> need it?

Done.

https://codereview.chromium.org/14741004/diff/1/Source/core/rendering/RenderL...
Source/core/rendering/RenderLayerCompositor.cpp:343:
updateRoot->updateHasOutOfFlowPositionedDescendant();
On 2013/05/01 20:25:07, hartmanng wrote:
> why do we only update this inside the if block?
That call is recursive, so I only want to call it once in the first call to
updateCompositingRequirementsState.

[hartmanng, 2013-05-02 14:04:01]

I think the plan for the split into 3 patches looks good

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

https://codereview.chromium.org/14741004/diff/1/Source/core/rendering/RenderL...
Source/core/rendering/RenderLayer.cpp:2128: TRACE_EVENT0("blink-rendering",
"RenderLayer::updateNeedsCompositedScrolling\n");
Might not be a bad idea to leave these trace events in?

https://codereview.chromium.org/14741004/diff/10001/LayoutTests/compositing/o...
File
LayoutTests/compositing/overflow/automatically-opt-into-composited-scrolling.html
(right):

https://codereview.chromium.org/14741004/diff/10001/LayoutTests/compositing/o...
LayoutTests/compositing/overflow/automatically-opt-into-composited-scrolling.html:93:
window.internals.FORCE_COMPOSITED_SCROLLING_ON);
This doesn't match the .idl file, shouldn't it be "ForceCompositedScrollingOff"?

https://codereview.chromium.org/14741004/diff/10001/LayoutTests/compositing/o...
LayoutTests/compositing/overflow/automatically-opt-into-composited-scrolling.html:99:
window.internals.settings.DO_NOT_FORCE_COMPOSITED_SCROLLING);
same as above

https://codereview.chromium.org/14741004/diff/10001/LayoutTests/compositing/o...
File
LayoutTests/compositing/overflow/build-paint-order-list-where-opt-in-decisions-can-affect-each-other.html
(right):

https://codereview.chromium.org/14741004/diff/10001/LayoutTests/compositing/o...
LayoutTests/compositing/overflow/build-paint-order-list-where-opt-in-decisions-can-affect-each-other.html:118:
window.internals.FORCE_COMPOSITED_SCROLLING_OFF);
here and below too

https://codereview.chromium.org/14741004/diff/10001/LayoutTests/compositing/o...
File LayoutTests/compositing/overflow/build-paint-order-lists.html (right):

https://codereview.chromium.org/14741004/diff/10001/LayoutTests/compositing/o...
LayoutTests/compositing/overflow/build-paint-order-lists.html:100:
window.internals.FORCE_COMPOSITED_SCROLLING_OFF);
and here, am I missing something or is this just a rebase error?

https://codereview.chromium.org/14741004/diff/10001/LayoutTests/compositing/o...
File
LayoutTests/compositing/overflow/out-of-flow-pos-descendants-should-affect-all-ancestors.html
(right):

https://codereview.chromium.org/14741004/diff/10001/LayoutTests/compositing/o...
LayoutTests/compositing/overflow/out-of-flow-pos-descendants-should-affect-all-ancestors.html:68:
<div style="width: 450px; height: 450px">
I know this was me, but we should probably clean up the style and move it into
<head>

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

https://codereview.chromium.org/14741004/diff/10001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayer.cpp:633: TRACE_EVENT0("blink",
"RenderLayer::updateCanBeStackingContainer");
should this be "blink" or "blink-rendering"?

https://codereview.chromium.org/14741004/diff/10001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayer.cpp:1198:
childContainingBlocks.remove(renderer());
this might deserve a comment

https://codereview.chromium.org/14741004/diff/10001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayer.cpp:2122: #if
ENABLE(ACCELERATED_OVERFLOW_SCROLLING)
is this macro required anymore?

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

https://codereview.chromium.org/14741004/diff/21001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayer.cpp:1099: // PATCH 3
There should be an assert in here, right?