Remove flakiness from media/video-poster.html
Created attachment 82856 [details] Patch
Created attachment 83110 [details] Patch Updated patch to reflect r78778
Comment on attachment 83110 [details] Patch ok
Comment on attachment 83110 [details] Patch Clearing flags on attachment: 83110 Committed r79161: <http://trac.webkit.org/changeset/79161>
All reviewed patches have been landed. Closing bug.
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.
Filed https://bugs.webkit.org/show_bug.cgi?id=54908 about the events fired for the poster.
Re-opening this bug as the new test is wrong.
Created attachment 83241 [details] Patch Stop relying on buggy events not mentioned in the spec.
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.
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.
Created attachment 83328 [details] Patch
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).
Comment on attachment 83328 [details] Patch Thanks for the quick turn-around on this!
Comment on attachment 83328 [details] Patch Removing cq+, I will land this.
Comment on attachment 83328 [details] Patch Clearing flags, landed in http://trac.webkit.org/changeset/79363
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. :)