[webkit-reviews] review denied: [Bug 84570] audio controls have a 1px surplus outline : [Attachment 139129] Patch

[bugzilla-daemon at webkit.org]

Eric Carlson <eric.carlson at apple.com> has denied Silvia Pfeiffer
<silviapf at chromium.org>'s request for review:
Bug 84570: audio controls have a 1px surplus outline
https://bugs.webkit.org/show_bug.cgi?id=84570

Attachment 139129: Patch
https://bugs.webkit.org/attachment.cgi?id=139129&action=review

------- Additional Comments from Eric Carlson <eric.carlson at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=139129&action=review


> LayoutTests/ChangeLog:10
> +	   * media/audio-controls-no-image-outline.html: Added.

It looks like you forgot to "svn add" the new test results.

> LayoutTests/media/audio-controls-no-image-outline.html:3
> +    This tests that audio controls do not fade out when the audio is
playing.

Is this comment correct?

> LayoutTests/media/audio-controls-no-image-outline.html:13
> +<p id="result">
> +    FAIL: Test did not run.
> +</p>
> +<audio id="audio" controls autoplay onplaying="playing()"
src="content/silence.wav"></audio><br>
> +<script>
> +    if (window.layoutTestController) {
> +	   layoutTestController.waitUntilDone();
> +	   layoutTestController.dumpAsText();
> +    }

Nit: Is there any reason to not include video-test.js (as the other media tests
do) and use its helpers instead of rolling your own?

> LayoutTests/media/audio-controls-no-image-outline.html:21
> +	   setTimeout(function() {
> +	       var controls =
internals.shadowRoot(document.getElementById("audio")).firstChild.firstChild;
> +	       var opacity = getComputedStyle(controls).opacity;
> +	       document.getElementById("result").innerText = opacity < 1 ?
"FAIL" : "PASS";
> +	       layoutTestController.notifyDone();
> +	   }, 250)

How does this test your changes?

If it does test this fix, is the setTimeout really necessary? We have been
trying to get rid of all timeouts in media tests because they have been the
source of much flakiness, so I would really prefer to see this done another
way.