RAL: Eliminate n^2 walk to find repaint containers

RAL: Eliminate n^2 walk to find repaint containers

Instead of walking up from every renderer to find the containerForRepaint,
pass the current repaint container along through repaintTreeAfterLayout
and update it when we pass through a renderer that's a repaint container.

This patch also introduces an isRepaintContainer function. This is guaranteed
to be able to be a repaint container, but due to filters and regions,
isn't guaranteed to be the repaint container for non-composited descendant
renderers. The magic logic to handle these special cases has been moved to
an adjustCompositedContainerForSpecialAncestors function, which has been
split out of containerForRepaint.

This patch also adds a switch in RenderBox to avoid unnecessarily mallocing
LayoutStates when we know our repaint container isn't the view.

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

[leviw_travelin_and_unemployed, 2014-05-06 18:23:24]

ptal

[Julien - ping for review, 2014-05-07 17:50:03]

lgtm but let's add a lot of missing 'const'.

+Vollick: For the composited + positioned weirdness as I think it's not correct.

https://codereview.chromium.org/264183002/diff/70001/Source/core/rendering/Re...
File Source/core/rendering/RenderBlock.cpp (right):

https://codereview.chromium.org/264183002/diff/70001/Source/core/rendering/Re...
Source/core/rendering/RenderBlock.cpp:377: RenderLayerModelObject*
repaintContainerForChild = (*it)->containerForRepaint();
That's kind of crazy that a positioned element doesn't paint into its containing
block or an ancestor of it. It sounds like we could end up improperly painting
due to that.

https://codereview.chromium.org/264183002/diff/70001/Source/core/rendering/Re...
File Source/core/rendering/RenderBox.cpp (right):

https://codereview.chromium.org/264183002/diff/70001/Source/core/rendering/Re...
Source/core/rendering/RenderBox.cpp:1615: // FIXME: LayoutState should be
enabled for other repaint containers than the RenderView. crbug.com/363834
I choked on this line but Levi pointed out that we already disable LayoutState
if repaintContainer != view()

https://codereview.chromium.org/264183002/diff/70001/Source/core/rendering/Re...
File Source/core/rendering/RenderView.h (right):

https://codereview.chromium.org/264183002/diff/70001/Source/core/rendering/Re...
Source/core/rendering/RenderView.h:218: if ((!doingFullRepaint() ||
(RuntimeEnabledFeatures::repaintAfterLayoutEnabled() &&
!m_layoutStateDisableCount)) || m_layoutState->isPaginated() ||
renderer.hasColumns() || renderer.flowThreadContainingBlock()
Can we use layoutStateEnabled() instead of that as m_layoutState should not be
null there?

Also that means that the comment above is no longer valid.

https://codereview.chromium.org/264183002/diff/70001/Source/core/rendering/sv...
File Source/core/rendering/svg/RenderSVGModelObject.cpp (right):

https://codereview.chromium.org/264183002/diff/70001/Source/core/rendering/sv...
Source/core/rendering/svg/RenderSVGModelObject.cpp:127: void
RenderSVGModelObject::repaintTreeAfterLayout(RenderLayerModelObject&
repaintContainer)
This should be a const RenderLayerModelObject& as we shouldn't be modifying
repaintContainer anywhere in the logic.

https://codereview.chromium.org/264183002/diff/70001/Source/core/rendering/sv...
Source/core/rendering/svg/RenderSVGModelObject.cpp:140:
setPreviousPositionFromRepaintContainer(positionFromRepaintContainer(&newRepaintContainer));
It really looks like clippedOverflowrectForRepaint and
positionFromRepaintContainer should take reference as they don't expect a NULL
repaintContainer. This can be done in this change or in a follow-up.

[Julien - ping for review, 2014-05-07 17:50:19]

+Vollick for real

[leviw_travelin_and_unemployed, 2014-05-07 17:58:48]

Thanks for the review!

https://codereview.chromium.org/264183002/diff/70001/Source/core/rendering/Re...
File Source/core/rendering/RenderBlock.cpp (right):

https://codereview.chromium.org/264183002/diff/70001/Source/core/rendering/Re...
Source/core/rendering/RenderBlock.cpp:377: RenderLayerModelObject*
repaintContainerForChild = (*it)->containerForRepaint();
On 2014/05/07 17:50:03, Julien Chaffraix - PST wrote:
> That's kind of crazy that a positioned element doesn't paint into its
containing
> block or an ancestor of it. It sounds like we could end up improperly painting
> due to that.

Super weird, I agree.

https://codereview.chromium.org/264183002/diff/70001/Source/core/rendering/Re...
File Source/core/rendering/RenderBox.cpp (right):

https://codereview.chromium.org/264183002/diff/70001/Source/core/rendering/Re...
Source/core/rendering/RenderBox.cpp:1615: // FIXME: LayoutState should be
enabled for other repaint containers than the RenderView. crbug.com/363834
On 2014/05/07 17:50:03, Julien Chaffraix - PST wrote:
> I choked on this line but Levi pointed out that we already disable LayoutState
> if repaintContainer != view()

As we discussed, I have a plan to fix this (which should be full of win), but am
waiting to be able to clean up the crazyness happening in LayoutState that the
old Repaint logic relies on.

https://codereview.chromium.org/264183002/diff/70001/Source/core/rendering/Re...
File Source/core/rendering/RenderView.h (right):

https://codereview.chromium.org/264183002/diff/70001/Source/core/rendering/Re...
Source/core/rendering/RenderView.h:218: if ((!doingFullRepaint() ||
(RuntimeEnabledFeatures::repaintAfterLayoutEnabled() &&
!m_layoutStateDisableCount)) || m_layoutState->isPaginated() ||
renderer.hasColumns() || renderer.flowThreadContainingBlock()
On 2014/05/07 17:50:03, Julien Chaffraix - PST wrote:
> Can we use layoutStateEnabled() instead of that as m_layoutState should not be
> null there?
> 
> Also that means that the comment above is no longer valid.

We can definitely use layoutStateEnabled(). Also good point re: the comment (at
least for RAL).

https://codereview.chromium.org/264183002/diff/70001/Source/core/rendering/sv...
File Source/core/rendering/svg/RenderSVGModelObject.cpp (right):

https://codereview.chromium.org/264183002/diff/70001/Source/core/rendering/sv...
Source/core/rendering/svg/RenderSVGModelObject.cpp:140:
setPreviousPositionFromRepaintContainer(positionFromRepaintContainer(&newRepaintContainer));
On 2014/05/07 17:50:03, Julien Chaffraix - PST wrote:
> It really looks like clippedOverflowrectForRepaint and
> positionFromRepaintContainer should take reference as they don't expect a NULL
> repaintContainer. This can be done in this change or in a follow-up.

There's one exception to this. absoluteClippedOverflowRect passes null to denote
"absolute," but it should be fixed to return the view it wants to translate
into.

[Ian Vollick, 2014-05-07 22:55:57]

https://codereview.chromium.org/264183002/diff/70001/Source/core/rendering/Re...
File Source/core/rendering/RenderBlock.cpp (right):

https://codereview.chromium.org/264183002/diff/70001/Source/core/rendering/Re...
Source/core/rendering/RenderBlock.cpp:377: RenderLayerModelObject*
repaintContainerForChild = (*it)->containerForRepaint();
On 2014/05/07 17:58:48, leviw wrote:
> On 2014/05/07 17:50:03, Julien Chaffraix - PST wrote:
> > That's kind of crazy that a positioned element doesn't paint into its
> containing
> > block or an ancestor of it. It sounds like we could end up improperly
painting
> > due to that.
> 
> Super weird, I agree.

Oh, that is strange. I wonder what bugs this is currently causing..

[eseidel, 2014-05-08 00:43:01]

https://codereview.chromium.org/264183002/diff/70001/Source/core/rendering/Re...
File Source/core/rendering/RenderBlock.cpp (right):

https://codereview.chromium.org/264183002/diff/70001/Source/core/rendering/Re...
Source/core/rendering/RenderBlock.cpp:377: RenderLayerModelObject*
repaintContainerForChild = (*it)->containerForRepaint();
I'm not sure it is odd.  But I admit to not fully keeping painting in my head. 
http://www.w3.org/TR/CSS2/zindex.html

On 2014/05/07 22:55:58, Ian Vollick wrote:
> On 2014/05/07 17:58:48, leviw wrote:
> > On 2014/05/07 17:50:03, Julien Chaffraix - PST wrote:
> > > That's kind of crazy that a positioned element doesn't paint into its
> > containing
> > > block or an ancestor of it. It sounds like we could end up improperly
> painting
> > > due to that.
> > 
> > Super weird, I agree.
> 
> Oh, that is strange. I wonder what bugs this is currently causing..

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

https://codereview.chromium.org/264183002/diff/70001/Source/core/rendering/Re...
Source/core/rendering/RenderLayer.cpp:1114: return compositingState() ==
PaintsIntoOwnBacking || compositingState() == PaintsIntoGroupedBacking;
When is this compositing information determined?  During compositing?  Or during
style resolve.  If it's during compositing, then this is making layout dependant
on compositing which is dependent on layout, no?

[leviw_travelin_and_unemployed, 2014-05-08 01:28:27]

On 2014/05/08 00:43:01, eseidel wrote:
>
https://codereview.chromium.org/264183002/diff/70001/Source/core/rendering/Re...
> File Source/core/rendering/RenderBlock.cpp (right):
> 
>
https://codereview.chromium.org/264183002/diff/70001/Source/core/rendering/Re...
> Source/core/rendering/RenderBlock.cpp:377: RenderLayerModelObject*
> repaintContainerForChild = (*it)->containerForRepaint();
> I'm not sure it is odd.  But I admit to not fully keeping painting in my head.

> http://www.w3.org/TR/CSS2/zindex.html
> 
> On 2014/05/07 22:55:58, Ian Vollick wrote:
> > On 2014/05/07 17:58:48, leviw wrote:
> > > On 2014/05/07 17:50:03, Julien Chaffraix - PST wrote:
> > > > That's kind of crazy that a positioned element doesn't paint into its
> > > containing
> > > > block or an ancestor of it. It sounds like we could end up improperly
> > painting
> > > > due to that.
> > > 
> > > Super weird, I agree.
> > 
> > Oh, that is strange. I wonder what bugs this is currently causing..
> 
>
https://codereview.chromium.org/264183002/diff/70001/Source/core/rendering/Re...
> File Source/core/rendering/RenderLayer.cpp (right):
> 
>
https://codereview.chromium.org/264183002/diff/70001/Source/core/rendering/Re...
> Source/core/rendering/RenderLayer.cpp:1114: return compositingState() ==
> PaintsIntoOwnBacking || compositingState() == PaintsIntoGroupedBacking;
> When is this compositing information determined?  During compositing?  Or
during
> style resolve.  If it's during compositing, then this is making layout
dependant
> on compositing which is dependent on layout, no?

It's determined when compositing is updated. This is the current state of
affairs for repaint and has been for some time (which is why you'll see
RenderObject::containerForRepaint disable compositing asserts). Julien has begun
looking into moving repaint until after compositing update. This should
alleviate this issue, but currently also breaks our repaint test harness.

[ojan, 2014-05-10 17:43:12]

https://codereview.chromium.org/264183002/diff/70001/Source/core/rendering/Re...
File Source/core/rendering/RenderBlock.cpp (right):

https://codereview.chromium.org/264183002/diff/70001/Source/core/rendering/Re...
Source/core/rendering/RenderBlock.cpp:377: RenderLayerModelObject*
repaintContainerForChild = (*it)->containerForRepaint();
In theory, all the positioned objects in this loop should have the same repaint
container, right? Can we just call containerForRepaint once outside the loop?

[ojan, 2014-05-10 18:02:30]

https://codereview.chromium.org/264183002/diff/70001/Source/core/rendering/Re...
File Source/core/rendering/RenderBlock.cpp (right):

https://codereview.chromium.org/264183002/diff/70001/Source/core/rendering/Re...
Source/core/rendering/RenderBlock.cpp:377: RenderLayerModelObject*
repaintContainerForChild = (*it)->containerForRepaint();
On 2014/05/10 17:43:12, ojan wrote:
> In theory, all the positioned objects in this loop should have the same
repaint
> container, right? Can we just call containerForRepaint once outside the loop?

Hmmm. I guess the repaint container would be different for different position
values. In either case, we'd only need to call conatinerForRepaint once per
position value we encounter.

[leviw_travelin_and_unemployed, 2014-05-13 13:31:00]

PTAL if you can. I've made a few cosmetic changes (including fixing the const
issues).

[leviw_travelin_and_unemployed, 2014-05-13 13:35:37]

D'oh... There's another ASSERT that needs to be disabled (in
RenderObject::positionFromRepaintContainer).

[leviw_travelin_and_unemployed, 2014-05-13 15:33:00]

On 2014/05/13 13:35:37, leviw wrote:
> D'oh... There's another ASSERT that needs to be disabled (in
> RenderObject::positionFromRepaintContainer).

Should be good now, though I'm sad about disabling the asserts :(

[Julien - ping for review, 2014-05-16 10:21:05]

lgtm

https://codereview.chromium.org/264183002/diff/150001/Source/core/rendering/R...
File Source/core/rendering/RenderBlock.cpp (right):

https://codereview.chromium.org/264183002/diff/150001/Source/core/rendering/R...
Source/core/rendering/RenderBlock.cpp:379: const RenderLayerModelObject&
repaintContainerForChild = *box->containerForRepaint();
I don't think Ojan's optimization would work here as the positioned descendants
can be anywhere in the current subtree (they don't have to share a common
composited ancestor).

If I missed something, I would like this to be duly noted in the code though!

https://codereview.chromium.org/264183002/diff/150001/Source/core/rendering/R...
File Source/core/rendering/RenderView.h (right):

https://codereview.chromium.org/264183002/diff/150001/Source/core/rendering/R...
Source/core/rendering/RenderView.h:218: if ((!doingFullRepaint() ||
(RuntimeEnabledFeatures::repaintAfterLayoutEnabled() &&
!m_layoutStateDisableCount)) || m_layoutState->isPaginated() ||
renderer.hasColumns() || renderer.flowThreadContainingBlock()
This still applies to the new patch and it hasn't been taken into account:

>> Can we use layoutStateEnabled() instead of that as m_layoutState should not
be
>> null there?
>> 
>> Also that means that the comment above is no longer valid.

> We can definitely use layoutStateEnabled(). Also good point re: the comment
(at
> least for RAL).

[leviw_travelin_and_unemployed, 2014-05-16 13:39:03]

The CQ bit was checked by leviw@chromium.org

[commit-bot: I haz the power, 2014-05-16 13:39:34]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/leviw@chromium.org/264183002/190001

[leviw_travelin_and_unemployed, 2014-05-16 14:05:45]

The CQ bit was checked by leviw@chromium.org

[commit-bot: I haz the power, 2014-05-16 14:05:56]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/leviw@chromium.org/264183002/210001

[commit-bot: I haz the power, 2014-05-16 15:38:10]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/7382)

[commit-bot: I haz the power, 2014-05-16 16:23:57]

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

[commit-bot: I haz the power, 2014-05-16 16:23:57]

Try jobs failed on following builders:
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/7399)

[leviw_travelin_and_unemployed, 2014-05-16 16:45:06]

The CQ bit was checked by leviw@chromium.org

[commit-bot: I haz the power, 2014-05-16 16:45:56]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/leviw@chromium.org/264183002/230001

[commit-bot: I haz the power, 2014-05-16 18:15:46]

Change committed as 174197