Update automatically-opt-into-composited-scrolling.html

Update automatically-opt-into-composited-scrolling.html

This test is intended to generate all paint orderings that are noteworthy wrt
opting into composited scrollincg. Looking at
http://www.w3.org/TR/CSS2/visuren.html#z-index there are several categories of
element considered when stacking elements in a stacking context. Categories 3-5
do not appear to overlap one another

so we've chosen one element from the categories which can overlap and then
permuted them.

If the elements are given unique z-indices, we can force any paint ordering
pre-promotion, so we test all of them.

If a non-descendant also has z-index:0, then its ordering wrt the promoted
container may change, so we test all valid orderings where this is the case.

Similarly, if a descendant and a non-descendant both have z-index:0, they could
also change order after promotion, so we consider those permutations as well.

In addition to covering all interesting permutations and types of elements, we
also have to consider cases where the element to be promoted is
positioned-but-not-a-stacking-context or has an ancestor in the same stacking
context that is positioned-but-not-a-stacking-context. These properties affect
paint ordering as per: http://www.w3.org/TR/CSS2/visuren.html#z-index.

R=jchaffraix

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

[hartmanng, 2013-04-08 15:52:28]

Continuing review from https://codereview.appspot.com/7926043.

jchaffraix, enne, please take a look.

Julien, I tried to clarify the comment about recalculating style from your
previous review, I believe it was the only outstanding comment.

[Julien - ping for review, 2013-04-09 17:41:59]

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

https://codereview.chromium.org/13722015/diff/2001/LayoutTests/compositing/ov...
LayoutTests/compositing/overflow/automatically-opt-into-composited-scrolling.html:54:
<script type="text/javascript" charset="utf-8"
src="automatically-opt-into-composited-scrolling.js"></script>
Can we remove the useless attributes from new testing code? (like 'type' and
'charset')

I won't repeat that comment but it's *very* widespread in your code and removes
the attention from what is important.

https://codereview.chromium.org/13722015/diff/2001/LayoutTests/compositing/ov...
LayoutTests/compositing/overflow/automatically-opt-into-composited-scrolling.html:56:
function didPass(count, ordering, hasPositionedAncestor, containerIsPositioned)
didPass is misnamed as it can print FAIL.

How about runOneTest or testPermutation? (alternatively doTest but you would
have to rename the function below)

https://codereview.chromium.org/13722015/diff/2001/LayoutTests/compositing/ov...
File
LayoutTests/compositing/overflow/automatically-opt-into-composited-scrolling.js
(right):

https://codereview.chromium.org/13722015/diff/2001/LayoutTests/compositing/ov...
LayoutTests/compositing/overflow/automatically-opt-into-composited-scrolling.js:20:
var layerTree = window.internals.layerTreeAsText(document);
This is really magical unless you start to pull the code fro m layerTreeAsText.
It is also very fragile as if you had a composited layer unrelated to what you
are testing, this will happily return true, regardless of whether you opted in.

https://codereview.chromium.org/13722015/diff/2001/LayoutTests/compositing/ov...
LayoutTests/compositing/overflow/automatically-opt-into-composited-scrolling.js:35:
window.internals.boundingBox(element);
I don't understand why you need this line as document.body.offsetTop should
force a layout too!

Also you probably want to avoid using internals for something this trivial:
element.offsetTop;

https://codereview.chromium.org/13722015/diff/2001/LayoutTests/compositing/ov...
LayoutTests/compositing/overflow/automatically-opt-into-composited-scrolling.js:37:
var errorCode = 0;
errorCode is useless and should be removed: the funtion raises an exception if
something goes wrong. We also never check it.

https://codereview.chromium.org/13722015/diff/2001/LayoutTests/compositing/ov...
LayoutTests/compositing/overflow/automatically-opt-into-composited-scrolling.js:160:
if (window.internals) {
I don't see why you check window.internals here

https://codereview.chromium.org/13722015/diff/2001/LayoutTests/compositing/ov...
LayoutTests/compositing/overflow/automatically-opt-into-composited-scrolling.js:179:
for (var siblingIndex = 0; siblingIndex < 4; ++siblingIndex) {
4 is a magic constant here, indices.length is way better!

https://codereview.chromium.org/13722015/diff/2001/LayoutTests/compositing/ov...
LayoutTests/compositing/overflow/automatically-opt-into-composited-scrolling.js:182:
for (var containerSubindex = 0; containerSubindex < 3; ++containerSubindex) {
Again, 3 is a magic constant, you should use siblingIndices.length

[hartmanng, 2013-04-09 20:40:09]

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

https://codereview.chromium.org/13722015/diff/2001/LayoutTests/compositing/ov...
LayoutTests/compositing/overflow/automatically-opt-into-composited-scrolling.html:54:
<script type="text/javascript" charset="utf-8"
src="automatically-opt-into-composited-scrolling.js"></script>
On 2013/04/09 17:41:59, jchaffraix wrote:
> Can we remove the useless attributes from new testing code? (like 'type' and
> 'charset')
> 
> I won't repeat that comment but it's *very* widespread in your code and
removes
> the attention from what is important.

Yeah, sorry, a lot of this was originally based on an older test. I've taken out
all the useless attributes I can find.

https://codereview.chromium.org/13722015/diff/2001/LayoutTests/compositing/ov...
LayoutTests/compositing/overflow/automatically-opt-into-composited-scrolling.html:56:
function didPass(count, ordering, hasPositionedAncestor, containerIsPositioned)
On 2013/04/09 17:41:59, jchaffraix wrote:
> didPass is misnamed as it can print FAIL.
> 
> How about runOneTest or testPermutation? (alternatively doTest but you would
> have to rename the function below)

Done.

https://codereview.chromium.org/13722015/diff/2001/LayoutTests/compositing/ov...
File
LayoutTests/compositing/overflow/automatically-opt-into-composited-scrolling.js
(right):

https://codereview.chromium.org/13722015/diff/2001/LayoutTests/compositing/ov...
LayoutTests/compositing/overflow/automatically-opt-into-composited-scrolling.js:20:
var layerTree = window.internals.layerTreeAsText(document);
On 2013/04/09 17:41:59, jchaffraix wrote:
> This is really magical unless you start to pull the code fro m
layerTreeAsText.
> It is also very fragile as if you had a composited layer unrelated to what you
> are testing, this will happily return true, regardless of whether you opted
in.

Good point, although in this case there is only 1 div that could become
composited, this js file could feasibly be included in more complex layout tests
down the road.

I switched to a better way using window.internals.nonFastScrollableRects().

https://codereview.chromium.org/13722015/diff/2001/LayoutTests/compositing/ov...
LayoutTests/compositing/overflow/automatically-opt-into-composited-scrolling.js:35:
window.internals.boundingBox(element);
On 2013/04/09 17:41:59, jchaffraix wrote:
> I don't understand why you need this line as document.body.offsetTop should
> force a layout too!
> 
> Also you probably want to avoid using internals for something this trivial:
> element.offsetTop;

Ah, didn't realize that. Removed.

https://codereview.chromium.org/13722015/diff/2001/LayoutTests/compositing/ov...
LayoutTests/compositing/overflow/automatically-opt-into-composited-scrolling.js:37:
var errorCode = 0;
On 2013/04/09 17:41:59, jchaffraix wrote:
> errorCode is useless and should be removed: the funtion raises an exception if
> something goes wrong. We also never check it.

Done.

https://codereview.chromium.org/13722015/diff/2001/LayoutTests/compositing/ov...
LayoutTests/compositing/overflow/automatically-opt-into-composited-scrolling.js:160:
if (window.internals) {
On 2013/04/09 17:41:59, jchaffraix wrote:
> I don't see why you check window.internals here

The particular didPass() function in this case requires internals to be able to
do anything useful. Moved the window.internals check inside that function, it
makes a lot more sense there.

https://codereview.chromium.org/13722015/diff/2001/LayoutTests/compositing/ov...
LayoutTests/compositing/overflow/automatically-opt-into-composited-scrolling.js:179:
for (var siblingIndex = 0; siblingIndex < 4; ++siblingIndex) {
On 2013/04/09 17:41:59, jchaffraix wrote:
> 4 is a magic constant here, indices.length is way better!

Done.

https://codereview.chromium.org/13722015/diff/2001/LayoutTests/compositing/ov...
LayoutTests/compositing/overflow/automatically-opt-into-composited-scrolling.js:182:
for (var containerSubindex = 0; containerSubindex < 3; ++containerSubindex) {
On 2013/04/09 17:41:59, jchaffraix wrote:
> Again, 3 is a magic constant, you should use siblingIndices.length

Done.

[enne (OOO), 2013-04-09 21:26:53]

Taking myself off the reviewer line and punting to jchaffraix.

[Julien - ping for review, 2013-04-09 22:05:06]

https://codereview.chromium.org/13722015/diff/2001/LayoutTests/compositing/ov...
File
LayoutTests/compositing/overflow/automatically-opt-into-composited-scrolling.js
(right):

https://codereview.chromium.org/13722015/diff/2001/LayoutTests/compositing/ov...
LayoutTests/compositing/overflow/automatically-opt-into-composited-scrolling.js:160:
if (window.internals) {
On 2013/04/09 20:40:09, hartmanng wrote:
> On 2013/04/09 17:41:59, jchaffraix wrote:
> > I don't see why you check window.internals here
> 
> The particular didPass() function in this case requires internals to be able
to
> do anything useful. Moved the window.internals check inside that function, it
> makes a lot more sense there.

Ah, it makes sense indeed. You could also require this test to run under DRT and
bail out early with an error if it is not the case (I don't see what you would
get by running it manually).

[hartmanng, 2013-04-10 14:52:46]

On 2013/04/09 22:05:06, jchaffraix wrote:
> Ah, it makes sense indeed. You could also require this test to run under DRT
and
> bail out early with an error if it is not the case (I don't see what you would
> get by running it manually).

I've actually found it useful to be able to visually inspect specific test cases
if something is failing, to get a better idea of the situation and why it needs
fixing.

[Julien - ping for review, 2013-04-18 01:07:17]

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

https://codereview.chromium.org/13722015/diff/14001/LayoutTests/compositing/o...
LayoutTests/compositing/overflow/automatically-opt-into-composited-scrolling.html:54:
<script src="automatically-opt-into-composited-scrolling.js"></script>
This resource should be in the resources/ directory to match other tests.

https://codereview.chromium.org/13722015/diff/14001/LayoutTests/compositing/o...
LayoutTests/compositing/overflow/automatically-opt-into-composited-scrolling.html:68:
container.style.webkitTransform = 'translateZ(0px)';
I really don't understand the need for this dirtying. Forcing layout through
document.body.offsetTop is OK but why would you forcibly dirty our layout bits?

https://codereview.chromium.org/13722015/diff/14001/LayoutTests/compositing/o...
LayoutTests/compositing/overflow/automatically-opt-into-composited-scrolling.html:78:
container.style.webkitTransform = 'translateZ(0px)';
It seems artificial to change the WebKit transform *and*
settings.setAcceleratedCompositingForOverflowScrollEnabled. Could you explain
why we need both? (this probably deserves a comment on its own as it's unclear
to someone reading this code why it's needed)

[hartmanng, 2013-04-18 14:41:48]

Thanks for the review!

Posted another patch, PTAL.

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

https://codereview.chromium.org/13722015/diff/14001/LayoutTests/compositing/o...
LayoutTests/compositing/overflow/automatically-opt-into-composited-scrolling.html:54:
<script src="automatically-opt-into-composited-scrolling.js"></script>
On 2013/04/18 01:07:17, Julien Chaffraix wrote:
> This resource should be in the resources/ directory to match other tests.

Done.

https://codereview.chromium.org/13722015/diff/14001/LayoutTests/compositing/o...
LayoutTests/compositing/overflow/automatically-opt-into-composited-scrolling.html:68:
container.style.webkitTransform = 'translateZ(0px)';
On 2013/04/18 01:07:17, Julien Chaffraix wrote:
> I really don't understand the need for this dirtying. Forcing layout through
> document.body.offsetTop is OK but why would you forcibly dirty our layout
bits?

We need to be sure that we will force a complete style recalc later (on line 72
in this patch), even if the style is the same; otherwise
window.internals.settings.setAcceleratedCompositingForOverflowScrollEnabled(false)
alone won't cause a composited layer to become uncomposited. If the value of
container.style.webkitTransform is already '', then offsetTop will cause us to
enter the style recalc function, but it will early-out when it detects that the
style hasn't changed. We need it not to early-out so that we recompute the
element's compositability.

https://codereview.chromium.org/13722015/diff/14001/LayoutTests/compositing/o...
LayoutTests/compositing/overflow/automatically-opt-into-composited-scrolling.html:78:
container.style.webkitTransform = 'translateZ(0px)';
On 2013/04/18 01:07:17, Julien Chaffraix wrote:
> It seems artificial to change the WebKit transform *and*
> settings.setAcceleratedCompositingForOverflowScrollEnabled. Could you explain
> why we need both? (this probably deserves a comment on its own as it's unclear
> to someone reading this code why it's needed)

First, we need to set
settings.setAcceleratedCompositingForOverflowScrollEnabled(false) to force us
not to opt in, I don't think there's a way around that. The element could be
automatically opting in, even if the webkit transform is '', and we need to
compare non-opted-in vs opted-in, so we _need_ to be sure that this element will
_not_ be opted in at this point.

The problem with this is that
settings.setAcceleratedCompositingForOverflowScrollEnabled will not set any
style dirty bits. When we go to re-enable this later, we again need a full style
recalc to force us to reevaluate compositability, so we have to explicitly dirty
the style and then call document.body.offsetTop to clean it.

Comment added in the code too.

[Julien - ping for review, 2013-04-18 20:00:14]

OK, LGTM

[commit-bot: I haz the power, 2013-04-23 20:58:30]

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

[commit-bot: I haz the power, 2013-04-23 21:44:54]

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

[commit-bot: I haz the power, 2013-04-24 00:41:31]

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

[commit-bot: I haz the power, 2013-04-24 00:41:57]

Change committed as 148949