| Summary: | media element handle adding source immediately before src. | ||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Jeremy Jones <jeremyj-wk> | ||||||||||||||||||||||||||||||||
| Component: | Media | Assignee: | Jeremy Jones <jeremyj-wk> | ||||||||||||||||||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||||||
| Severity: | Normal | CC: | buildbot, commit-queue, ddkilzer, eric.carlson, rniwa, webkit-bug-importer | ||||||||||||||||||||||||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||||||||||||
| Version: | WebKit Nightly Build | ||||||||||||||||||||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||||||||||||||||||||
| OS: | Unspecified | ||||||||||||||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||||||||||||||
|
Description
Jeremy Jones
2017-07-07 16:34:24 PDT
Created attachment 314899 [details]
test only
Created attachment 314900 [details]
Patch
Comment on attachment 314899 [details] test only Attachment 314899 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/4074140 New failing tests: media/video-source-before-src.html Created attachment 314904 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122 Port: ios-simulator-wk2 Platform: Mac OS X 10.12.5
Comment on attachment 314900 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=314900&action=review r=me (although maybe you want someone with more WebKit media experience to review) > Source/WebCore/ChangeLog:9 > + Test: media/video-source-before-src.html The test crashes without the fix! Yay! > Source/WebCore/ChangeLog:11 > + Adding a soruce causes selectMediaResource block to be enqueued. Typo: soruce = > source > Source/WebCore/ChangeLog:13 > + If dataLoadingPermitted prevents creating the m_player but sets the srcAttr. > + The enqeueud selectMediaResource will be in a bad state, with a srcAttr but no m_player. These read best like a single sentence: If ... srcAttr, then the enqueued .... Note that "enqueued" is also typoed. > Source/WebCore/html/HTMLMediaElement.cpp:1328 > + LOG(Media, "HTMLMediaElement::selectMediaResource(%p) - has srcAttr but m_player is not created", this); Should this be a RELEASE_LOG_ERROR() or at least a LOG_ERROR()? Comment on attachment 314900 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=314900&action=review >> Source/WebCore/ChangeLog:11 >> + Adding a soruce causes selectMediaResource block to be enqueued. > > Typo: soruce = > source ... causes *a* selectMediaResource block ... > Source/WebCore/ChangeLog:15 > + This fix prevents selectMediaResource when adding a source to match how it prevents player This fix prevents selectMediaResource *from being called if data loading is not permitted* when adding a source *element* ... Comment on attachment 314900 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=314900&action=review >> Source/WebCore/ChangeLog:9 >> + Test: media/video-source-before-src.html > > The test crashes without the fix! Yay! +1 >>> Source/WebCore/ChangeLog:11 >>> + Adding a soruce causes selectMediaResource block to be enqueued. >> >> Typo: soruce = > source > > ... causes *a* selectMediaResource block ... Done. Done. >> Source/WebCore/ChangeLog:13 >> + The enqeueud selectMediaResource will be in a bad state, with a srcAttr but no m_player. > > These read best like a single sentence: > > If ... srcAttr, then > the enqueued .... > > Note that "enqueued" is also typoed. Done. >> Source/WebCore/ChangeLog:15 >> + This fix prevents selectMediaResource when adding a source to match how it prevents player > > This fix prevents selectMediaResource *from being called if data loading is not permitted* when adding a source *element* ... Done. >> Source/WebCore/html/HTMLMediaElement.cpp:1328 >> + LOG(Media, "HTMLMediaElement::selectMediaResource(%p) - has srcAttr but m_player is not created", this); > > Should this be a RELEASE_LOG_ERROR() or at least a LOG_ERROR()? Done. Created attachment 315005 [details]
Patch for landing.
Comment on attachment 315005 [details] Patch for landing. Clearing flags on attachment: 315005 Committed r219305: <http://trac.webkit.org/changeset/219305> *** Bug 174489 has been marked as a duplicate of this bug. *** Created attachment 315399 [details]
Patch
Comment on attachment 315399 [details] Patch Attachment 315399 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/4117130 New failing tests: media/video-source-before-src.html Created attachment 315402 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123 Port: ios-simulator-wk2 Platform: Mac OS X 10.12.5
Comment on attachment 315399 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=315399&action=review r=me assuming iOS sim failure isn’t this. > LayoutTests/media/video-source-before-src.html:12 > +var video Nit: Add semi-colon. > LayoutTests/media/video-source-before-src.html:15 > + video = document.getElementsByTagName("video")[0] Nit: semi-colon > LayoutTests/media/video-source-before-src.html:19 > + video.src = "src.mp4" Nit on above e lines: semi-colon (In reply to David Kilzer (:ddkilzer) from comment #15) > Comment on attachment 315399 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=315399&action=review > > r=me assuming iOS sim failure isn’t this. Oops, it is this test. (In reply to David Kilzer (:ddkilzer) from comment #16) > (In reply to David Kilzer (:ddkilzer) from comment #15) > > Comment on attachment 315399 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=315399&action=review > > > > r=me assuming iOS sim failure isn’t this. > > Oops, it is this test. iOS needs different expectations. Comment on attachment 315399 [details]
Patch
r=me if you fix the iOS expectations before landing.
Created attachment 315501 [details]
Patch
(In reply to David Kilzer (:ddkilzer) from comment #15) > Comment on attachment 315399 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=315399&action=review > > r=me assuming iOS sim failure isn’t this. It is. :-( > > > LayoutTests/media/video-source-before-src.html:12 > > +var video > > Nit: Add semi-colon. Done. > > > LayoutTests/media/video-source-before-src.html:15 > > + video = document.getElementsByTagName("video")[0] > > Nit: semi-colon Done. > > > LayoutTests/media/video-source-before-src.html:19 > > + video.src = "src.mp4" > > Nit on above e lines: semi-colon Done. Comment on attachment 315501 [details] Patch Attachment 315501 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/4122472 New failing tests: media/video-source-before-src.html Created attachment 315509 [details]
Archive of layout-test-results from ews101 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 315501 [details] Patch Attachment 315501 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/4122475 New failing tests: media/video-source-before-src.html Created attachment 315510 [details]
Archive of layout-test-results from ews104 for mac-elcapitan-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Comment on attachment 315501 [details] Patch Attachment 315501 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/4122490 New failing tests: media/video-source-before-src.html Created attachment 315511 [details]
Archive of layout-test-results from ews113 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 315501 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=315501&action=review > LayoutTests/media/video-source-before-src-expected.txt:4 > +Append source element before setting src attribute. > +Platforms that support setVideoPlaybackRequiresUserGesture will have the expected, empty, currentSrc. Platforms that don't should have the unexpected "src.mp4". > + > +EXPECTED (relativeURL(video.currentSrc) == ''), OBSERVED 'src.mp4' FAIL Now the mac results are failing. :( (In reply to David Kilzer (:ddkilzer) from comment #27) > Comment on attachment 315501 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=315501&action=review > > > LayoutTests/media/video-source-before-src-expected.txt:4 > > +Append source element before setting src attribute. > > +Platforms that support setVideoPlaybackRequiresUserGesture will have the expected, empty, currentSrc. Platforms that don't should have the unexpected "src.mp4". > > + > > +EXPECTED (relativeURL(video.currentSrc) == ''), OBSERVED 'src.mp4' FAIL > > Now the mac results are failing. :( Missing a blank line in -expected. Fixed. Created attachment 315685 [details]
Patch for landing.
Comment on attachment 315685 [details] Patch for landing. Attachment 315685 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/4136857 New failing tests: security/contentSecurityPolicy/video-with-data-url-allowed-by-media-src-star.html Created attachment 315692 [details]
Archive of layout-test-results from ews100 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 315685 [details] Patch for landing. Attachment 315685 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/4136887 New failing tests: http/tests/canvas/canvas-tainted-after-draw-image.html Created attachment 315697 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123 Port: ios-simulator-wk2 Platform: Mac OS X 10.12.5
Comment on attachment 315685 [details] Patch for landing. Attachment 315685 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/4137008 New failing tests: security/contentSecurityPolicy/video-with-data-url-allowed-by-media-src-star.html Created attachment 315701 [details]
Archive of layout-test-results from ews115 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 315685 [details] Patch for landing. Rejecting attachment 315685 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'validate-changelog', '--check-oops', '--non-interactive', 315685, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit ChangeLog entry in LayoutTests/ChangeLog contains OOPS!. Full output: http://webkit-queues.webkit.org/results/4137477 Created attachment 315716 [details]
Patch for landing test fix.
The commit-queue encountered the following flaky tests while processing attachment 315716 [details]: http/tests/security/cross-origin-xsl-BLOCKED.html bug 51054 (authors: abarth@webkit.org, jochen@chromium.org, and rniwa@webkit.org) The commit-queue is continuing to process your patch. The commit-queue encountered the following flaky tests while processing attachment 315716 [details]:
The commit-queue is continuing to process your patch.
Comment on attachment 315716 [details] Patch for landing test fix. Clearing flags on attachment: 315716 Committed r219579: <http://trac.webkit.org/changeset/219579> |