Issue 7759045: Fix opt-in checks for out-of-flow positioned descendants

Can't Edit
Can't Publish+Mail
Start Review

Created:
12 years, 10 months ago by vollick

Modified:
12 years, 9 months ago

Reviewers:
danakj, hartmanng, shawnsingh

Base URL:
git://git.webkit.org/WebKit.git@master

Visibility:
Public.

More Reviews

Description

This is an aggregate patch. It will probably need to be split up before landing. It contains several fixes Glenn and I have had in flight. All of these patches were needed to get the code working correctly, and all needed to be modified updated. The pieces are 1) The "early out" work. That patch introduces the RAII object I make heavy use of in this patch. The object has been modified significantly. 2) An implementation of the plan outlined here (see the last comment): https://bugs.webkit.org/show_bug.cgi?id=109591 3) Code for maintaining m_hasFixedPositionDescendant. 4) Many fixes to ensure that we dirty the right state and schedule updates so that we can have some confidence that the values we need will be clean when we use them. I would really appreciate a thorough review. If the correctness of any this code feels fishy, or if you have to think about it for too long, please flag it, even if it turns out to be correct. Also, if you have ideas for how this patch might be split, please note them. BUG=

Patch Set 1 #

Patch Set 2 : . #

Patch Set 3 : . #

Patch Set 4 : . #

Patch Set 5 : . #

Total comments: 27

Patch Set 6 : . #

Patch Set 7 : . #

Total comments: 1

Patch Set 8 : . #

Patch Set 9 : . #

Patch Set 10 : About to gut the unnecessary optimizations. #

Patch Set 11 : . #

Patch Set 12 : Massive overhaul. #

Patch Set 13 : Removed debugging crud from the layout tests. #

Created: 12 years, 9 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+952 lines, -167 lines)			Patch
M	LayoutTests/compositing/overflow/automatically-opt-into-composited-scrolling.html	View	1 2 3 4 5 6 7 8 9 10 11	2 chunks	+11 lines, -6 lines	0 comments	Download
A	LayoutTests/compositing/overflow/automatically-opt-into-composited-scrolling-after-style-change.html	View	1 2 3 4 5 6 7 8 9 10 11 12	1 chunk	+169 lines, -0 lines	0 comments	Download
A	LayoutTests/compositing/overflow/automatically-opt-into-composited-scrolling-after-style-change-expected.txt	View	1 2 3 4 5 6 7 8 9 10 11	1 chunk	+39 lines, -0 lines	0 comments	Download
A	LayoutTests/compositing/overflow/do-not-opt-in-with-out-of-flow-descendant.html	View	1 2 3 4 5 6 7 8 9 10 11 12	1 chunk	+129 lines, -0 lines	0 comments	Download
A	LayoutTests/compositing/overflow/do-not-opt-in-with-out-of-flow-descendant-expected.txt	View	1 2 3 4 5 6 7	1 chunk	+15 lines, -0 lines	0 comments	Download
A	LayoutTests/compositing/overflow/invisible-descendants-should-not-affect-opt-in.html	View	1 2 3 4 5 6 7	1 chunk	+116 lines, -0 lines	0 comments	Download
A	LayoutTests/compositing/overflow/invisible-descendants-should-not-affect-opt-in-expected.txt	View	1 2 3 4 5 6 7	1 chunk	+7 lines, -0 lines	0 comments	Download
M	Source/WebCore/dom/Document.cpp	View	1 2 3 4 5 6 7 8 9 10 11	2 chunks	+6 lines, -0 lines	0 comments	Download
M	Source/WebCore/rendering/RenderLayer.h	View	1 2 3 4 5 6 7 8 9 10 11	9 chunks	+49 lines, -8 lines	0 comments	Download
M	Source/WebCore/rendering/RenderLayer.cpp	View	1 2 3 4 5 6 7 8 9 10 11	31 chunks	+342 lines, -113 lines	0 comments	Download
M	Source/WebCore/rendering/RenderLayerCompositor.h	View	1 2 3	1 chunk	+7 lines, -1 line	0 comments	Download
M	Source/WebCore/rendering/RenderLayerCompositor.cpp	View	1 2 3 4 5 6 7 8 9 10 11	8 chunks	+16 lines, -9 lines	0 comments	Download
M	Source/WebCore/rendering/RenderObject.h	View	1 2 3 5 6 7 8 9 10 11	1 chunk	+4 lines, -0 lines	0 comments	Download
M	Source/WebCore/rendering/RenderObject.cpp	View	1 2 3 4 5 6 7 8 9 10 11	1 chunk	+42 lines, -30 lines	0 comments	Download

Messages

Total messages: 7

Expand All Messages | Collapse All Messages

vollick

+shawnsingh, +hartmanng Hey! This beast is finally ready for a review. Please be merciless.

12 years, 9 months ago (2013-03-26 20:35:18 UTC) #1

hartmanng

Here's my first look through. I think I got less careful towards the end though, ...

12 years, 9 months ago (2013-03-26 22:51:59 UTC) #3

vollick

New patch. https://codereview.appspot.com/7759045/diff/11001/Source/WebCore/rendering/RenderBlock.cpp File Source/WebCore/rendering/RenderBlock.cpp (right): https://codereview.appspot.com/7759045/diff/11001/Source/WebCore/rendering/RenderBlock.cpp#newcode113 Source/WebCore/rendering/RenderBlock.cpp:113: const size_t MaximumDeltaListLength = 16; On 2013/03/26 ...

12 years, 9 months ago (2013-03-27 16:27:34 UTC) #4

New patch.

https://codereview.appspot.com/7759045/diff/11001/Source/WebCore/rendering/Re...
File Source/WebCore/rendering/RenderBlock.cpp (right):

https://codereview.appspot.com/7759045/diff/11001/Source/WebCore/rendering/Re...
Source/WebCore/rendering/RenderBlock.cpp:113: const size_t
MaximumDeltaListLength = 16;
On 2013/03/26 22:52:00, hartmanng wrote:
> I know this is just an arbitrary number, but is it "final"? Ie, have you
> experimented with it yet? Definitely something we should do for justification
> before putting it up on bugzilla.

Nope, no testing at all. I suppose I could do some benchmarks and plot this
number over both speed of hasOutOfFlowPositionedDescendant and total # of deltas
I'm hanging onto.

https://codereview.appspot.com/7759045/diff/11001/Source/WebCore/rendering/Re...
Source/WebCore/rendering/RenderBlock.cpp:3764: if (!descendantSet ||
descendantSet->isEmpty()) {
On 2013/03/26 22:52:00, hartmanng wrote:
> ASSERT(descendantSet);
> if (!descendantSet ... )
> 
> 
> that seems wrong. Either we should be guaranteed to have it (and use ASSERT)
or
> it could go either way (and use the if).
> 
Good point. Actually, this whole function was a bit of a mess, so I've reworked
it. PTAL.
> 
> After looking through the rest of the function, I also have to ask, why do we
> early-out if descendantSet is NULL or empty? We don't use it below. Shouldn't
we
> still clean up the rendererListDeltas if they're overfull?

https://codereview.appspot.com/7759045/diff/11001/Source/WebCore/rendering/Re...
Source/WebCore/rendering/RenderBlock.cpp:3814: rendererListDeltas->resetCount =
0;
On 2013/03/26 22:52:00, hartmanng wrote:
> I feel like this could be taken care of in a constructor, but nbd
Done.

https://codereview.appspot.com/7759045/diff/11001/Source/WebCore/rendering/Re...
Source/WebCore/rendering/RenderBlock.cpp:3823:
maintainTrackedRendererListDeltas(this);
On 2013/03/26 22:52:00, hartmanng wrote:
> maybe this is just being picky, but it makes more sense to me if we do this
> before adding a new item. Seems strange to add a new item and then potentially
> immediately clear the whole list.

Done.

https://codereview.appspot.com/7759045/diff/11001/Source/WebCore/rendering/Re...
Source/WebCore/rendering/RenderBlock.cpp:3859: if (!rendererListDeltas)
On 2013/03/26 22:52:00, hartmanng wrote:
> ASSERT(rendererListDeltas);
> if (!rendererListDeltas)
> 
> like above

Done.

https://codereview.appspot.com/7759045/diff/11001/Source/WebCore/rendering/Re...
Source/WebCore/rendering/RenderBlock.cpp:3867:
maintainTrackedRendererListDeltas(container);
On 2013/03/26 22:52:00, hartmanng wrote:
> again, makes more sense to me if this comes before appending to the list.

Done.

https://codereview.appspot.com/7759045/diff/11001/Source/WebCore/rendering/Re...
Source/WebCore/rendering/RenderBlock.cpp:4253:
insertIntoTrackedRendererMaps(descendant, gPercentHeightDescendantsMap,
gPercentHeightContainerMap, false, dummy);
On 2013/03/26 22:52:00, hartmanng wrote:
> not really sure what a "percent height descendant" is, why doesn't it need to
be
> tracked?
We only look at gPositionedDescendants when checking has-out-of-flow-etc-etc, so
we only use those deltas. Unfortunately, both sets of maps get updated with the
same functions so I have to pass in the dummy..

https://codereview.appspot.com/7759045/diff/11001/Source/WebCore/rendering/Re...
File Source/WebCore/rendering/RenderBlock.h (right):

https://codereview.appspot.com/7759045/diff/11001/Source/WebCore/rendering/Re...
Source/WebCore/rendering/RenderBlock.h:63: bool isAddition;
On 2013/03/26 22:52:00, hartmanng wrote:
> this should probably be an enum

Done.

https://codereview.appspot.com/7759045/diff/11001/Source/WebCore/rendering/Re...
File Source/WebCore/rendering/RenderLayer.cpp (right):

https://codereview.appspot.com/7759045/diff/11001/Source/WebCore/rendering/Re...
Source/WebCore/rendering/RenderLayer.cpp:293:
gCompositedScrollingUpdatesMap->get(stackingContext)->descendants.remove(this);
On 2013/03/26 22:52:00, hartmanng wrote:
> may want an ASSERT or something here to check
> 
>
gCompositedScrollingUpdatesMap->get(stackingContext)->descendants.contains(this)
There's a good chance that 'this' isn't in the list. This remove is a
precaution, and if it's not in the list, it's not an error.

https://codereview.appspot.com/7759045/diff/11001/Source/WebCore/rendering/Re...
Source/WebCore/rendering/RenderLayer.cpp:722:
scheduleUpdateNeedsCompositedScrolling();
On 2013/03/26 22:52:00, hartmanng wrote:
> we should probably still only schedule an update if the value actually changed
> since last time

Done.

https://codereview.appspot.com/7759045/diff/11001/Source/WebCore/rendering/Re...
Source/WebCore/rendering/RenderLayer.cpp:1154: if
(m_hasFixedPositionedDescendantDirty || m_visibleDescendantStatusDirty)
On 2013/03/26 22:52:00, hartmanng wrote:
> why no ifdefs anymore?
I made it so that this function is a no op if !USE(ACCELERATED_COMPOSITING)

https://codereview.appspot.com/7759045/diff/11001/Source/WebCore/rendering/Re...
Source/WebCore/rendering/RenderLayer.cpp:1850: RenderLayer* stackingContext =
oldChild;
On 2013/03/26 22:52:00, hartmanng wrote:
> oldChild->stackingContext() ?

That function skips right over oldChild, unfortunately. I need to start the
searching at oldChild.

vollick

That business with the deltas in RenderBlock was premature optimization. We only spend about 0.3ms ...

12 years, 9 months ago (2013-03-29 02:16:35 UTC) #5

vollick

I'm pretty excited about this patch. Please review if you have a moment. As I ...

12 years, 9 months ago (2013-03-30 05:19:06 UTC) #6

vollick

12 years, 9 months ago (2013-03-30 18:42:25 UTC) #7

I've split this patch into two pieces
 1) An update to hartmanng's https://bugs.webkit.org/show_bug.cgi?id=109966
    https://codereview.appspot.com/8166047/
 2) A reworking of the out-of-flow positioned descendants checks.
    https://codereview.appspot.com/8200043/

The second depends on the first. PTAL at these patches when you have a sec.

Expand All Messages | Collapse All Messages