Bug 82150 - Simplify volume slider rendering
Summary: Simplify volume slider rendering
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Victor Carbune
URL:
Keywords: InRadar
Depends on: 83522
Blocks: 82124 82476
  Show dependency treegraph
 
Reported: 2012-03-25 16:14 PDT by Victor Carbune
Modified: 2012-06-08 12:30 PDT (History)
14 users (show)

See Also:


Attachments
Patch (7.64 KB, patch)
2012-03-25 16:23 PDT, Victor Carbune
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-03 (9.57 MB, application/zip)
2012-03-25 16:54 PDT, WebKit Review Bot
no flags Details
Updated patch and added test (60.17 KB, patch)
2012-03-27 05:59 PDT, Victor Carbune
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-02 (10.38 MB, application/zip)
2012-03-27 06:53 PDT, WebKit Review Bot
no flags Details
Removed pixel test and added text test (14.66 KB, patch)
2012-03-28 09:35 PDT, Victor Carbune
no flags Details | Formatted Diff | Diff
Fixed nits (14.53 KB, patch)
2012-03-28 10:31 PDT, Victor Carbune
no flags Details | Formatted Diff | Diff
Changed patch to work with Safari (17.18 KB, patch)
2012-04-05 07:30 PDT, Victor Carbune
no flags Details | Formatted Diff | Diff
Fixed z-index for volume button on Safari (17.86 KB, patch)
2012-04-06 06:12 PDT, Victor Carbune
no flags Details | Formatted Diff | Diff
Same patch. New baselines for Mac. (30.89 KB, patch)
2012-04-10 07:09 PDT, Victor Carbune
no flags Details | Formatted Diff | Diff
GTK media controls update + rebaseline (21.13 KB, patch)
2012-04-19 17:25 PDT, Philippe Normand
no flags Details | Formatted Diff | Diff
Merged patch (54.11 KB, patch)
2012-04-20 11:05 PDT, Victor Carbune
no flags Details | Formatted Diff | Diff
Rebased patch (34.14 KB, patch)
2012-04-23 06:41 PDT, Victor Carbune
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Victor Carbune 2012-03-25 16:14:27 PDT
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.
Comment 1 Victor Carbune 2012-03-25 16:23:44 PDT
Created attachment 133700 [details]
Patch

Uploading for EWS bots - some tests might need rebaseline
Comment 2 WebKit Review Bot 2012-03-25 16:26:09 PDT
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 3 WebKit Review Bot 2012-03-25 16:53:54 PDT
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
Comment 4 WebKit Review Bot 2012-03-25 16:54:02 PDT
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
Comment 5 Victor Carbune 2012-03-27 05:59:52 PDT
Created attachment 134039 [details]
Updated patch and added test
Comment 6 WebKit Review Bot 2012-03-27 06:53:14 PDT
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
Comment 7 WebKit Review Bot 2012-03-27 06:53:21 PDT
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 8 Eric Carlson 2012-03-27 07:14:23 PDT
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?
Comment 9 Victor Carbune 2012-03-28 09:35:23 PDT
Created attachment 134315 [details]
Removed pixel test and added text test
Comment 10 Victor Carbune 2012-03-28 09:43:41 PDT
(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 11 Eric Carlson 2012-03-28 10:09:26 PDT
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?
Comment 12 Radar WebKit Bug Importer 2012-03-28 10:13:26 PDT
<rdar://problem/11139107>
Comment 13 Victor Carbune 2012-03-28 10:31:03 PDT
Created attachment 134328 [details]
Fixed nits
Comment 14 Victor Carbune 2012-03-28 10:33:39 PDT
(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 15 WebKit Review Bot 2012-03-28 18:07:36 PDT
Comment on attachment 134328 [details]
Fixed nits

Clearing flags on attachment: 134328

Committed r112484: <http://trac.webkit.org/changeset/112484>
Comment 16 WebKit Review Bot 2012-03-28 18:08:06 PDT
All reviewed patches have been landed.  Closing bug.
Comment 17 Enrica Casucci 2012-03-29 10:41:05 PDT
This broke the tests on all the Mac builds. I've added the failing tests to platform/mac/test_expectations.txt.
Comment 18 Enrica Casucci 2012-03-29 10:46:53 PDT
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.
Comment 19 Victor Carbune 2012-04-05 06:36:28 PDT
Last patch has been rolled out because of mac regressions. Marking this as reopened.
Comment 20 Victor Carbune 2012-04-05 07:30:27 PDT
Created attachment 135820 [details]
Changed patch to work with Safari
Comment 21 Victor Carbune 2012-04-06 06:12:38 PDT
Created attachment 136015 [details]
Fixed z-index for volume button on Safari
Comment 22 Eric Carlson 2012-04-06 10:55:28 PDT
Comment on attachment 136015 [details]
Fixed z-index for volume button on Safari

Thanks Victor!
Comment 23 WebKit Review Bot 2012-04-09 13:10:57 PDT
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>
Comment 24 WebKit Review Bot 2012-04-09 13:11:28 PDT
All reviewed patches have been landed.  Closing bug.
Comment 25 Dirk Pranke 2012-04-09 14:03:07 PDT
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.
Comment 26 Beth Dakin 2012-04-09 16:06:57 PDT
The Mac tests are failing again. Do they just need a new baseline?
Comment 27 Beth Dakin 2012-04-09 16:27:08 PDT
Sorry, I had to roll out this patch again because of the failures:

https://bugs.webkit.org/show_bug.cgi?id=83522
Comment 28 Beth Dakin 2012-04-09 16:28:46 PDT
Re-opening since this was rolled out.
Comment 29 Victor Carbune 2012-04-10 07:09:18 PDT
Created attachment 136449 [details]
Same patch. New baselines for Mac.
Comment 30 Victor Carbune 2012-04-10 07:25:39 PDT
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 31 Dirk Pranke 2012-04-10 15:18:05 PDT
Comment on attachment 136449 [details]
Same patch. New baselines for Mac.

looks fine by me as far as test_expectations.txt goes.
Comment 32 Philippe Normand 2012-04-12 07:21:26 PDT
I'll check the GTK media tests with this patch
Comment 33 Philippe Normand 2012-04-19 17:25:17 PDT
Created attachment 138011 [details]
GTK media controls update + rebaseline
Comment 34 Victor Carbune 2012-04-20 11:05:59 PDT
Created attachment 138121 [details]
Merged patch
Comment 35 WebKit Review Bot 2012-04-20 17:14:57 PDT
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
Comment 36 Victor Carbune 2012-04-22 15:25:43 PDT
(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!
Comment 37 Victor Carbune 2012-04-23 06:41:42 PDT
Created attachment 138337 [details]
Rebased patch
Comment 38 WebKit Review Bot 2012-04-23 15:41:22 PDT
Comment on attachment 138337 [details]
Rebased patch

Clearing flags on attachment: 138337

Committed r114957: <http://trac.webkit.org/changeset/114957>
Comment 39 WebKit Review Bot 2012-04-23 15:42:16 PDT
All reviewed patches have been landed.  Closing bug.
Comment 40 Alexey Proskuryakov 2012-06-08 12:25:28 PDT
This has caused bug 88615.
Comment 41 Eric Carlson 2012-06-08 12:30:04 PDT
(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.