CSS Transition: Fix flaky opacity-transition-zindex.html

CSS Transition: Fix flaky opacity-transition-zindex.html

We wait for the compositor before announcing the test has completed.

BUG=664859
Committed: https://crrev.com/82fda4f8137102335b166ec1a7808d891459607e
Cr-Commit-Position: refs/heads/master@{#439997}

[Eric Willigers, 2016-12-20 06:53:20]

The CQ bit was checked by ericwilligers@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-20 06:53:39]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-20 07:16:27]

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

[commit-bot: I haz the power, 2016-12-20 07:16:28]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Eric Willigers, 2016-12-20 08:56:35]

Patchset #1 (id:1) has been deleted

[Eric Willigers, 2016-12-20 08:56:55]

Patchset #1 (id:20001) has been deleted

[Eric Willigers, 2016-12-20 12:21:22]

The CQ bit was checked by ericwilligers@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-20 12:21:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Eric Willigers, 2016-12-20 12:21:32]

ericwilligers@chromium.org changed reviewers:
+ suzyh@chromium.org

[commit-bot: I haz the power, 2016-12-20 12:48:58]

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

[commit-bot: I haz the power, 2016-12-20 12:48:59]

Dry run: This issue passed the CQ dry run.

[suzyh_UTC10 (ex-contributor), 2016-12-21 00:37:45]

I don't fully understand this test. I suspect it is overly complex for what it
is trying to do, and could do with some comment or description text to explain
what it is testing. Would this test file be suitable for conversion to
testharness, which expects description text to be attached to each test, or
would that be too much extra work for no gain?

https://codereview.chromium.org/2591623002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/transitions/opacity-transition-zindex-expected.txt
(right):

https://codereview.chromium.org/2591623002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/transitions/opacity-transition-zindex-expected.txt:5:
LayoutBlockFlow {BODY} at (0,10) size 800x300
I'm surprised about this having an -expected.txt and an -expected.png. Wouldn't
the test only compare the output against one of them?

https://codereview.chromium.org/2591623002/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/transitions/opacity-transition-zindex.html
(right):

https://codereview.chromium.org/2591623002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/transitions/opacity-transition-zindex.html:6: * {
What is the purpose of adding this clause?

https://codereview.chromium.org/2591623002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/transitions/opacity-transition-zindex.html:11:
.container {
Why are there separate .container and #first clauses? This test setup seems
unnecessarily complex, obscuring the purpose of the test.

[alancutter (OOO until 2018), 2016-12-21 00:43:18]

alancutter@chromium.org changed reviewers:
+ alancutter@chromium.org

[alancutter (OOO until 2018), 2016-12-21 00:43:18]

Why are expectations changing?

[Eric Willigers, 2016-12-21 01:22:36]

> Why are expectations changing?

1. I eliminated the effect of Blink's implicit 'body { margin: 8px; }'

2. I replaced 4 platform-specific expectation .png files with a single file.

https://codereview.chromium.org/2591623002/diff/40001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/transitions/opacity-transition-zindex-expected.txt
(right):

https://codereview.chromium.org/2591623002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/transitions/opacity-transition-zindex-expected.txt:5:
LayoutBlockFlow {BODY} at (0,10) size 800x300
On 2016/12/21 00:37:45, suzyh wrote:
> I'm surprised about this having an -expected.txt and an -expected.png.
Wouldn't
> the test only compare the output against one of them?

If we remove the txt,
  ...html failed unexpectedly (-expected.txt was missing)

If we remove the png,
  ...html failed unexpectedly (-expected.png was missing)

https://codereview.chromium.org/2591623002/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/transitions/opacity-transition-zindex.html
(right):

https://codereview.chromium.org/2591623002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/transitions/opacity-transition-zindex.html:6: * {
On 2016/12/21 00:37:45, suzyh wrote:
> What is the purpose of adding this clause?

The test expectations previously depended on the following margin in Blink's
user-agent stylesheet html.css

body {
    display: block;
    margin: 8px
}

This made the expectations in the .txt more complex to understand.

There is no need for the test to be dependent on Blink's user-agent stylesheet.

[Eric Willigers, 2016-12-21 01:28:47]

> Would this test file be suitable for conversion to testharness?

No. This rendering test is all about the visual output, not about the values
returned by getComputedStyle.

See the bug where the test was introduced:
https://bugs.webkit.org/show_bug.cgi?id=20047
Incorrect rendering during opacity transition on element with positioned
children

[Eric Willigers, 2016-12-21 01:51:12]

https://codereview.chromium.org/2591623002/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/transitions/opacity-transition-zindex.html
(right):

https://codereview.chromium.org/2591623002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/transitions/opacity-transition-zindex.html:11:
.container {
On 2016/12/21 00:37:45, suzyh wrote:
> Why are there separate .container and #first clauses? This test setup seems
> unnecessarily complex, obscuring the purpose of the test.

Now consolidated. There is no need for .class when #id suffices.

[Eric Willigers, 2016-12-21 02:07:19]

Another option would be to have expected.html instead of text/png

[alancutter (OOO until 2018), 2016-12-21 02:08:56]

On 2016/12/21 at 02:07:19, ericwilligers wrote:
> Another option would be to have expected.html instead of text/png

That would be preferred but optional for this patch.

lgtm.

[Eric Willigers, 2016-12-21 02:58:40]

The CQ bit was checked by ericwilligers@chromium.org

[commit-bot: I haz the power, 2016-12-21 02:59:03]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-21 03:18:15]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1482289120374440,
"parent_rev": "899c1c11d4cf9f88dd18ad9d17f67e674c18367f", "commit_rev":
"ce1b2613ba287cc23170533a5fefce144c468a7a"}

[commit-bot: I haz the power, 2016-12-21 03:18:46]

Description was changed from

==========
CSS Transition: Fix flaky opacity-transition-zindex.html

We wait for the compositor before announcing the test has completed.

BUG=664859
==========

to

==========
CSS Transition: Fix flaky opacity-transition-zindex.html

We wait for the compositor before announcing the test has completed.

BUG=664859

Review-Url: https://codereview.chromium.org/2591623002
==========

[commit-bot: I haz the power, 2016-12-21 03:18:47]

Committed patchset #2 (id:60001)

[commit-bot: I haz the power, 2016-12-21 03:22:40]

Description was changed from

==========
CSS Transition: Fix flaky opacity-transition-zindex.html

We wait for the compositor before announcing the test has completed.

BUG=664859

Review-Url: https://codereview.chromium.org/2591623002
==========

to

==========
CSS Transition: Fix flaky opacity-transition-zindex.html

We wait for the compositor before announcing the test has completed.

BUG=664859

Committed: https://crrev.com/82fda4f8137102335b166ec1a7808d891459607e
Cr-Commit-Position: refs/heads/master@{#439997}
==========

[commit-bot: I haz the power, 2016-12-21 03:22:41]

Patchset 2 (id:??) landed as
https://crrev.com/82fda4f8137102335b166ec1a7808d891459607e
Cr-Commit-Position: refs/heads/master@{#439997}