opt-in layout test (with separate js file)

opt-in layout test (with separate js file)

This issue is obsolete, please see https://codereview.chromium.org/13722015/ instead.

BUG= http://crbug.com/172875

[hartmanng, 2013-03-21 15:29:35]

This and the other patch I'm going to send you
(https://codereview.appspot.com/7835044/) may still need a bit of touch-up, but
on a high level they should be pretty much how the patches will look like I
think. (This one is the one to refactor the js out of
automatically-opt-in*.html)

[hartmanng, 2013-03-21 18:17:58]

Fixed style nits from this morning, added TestExpectations failure. I think it's
ready to upload. For changelog, I was thinking something along the lines of:



Part 1 of splitting https://bugs.webkit.org/show_bug.cgi?id=109302 up into
smaller pieces.

In this piece, we add an exhaustive, but failing, layout test demonstrating the
missed cases in the current implementation of
RenderLayer::updateDescendantsAreContiguousInStackingOrder(). This will be fixed
and the layout test will be passing after landing subsequent patches.

[vollick, 2013-03-21 18:53:57]

I think the code looks great, but we'll need an exhaustive comment explaining
the exhaustive test. We should chat offline about the argument we'll make.

https://codereview.appspot.com/7926043/diff/13001/LayoutTests/compositing/ove...
File
LayoutTests/compositing/overflow/automatically-opt-into-composited-scrolling.html
(right):

https://codereview.appspot.com/7926043/diff/13001/LayoutTests/compositing/ove...
LayoutTests/compositing/overflow/automatically-opt-into-composited-scrolling.html:101:
permute(didPass);
I wonder if it might be nice to spit out a Pass message when we succeed? I worry
that the test will somehow exit early, and it'll look like it passed when it
didn't.

[vollick, 2013-03-21 20:45:44]

Seems like we can argue that
a) We've got a representative from each of the interesting categories in the
paint order list here http://www.w3.org/TR/CSS2/visuren.html#z-index  (Assuming
that the difference between 3-5 is uninteresting -- jchaffraix, is it?).
b) We're considered all the interesting permutations of z-index. We have all the
forced orderings (by making the indices differenc), cases where we have another
sibling with z-index zero (with whom we could potentially change our order), and
cases where our descendants and non descendants have z-index zero (and could
potentially reorder after promotion).

https://codereview.appspot.com/7926043/diff/13001/LayoutTests/compositing/ove...
File
LayoutTests/compositing/overflow/automatically-opt-into-composited-scrolling.js
(right):

https://codereview.appspot.com/7926043/diff/13001/LayoutTests/compositing/ove...
LayoutTests/compositing/overflow/automatically-opt-into-composited-scrolling.js:79:

Care about cases where there's one, two or three elements with z-index zero.
 - [24] One, because it forces an ordering of elements.
 - [36] Two, because it will test the change of paint ordering w.r.t. ourselves
and the other element with z-index zero.
 - [8 ] Three (where a descendant AND a sibling have z-index 0), because it will
test how paint order is affected when descendants and non-descendants share the
same z-index.

So could we have three nested loops to generate these permutations? The inner
loops could be refactored into a separate function that each of these three
nested loops call.

https://codereview.appspot.com/7926043/diff/13001/LayoutTests/compositing/ove...
LayoutTests/compositing/overflow/automatically-opt-into-composited-scrolling.js:82:
for (var siblingIsSuccessor = 0; siblingIsSuccessor <= 1; ++siblingIsSuccessor)
{
Establishes that we handle tree order correctly.

https://codereview.appspot.com/7926043/diff/13001/LayoutTests/compositing/ove...
LayoutTests/compositing/overflow/automatically-opt-into-composited-scrolling.js:83:
for (var hasPositionedAncestor = 0; hasPositionedAncestor <= 1;
++hasPositionedAncestor) {
Shows that we handle 6 from http://www.w3.org/TR/CSS2/visuren.html#z-index
correctly (positioned, non stacking containers).

https://codereview.appspot.com/7926043/diff/13001/LayoutTests/compositing/ove...
LayoutTests/compositing/overflow/automatically-opt-into-composited-scrolling.js:142:
var currentZIndex = siblingPrecedesContainer &&
siblingShouldBeZeroIfPrecedesContainer == 1 ? 0 : -1;
get rid of the "siblingPrecedesContainer".

[hartmanng, 2013-03-27 19:54:09]

Hey Shawn, can you take a look at this patch before we upload it to bugzilla and
have enne and Julien review it?

This is patch 1 from our plan at
https://docs.google.com/a/google.com/document/d/13DL9rRjiLXK4mBzkR8iYXex5hNuU...

[shawnsingh_google.com, 2013-03-27 20:33:44]

OK sure - i'll try to find time to look today.  Sorry for getting backed up
on all this feedback.  Been a pretty hectic month for me.


On Wed, Mar 27, 2013 at 12:54 PM, <hartmanng@chromium.org> wrote:

> Hey Shawn, can you take a look at this patch before we upload it to
> bugzilla and have enne and Julien review it?
>
> This is patch 1 from our plan at
> https://docs.google.com/a/**google.com/document/d/**
>
13DL9rRjiLXK4mBzkR8iYXex5hNuUQ**m4H-vlJY7iSIj4/view<https://docs.google.com/a/google.com/document/d/13DL9rRjiLXK4mBzkR8iYXex5hNuUQm4H-vlJY7iSIj4/view>
>
>
https://codereview.appspot.**com/7926043/<https://codereview.appspot.com/7926...
>

[hartmanng, 2013-03-27 21:20:00]

On 2013/03/27 20:33:44, shawnsingh_google.com wrote:
> OK sure - i'll try to find time to look today.  Sorry for getting backed up
> on all this feedback.  Been a pretty hectic month for me.
> 

No problem, thanks for taking a look!

[shawnsingh, 2013-04-03 02:38:49]

Pretty cool, great job.

I have some general thoughts - but perhaps they should not block you from moving
forward with this patch.

As I undersatnd it, this test checks the criteria that before+after opt-in
should not change stacking order when we opt-in.  --->  Are we sure that
"maintaining correct stacking order" == "we should opt-in"?  Are there any cases
where opting-in to composited scrolling is actually Not desired, even if it
wouldn't have changed the stacking order?

I also worry that it's a bit tautological that we use this criteria here, and
we're proposing to also use this criteria in the code itself (the before+after
comparison to make sure paint order is still good).   While I can't think of any
concrete example that shows why this isn't good, I worry that we might
eventually hit some cases where the test may pass when it shouldnt, e.g. a
double-negative bug where messing up the stacking order may cause us to
incorrectly opt-in.

Maybe additional tests can cover such scenarios later.


Additional minor comments below =)

https://codereview.appspot.com/7926043/diff/29001/LayoutTests/compositing/ove...
File
LayoutTests/compositing/overflow/automatically-opt-into-composited-scrolling.html
(right):

https://codereview.appspot.com/7926043/diff/29001/LayoutTests/compositing/ove...
LayoutTests/compositing/overflow/automatically-opt-into-composited-scrolling.html:81:
var shouldOptIn = oldStackingOrder.length == newStackingOrder.length;
javascript should probably use === here, right?

https://codereview.appspot.com/7926043/diff/29001/LayoutTests/compositing/ove...
LayoutTests/compositing/overflow/automatically-opt-into-composited-scrolling.html:83:
if (oldStackingOrder[i] != newStackingOrder[i]) {
and !== here?   More of a question than a real suggestion, though.

https://codereview.appspot.com/7926043/diff/29001/LayoutTests/compositing/ove...
File
LayoutTests/compositing/overflow/automatically-opt-into-composited-scrolling.js
(right):

https://codereview.appspot.com/7926043/diff/29001/LayoutTests/compositing/ove...
LayoutTests/compositing/overflow/automatically-opt-into-composited-scrolling.js:16:
document.body.offsetTop;
I wonder if we should propose to WebCore to create something like
windows.internals.forceStyleRecalc  or windows.testRunner.forceStyleRecalc
instead of using this very oddly encoded trick.

Same for forcing a layout...  Though what i'm saying here is probably something
to discuss at another time.

https://codereview.appspot.com/7926043/diff/29001/LayoutTests/compositing/ove...
LayoutTests/compositing/overflow/automatically-opt-into-composited-scrolling.js:79:
if (numElementsWithZIndexZero == 3 && containerIndex != firstChildIndex - 1 &&
containerIndex != secondChildIndex - 1)
do we also want === here?  and the same for the rest of this code.

[hartmanng, 2013-04-03 13:59:32]

On 2013/04/03 02:38:49, shawnsingh wrote:
> Pretty cool, great job.
> 
> I have some general thoughts - but perhaps they should not block you from
moving
> forward with this patch.
> 
> As I undersatnd it, this test checks the criteria that before+after opt-in
> should not change stacking order when we opt-in.  --->  Are we sure that
> "maintaining correct stacking order" == "we should opt-in"?  Are there any
cases
> where opting-in to composited scrolling is actually Not desired, even if it
> wouldn't have changed the stacking order?

There are certainly cases where "maintaining correct stacking order" != "we
should opt-in" (for example, if we have out-of-flow fixed-positioned
descendants), but these cases should be handled elsewhere in the code (like in a
few of Ian's patches). In this set of patches, we're basically trying to answer
the question "does stacking order make it dangerous/incorrect to opt in?"

This layout test is intentionally a simple case that doesn't include things like
out-of-flow positioned descendants, so that we can specifically isolate and test
the effects of stacking order on our opt-in decision, but you're right that this
isn't the only criterion that matters in general.


> 
> I also worry that it's a bit tautological that we use this criteria here, and
> we're proposing to also use this criteria in the code itself (the before+after
> comparison to make sure paint order is still good).   While I can't think of
any
> concrete example that shows why this isn't good, I worry that we might
> eventually hit some cases where the test may pass when it shouldnt, e.g. a
> double-negative bug where messing up the stacking order may cause us to
> incorrectly opt-in.

Again, the idea here is that we're working under the premise that the stacking
order shouldn't prevent opt-in if it remains constant before and after opt-in.
We could still want to opt out for other reasons, but if the stacking order
doesn't change, then we would need a non-stacking-order-based reason to opt out.

In the layout test, we're using hit testing code to determine the stacking
order, so we can be confident that this is the "real" correct stacking order. So
what we're really testing isn't necessarily so much "should this opt in" as
"does our logic for the opt-in decision based on stacking order match the actual
hit-testing-based stacking order".

> 
> Maybe additional tests can cover such scenarios later.
> 
> 
> Additional minor comments below =)
> 
>
https://codereview.appspot.com/7926043/diff/29001/LayoutTests/compositing/ove...
> File
>
LayoutTests/compositing/overflow/automatically-opt-into-composited-scrolling.html
> (right):
> 
>
https://codereview.appspot.com/7926043/diff/29001/LayoutTests/compositing/ove...
>
LayoutTests/compositing/overflow/automatically-opt-into-composited-scrolling.html:81:
> var shouldOptIn = oldStackingOrder.length == newStackingOrder.length;
> javascript should probably use === here, right?
> 
>
https://codereview.appspot.com/7926043/diff/29001/LayoutTests/compositing/ove...
>
LayoutTests/compositing/overflow/automatically-opt-into-composited-scrolling.html:83:
> if (oldStackingOrder[i] != newStackingOrder[i]) {
> and !== here?   More of a question than a real suggestion, though.
> 
> 
>
https://codereview.appspot.com/7926043/diff/29001/LayoutTests/compositing/ove...
>
LayoutTests/compositing/overflow/automatically-opt-into-composited-scrolling.js:79:
> if (numElementsWithZIndexZero == 3 && containerIndex != firstChildIndex - 1 &&
> containerIndex != secondChildIndex - 1)
> do we also want === here?  and the same for the rest of this code.

Yep, changed all the "==" and "!=" to "===" and "!==" - I'm not used to
javascript operators.


https://codereview.appspot.com/7926043/diff/29001/LayoutTests/compositing/ove...
> File
>
LayoutTests/compositing/overflow/automatically-opt-into-composited-scrolling.js
> (right):
> 
>
https://codereview.appspot.com/7926043/diff/29001/LayoutTests/compositing/ove...
>
LayoutTests/compositing/overflow/automatically-opt-into-composited-scrolling.js:16:
> document.body.offsetTop;
> I wonder if we should propose to WebCore to create something like
> windows.internals.forceStyleRecalc  or windows.testRunner.forceStyleRecalc
> instead of using this very oddly encoded trick.
> 
> Same for forcing a layout...  Though what i'm saying here is probably
something
> to discuss at another time.
> 

I like this suggestion, but I'd prefer it in a separate patch, I'd like to keep
the set I'm working on as simple as we can.

[shawnsingh, 2013-04-03 19:17:46]

based on your reply...

If the purpose of this test is only to make sure we don't incorrectly opt-in,
then shouldn't this test only fail if shouldOptIn == false and didOptIn == true?
 But right now we're also checking that we did NOT opt-in when shouldOptIn is
true.
  
Also, maybe the changelog should reflect this, and it might be helpful to have a
comment describing the purpose of each test in the html of the layout tests.

I guess the points i'm trying to make shouldn't necessarily block you from
submitting it to WebKit folks, though.  It's pretty clear that the test does add
useful coverage.

https://codereview.appspot.com/7926043/diff/37001/LayoutTests/ChangeLog
File LayoutTests/ChangeLog (right):

https://codereview.appspot.com/7926043/diff/37001/LayoutTests/ChangeLog#newco...
LayoutTests/ChangeLog:12377: +        This test is intended to generate all
paint ordering that are
ordering --> orderings

[jchaffraix, 2013-04-03 20:55:24]

https://codereview.appspot.com/7926043/diff/37001/LayoutTests/compositing/ove...
File
LayoutTests/compositing/overflow/automatically-opt-into-composited-scrolling.html
(right):

https://codereview.appspot.com/7926043/diff/37001/LayoutTests/compositing/ove...
LayoutTests/compositing/overflow/automatically-opt-into-composited-scrolling.html:54:
top: -20px;
If the class is any indication (ie it's for a normal flow element), does this
property do anything?

https://codereview.appspot.com/7926043/diff/37001/LayoutTests/compositing/ove...
LayoutTests/compositing/overflow/automatically-opt-into-composited-scrolling.html:58:
<script type="text/javascript" charset="utf-8"
src="automatically-opt-into-composited-scrolling.js"></script>
Usually it's better to keep everything inside the same file unless you intent to
share this JavaScript file with other test(s).

https://codereview.appspot.com/7926043/diff/37001/LayoutTests/compositing/ove...
LayoutTests/compositing/overflow/automatically-opt-into-composited-scrolling.html:67:
document.body.offsetTop;
I don't think I understand the rationale behind this initialization. Why don't
we force a recalcStyle every time you change the property?

https://codereview.appspot.com/7926043/diff/37001/LayoutTests/compositing/ove...
LayoutTests/compositing/overflow/automatically-opt-into-composited-scrolling.html:69:
// force to not promote.
This comment is not really useful.

https://codereview.appspot.com/7926043/diff/37001/LayoutTests/compositing/ove...
LayoutTests/compositing/overflow/automatically-opt-into-composited-scrolling.html:94:
write('FAIL - automatically opted in and changed stacking order ' + count);
You may want to dump more information if it fails as it will be difficult to
understand what went wrong in this case.

https://codereview.appspot.com/7926043/diff/37001/LayoutTests/compositing/ove...
File
LayoutTests/compositing/overflow/opt-into-composited-scrolling-positioned-ancestor-expected.txt
(right):

https://codereview.appspot.com/7926043/diff/37001/LayoutTests/compositing/ove...
LayoutTests/compositing/overflow/opt-into-composited-scrolling-positioned-ancestor-expected.txt:1:
Passed - layer tree.
Usually it's nice to dump a description of what you are testing. That makes the
test less obscure by documenting the intent better.

https://codereview.appspot.com/7926043/diff/37001/LayoutTests/compositing/ove...
File
LayoutTests/compositing/overflow/opt-into-composited-scrolling-positioned-ancestor.html
(right):

https://codereview.appspot.com/7926043/diff/37001/LayoutTests/compositing/ove...
LayoutTests/compositing/overflow/opt-into-composited-scrolling-positioned-ancestor.html:1:
<!DOCTYPE html PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN"
We usually prefer the HTML doctype: <!DOCTYPE html>

https://codereview.appspot.com/7926043/diff/37001/LayoutTests/compositing/ove...
LayoutTests/compositing/overflow/opt-into-composited-scrolling-positioned-ancestor.html:4:
<html lang="en">
No need for lang.

https://codereview.appspot.com/7926043/diff/37001/LayoutTests/compositing/ove...
LayoutTests/compositing/overflow/opt-into-composited-scrolling-positioned-ancestor.html:7:
<title>Opting into composited scrolling</title>
No need for title nor meta as it's not dumped in the output.

[hartmanng, 2013-04-04 18:07:18]

https://codereview.appspot.com/7926043/diff/37001/LayoutTests/ChangeLog
File LayoutTests/ChangeLog (right):

https://codereview.appspot.com/7926043/diff/37001/LayoutTests/ChangeLog#newco...
LayoutTests/ChangeLog:12377: +        This test is intended to generate all
paint ordering that are
On 2013/04/03 19:17:47, shawnsingh wrote:
> ordering --> orderings

Done.

https://codereview.appspot.com/7926043/diff/37001/LayoutTests/compositing/ove...
File
LayoutTests/compositing/overflow/automatically-opt-into-composited-scrolling.html
(right):

https://codereview.appspot.com/7926043/diff/37001/LayoutTests/compositing/ove...
LayoutTests/compositing/overflow/automatically-opt-into-composited-scrolling.html:54:
top: -20px;
On 2013/04/03 20:55:24, jchaffraix wrote:
> If the class is any indication (ie it's for a normal flow element), does this
> property do anything?

nope, must've been left over from before.

https://codereview.appspot.com/7926043/diff/37001/LayoutTests/compositing/ove...
LayoutTests/compositing/overflow/automatically-opt-into-composited-scrolling.html:58:
<script type="text/javascript" charset="utf-8"
src="automatically-opt-into-composited-scrolling.js"></script>
On 2013/04/03 20:55:24, jchaffraix wrote:
> Usually it's better to keep everything inside the same file unless you intent
to
> share this JavaScript file with other test(s).

I do intend to reuse the javascript file to generate the relevant permutations
to test my next patch (https://codereview.appspot.com/7835044/)

https://codereview.appspot.com/7926043/diff/37001/LayoutTests/compositing/ove...
LayoutTests/compositing/overflow/automatically-opt-into-composited-scrolling.html:67:
document.body.offsetTop;
On 2013/04/03 20:55:24, jchaffraix wrote:
> I don't think I understand the rationale behind this initialization. Why don't
> we force a recalcStyle every time you change the property?

It is my understanding that javascript doesn't necessarily cause a recalc
synchronously. Pulling the document.body.offsetTop value should cause it to
perform an immediate update so that it gets the right value. Please correct me
if I'm misunderstanding that.

https://codereview.appspot.com/7926043/diff/37001/LayoutTests/compositing/ove...
LayoutTests/compositing/overflow/automatically-opt-into-composited-scrolling.html:69:
// force to not promote.
On 2013/04/03 20:55:24, jchaffraix wrote:
> This comment is not really useful.

removed.

https://codereview.appspot.com/7926043/diff/37001/LayoutTests/compositing/ove...
LayoutTests/compositing/overflow/automatically-opt-into-composited-scrolling.html:94:
write('FAIL - automatically opted in and changed stacking order ' + count);
On 2013/04/03 20:55:24, jchaffraix wrote:
> You may want to dump more information if it fails as it will be difficult to
> understand what went wrong in this case.

Done.

https://codereview.appspot.com/7926043/diff/37001/LayoutTests/compositing/ove...
File
LayoutTests/compositing/overflow/opt-into-composited-scrolling-positioned-ancestor-expected.txt
(right):

https://codereview.appspot.com/7926043/diff/37001/LayoutTests/compositing/ove...
LayoutTests/compositing/overflow/opt-into-composited-scrolling-positioned-ancestor-expected.txt:1:
Passed - layer tree.
On 2013/04/03 20:55:24, jchaffraix wrote:
> Usually it's nice to dump a description of what you are testing. That makes
the
> test less obscure by documenting the intent better.

Done.

https://codereview.appspot.com/7926043/diff/37001/LayoutTests/compositing/ove...
File
LayoutTests/compositing/overflow/opt-into-composited-scrolling-positioned-ancestor.html
(right):

https://codereview.appspot.com/7926043/diff/37001/LayoutTests/compositing/ove...
LayoutTests/compositing/overflow/opt-into-composited-scrolling-positioned-ancestor.html:1:
<!DOCTYPE html PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN"
On 2013/04/03 20:55:24, jchaffraix wrote:
> We usually prefer the HTML doctype: <!DOCTYPE html>

Done.

https://codereview.appspot.com/7926043/diff/37001/LayoutTests/compositing/ove...
LayoutTests/compositing/overflow/opt-into-composited-scrolling-positioned-ancestor.html:4:
<html lang="en">
On 2013/04/03 20:55:24, jchaffraix wrote:
> No need for lang.

Done.

https://codereview.appspot.com/7926043/diff/37001/LayoutTests/compositing/ove...
LayoutTests/compositing/overflow/opt-into-composited-scrolling-positioned-ancestor.html:7:
<title>Opting into composited scrolling</title>
On 2013/04/03 20:55:24, jchaffraix wrote:
> No need for title nor meta as it's not dumped in the output.

Done.

[jchaffraix, 2013-04-05 18:24:56]

https://codereview.appspot.com/7926043/diff/37001/LayoutTests/compositing/ove...
File
LayoutTests/compositing/overflow/automatically-opt-into-composited-scrolling.html
(right):

https://codereview.appspot.com/7926043/diff/37001/LayoutTests/compositing/ove...
LayoutTests/compositing/overflow/automatically-opt-into-composited-scrolling.html:67:
document.body.offsetTop;
You are totally right. My comment was more that we usually do the invalidation
followed by a forced recalcStyle + repaint / layout and this code didn't seem to
follow that. Maybe it's just the comment above that made me misunderstood the
actual intent.

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

Moving this code review to the official Chromium location:
https://codereview.chromium.org/13722015, we can continue to iterate there.