Replace the current contiguity check for composited scrolling

Replace the current contiguity check for composited scrolling

This should be a more correct, future-proof method based on comparing
paint-order lists before and after promotion to ensure there is no paint-order
change caused by opting in.

Performance numbers: https://docs.google.com/spreadsheet/ccc?key=0AtW7ar1zSvWFdEo2b0drQWx2X3hoSU5UNGJYeDcyUVE&usp=sharing

R=esprehn@chromium.org, jchaffraix@chromium.org

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

[hartmanng, 2013-04-08 16:21:31]

Shawn and Ian, please pre-review this patch as well. Note that it depends on
https://codereview.chromium.org/13467028/.

[Ian Vollick, 2013-04-16 19:24:33]

Main comment: I think this patch also depends on
https://codereview.chromium.org/13913013/ and should probably be rebased onto
it.

Other than that, just some minor nits.

https://codereview.chromium.org/13427009/diff/1/Source/WebCore/rendering/Rend...
File Source/WebCore/rendering/RenderLayer.cpp (right):

https://codereview.chromium.org/13427009/diff/1/Source/WebCore/rendering/Rend...
Source/WebCore/rendering/RenderLayer.cpp:784: //
(https://bugs.webkit.org/show_bug.cgi?id=109966)
That patch is a prereq for this, so we don't need this comment.

https://codereview.chromium.org/13427009/diff/1/Source/WebCore/rendering/Rend...
Source/WebCore/rendering/RenderLayer.cpp:810: #if USE(ACCELERATED_COMPOSITING)
These guards can go now.

[hartmanng, 2013-04-17 20:21:46]

On 2013/04/16 19:24:33, vollick wrote:
> Main comment: I think this patch also depends on
> https://codereview.chromium.org/13913013/ and should probably be rebased onto
> it.

Done.

Also note that I changed TestExpectations - presubmit informed me that the first
3 tests I had listed as failing don't exist (due to blink, I'm guessing?) and
the 4th actually passed, so I took out all the failing tests.

I've also verified that this passes the following layout tests on debug:

- compositing/overflow/opt-into-composited-scrolling-positioned-ancestor.html
- compositing/overflow/automatically-opt-into-composited-scrolling.html
- compositing/overflow/invisible-descendants-should-not-affect-opt-in.html
- compositing/overflow/dynamic-composited-scrolling-status.html
- compositing/overflow/do-not-opt-in-with-out-of-flow-descendant.html
-
compositing/overflow/automatically-opt-into-composited-scrolling-after-style-change.html
-
compositing/overflow/build-paint-order-list-where-opt-in-decisions-can-affect-each-other.html
- compositing/overflow/build-paint-order-lists.html


are there any important ones I'm missing?

[hartmanng, 2013-04-17 20:21:53]

https://codereview.chromium.org/13427009/diff/1/Source/WebCore/rendering/Rend...
File Source/WebCore/rendering/RenderLayer.cpp (right):

https://codereview.chromium.org/13427009/diff/1/Source/WebCore/rendering/Rend...
Source/WebCore/rendering/RenderLayer.cpp:784: //
(https://bugs.webkit.org/show_bug.cgi?id=109966)
On 2013/04/16 19:24:33, vollick wrote:
> That patch is a prereq for this, so we don't need this comment.

Done.

https://codereview.chromium.org/13427009/diff/1/Source/WebCore/rendering/Rend...
Source/WebCore/rendering/RenderLayer.cpp:810: #if USE(ACCELERATED_COMPOSITING)
On 2013/04/16 19:24:33, vollick wrote:
> These guards can go now.

Done.

[hartmanng, 2013-04-24 17:52:30]

Julien, here's the third (and final!) patch for my opt-in feature. This patch
hooks up the paint order list building and layout tests from the previous 2
patches, and replaces the old contiguity check.

Please take a look.

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

final LGTM!

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

https://codereview.chromium.org/13427009/diff/15002/Source/core/rendering/Ren...
Source/core/rendering/RenderLayer.cpp:546: // A = positioned ancestor of
currentLayer
A as in Positioned?

https://codereview.chromium.org/13427009/diff/15002/Source/core/rendering/Ren...
Source/core/rendering/RenderLayer.cpp:644: size_t
posZOrderListSizeBeforePromote, negZOrderListSizeBeforePromote,
posZOrderListSizeAfterPromote, negZOrderListSizeAfterPromote;
It's not really in the style guide but it's usually preferred to have a single
variable per line.

https://codereview.chromium.org/13427009/diff/15002/Source/core/rendering/Ren...
Source/core/rendering/RenderLayer.cpp:659: bool didUpdate = (canPromote !=
m_canBePromotedToStackingContainer);
You changed tense here which makes it hard to see. canPromote seems to imply
that we are going to vs didUpdate which means we did.

I would stick with one tense and change didUpdate to
shouldUpdateCompositedScrolling or something similar.

https://codereview.chromium.org/13427009/diff/15002/Source/core/rendering/Ren...
Source/core/rendering/RenderLayer.cpp:1177: return
m_canBePromotedToStackingContainer;
ASSERT(!m_canBePromotedToStackingContainerDirty);

(or else we are happily querying a dirty status which seems wrong to me)

https://codereview.chromium.org/13427009/diff/15002/Source/core/rendering/Ren...
Source/core/rendering/RenderLayer.cpp:5878: // call UpdateCanBeStackingContainer
for all descendants,
Sentences start with a capital so "Call ..."

[esprehn, 2013-04-26 02:39:28]

Not lgtm. Are you sure this won't cause a perf regression? You're building the z
order lists multiple times recursively all the way down the tree. What if I had
thousands of layers like G+ does?

Please also don't pass around int size_t sizes with Vectors. The size is inside
the vector to prevent off by one bugs, you should never pass around the size
with it.

[hartmanng, 2013-05-06 21:41:51]

New patch (Patch Set 6) added to fix review comments and address Elliot's
performance concerns. This patch is now based one Ian's work at
https://codereview.chromium.org/14863002. PTAL.


On 2013/04/26 02:39:28, esprehn wrote:
> Not lgtm. Are you sure this won't cause a perf regression? You're building the
z
> order lists multiple times recursively all the way down the tree. What if I
had
> thousands of layers like G+ does?

There was a slight perf regression, but it was always (IMO) reasonably small,
especially compared to the scrolling performance gains we get from the patch.
Regardless, thanks to Ian's work (https://codereview.chromium.org/14858004/ and
https://codereview.chromium.org/14863002) we're actually seeing performance
gains from this approach now, since Ian has eliminated the need to recursively
clean entire subtrees at once. See perf numbers here:
https://docs.google.com/spreadsheet/ccc?key=0AtW7ar1zSvWFdEo2b0drQWx2X3hoSU5U....


> Please also don't pass around int size_t sizes with Vectors. The size is
inside
> the vector to prevent off by one bugs, you should never pass around the size
> with it.

Done.

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

https://codereview.chromium.org/13427009/diff/15002/Source/core/rendering/Ren...
Source/core/rendering/RenderLayer.cpp:546: // A = positioned ancestor of
currentLayer
On 2013/04/25 21:09:04, Julien Chaffraix wrote:
> A as in Positioned?

Done.

https://codereview.chromium.org/13427009/diff/15002/Source/core/rendering/Ren...
Source/core/rendering/RenderLayer.cpp:644: size_t
posZOrderListSizeBeforePromote, negZOrderListSizeBeforePromote,
posZOrderListSizeAfterPromote, negZOrderListSizeAfterPromote;
On 2013/04/25 21:09:04, Julien Chaffraix wrote:
> It's not really in the style guide but it's usually preferred to have a single
> variable per line.

Removed these variables, no longer relevant.

https://codereview.chromium.org/13427009/diff/15002/Source/core/rendering/Ren...
Source/core/rendering/RenderLayer.cpp:659: bool didUpdate = (canPromote !=
m_canBePromotedToStackingContainer);
On 2013/04/25 21:09:04, Julien Chaffraix wrote:
> You changed tense here which makes it hard to see. canPromote seems to imply
> that we are going to vs didUpdate which means we did.
> 
> I would stick with one tense and change didUpdate to
> shouldUpdateCompositedScrolling or something similar.

Done.

https://codereview.chromium.org/13427009/diff/15002/Source/core/rendering/Ren...
Source/core/rendering/RenderLayer.cpp:1177: return
m_canBePromotedToStackingContainer;
On 2013/04/25 21:09:04, Julien Chaffraix wrote:
> ASSERT(!m_canBePromotedToStackingContainerDirty);
> 
> (or else we are happily querying a dirty status which seems wrong to me)

Done.

https://codereview.chromium.org/13427009/diff/15002/Source/core/rendering/Ren...
Source/core/rendering/RenderLayer.cpp:5878: // call UpdateCanBeStackingContainer
for all descendants,
On 2013/04/25 21:09:04, Julien Chaffraix wrote:
> Sentences start with a capital so "Call ..."

Done.

[hartmanng, 2013-05-07 00:24:44]

+esprehn

Sorry Elliot, I assumed you had added yourself as a reviewer already. Can you
please take a look at this patch? I want to be sure it fixes the issues you
brought up.

[esprehn, 2013-05-07 00:49:15]

Looks much better! You have some dead code though, and I don't like having the
sizing logic in there and then this combined get*At() method. Just use two loops
like in the computePaintOrderLists() method.

I'd also prefer if you didn't put huge comments into RenderLayer.cpp, that kind
of thing belongs in Docs. You want short single paragraph comments that explain
purpose of methods, not what they do or give huge examples.

https://codereview.chromium.org/13427009/diff/46001/LayoutTests/TestExpectations
File LayoutTests/TestExpectations (right):

https://codereview.chromium.org/13427009/diff/46001/LayoutTests/TestExpectati...
LayoutTests/TestExpectations:1436: crbug.com/238282
virtual/softwarecompositing/overflow/opt-into-composited-scrolling-positioned-ancestor.html
[ Failure ]
Are you going to fix these? We're trying to combat test rot.

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

https://codereview.chromium.org/13427009/diff/46001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayer.cpp:527: // stacking order direction).
This comment is not needed.

https://codereview.chromium.org/13427009/diff/46001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayer.cpp:528: static const RenderLayer*
getStackingOrderElementAt(const Vector<RenderLayer*>* posZOrderList, const
Vector<RenderLayer*>* negZOrderList, const StackingOrderDirection direction,
const size_t index)
I'd prefer you remove this method. It just makes the code harder to follow.

https://codereview.chromium.org/13427009/diff/46001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayer.cpp:609: // We will opt in, keeping paint
order constant.
I'd prefer if you created a much shorter and simpler comment that just explains
the purpose of this method which is decided if making an element that wasn't a
stacking context into one would change the paint order and that you need it to
check for promoting to stacking contexts for scrolling. 

You don't need all these examples in the code. I'd rather a Google doc you send
to blink-dev that describes this whole system.

This comment is crazy long to put inside RenderLayer.cpp. Aim for one short
paragraph.

https://codereview.chromium.org/13427009/diff/46001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayer.cpp:619: for (size_t index = 0; index <
sizeBeforePromote && index < sizeAfterPromote; index++) {
Use two separate loops please. Get rid of that combined get*() method. You can
achieve the same thing by just reversing the loop start and end and such.

https://codereview.chromium.org/13427009/diff/46001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayer.cpp:639: // zOrderListAfterPromote from each
direction.
Remove this comment. Comments that reference other comments are bad news.

https://codereview.chromium.org/13427009/diff/46001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayer.cpp:654: // examples for
compareLayerListsBeforeAndAfterPromote().
Same deal, don't reference across doc comments like this. Just remove the second
line.

https://codereview.chromium.org/13427009/diff/46001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayer.cpp:682: sizeBeforePromote +=
negZOrderListBeforePromote->size();
This size variable is unused. These two if blocks (and the one below) are dead
code.

https://codereview.chromium.org/13427009/diff/46001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayer.cpp:688: sizeAfterPromote +=
negZOrderListAfterPromote->size();
This size variable is unused. Please remove the code.

[hartmanng, 2013-05-13 17:44:18]

Hey sorry for my lack of updates on this patch, I was away much of last week.

I've uploaded a new patch (patch set 7) which I think addresses all the comments
from the last review, PTAL

https://codereview.chromium.org/13427009/diff/46001/LayoutTests/TestExpectations
File LayoutTests/TestExpectations (right):

https://codereview.chromium.org/13427009/diff/46001/LayoutTests/TestExpectati...
LayoutTests/TestExpectations:1436: crbug.com/238282
virtual/softwarecompositing/overflow/opt-into-composited-scrolling-positioned-ancestor.html
[ Failure ]
On 2013/05/07 00:49:16, esprehn wrote:
> Are you going to fix these? We're trying to combat test rot.

Yep! Already have an in-progress patch to fix these:
https://codereview.chromium.org/14999005/

(sorry, I thought I had included that link somewhere, but it looks like I missed
it).

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

https://codereview.chromium.org/13427009/diff/46001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayer.cpp:527: // stacking order direction).
On 2013/05/07 00:49:16, esprehn wrote:
> This comment is not needed.

Done.

https://codereview.chromium.org/13427009/diff/46001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayer.cpp:528: static const RenderLayer*
getStackingOrderElementAt(const Vector<RenderLayer*>* posZOrderList, const
Vector<RenderLayer*>* negZOrderList, const StackingOrderDirection direction,
const size_t index)
On 2013/05/07 00:49:16, esprehn wrote:
> I'd prefer you remove this method. It just makes the code harder to follow.

Done.

https://codereview.chromium.org/13427009/diff/46001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayer.cpp:609: // We will opt in, keeping paint
order constant.
On 2013/05/07 00:49:16, esprehn wrote:
> I'd prefer if you created a much shorter and simpler comment that just
explains
> the purpose of this method which is decided if making an element that wasn't a
> stacking context into one would change the paint order and that you need it to
> check for promoting to stacking contexts for scrolling. 
> 
> You don't need all these examples in the code. I'd rather a Google doc you
send
> to blink-dev that describes this whole system.
> 
> This comment is crazy long to put inside RenderLayer.cpp. Aim for one short
> paragraph.

Done.

https://codereview.chromium.org/13427009/diff/46001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayer.cpp:619: for (size_t index = 0; index <
sizeBeforePromote && index < sizeAfterPromote; index++) {
On 2013/05/07 00:49:16, esprehn wrote:
> Use two separate loops please. Get rid of that combined get*() method. You can
> achieve the same thing by just reversing the loop start and end and such.

Done.

https://codereview.chromium.org/13427009/diff/46001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayer.cpp:639: // zOrderListAfterPromote from each
direction.
On 2013/05/07 00:49:16, esprehn wrote:
> Remove this comment. Comments that reference other comments are bad news.

Done.

https://codereview.chromium.org/13427009/diff/46001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayer.cpp:654: // examples for
compareLayerListsBeforeAndAfterPromote().
On 2013/05/07 00:49:16, esprehn wrote:
> Same deal, don't reference across doc comments like this. Just remove the
second
> line.

Done.

https://codereview.chromium.org/13427009/diff/46001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayer.cpp:682: sizeBeforePromote +=
negZOrderListBeforePromote->size();
On 2013/05/07 00:49:16, esprehn wrote:
> This size variable is unused. These two if blocks (and the one below) are dead
> code.

Done.

https://codereview.chromium.org/13427009/diff/46001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayer.cpp:688: sizeAfterPromote +=
negZOrderListAfterPromote->size();
On 2013/05/07 00:49:16, esprehn wrote:
> This size variable is unused. Please remove the code.

Done.

[esprehn, 2013-05-15 06:50:17]

LGTM. Few style nits, but this looks great.

https://codereview.chromium.org/13427009/diff/67001/LayoutTests/TestExpectations
File LayoutTests/TestExpectations (right):

https://codereview.chromium.org/13427009/diff/67001/LayoutTests/TestExpectati...
LayoutTests/TestExpectations:1413: crbug.com/238282
virtual/softwarecompositing/overflow/opt-into-composited-scrolling-positioned-ancestor.html
[ Failure ]
Hopefully we can fix these soon.

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

https://codereview.chromium.org/13427009/diff/67001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayer.cpp:567: break;
This should be in the loop condition like i < maxIndex && layerAfterPromote !=
this

https://codereview.chromium.org/13427009/diff/67001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayer.cpp:572: ?
posZOrderListBeforePromote->at(posZOrderListBeforePromote->size() - i - 1)
This kind of math is really scary. I guess we do bounds checks in Vector now
though.

https://codereview.chromium.org/13427009/diff/67001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayer.cpp:582: break;
Same deal

[hartmanng, 2013-05-15 15:18:58]

Thanks!

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

https://codereview.chromium.org/13427009/diff/67001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayer.cpp:567: break;
On 2013/05/15 06:50:18, esprehn wrote:
> This should be in the loop condition like i < maxIndex && layerAfterPromote !=
> this

Done.

https://codereview.chromium.org/13427009/diff/67001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayer.cpp:572: ?
posZOrderListBeforePromote->at(posZOrderListBeforePromote->size() - i - 1)
On 2013/05/15 06:50:18, esprehn wrote:
> This kind of math is really scary. I guess we do bounds checks in Vector now
> though.

Yeah I know what you mean - can you think of a way to make it safer?

https://codereview.chromium.org/13427009/diff/67001/Source/core/rendering/Ren...
Source/core/rendering/RenderLayer.cpp:582: break;
On 2013/05/15 06:50:18, esprehn wrote:
> Same deal

Done.

[commit-bot: I haz the power, 2013-05-30 17:47:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hartmanng@chromium.org/13427009/84001

[commit-bot: I haz the power, 2013-05-30 18:45:20]

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

[commit-bot: I haz the power, 2013-05-30 20:37:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hartmanng@chromium.org/13427009/96002

[commit-bot: I haz the power, 2013-05-30 21:54:24]

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-05-31 20:47:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hartmanng@chromium.org/13427009/120001

[Ian Vollick, 2013-05-31 23:42:39]

Committed patchset #13 manually as r151585 (presubmit successful).