Bug 54685 – Remove flakiness from media/video-poster.html (in service of http://crbug.com/60845)

• Component: New Bugs
• Status: RESOLVED FIXED

[Ami Fischman]

Remove flakiness from media/video-poster.html

[Ami Fischman]

Created attachment 82856 [details]
Patch

[Ami Fischman]

Created attachment 83110 [details]
Patch

Updated patch to reflect r78778

[Kent Tamura]

Comment on attachment 83110 [details]
Patch

ok

[WebKit Commit Bot]

Comment on attachment 83110 [details]
Patch

Clearing flags on attachment: 83110

Committed r79161: <http://trac.webkit.org/changeset/79161>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Eric Carlson]

Comment on attachment 83110 [details]
Patch


> LayoutTests/media/video-poster.html:90
> +          video.addEventListener("load", function() { testPoster(); });
> +          video.addEventListener("error", function() { testPoster(); });

This is bad is several ways and should not work at all, read the 'poster' section of the spec: http://www.w3.org/TR/html5/video.html#attr-video-poster.

As per the spec, a <video> element should NEVER fire a 'load' event, and should not fire an 'error' event if a poster fails to load. It is a bug that we fire these events, it looks we do as a side effect of using HTMLImageLoader to load the poster.

[Eric Carlson]

Filed https://bugs.webkit.org/show_bug.cgi?id=54908 about the events fired for the poster.

[Eric Carlson]

Re-opening this bug as the new test is wrong.

[Ami Fischman]

Created attachment 83241 [details]
Patch

Stop relying on buggy events not mentioned in the spec.

[Ami Fischman]

Thanks for the heads-up, Eric!
Attached patch replaces the listen-for-unspecified-events with a sleep loop, which is a bit icky, but I couldn't find another way to listen directly for the change being tested.  If you have suggestions on that I'm happy to hear them.

[Eric Carlson]

Comment on attachment 83241 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=83241&action=review

Thanks for the quick response!

It looks like this diff is against r78778, not TOT, so I don't think the commit bot will work.  Noted stylistic changes it would be good to have as long as this needs to be revised anyway.


> LayoutTests/media/video-poster.html:50
> +              if (video.clientWidth == expectedWidth && 
> +                  video.clientHeight == expectedHeight) {

I don't think it helps readability to break the test into two lines

> LayoutTests/media/video-poster.html:53
> +                callback();
> +                return;
> +              }

An early return when the rest of the function only has one line seems unnecessary, I think an "else" would be easier to follow.

> LayoutTests/media/video-poster.html:57
> +              window.setTimeout(listenForWidthAndHeight, 20, 
> +                                expectedWidth, expectedHeight, callback);

No need to break the function onto two lines.

> LayoutTests/media/video-poster.html:59
> +                

As long as you are editing the file, can you kill this extra white-space?

> LayoutTests/media/video-poster.html:89
> +                listenForWidthAndHeight(
> +                  currentPoster.width, currentPoster.height, testPoster);

No line break please.

[Ami Fischman]

Created attachment 83328 [details]
Patch

[Ami Fischman]

Thanks for the quick review.  Re-based diff & all style comments addressed (I thought there was at least a "soft" 80-column rule in this codebase, hence the commented-upon newlines).

[Eric Carlson]

Comment on attachment 83328 [details]
Patch

Thanks for the quick turn-around on this!

[Eric Carlson]

Comment on attachment 83328 [details]
Patch

Removing cq+, I will land this.

[Eric Carlson]

Comment on attachment 83328 [details]
Patch

Clearing flags, landed in http://trac.webkit.org/changeset/79363

[Eric Seidel (no email)]

The cq is (currently) unable to land patches while the tree is red.  In this case another media test has the SL bot red, so the CQ constantly attempts but fails to land patches like these. :)