Updated the media layout tests, which unveild a bug in our V8 bindings.
Created attachment 41278 [details] Round 1 Not marking cq? as it contains a binary file.
Comment on attachment 41278 [details] Round 1 Please split these two unrelated changes out in to separate bugs + patches.
Created attachment 41301 [details] Round 2 V8 change split to https://bugs.webkit.org/show_bug.cgi?id=30448
Comment on attachment 41301 [details] Round 2 Why delay setting the src until the onload? Could work just as well in a <script> tag after the <video> element.
> Why delay setting the src until the onload? Could work just as well in a > <script> tag after the <video> element. > That would also work, would it make it a better test?
I remembered Eric Carlson suggested to use an onload handler instead of additional <script> blocks in https://bugs.webkit.org/show_bug.cgi?id=28327#c52 But if that was for a different reason than this, then I digress :)
Comment on attachment 41301 [details] Round 2 > diff --git a/LayoutTests/media/content/scaled-matrix.ogv b/LayoutTests/media/content/scaled-matrix.ogv > new file mode 100644 > index 0000000..1684a7b > Binary files /dev/null and b/LayoutTests/media/content/scaled-matrix.ogv differ Converting this movie to ogg format doesn't sense unless ogg files can contain a scaling matrix. If they can't it would make more sense to change video-no-audio.html to use a different video-only movie. r=me with this change if necessary
Comment on attachment 41301 [details] Round 2 Let commit bot land it
Comment on attachment 41301 [details] Round 2 Rejecting patch 41301 from commit-queue. Patch https://bugs.webkit.org/attachment.cgi?id=41301 from bug 30435 failed to download and apply.
Comment on attachment 41301 [details] Round 2 eric.carlson: review+ However, we can't land this patch because diff --git a/LayoutTests/media/content/scaled-matrix.ogv b/LayoutTests/media/content/scaled-matrix.ogv new file mode 100644 index 0000000..1684a7b Binary files /dev/null and b/LayoutTests/media/content/scaled-matrix.ogv differ we don't have scaled-matrix.ogv! Please either upload a diff with the binary content or attach the file separately to this bug.
Doh forgive my noobness -- I'll fix up the patch one more time and get someone who sits by me to land the patch.
> Converting this movie to ogg format doesn't sense unless ogg files can contain > a scaling matrix. If they can't it would make more sense to change > video-no-audio.html to use a different video-only movie. Can you please at least comment on the issue I raised?
Created attachment 41541 [details] Round 3 I had serious issues rebaselining due to QuickTime version mismatch. You might have to tackle them Eric :(
Comment on attachment 41541 [details] Round 3 r=me
I believe committing this will cause Mac layout tests to fail... guess one way to find out :s
Comment on attachment 41541 [details] Round 3 Rejecting patch 41541 from commit-queue. Failed to run "['/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', '--reviewer', 'Eric Carlson', '--force']" exit_code: 1 Last 500 characters of output: ith fuzz 3. patching file LayoutTests/media/audio-constructor-src.html Hunk #1 succeeded at 3 with fuzz 2. patching file LayoutTests/media/audio-play-event-expected.txt patching file LayoutTests/media/audio-play-event.html patching file LayoutTests/media/video-no-audio.html patching file LayoutTests/media/video-source-add-src.html patching file LayoutTests/media/video-src-change.html Hunk #1 FAILED at 7. 1 out of 1 hunk FAILED -- saving rejects to file LayoutTests/media/video-src-change.html.rej
Ping?
I can upload a new version of the patch, but I can't create the baselines because I don't have Snow Leopard (QuickTime generates a completely different UI).
Oh. OK. The commit-queue is still running on Leopard, so only the leopard tests have to pass for it to actually commit. But yes, you would need to look at the bots post-commit and post a patch with snow leopard results as well. Sadly webkit doesn't have try-bots or a rebaselining tool to solve this for you yet.
Yikes.. ok I'll try and figure something out. Thanks for the pointers!
I check in changes like this during a quiet time of the day and update the other port build results from the failed build-bot results. It is a bit of a pain, but we don't have anything better just yet.
That might work.. but it's not a long-term solution. I might be better off learning The Right Way to rebaseline media layout tests. In the meantime I need to re-upload the patches.. they've gone a bit stale :)
There is plenty of historical precident for webkit committers using the build bots as try-bots, at least in terms of "check in and then get the new results off the bots" behavior. Whether that is good or not is a topic for another discussion. Regardless, existing committers do that all the time. It's more difficult for you however, because if you manage to break a bot which the commit-queue blocks on: http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/modules/buildbot.py#L47 then it won't be able to commit any follow-up patches for you as it will be waiting for the bots to roll green. The commit-queue is only useful for committing patches which are not expected to break any bots. It's useless for committing tree fixes, or patches which are expected to break things. :(
Created attachment 43822 [details] Round 3 updated Updated to work against tip of tree.
Created attachment 43823 [details] Actual result
Created attachment 43824 [details] Expected result
I've attached a sample expected/actual result... I have no idea why the buttons change position :( I'm running the following: Mac 10.5.8 QuickTime Player 7.6.4 (518.35) QuickTime Version 7.6.4 (1327.73) Hope that helps!
style-queue ran check-webkit-style on attachment 43822 [details] without any errors.
Comment on attachment 43822 [details] Round 3 updated Oh, I didn't realize Andrew wasn't a committer (at least he's not in committers.py). cq+ing
Comment on attachment 43822 [details] Round 3 updated Rejecting patch 43822 from commit-queue. Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--exit-after-n-failures=1', '--quiet']" exit_code: 1 Running build-dumprendertree Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests Testing 11752 test cases. media/video-no-audio.html -> failed Exiting early after 1 failures. 9492 tests run. 278.20s total testing time 9491 test cases (99%) succeeded 1 test case (<1%) had incorrect layout 5 test cases (<1%) had stderr output
This patch will need update, it cannot be landed as is.
My mac isn't configured properly and I cannot generate proper rebaselines. I'll resolve those issues and upload a new patch soonish.
Created attachment 45448 [details] Round 4 Has new image results, will have to attach and someone land manually.
Created attachment 45449 [details] Round 4 image attachment
Finally figured it out!!
Comment on attachment 45448 [details] Round 4 r=me
Comment on attachment 45448 [details] Round 4 Changing Maciej's r+ to an r-. This bug is lacking the binary data for the changed png files, so the commit-queue cannot land it. Andrew is not yet a committer (according to committers.py), so someone would have to land this for him. Better would be for Andrew to attach a patch containing the png changes. "bugzilla-tool post-diff" or "bugzilla-tool post-commits" will handle git patches correctly. Likewise one could just do a git diff --binary and attach that.
Created attachment 45831 [details] Round 5
Created attachment 45832 [details] Round 6
Created attachment 45833 [details] Round 6 + proper ChangeLog
wow sorry for the spam... had to figure out bugzilla-tool but wow is it ever nifty!!! thanks for the tip!
style-queue ran check-webkit-style on attachment 45833 [details] without any errors.
Comment on attachment 45833 [details] Round 6 + proper ChangeLog Is this result now platform specific? RUN(mediaElement.src = 'content/silence.mpg') 3 RUN(mediaElement.src = 'content/test.wav')
Comment on attachment 45833 [details] Round 6 + proper ChangeLog > - <video src="content/scaled-matrix.mov" controls onload="finish()"></video> > + <video controls onload="finish()"></video> This isn't new to your patch, but you should change this so it doesn't depend on a 'load' event being fired by the <audio> element.
Comment on attachment 45833 [details] Round 6 + proper ChangeLog > + * platform/mac-leopard/media/video-no-audio-expected.checksum: New results due to changing source video. > + * platform/mac-leopard/media/video-no-audio-expected.png: Ditto. > + * platform/mac-leopard/media/video-no-audio-expected.txt: Ditto. You should check these in for SnowLeopard as well. > - waitForEvent("loadstart", function () { testExpected("relativeURL(audio.currentSrc)", "content/test.wav"); }); > + waitForEvent("loadstart", function () { testExpected("relativeURL(audio.currentSrc)", audioFile); }); Eric's comment is correct. While all ports support wav at the moment, the spec doesn't require it so you should change this to use findMediaFile(). > diff --git a/LayoutTests/platform/mac-leopard/media/video-no-audio-expected.png b/LayoutTests/platform/mac-leopard/media/video-no-audio-expected.png > index 208d4788bdc6f6338561ea69aa9afd31e24b6202..e6b4ec557a2560b4e58d0afa28b3f6f470bd65b3 100644 > GIT binary patch > literal 20611 > > literal 21975 Do you need to set the svn:mime-type on these files somehow? r=me with these changes.
(In reply to comment #46) > (From update of attachment 45833 [details]) > > > + * platform/mac-leopard/media/video-no-audio-expected.checksum: New results due to changing source video. > > + * platform/mac-leopard/media/video-no-audio-expected.png: Ditto. > > + * platform/mac-leopard/media/video-no-audio-expected.txt: Ditto. > > You should check these in for SnowLeopard as well. platform/mac-snowleopard/media doesn't exist... where you referring to platform/mac/media? > > - waitForEvent("loadstart", function () { testExpected("relativeURL(audio.currentSrc)", "content/test.wav"); }); > > + waitForEvent("loadstart", function () { testExpected("relativeURL(audio.currentSrc)", audioFile); }); > > Eric's comment is correct. While all ports support wav at the moment, the spec > doesn't require it so you should change this to use findMediaFile(). I'm confused... isn't that what my patch is doing in LayoutTests/media/audio-play-event.html ? > > diff --git a/LayoutTests/platform/mac-leopard/media/video-no-audio-expected.png b/LayoutTests/platform/mac-leopard/media/video-no-audio-expected.png > > index 208d4788bdc6f6338561ea69aa9afd31e24b6202..e6b4ec557a2560b4e58d0afa28b3f6f470bd65b3 100644 > > GIT binary patch > > literal 20611 > > > > > literal 21975 > > Do you need to set the svn:mime-type on these files somehow? Using git I can't set the mime-type.. would have to be a follow-up patch or the committer could set it.
Created attachment 45857 [details] Round 7
Round 7 replaces onload with oncanplaythrough (nice catch!)
style-queue ran check-webkit-style on attachment 45857 [details] without any errors.
Comment on attachment 45857 [details] Round 7 (In reply to comment #47) > (In reply to comment #46) > > (From update of attachment 45833 [details] [details]) > > > > > + * platform/mac-leopard/media/video-no-audio-expected.checksum: New results due to changing source video. > > > + * platform/mac-leopard/media/video-no-audio-expected.png: Ditto. > > > + * platform/mac-leopard/media/video-no-audio-expected.txt: Ditto. > > > > You should check these in for SnowLeopard as well. > > platform/mac-snowleopard/media doesn't exist... where you referring to > platform/mac/media? > Yes, SnowLeopard is the current mac platform. > > > - waitForEvent("loadstart", function () { testExpected("relativeURL(audio.currentSrc)", "content/test.wav"); }); > > > + waitForEvent("loadstart", function () { testExpected("relativeURL(audio.currentSrc)", audioFile); }); > > > > Eric's comment is correct. While all ports support wav at the moment, the spec > > doesn't require it so you should change this to use findMediaFile(). > > I'm confused... isn't that what my patch is doing in > LayoutTests/media/audio-play-event.html ? > Yes, but I copied the wrong line from the diff. The problem is in audio-play-event.html where you are calling "run()" with the name of a platform specific media file. "run()" logs the name of the file passed to it so the test result has the platform specific name. r=me with these changes
Checking on the status of this patch. It's been approved for landing for almost a month.
Comment on attachment 41541 [details] Round 3 Cleared Eric Carlson's review+ from obsolete attachment 41541 [details] so that this bug does not appear in http://webkit.org/pending-commit.
Comment on attachment 43822 [details] Round 3 updated Cleared Darin Adler's review+ from obsolete attachment 43822 [details] so that this bug does not appear in http://webkit.org/pending-commit.
Now that I'm a committer and have some more spare time I'll tackle this and upload a new patch and attempt to land it by myself after the review.
Created attachment 49542 [details] Round 8
OK so I fixed the RUN(...) stuff in audio-play-event.html so it's platform independent and copied* the image results from mac-leopard to mac * I'm positive it won't pass on mac (i.e., Snow Leopard) because Snow Leopard displays a fullscreen button in the controls.. not sure what to do here other than land the patch and grab the correct results off a bot? Guidance appreciated!
Comment on attachment 49542 [details] Round 8 r=me (and many others before me). Please find a snow leopard machine and get some results for it to check in before you land it.
Committed as http://trac.webkit.org/changeset/56481
media/video-no-audio.html has failed on the Tiger bot ever since this commit. Please fix. :)
What the!? I thought I resolved that in https://trac.webkit.org/changeset/56807 Investigating...
I'm looking at http://build.webkit.org/builders/Tiger%20Intel%20Release and not seeing any media layout tests failing. I'm still mastering the ropes here so by all means point out where/what I'm screwing up :)
My sincerest apologies. I just finished writing a tool to help me look back in time to find failure like these. Due to some problem builds on tiger, I started my crawl at r56619, which is in between when it broke and when you fixed it. So my message to you was in error! Again, sorry for the confusion.
No worries!