REGRESSION: Setting invalid media "src" does not cause "error" event
Created attachment 146048 [details] Patch
Comment on attachment 146048 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=146048&action=review > Source/WebCore/platform/graphics/mac/MediaPlayerPrivateQTKit.mm:85 > +SOFT_LINK_POINTER(QTKit, QTMovieOpenAsyncRequiredAttribute, NSString *) This is will ASSERT if run on Leopard or older. Do we care? > LayoutTests/http/tests/media/video-src-invalid-error.html:5 > + > + Nit: you have an extra blank line here. > LayoutTests/http/tests/media/video-src-invalid-error.html:9 > + failTest("stalled but should not" ); Nit: you have an extra space before the closing brace. > LayoutTests/http/tests/media/video-src-invalid-error.html:24 > + run("video.setAttribute('src', 'bogus.mp4')"); This will only work on ports that support MPEG-4. You should use findMediaFile() and not log the file name.
(In reply to comment #2) > (From update of attachment 146048 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=146048&action=review > > > Source/WebCore/platform/graphics/mac/MediaPlayerPrivateQTKit.mm:85 > > +SOFT_LINK_POINTER(QTKit, QTMovieOpenAsyncRequiredAttribute, NSString *) > > This is will ASSERT if run on Leopard or older. Do we care? Gah, that's annoying. SOFT_LINK_POINTER_OPTIONAL then? > > LayoutTests/http/tests/media/video-src-invalid-error.html:5 > > + > > + > > Nit: you have an extra blank line here. Removed. > > LayoutTests/http/tests/media/video-src-invalid-error.html:9 > > + failTest("stalled but should not" ); > > Nit: you have an extra space before the closing brace. Removed. > > LayoutTests/http/tests/media/video-src-invalid-error.html:24 > > + run("video.setAttribute('src', 'bogus.mp4')"); > > This will only work on ports that support MPEG-4. You should use findMediaFile() and not log the file name. The whole point is to find an invalid URL. So I could just leave off the .mp4 part entirely. ;)
(In reply to comment #3) > (In reply to comment #2) > > (From update of attachment 146048 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=146048&action=review > > > > > Source/WebCore/platform/graphics/mac/MediaPlayerPrivateQTKit.mm:85 > > > +SOFT_LINK_POINTER(QTKit, QTMovieOpenAsyncRequiredAttribute, NSString *) > > > > This is will ASSERT if run on Leopard or older. Do we care? > > Gah, that's annoying. SOFT_LINK_POINTER_OPTIONAL then? > Or just use the name like we do for QTMovieAllowPersistentCacheAttribute. > > > > LayoutTests/http/tests/media/video-src-invalid-error.html:24 > > > + run("video.setAttribute('src', 'bogus.mp4')"); > > > > This will only work on ports that support MPEG-4. You should use findMediaFile() and not log the file name. > > The whole point is to find an invalid URL. So I could just leave off the .mp4 part entirely. ;) > That won't work if a platform checks the type internally (though I don't know if any do or not).
(In reply to comment #4) > (In reply to comment #3) > > (In reply to comment #2) > > > (From update of attachment 146048 [details] [details] [details]) > > > View in context: https://bugs.webkit.org/attachment.cgi?id=146048&action=review > > > > > > > Source/WebCore/platform/graphics/mac/MediaPlayerPrivateQTKit.mm:85 > > > > +SOFT_LINK_POINTER(QTKit, QTMovieOpenAsyncRequiredAttribute, NSString *) > > > > > > This is will ASSERT if run on Leopard or older. Do we care? > > > > Gah, that's annoying. SOFT_LINK_POINTER_OPTIONAL then? > > > Or just use the name like we do for QTMovieAllowPersistentCacheAttribute. Eh. That seems to be what SOFT_LINK_POINTER_OPTIONAL is for. > > > > LayoutTests/http/tests/media/video-src-invalid-error.html:24 > > > > + run("video.setAttribute('src', 'bogus.mp4')"); > > > > > > This will only work on ports that support MPEG-4. You should use findMediaFile() and not log the file name. > > > > The whole point is to find an invalid URL. So I could just leave off the .mp4 part entirely. ;) > > > That won't work if a platform checks the type internally (though I don't know if any do or not). Because ports have to be able to load ".php" or ".aspx" URLs, I doubt any reject URLs with strange extensions without fetching them first.
Created attachment 146062 [details] Patch
Comment on attachment 146062 [details] Patch Attachment 146062 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12908366 New failing tests: media/track/track-cue-nothing-to-render.html media/track/track-cue-mutable-text.html media/track/track-cue-mutable-fragment.html
Created attachment 146293 [details] Archive of layout-test-results from ec2-cr-linux-01 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-01 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Created attachment 146364 [details] Patch Fixes the failing text track tests.
Committed r119940: <http://trac.webkit.org/changeset/119940>
(In reply to comment #10) > Committed r119940: <http://trac.webkit.org/changeset/119940> This caused ~10 media tests to start failing. See for example <http://build.webkit.org/results/Lion%20Release%20(Tests)/r119940%20(9220)/results.html>.
(In reply to comment #11) > (In reply to comment #10) > > Committed r119940: <http://trac.webkit.org/changeset/119940> > > This caused ~10 media tests to start failing. See for example <http://build.webkit.org/results/Lion%20Release%20(Tests)/r119940%20(9220)/results.html>. I reverted the change in <http://trac.webkit.org/r119978>.