Bug 82150 – Simplify volume slider rendering

• Component: Media
• Status: RESOLVED FIXED

[Victor Carbune]

There are simpler ways to render the volume slider on top of the mute button, rather than the current approach of having a particular renderer.

[Victor Carbune]

Created attachment 133700 [details]
Patch

Uploading for EWS bots - some tests might need rebaseline

[WebKit Review Bot]

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.

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Victor Carbune]

Created attachment 134039 [details]
Updated patch and added test

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Eric Carlson]

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?

[Victor Carbune]

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

[Victor Carbune]

(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).

[Eric Carlson]

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?

[Radar WebKit Bug Importer]

<rdar://problem/11139107>

[Victor Carbune]

Created attachment 134328 [details]
Fixed nits

[Victor Carbune]

(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).

[WebKit Review Bot]

Comment on attachment 134328 [details]
Fixed nits

Clearing flags on attachment: 134328

Committed r112484: <http://trac.webkit.org/changeset/112484>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Enrica Casucci]

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

[Enrica Casucci]

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.

[Victor Carbune]

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

[Victor Carbune]

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

[Victor Carbune]

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

[Eric Carlson]

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

Thanks Victor!

[WebKit Review Bot]

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>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Dirk Pranke]

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.

[Beth Dakin]

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

[Beth Dakin]

Sorry, I had to roll out this patch again because of the failures:

https://bugs.webkit.org/show_bug.cgi?id=83522

[Beth Dakin]

Re-opening since this was rolled out.

[Victor Carbune]

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

[Victor Carbune]

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.

[Dirk Pranke]

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

looks fine by me as far as test_expectations.txt goes.

[Philippe Normand]

I'll check the GTK media tests with this patch

[Philippe Normand]

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

[Victor Carbune]

Created attachment 138121 [details]
Merged patch

[WebKit Review Bot]

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

[Victor Carbune]

(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!

[Victor Carbune]

Created attachment 138337 [details]
Rebased patch

[WebKit Review Bot]

Comment on attachment 138337 [details]
Rebased patch

Clearing flags on attachment: 138337

Committed r114957: <http://trac.webkit.org/changeset/114957>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Alexey Proskuryakov]

This has caused bug 88615.

[Eric Carlson]

(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.