On GTK, the media/video-src-invalid-ponter.html test gives the correct width but the wrong height.
Committed r86911 adding the new test to the GTK skipped list.
Created attachment 94976 [details] proposed patch
(In reply to comment #0) > On GTK, the media/video-src-invalid-ponter.html test gives the correct width but the wrong height. Are you sure? What I saw locally is that sometimes the test times out, otherwise it succeeds locally. Hence the attached patch to avoid the timeout
(In reply to comment #3) > (In reply to comment #0) > > On GTK, the media/video-src-invalid-ponter.html test gives the correct width but the wrong height. > > Are you sure? What I saw locally is that sometimes the test times out, otherwise it succeeds locally. Definitely. Check the test results from the URL field above. It shows the video sized to 73 (correct) by 150 (incorrect, but probably the default video element width). However, if it's not reproducing any longer, then aces. ;) > Hence the attached patch to avoid the timeout So the patch seems to significantly change the behavior of the test by calling |video.load()|. Shouldn't we see the correct behavior without calling load()? The original test gave the video element a whole second to resize and display the poster. Is that not enough time?
(In reply to comment #4) > (In reply to comment #3) > > (In reply to comment #0) > > > On GTK, the media/video-src-invalid-ponter.html test gives the correct width but the wrong height. > > > > Are you sure? What I saw locally is that sometimes the test times out, otherwise it succeeds locally. > > Definitely. Check the test results from the URL field above. It shows the video sized to 73 (correct) by 150 (incorrect, but probably the default video element width). > Oh I didn't that link indeed. Yes it's odd. I recall the Release bot might need a GStreamer upgrade to be in par with the Debug bots. At least locally the test passes but I use a fairly recent GStreamer version. > However, if it's not reproducing any longer, then aces. ;) > > > Hence the attached patch to avoid the timeout > > So the patch seems to significantly change the behavior of the test by calling |video.load()|. Shouldn't we see the correct behavior without calling load()? The original test gave the video element a whole second to resize and display the poster. Is that not enough time? On my slow laptop I could see the timeout happen. The GTK bots are know to not be lighning fast either... In general for media tests (at least) I believe we should react more on events instead of waiting on something to happen before a XXXX ms timeout to fire.
(In reply to comment #5) > (In reply to comment #4) > > So the patch seems to significantly change the behavior of the test by calling |video.load()|. Shouldn't we see the correct behavior without calling load()? The original test gave the video element a whole second to resize and display the poster. Is that not enough time? > > On my slow laptop I could see the timeout happen. The GTK bots are know to not be lighning fast either... > In general for media tests (at least) I believe we should react more on events instead of waiting on something to happen before a XXXX ms timeout to fire. Okay, I'd be fine with getting rid of the 1s timeout. What about |video.load()|? Does the test pass on GTK+ without that, but also without the timeout?
(In reply to comment #6) > (In reply to comment #5) > > (In reply to comment #4) > > > So the patch seems to significantly change the behavior of the test by calling |video.load()|. Shouldn't we see the correct behavior without calling load()? The original test gave the video element a whole second to resize and display the poster. Is that not enough time? > > > > On my slow laptop I could see the timeout happen. The GTK bots are know to not be lighning fast either... > > In general for media tests (at least) I believe we should react more on events instead of waiting on something to happen before a XXXX ms timeout to fire. > > Okay, I'd be fine with getting rid of the 1s timeout. What about |video.load()|? Does the test pass on GTK+ without that, but also without the timeout? Doesn't pass because if load() isn't called there's no loadstart event.
Can this be reviewed please? Eric, would you have some time to look at it? Thanks!
Comment on attachment 94976 [details] proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=94976&action=review > LayoutTests/ChangeLog:9 > + Wait loadstart event to check the poster dimensions and avoid a > + potential timeout of the test. Nit: Wait for loadstart event...
Committed r90554: <http://trac.webkit.org/changeset/90554>