Here's a testcase where the value of muted gets reset when the media resource actually starts playing. The element does get muted, according to the controls for the element, but that value isn't being passed down to, or is being ignored by, the player.
I'm testing this on Chrome/Chromium, and Paul Irish is telling me that this might not belong in WebKit proper. Let me know if I need to refile elsewhere.
See also https://code.google.com/p/chromium/issues/detail?id=70777
Testcase does not reproduce in WebKit/Safari, as only a .ogv source is provided.
http://scotland.proximity.on.ca/dxr/tmp/mute-mp3.html Also fails, as described above.
Verified that this also reproduces on WebKit/Safari.
Created attachment 88351 [details] Possible fix Here's a possible fix. This change fixes the case I linked above. I couldn't figure out where the tests for the media element were, and no one on irc was around to answer. If you can guide me, I'll do a test for this, too.
David, the media layout tests are in LayoutTests/media/. Writing a test may be difficult, because HTMLMediaElement::isMuted() will return true, even if the MediaPlayer is still generating sound.
Created attachment 88364 [details] Test for mute before play. Here's a test.
Attachment 88364 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style']" exit_code: 1 Total errors found: 0 in 0 files If any of these errors are false positives, please file a bug against check-webkit-style.
The test case passes on my install of Safari, which does not have your fix. I don't believe the test case tests the failure condition correctly. (This is what I mean by it being a hard test case to write.) :)
Comment on attachment 88364 [details] Test for mute before play. mmm, clearly I shouldn't be writing tests at 1 am :) Yeah, I agree that this is untestable from content scripts.
(In reply to comment #11) > (From update of attachment 88364 [details]) > mmm, clearly I shouldn't be writing tests at 1 am :) Yeah, I agree that this is untestable from content scripts. It should be possible to create a manual test in Source/WebCore/manual-tests/. I think you want to have a test that sets muted to true before setting src, and verify that no sound is emitted when the movie plays.
(In reply to comment #12) > > It should be possible to create a manual test in Source/WebCore/manual-tests/. I think you want to have a test that sets muted to true before setting src, and verify that no sound is emitted when the movie plays. Oh, and the test should be part of the patch instead of a separate attachment.
OK, I'll do that. Tell me, what's the difference here: ./third_party/WebKit/Source/WebCore/manual-tests ./third_party/WebKit/WebCore/manual-tests
(In reply to comment #14) > OK, I'll do that. Tell me, what's the difference here: > > ./third_party/WebKit/Source/WebCore/manual-tests > ./third_party/WebKit/WebCore/manual-tests WebKit/WebCore/ doesn't exist any more, you probably had a modified file in there somewhere when the repository was updated.
Created attachment 88547 [details] Fix + manual test Here's the fix with a manual test. I was unable to find any test media in the tree that also has sound, so I've followed the example of some other manual tests and linked to urls in svn. If there is a more suitable media resource, please let me know. Once again, irc is getting me nowhere.
Comment on attachment 88547 [details] Fix + manual test View in context: https://bugs.webkit.org/attachment.cgi?id=88547&action=review "bugzilla-57673.html" won't be a terribly helpful name to someone browsing the test directory later, maybe something like "media-muted.html" to follow the pattern of several of the existing manual media tests? This patch is missing the ChangeLog. If you aren't sure how to generate it, http://www.webkit.org/coding/contributing.html has a good description. Also if you set the cq? flag when you set r?, the reviewer will set cq+ when your patch is r+'ed and the commit bot will commit it automatically. Marking r- because of the missing ChangeLog, but please also consider the load() comment and test name suggestion. > third_party/WebKit/Source/WebCore/manual-tests/bugzilla-57673.html:22 > + vid.src = "http://src.chromium.org/svn/trunk/src/chrome/test/data/media/" + findMediaFile("video", "bear"); > + > + vid.load(); vid.load() is unnecessary, setting vid.src triggers the resource selection algorithm.
(In reply to comment #16) > Once again, irc is getting me nowhere. I (eric_carlson) am usually on #webkit from early morning through early afternoon Pacific time.
Created attachment 88754 [details] Fix + manual test + review fixes Thanks for the review, Eric. Here's a new patch with the fixes you suggest, plus a ChangeLog entry. Hopefully I've done this the way you want.
Created attachment 88757 [details] Fix + manual test + review fixes (whitespace fix) That patch had some white space issues in the ChangeLog, here's a better one.
Comment on attachment 88757 [details] Fix + manual test + review fixes (whitespace fix) Thanks David!
Comment on attachment 88757 [details] Fix + manual test + review fixes (whitespace fix) Rejecting attachment 88757 [details] from commit-queue. Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-7', 'apply-..." exit_code: 2 Last 500 characters of output: ========================================== |--- third_party/WebKit/Source/WebCore/html/HTMLMediaElement.cpp (revision 82928) |+++ third_party/WebKit/Source/WebCore/html/HTMLMediaElement.cpp (working copy) -------------------------- No file to patch. Skipping patch. 1 out of 1 hunk ignored patching file third_party/WebKit/Source/WebCore/manual-tests/media-muted.html Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Eric Carlson', u'--for..." exit_code: 1 Full output: http://queues.webkit.org/results/8348803
(In reply to comment #22) > Rejecting attachment 88757 [details] from commit-queue. > Last 500 characters of output: > ========================================== > |--- third_party/WebKit/Source/WebCore/html/HTMLMediaElement.cpp (revision 82928) > |+++ third_party/WebKit/Source/WebCore/html/HTMLMediaElement.cpp (working copy) > -------------------------- > No file to patch. Skipping patch. It looks like the patch did not apply cleanly because the paths are prefixed with third_party/WebKit, which i the location of WebKit inside the Chromium tree. That prefix should be absent for patches on the WebKit side.
(In reply to comment #23) > It looks like the patch did not apply cleanly because the paths are prefixed with third_party/WebKit, which i the location of WebKit inside the Chromium tree. That prefix should be absent for patches on the WebKit side. More generally, see http://dev.chromium.org/developers/contributing-to-webkit and the pointer towards using webkit-patch upload, which will generally do The Right Thing as far as patch paths, ChangeLogs, etc.
Created attachment 88908 [details] Fix + manual test + review fixes (whitespace fix, webkit) Switched to a WebKit tree.
Comment on attachment 88908 [details] Fix + manual test + review fixes (whitespace fix, webkit) Hopefully the third time will be a charm :-)
Comment on attachment 88908 [details] Fix + manual test + review fixes (whitespace fix, webkit) Clearing flags on attachment: 88908 Committed r83373: <http://trac.webkit.org/changeset/83373>
All reviewed patches have been landed. Closing bug.