Bug 82150

Created attachment 133700 [details] Patch Uploading for EWS bots - some tests might need rebaseline

Attachment 133700 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebCore/ChangeLog:9: You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible. [changelog/nonewtests] [5] Total errors found: 1 in 4 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 133700 [details] Patch Attachment 133700 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12134498 New failing tests: media/media-document-audio-repaint.html media/video-no-audio.html media/controls-strict.html media/video-volume-slider.html media/controls-styling.html media/video-display-toggle.html media/audio-repaint.html media/audio-controls-rendering.html media/video-zoom-controls.html media/video-controls-rendering.html media/controls-without-preload.html media/media-controls-clone.html fast/layers/video-layer.html media/video-empty-source.html media/video-playing-and-pause.html media/controls-after-reload.html

Created attachment 133702 [details] Archive of layout-test-results from ec2-cr-linux-03 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-03 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

Created attachment 134039 [details] Updated patch and added test

Comment on attachment 134039 [details] Updated patch and added test Attachment 134039 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12145529 New failing tests: media/video-controls-rendering-toggle-display-none.html

Created attachment 134048 [details] Archive of layout-test-results from ec2-cr-linux-02 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-02 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

Comment on attachment 134039 [details] Updated patch and added test View in context: https://bugs.webkit.org/attachment.cgi?id=134039&action=review > Source/WTF/WTF.vcproj/WTF.sln:-6 > - > -Microsoft Visual Studio Solution File, Format Version 9.00 > -# Visual Studio 2005 > -Project("{8BC9CEB8-8B4A-11D0-8D11-00A0C91BC942}") = "WTFGenerated", "WTFGenerated.vcproj", "{5AE5F5E4-782D-4F63-B4D7-3977B52B9950}" > -EndProject > -Project("{8BC9CEB8-8B4A-11D0-8D11-00A0C91BC942}") = "WTF", "WTF.vcproj", "{AA8A5A85-592B-4357-BC60-E0E91E026AF6}" Is this change necessary? > LayoutTests/media/video-controls-rendering-toggle-display-none.html:13 > + if (window.layoutTestController) > + layoutTestController.waitUntilDone(); I would *really* prefer to not have pixel results for this test, they are a serious pain to maintain. Can't you test the change by checking the CSS OM from script?

Created attachment 134315 [details] Removed pixel test and added text test

(In reply to comment #8) > (From update of attachment 134039 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=134039&action=review > > Source/WTF/WTF.vcproj/WTF.sln:-6 > > - > > -Microsoft Visual Studio Solution File, Format Version 9.00 > > -# Visual Studio 2005 > > -Project("{8BC9CEB8-8B4A-11D0-8D11-00A0C91BC942}") = "WTFGenerated", "WTFGenerated.vcproj", "{5AE5F5E4-782D-4F63-B4D7-3977B52B9950}" > > -EndProject > > -Project("{8BC9CEB8-8B4A-11D0-8D11-00A0C91BC942}") = "WTF", "WTF.vcproj", "{AA8A5A85-592B-4357-BC60-E0E91E026AF6}" > Is this change necessary? I have no idea how this ended here. Removed. > > LayoutTests/media/video-controls-rendering-toggle-display-none.html:13 > > + if (window.layoutTestController) > > + layoutTestController.waitUntilDone(); > I would *really* prefer to not have pixel results for this test, they are a serious pain to maintain. Can't you test the change by checking the CSS OM from script? I agree - I just wasn't sure how to test it at the beginning, but I think the updated patch is good enough (and fails on the current trunk).

Comment on attachment 134315 [details] Removed pixel test and added text test View in context: https://bugs.webkit.org/attachment.cgi?id=134315&action=review > Source/WebCore/html/shadow/MediaControlRootElementChromium.cpp:130 > + > + Nit: you have an extra blank line here. > LayoutTests/media/video-controls-rendering-toggle-display-none.html:22 > + /* > + if (window.layoutTestController) > + layoutTestController.waitUntilDone(); > + */ Did you forgot to uncomment this, or is it unnecessary?

<rdar://problem/11139107>

Created attachment 134328 [details] Fixed nits

(In reply to comment #11) > (From update of attachment 134315 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=134315&action=review > > > Source/WebCore/html/shadow/MediaControlRootElementChromium.cpp:130 > > + > > + > > Nit: you have an extra blank line here. Removed. > > LayoutTests/media/video-controls-rendering-toggle-display-none.html:22 > > + /* > > + if (window.layoutTestController) > > + layoutTestController.waitUntilDone(); > > + */ > > Did you forgot to uncomment this, or is it unnecessary? I forgot to delete it (it's not required, since video-test.js already has this line).

Comment on attachment 134328 [details] Fixed nits Clearing flags on attachment: 134328 Committed r112484: <http://trac.webkit.org/changeset/112484>

All reviewed patches have been landed. Closing bug.

This broke the tests on all the Mac builds. I've added the failing tests to platform/mac/test_expectations.txt.

This broke the tests on all the Mac builds. I've added the failing tests to platform/mac/test_expectations.txt. I filed https://bugs.webkit.org/show_bug.cgi?id=82626 to track the need for a new baseline in all platform.

Last patch has been rolled out because of mac regressions. Marking this as reopened.

Created attachment 135820 [details] Changed patch to work with Safari

Created attachment 136015 [details] Fixed z-index for volume button on Safari

Comment on attachment 136015 [details] Fixed z-index for volume button on Safari Thanks Victor!

Comment on attachment 136015 [details] Fixed z-index for volume button on Safari Clearing flags on attachment: 136015 Committed r113609: <http://trac.webkit.org/changeset/113609>

All reviewed patches have been landed. Closing bug.

Hi Victor, It looks like the expectations you added in test_expectations.txt were slightly wrong (they should've been TEXT, not IMAGE+TEXT). Also, we don't generally try to suppress failures up front; it's usually better to just let the tests fail on the bots and then either the chromium gardener will rebaseline things or you can afterwards. While it is nice to try and keep things green, I've usually found that it's better to keep things red to remind/force yourself to fix the rebaseline. By suppressing the failures it's too easy to forget about them and leave the lines in (and the expectations wrong) indefinitely.

The Mac tests are failing again. Do they just need a new baseline?

Sorry, I had to roll out this patch again because of the failures: https://bugs.webkit.org/show_bug.cgi?id=83522

Re-opening since this was rolled out.

Created attachment 136449 [details] Same patch. New baselines for Mac.

I have added new baselines for Mac. This is the first time I re-baseline expectations on Mac so I don't know if it's enough what I uploaded with this patch (that's why I left rebaselining in a different bug). Dirk, I have left the TEXT expectations on Chromium to not have automatically the cq- from the webkit bot, but if you still prefer this patch to turn the tree red, I won't mind removing them. Either way, I will be handling the rebaselines for Chromium when the bots have cycled. Since there are so many of you CCed at this bug I would appreciate feedback before we need to rollout this patch again. I'm sorry for wasting already a lot of time with the other two rollouts. Thanks.

Comment on attachment 136449 [details] Same patch. New baselines for Mac. looks fine by me as far as test_expectations.txt goes.

I'll check the GTK media tests with this patch

Created attachment 138011 [details] GTK media controls update + rebaseline

Created attachment 138121 [details] Merged patch

Comment on attachment 138121 [details] Merged patch Rejecting attachment 138121 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 Last 500 characters of output: mium' 47>At revision 11316. ________ running '/usr/bin/python tools/clang/scripts/update.py --mac-only' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium' ________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium' Updating webkit projects from gyp files... LayoutTests/platform/chromium/test_expectations.txt:53: Duplicate or ambiguous override. fast/repaint/shadow-multiple-vertical.html [test/expectations] [5] Total errors found: 1 in 1 files Full output: http://queues.webkit.org/results/12470383

(In reply to comment #35) > (From update of attachment 138121 [details]) > Rejecting attachment 138121 [details] from commit-queue. > > Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 > > Last 500 characters of output: > mium' > 47>At revision 11316. > > ________ running '/usr/bin/python tools/clang/scripts/update.py --mac-only' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium' > > ________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium' > Updating webkit projects from gyp files... > LayoutTests/platform/chromium/test_expectations.txt:53: Duplicate or ambiguous override. fast/repaint/shadow-multiple-vertical.html [test/expectations] [5] > Total errors found: 1 in 1 files > > Full output: http://queues.webkit.org/results/12470383 I can't see how this cq error was in any way related to the patch. I tried rebase-ing the last patch, but the GTK tests have had some rebaselines in the meantime so it doesn't apply anymore. I will rebase only the initial patch for chrome and mac and I'll include GTK expectations update and css changes as well. The rebaseline should follow easily. Thank you for your effort, Philippe!

Created attachment 138337 [details] Rebased patch

Comment on attachment 138337 [details] Rebased patch Clearing flags on attachment: 138337 Committed r114957: <http://trac.webkit.org/changeset/114957>

All reviewed patches have been landed. Closing bug.

This has caused bug 88615.

(In reply to comment #40) > This has caused bug 88615. I am not sure I would call that a report a bug. AFAIK we never intended to support overriding the media controls style.