Only update composited scrolling state when necessary

Only update composited scrolling state when necessary

Here we introduce a dirty bit for m_descendantsAreContiguousInStackingOrder in
order to avoid recomputing fresh values. 

R=jchaffraix

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

[Ian Vollick, 2013-04-09 16:32:00]

On 2013/04/03 20:33:06, jchaffraix wrote:
>
https://codereview.appspot.com/8166047/diff/7001/LayoutTests/compositing/over...
> File
>
LayoutTests/compositing/overflow/automatically-opt-into-composited-scrolling-after-style-change.html
> (right):
> 
>
https://codereview.appspot.com/8166047/diff/7001/LayoutTests/compositing/over...
>
LayoutTests/compositing/overflow/automatically-opt-into-composited-scrolling-after-style-change.html:127:
> contentSuccessor.style.display = n ? 'block' : 'none';
> I find the massive nested for loops to be difficult to understand.
> 
> I think a single list of what you want to test would be more readable and show
> the intent more clearly. You can even use JSON object to make it super clear:
> 
> var tests = [ { 'width': '1px', 'height': '1px', 'overflowX': 'visible',
> 'overflowY': 'visible', 'display': 'block' }, { ... }, ... ];
> for (i = 0; i < tests.len; ++i)
>    writeResult(i, tests[i]);
Done.
> 
>
https://codereview.appspot.com/8166047/diff/7001/LayoutTests/compositing/over...
>
LayoutTests/compositing/overflow/automatically-opt-into-composited-scrolling-after-style-change.html:129:
> writeResult(count++, !!((i || j || n) && (k || m)));
> Head explosion on the iteration count logic.
I've rewritten this logic to use several self-documenting boolean variables.
> 
>
https://codereview.appspot.com/8166047/diff/7001/LayoutTests/compositing/over...
> File LayoutTests/compositing/overflow/dynamic-composited-scrolling-status.html
> (right):
> 
>
https://codereview.appspot.com/8166047/diff/7001/LayoutTests/compositing/over...
> LayoutTests/compositing/overflow/dynamic-composited-scrolling-status.html:39:
> window.setTimeout(reportResult, 0);
> Using setTimeout(..., 0) is a bad smell that something is wrong with your test
> case.
> 
> If you need to force a layout, you can always force it synchronously using
> document.body.offsetLeft which would remove the need for this change.
Fixed.
> 
>
https://codereview.appspot.com/8166047/diff/7001/Source/WebCore/dom/Document.cpp
> File Source/WebCore/dom/Document.cpp (right):
> 
>
https://codereview.appspot.com/8166047/diff/7001/Source/WebCore/dom/Document....
> Source/WebCore/dom/Document.cpp:138: #include "RenderLayer.h"
> I am with Shawn here. We shouldn't add knowledge of RenderLayer outside
> rendering as this is an internal detail and should stay this way.
Removed.
> 
>
https://codereview.appspot.com/8166047/diff/7001/Source/WebCore/rendering/Ren...
> File Source/WebCore/rendering/RenderLayer.cpp (right):
> 
>
https://codereview.appspot.com/8166047/diff/7001/Source/WebCore/rendering/Ren...
> Source/WebCore/rendering/RenderLayer.cpp:2087: void
> RenderLayer::scheduleUpdateNeedsCompositedScrolling()
> This function's name is a bit of a misnomer: scheduling means that we do the
> update later when we actually will force it to be done now (or soonish).
I've renamed these requestNeedsCompositedScrollingUpdate and
requestDescendantsAreContiguousInStackingOrderUpdate. Do  those names line up
better with the semantics of the functions?
> 
>
https://codereview.appspot.com/8166047/diff/7001/Source/WebCore/rendering/Ren...
> Source/WebCore/rendering/RenderLayer.cpp:2121: if (!m_renderView)
> Is it really possible not to have a RenderView?
It's not. Removed check.
> 
>
https://codereview.appspot.com/8166047/diff/7001/Source/WebCore/rendering/Ren...
> Source/WebCore/rendering/RenderLayer.cpp:2124:
> m_renderView->scopedCompositedScrollingUpdatersCount()++;
> Most of the lines are: m_renderView->doSomething()
> 
> It really seems like we should move this code to RenderView, maybe including
> ScopedCompositedScrollingUpdater (not sure if it would help though).
Moved to RenderView. Seems better to me. Please let me know what you think.
> 
>
https://codereview.appspot.com/8166047/diff/7001/Source/WebCore/rendering/Ren...
> Source/WebCore/rendering/RenderLayer.cpp:2125: if (!layer ||
> !layer->acceleratedCompositingForOverflowScrollEnabled())
> You check for !layer above so it's unneeded to check it here again.
Removed.
> 
>
https://codereview.appspot.com/8166047/diff/7001/Source/WebCore/rendering/Ren...
> Source/WebCore/rendering/RenderLayer.cpp:2152: while (stackingContext &&
> !stackingContext->isStackingContext())
> Wouldn't that warrant an enclosingStackingContext() function?
> 
> This could simplify the code to:
> if (RenderLayer* enclosingStackingContext = iter->enclosingStackingContext())
>     stackingContexts.add(enclosingStackingContext);
Done. Looks much nicer.
> 
>
https://codereview.appspot.com/8166047/diff/7001/Source/WebCore/rendering/Ren...
> Source/WebCore/rendering/RenderLayer.cpp:6099: if (isStackingContainer())
> If you are checking isStackingContainer here, shouldn't you also check for
> isStackingContainer above? This discrepancy seems fishy.
You're right, it was fishy. I've removed it. All that was really required here
was to request a need-comp-scroll update. If our stacking-containerness changes
as a result of that, we'll do the clearing of the zorder lists then.
> 
>
https://codereview.appspot.com/8166047/diff/7001/Source/WebCore/rendering/Ren...
> File Source/WebCore/rendering/RenderLayerCompositor.cpp (right):
> 
>
https://codereview.appspot.com/8166047/diff/7001/Source/WebCore/rendering/Ren...
> Source/WebCore/rendering/RenderLayerCompositor.cpp:2904:
> ScopedCompositedScrollingUpdater
> suspendCompositedScrollingUpdates(m_renderView);
> I am really concerned about having so many call sites to patch. That makes it
> easy to A) forget one or B) have one too many.
> 
> Ideally we would like to either do that lazily (ie it's transparent to the
> caller) or have a single update phase followed by just getting the results,
> ASSERT'ing it's not stale.
Removed.
> 
>
https://codereview.appspot.com/8166047/diff/7001/Source/WebCore/rendering/Ren...
> File Source/WebCore/rendering/RenderView.h (right):
> 
>
https://codereview.appspot.com/8166047/diff/7001/Source/WebCore/rendering/Ren...
> Source/WebCore/rendering/RenderView.h:231: unsigned&
> scopedCompositedScrollingUpdatersCount() { return
> m_scopedCompositedScrollingUpdatersCount; }
> This is a violation of the encapsulation as you allow arbitrary callers to
> modify the internal of this class.
> 
> The usual pattern is several methods which I would rather see here.
I was able to remove these after moving ScopedCompositedScrollingUpdater to
RenderView.
> 
>
https://codereview.appspot.com/8166047/diff/7001/Source/WebCore/rendering/Ren...
> Source/WebCore/rendering/RenderView.h:232: HashSet<RenderLayer*>&
> pendingCompositedScrollingUpdates() { return
> m_pendingCompositedScrollingUpdates; }
> Same comment here actually. Maybe moving most of the update code to RenderView
> would alleviate the need for them?
Yup, it did.

[Julien - ping for review, 2013-04-09 21:06:43]

Better!

https://codereview.chromium.org/13913013/diff/2001/LayoutTests/compositing/ov...
File
LayoutTests/compositing/overflow/automatically-opt-into-composited-scrolling-after-style-change.html
(right):

https://codereview.chromium.org/13913013/diff/2001/LayoutTests/compositing/ov...
LayoutTests/compositing/overflow/automatically-opt-into-composited-scrolling-after-style-change.html:47:
<script type="text/javascript" charset="utf-8">
Let's remove these unneeded attribute, they don't add anything and move the
focus away from the important bits.

https://codereview.chromium.org/13913013/diff/2001/LayoutTests/compositing/ov...
LayoutTests/compositing/overflow/automatically-opt-into-composited-scrolling-after-style-change.html:69:
var hasLayerTree = !!window.internals.layerTreeAsText(document);
you don't check if window.internals is available here. As a rule if this needs
to be run through DRT, you are better off just bailing out early and saying this
is an automated test.

https://codereview.chromium.org/13913013/diff/2001/Source/WebCore/rendering/R...
File Source/WebCore/rendering/RenderLayer.cpp (right):

https://codereview.chromium.org/13913013/diff/2001/Source/WebCore/rendering/R...
Source/WebCore/rendering/RenderLayer.cpp:2022: updateIsNormalFlowOnly();
Already mentioned somewhere else but isSelfPaintingLayer relies on isNormalFlow
so the order is wrong :-/

https://codereview.chromium.org/13913013/diff/2001/Source/WebCore/rendering/R...
Source/WebCore/rendering/RenderLayer.cpp:2029:
dirtyStackingContainerZOrderLists();
Wouldn't that dirty the z-order list of a stacking container twice? (see the
code above)

https://codereview.chromium.org/13913013/diff/2001/Source/WebCore/rendering/R...
Source/WebCore/rendering/RenderLayer.cpp:2052:
renderer()->view()->cancelScheduledCompositedScrollingUpdatesForLayer(const_cast<RenderLayer*>(this));
Couldn't cancelScheduledCompositedScrollingUpdatesForLayer take a const pointer
instead of having to do a const_cast?

https://codereview.chromium.org/13913013/diff/2001/Source/WebCore/rendering/R...
Source/WebCore/rendering/RenderLayer.cpp:5845: if (isNormalFlowOnly !=
m_isNormalFlowOnly) {
Better with an early return.

https://codereview.chromium.org/13913013/diff/2001/Source/WebCore/rendering/R...
Source/WebCore/rendering/RenderLayer.cpp:5940: oldVisibility =
oldStyle->visibility();
On one line?

EVisibility oldVisibility = oldStyle ? oldStyle->visibility() : VISIBLE;

https://codereview.chromium.org/13913013/diff/2001/Source/WebCore/rendering/R...
Source/WebCore/rendering/RenderLayer.cpp:5942: if (oldVisibility !=
renderer()->style()->visibility()) {
Again, better with an early return.

https://codereview.chromium.org/13913013/diff/2001/Source/WebCore/rendering/R...
Source/WebCore/rendering/RenderLayer.cpp:5952: int oldZIndex = 0;
Ditto, those should be one liners.

https://codereview.chromium.org/13913013/diff/2001/Source/WebCore/rendering/R...
File Source/WebCore/rendering/RenderView.cpp (right):

https://codereview.chromium.org/13913013/diff/2001/Source/WebCore/rendering/R...
Source/WebCore/rendering/RenderView.cpp:1185: if (m_renderView)
Cannot be NULL per your explanation.

https://codereview.chromium.org/13913013/diff/2001/Source/WebCore/rendering/R...
Source/WebCore/rendering/RenderView.cpp:1206: if (!m_renderView)
Cannot be NULL per your explanation.

https://codereview.chromium.org/13913013/diff/2001/Source/WebCore/rendering/R...
Source/WebCore/rendering/RenderView.cpp:1211: if
(!m_renderView->m_scopedCompositedScrollingUpdatersCount) {
Early return.

https://codereview.chromium.org/13913013/diff/2001/Source/WebCore/rendering/R...
Source/WebCore/rendering/RenderView.cpp:1218: if
(!pendingContiguityUpdates.isEmpty()) {
I don't think we need an isEmpty check as the iterator would just bail out
below.

https://codereview.chromium.org/13913013/diff/2001/Source/WebCore/rendering/R...
Source/WebCore/rendering/RenderView.cpp:1230: if
(!pendingCompositedScrollingUpdates.isEmpty()) {
Same comment.

https://codereview.chromium.org/13913013/diff/2001/Source/WebCore/rendering/R...
Source/WebCore/rendering/RenderView.cpp:1234: }
You do store the HashSet into local variables and clear them. It's good
defense-in-depth coding but we would probably ASSERT here that no one updated
our lists or else we would miss their update indefinitely.

[jamesr, 2013-04-09 21:09:13]

This seems like a hack around the fact that we compute compositing requirements
at the wrong time (i.e. too often).  I'd much rather just fix the compositing
computations to only happen when we want to do a commit instead of trying to
muck with the timing inside RenderLayer*

[jamesr, 2013-04-09 21:10:00]

https://codereview.chromium.org/13913013/diff/2001/Source/WebCore/rendering/R...
File Source/WebCore/rendering/RenderView.h (right):

https://codereview.chromium.org/13913013/diff/2001/Source/WebCore/rendering/R...
Source/WebCore/rendering/RenderView.h:52: struct
ScopedCompositedScrollingUpdater {
in particular this class seems very hacky

[Ian Vollick, 2013-04-12 03:31:11]

On 2013/04/09 21:10:00, jamesr wrote:
>
https://codereview.chromium.org/13913013/diff/2001/Source/WebCore/rendering/R...
> File Source/WebCore/rendering/RenderView.h (right):
> 
>
https://codereview.chromium.org/13913013/diff/2001/Source/WebCore/rendering/R...
> Source/WebCore/rendering/RenderView.h:52: struct
> ScopedCompositedScrollingUpdater {
> in particular this class seems very hacky

And it turns out to be unnecessary, too. In part, it was created to address a
problem that turned out to be fixed with the other changes in this patch. It was
also going to be used in a future patch to delay comp-scroll updates until after
layout, but as we discussed, that's the wrong thing to do anyhow. So I've
removed the class altogether.

[Ian Vollick, 2013-04-12 03:53:14]

Julien, I believe this latest patch addresses the comments from your last
review.

https://codereview.chromium.org/13913013/diff/2001/LayoutTests/compositing/ov...
File
LayoutTests/compositing/overflow/automatically-opt-into-composited-scrolling-after-style-change.html
(right):

https://codereview.chromium.org/13913013/diff/2001/LayoutTests/compositing/ov...
LayoutTests/compositing/overflow/automatically-opt-into-composited-scrolling-after-style-change.html:47:
<script type="text/javascript" charset="utf-8">
On 2013/04/09 21:06:43, jchaffraix wrote:
> Let's remove these unneeded attribute, they don't add anything and move the
> focus away from the important bits.

Done.

https://codereview.chromium.org/13913013/diff/2001/LayoutTests/compositing/ov...
LayoutTests/compositing/overflow/automatically-opt-into-composited-scrolling-after-style-change.html:69:
var hasLayerTree = !!window.internals.layerTreeAsText(document);
On 2013/04/09 21:06:43, jchaffraix wrote:
> you don't check if window.internals is available here. As a rule if this needs
> to be run through DRT, you are better off just bailing out early and saying
this
> is an automated test.

I've found it very useful to be able to view this test in a browser and set
break points during execution for debugging. Is the early out at the beginning
of this function maybe enough?

https://codereview.chromium.org/13913013/diff/2001/Source/WebCore/rendering/R...
File Source/WebCore/rendering/RenderLayer.cpp (right):

https://codereview.chromium.org/13913013/diff/2001/Source/WebCore/rendering/R...
Source/WebCore/rendering/RenderLayer.cpp:2022: updateIsNormalFlowOnly();
On 2013/04/09 21:06:43, jchaffraix wrote:
> Already mentioned somewhere else but isSelfPaintingLayer relies on
isNormalFlow
> so the order is wrong :-/

Done.

https://codereview.chromium.org/13913013/diff/2001/Source/WebCore/rendering/R...
Source/WebCore/rendering/RenderLayer.cpp:2029:
dirtyStackingContainerZOrderLists();
On 2013/04/09 21:06:43, jchaffraix wrote:
> Wouldn't that dirty the z-order list of a stacking container twice? (see the
> code above)
Should be ok, I think. This function starts looking for stacking a
container/context starting at our parent. Shouldn't dirty the same layer twice.

https://codereview.chromium.org/13913013/diff/2001/Source/WebCore/rendering/R...
Source/WebCore/rendering/RenderLayer.cpp:2052:
renderer()->view()->cancelScheduledCompositedScrollingUpdatesForLayer(const_cast<RenderLayer*>(this));
On 2013/04/09 21:06:43, jchaffraix wrote:
> Couldn't cancelScheduledCompositedScrollingUpdatesForLayer take a const
pointer
> instead of having to do a const_cast?
No longer applicable (yay).

https://codereview.chromium.org/13913013/diff/2001/Source/WebCore/rendering/R...
Source/WebCore/rendering/RenderLayer.cpp:5845: if (isNormalFlowOnly !=
m_isNormalFlowOnly) {
On 2013/04/09 21:06:43, jchaffraix wrote:
> Better with an early return.

Done.

https://codereview.chromium.org/13913013/diff/2001/Source/WebCore/rendering/R...
Source/WebCore/rendering/RenderLayer.cpp:5940: oldVisibility =
oldStyle->visibility();
On 2013/04/09 21:06:43, jchaffraix wrote:
> On one line?
> 
> EVisibility oldVisibility = oldStyle ? oldStyle->visibility() : VISIBLE;

Done.

https://codereview.chromium.org/13913013/diff/2001/Source/WebCore/rendering/R...
Source/WebCore/rendering/RenderLayer.cpp:5942: if (oldVisibility !=
renderer()->style()->visibility()) {
On 2013/04/09 21:06:43, jchaffraix wrote:
> Again, better with an early return.

Done.

https://codereview.chromium.org/13913013/diff/2001/Source/WebCore/rendering/R...
Source/WebCore/rendering/RenderLayer.cpp:5952: int oldZIndex = 0;
On 2013/04/09 21:06:43, jchaffraix wrote:
> Ditto, those should be one liners.

Done.

[Julien - ping for review, 2013-04-18 00:42:34]

LGTM with comments.

https://codereview.chromium.org/13913013/diff/20001/LayoutTests/compositing/o...
File
LayoutTests/compositing/overflow/do-not-opt-in-with-out-of-flow-descendant.html
(right):

https://codereview.chromium.org/13913013/diff/20001/LayoutTests/compositing/o...
LayoutTests/compositing/overflow/do-not-opt-in-with-out-of-flow-descendant.html:69:
write(layerTree);
The FAILED output could be improved. Ideally you would want to dump the
layerTree in these cases as it would make the test much much easier to debug.

https://codereview.chromium.org/13913013/diff/20001/Source/WebCore/rendering/...
File Source/WebCore/rendering/RenderLayer.cpp (left):

https://codereview.chromium.org/13913013/diff/20001/Source/WebCore/rendering/...
Source/WebCore/rendering/RenderLayer.cpp:602: ASSERT(!m_zOrderListsDirty);
I would have like to see any explanation why this was removed somewhere. It's
unfortunate as it weakens our pre-conditions.

https://codereview.chromium.org/13913013/diff/20001/Source/WebCore/rendering/...
File Source/WebCore/rendering/RenderLayer.cpp (right):

https://codereview.chromium.org/13913013/diff/20001/Source/WebCore/rendering/...
Source/WebCore/rendering/RenderLayer.cpp:1046: 
Nit: Unneeded extra-space.

https://codereview.chromium.org/13913013/diff/20001/Source/WebCore/rendering/...
Source/WebCore/rendering/RenderLayer.cpp:1296: RenderLayer*
RenderLayer::stackingContext() const
This function should be named stackingContextAncestor(), that would remove the
need for the comment in RenderLayer.h as now your code is self-describing :)

(unfortunately it wouldn't match stackingContainer but we could remove it in the
same way)

https://codereview.chromium.org/13913013/diff/20001/Source/WebCore/rendering/...
Source/WebCore/rendering/RenderLayer.cpp:1298: RenderLayer* layer = parent();
Note that this could be implemented (which I would prefer):
RenderLayer* ancestor = parent();
if (!ancestor)
    return 0;
while (!layer->isStackingContext())
   layer = layer->parent();

ASSERT(layer->isStackingContext()); // Not really useful.
return layer;

https://codereview.chromium.org/13913013/diff/20001/Source/WebCore/rendering/...
Source/WebCore/rendering/RenderLayer.cpp:1749: for (RenderLayer* layer = this;
layer; layer = layer->parent())
This could probably stop at the oldChild's containing block (see addChild).

https://codereview.chromium.org/13913013/diff/20001/Source/WebCore/rendering/...
Source/WebCore/rendering/RenderLayer.cpp:5558: if (stackingContainer)
There is only one case where this could be NULL: the RenderView.

The code could be improved based on that:
if (!stackingContainer)
    return 0;

stackingContainer->dirtyZOrderLists();
RenderLayer* stackingContext = this->stackingContext();
ASSERT(stackingContext);
if (stackingContext != stackingContainer)
   stackingContext->dirtyZOrderLists();

https://codereview.chromium.org/13913013/diff/20001/Source/WebCore/rendering/...
Source/WebCore/rendering/RenderLayer.cpp:5967: updateNeedsCompositedScrolling();
Wrong indentation for the previous 2 lines.

This also forces a full recomputation of the descendant dependent flags which is
annoying. Do we really need it? (could be a FIXME as it is not really something
we want in the long run)

[Ian Vollick, 2013-04-18 20:55:52]

Thanks for the review! I think I've addressed all your comments, but the change
is big enough that I was hoping you could check that it slgty. Can you PTAL?

https://codereview.chromium.org/13913013/diff/20001/LayoutTests/compositing/o...
File
LayoutTests/compositing/overflow/do-not-opt-in-with-out-of-flow-descendant.html
(right):

https://codereview.chromium.org/13913013/diff/20001/LayoutTests/compositing/o...
LayoutTests/compositing/overflow/do-not-opt-in-with-out-of-flow-descendant.html:69:
write(layerTree);
On 2013/04/18 00:42:34, Julien Chaffraix wrote:
> The FAILED output could be improved. Ideally you would want to dump the
> layerTree in these cases as it would make the test much much easier to debug.

Done.

https://codereview.chromium.org/13913013/diff/20001/Source/WebCore/rendering/...
File Source/WebCore/rendering/RenderLayer.cpp (right):

https://codereview.chromium.org/13913013/diff/20001/Source/WebCore/rendering/...
Source/WebCore/rendering/RenderLayer.cpp:1046: 
On 2013/04/18 00:42:34, Julien Chaffraix wrote:
> Nit: Unneeded extra-space.

Done.

https://codereview.chromium.org/13913013/diff/20001/Source/WebCore/rendering/...
Source/WebCore/rendering/RenderLayer.cpp:1296: RenderLayer*
RenderLayer::stackingContext() const
On 2013/04/18 00:42:34, Julien Chaffraix wrote:
> This function should be named stackingContextAncestor(), that would remove the
> need for the comment in RenderLayer.h as now your code is self-describing :)
> 
> (unfortunately it wouldn't match stackingContainer but we could remove it in
the
> same way)

Done.

https://codereview.chromium.org/13913013/diff/20001/Source/WebCore/rendering/...
Source/WebCore/rendering/RenderLayer.cpp:1298: RenderLayer* layer = parent();
On 2013/04/18 00:42:34, Julien Chaffraix wrote:
> Note that this could be implemented (which I would prefer):
> RenderLayer* ancestor = parent();
> if (!ancestor)
>     return 0;
> while (!layer->isStackingContext())
>    layer = layer->parent();
> 
> ASSERT(layer->isStackingContext()); // Not really useful.
> return layer;

Done.

https://codereview.chromium.org/13913013/diff/20001/Source/WebCore/rendering/...
Source/WebCore/rendering/RenderLayer.cpp:1749: for (RenderLayer* layer = this;
layer; layer = layer->parent())
On 2013/04/18 00:42:34, Julien Chaffraix wrote:
> This could probably stop at the oldChild's containing block (see addChild).

Done.

https://codereview.chromium.org/13913013/diff/20001/Source/WebCore/rendering/...
Source/WebCore/rendering/RenderLayer.cpp:5558: if (stackingContainer)
On 2013/04/18 00:42:34, Julien Chaffraix wrote:
> There is only one case where this could be NULL: the RenderView.
> 
> The code could be improved based on that:
> if (!stackingContainer)
>     return 0;
> 
> stackingContainer->dirtyZOrderLists();
> RenderLayer* stackingContext = this->stackingContext();
> ASSERT(stackingContext);
> if (stackingContext != stackingContainer)
>    stackingContext->dirtyZOrderLists();

Done.

https://codereview.chromium.org/13913013/diff/20001/Source/WebCore/rendering/...
Source/WebCore/rendering/RenderLayer.cpp:5967: updateNeedsCompositedScrolling();
On 2013/04/18 00:42:34, Julien Chaffraix wrote:
> Wrong indentation for the previous 2 lines.
> 
> This also forces a full recomputation of the descendant dependent flags which
is
> annoying. Do we really need it? (could be a FIXME as it is not really
something
> we want in the long run)

Until we put the updates into a well defined phase, we have to
updateNeedsCompositedScrolling here, unfortunately. I've added a FIXME. I've
also updated the code so that we don't have to do this update as often (when we
add an out of flow positioned descendant, I now call setAncestorChainBlah rather
than dirtyAncestorChainBlah so we have to dirty/update less frequently).

[Julien - ping for review, 2013-04-19 17:11:18]

Updated change, lgtm!

[commit-bot: I haz the power, 2013-04-20 02:52:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vollick@chromium.org/13913013/49001

[commit-bot: I haz the power, 2013-04-20 03:36:51]

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

[commit-bot: I haz the power, 2013-04-20 04:35:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vollick@chromium.org/13913013/53002

[commit-bot: I haz the power, 2013-04-20 04:49:37]

Retried try job too often on linux_layout_rel for step(s) test_shell_tests,
webkit_lint, webkit_tests, webkit_unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_layo...

[commit-bot: I haz the power, 2013-04-23 16:41:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vollick@chromium.org/13913013/60001

[commit-bot: I haz the power, 2013-04-23 19:15:02]

Change committed as 148916