Bug 85990

Created attachment 140931 [details] Preliminary patch

Comment on attachment 140931 [details] Preliminary patch Attachment 140931 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12645690

Comment on attachment 140931 [details] Preliminary patch View in context: https://bugs.webkit.org/attachment.cgi?id=140931&action=review > Source/WebCore/html/shadow/MediaControlElements.cpp:375 > +void MediaControlVolumeSliderContainerElement::displayBelowMuteButton(bool status) > +{ > + DEFINE_STATIC_LOCAL(AtomicString, classAttribute, ("class")); > + DEFINE_STATIC_LOCAL(AtomicString, classAttributeValue, ("webkit-media-controls-volume-slider-below")); > + ExceptionCode ec; > + > + setAttribute(classAttribute, classAttributeValue, ec); > + setNeedsStyleRecalc(); > +} Don't you need to do something with "status"? > Source/WebCore/html/shadow/MediaControlElements.cpp:380 > +RenderObject* MediaControlVolumeSliderContainerElement::createRenderer(RenderArena *arena, RenderStyle *style) > +{ > + return new (arena) RenderMediaVolumeSliderContainer(this); > +} "style" isn't used so this will fail to compile on Mac/Windows.

Comment on attachment 140931 [details] Preliminary patch View in context: https://bugs.webkit.org/attachment.cgi?id=140931&action=review I don't like this. We shouldn't be mutating DOM inside layout. > Source/WebCore/css/mediaControlsChromium.css:178 > +::-webkit-media-controls * div[class="webkit-media-controls-volume-slider-below"] { Why do you need the * selector there? > Source/WebCore/html/shadow/MediaControlElements.cpp:351 > + slider->displayBelowMuteButton(true); Mutating DOM inside layout seems like a hideous violation of layering to me.

Created attachment 141049 [details] Added back custom rendering

(In reply to comment #4) > (From update of attachment 140931 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=140931&action=review > > I don't like this. We shouldn't be mutating DOM inside layout. > > > Source/WebCore/css/mediaControlsChromium.css:178 > > +::-webkit-media-controls * div[class="webkit-media-controls-volume-slider-below"] { > > Why do you need the * selector there? The mute button and volume slider are in the same <div> container (and not direct children of the panel element. I suggest we keep this for two reasons: i) we can let css position the slider within the <div> container in most of the cases ii) within the layout() method the positioning is done only on the y-axis, when it's needed to be rendered below. > > Source/WebCore/html/shadow/MediaControlElements.cpp:351 > > + slider->displayBelowMuteButton(true); > > Mutating DOM inside layout seems like a hideous violation of layering to me. It was a possible alternative that I initially thought it deserves some attention. My bad.

Comment on attachment 141049 [details] Added back custom rendering I need to run and see how this looks on a mac machine before we can check this in.

Created attachment 141160 [details] Updated patch

Comment on attachment 141160 [details] Updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=141160&action=review This looks fine to me modulo the minor comments, but it would probably be good to get a nod from dglazkov as well since he had strong feelings about the previous version. > Source/WebCore/ChangeLog:15 > + Changed positioning of the slider to absolute, otherwise it is possible to position it from the layout() method. Nit: do you mean "otherwise it is *not* possible"? > Source/WebCore/html/shadow/MediaControlElements.cpp:263 > + // Setting the display:none property on controls is not working > + // because localToAbsolute is behaving differently(?!) > + > + // startTimer(); I think it would be better to just remove the comment and code. You and I won't forget about it just because it isn't here :-( > LayoutTests/media/media-volume-slider-rendered-below.html:17 > + function test() { Nit: a function's opening brace should be on its own line. > LayoutTests/media/media-volume-slider-rendered-below.html:29 > + consoleWrite("ERROR: unable to get controls coordinates."); > + > + layoutTestController.notifyDone(); Nit: You could simplify slightly this by using failTest(). > LayoutTests/media/media-volume-slider-rendered-below.html:48 > + function initialize() { Nit: the brace should be on a new line.

Created attachment 141205 [details] Updated patch

Comment on attachment 141205 [details] Updated patch ok.

Created attachment 141216 [details] Rebased patch

Comment on attachment 141216 [details] Rebased patch Bad rebase, sorry.

Created attachment 141230 [details] Patch for landing

Comment on attachment 141230 [details] Patch for landing Rejecting attachment 141230 [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: controls-strict-expected.txt patching file LayoutTests/platform/mac/media/video-controls-rendering-expected.txt patching file LayoutTests/platform/mac/media/video-display-toggle-expected.txt patching file LayoutTests/platform/mac/media/video-playing-and-pause-expected.txt patching file LayoutTests/platform/mac/test_expectations.txt Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Eric Carls..." exit_code: 1 cwd: /mnt/git/webkit-commit-queue/ Full output: http://queues.webkit.org/results/12648966

Created attachment 141257 [details] PaPatch Seems I wrote 89950 instead of 85990 in test expectations. Corrected.

Created attachment 141263 [details] Another rebase I'm not sure why the CQ fails to apply this patch

Comment on attachment 141263 [details] Another rebase Rejecting attachment 141263 [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: file LayoutTests/platform/mac/media/video-controls-rendering-expected.txt patching file LayoutTests/platform/mac/media/video-display-toggle-expected.txt patching file LayoutTests/platform/mac/media/video-playing-and-pause-expected.txt patching file LayoutTests/platform/mac/test_expectations.txt Hunk #1 succeeded at 204 with fuzz 1. Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Eric Carls..." exit_code: 1 cwd: /mnt/git/webkit-commit-queue/ Full output: http://queues.webkit.org/results/12678663

Created attachment 141609 [details] Patch for landing

Comment on attachment 141609 [details] Patch for landing Rejecting attachment 141609 [details] from commit-queue. victor@rosedu.org does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py. - If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags. - If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed). The commit-queue restarts itself every 2 hours. After restart the commit-queue will correctly respect your committer rights.

Comment on attachment 141609 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=141609&action=review > Source/WebCore/ChangeLog:6 > + Reviewed by NOBODY (OOPS!). This will be filled in by webkit-patch land-safely (but only if you have an actual active r+ on the bug). Don't let it be cleared by webkit-patch upload

Comment on attachment 141609 [details] Patch for landing Clearing flags on attachment: 141609 Committed r116900: <http://trac.webkit.org/changeset/116900>

All reviewed patches have been landed. Closing bug.

This patch caused the EFL bots to fail media/video-controls-rendering-toggle-display-none.html and media/video-controls-toggling.html, I've skipped them for now. It looks like the commit has caused failures for GTK+ as well.

(In reply to comment #24) > This patch caused the EFL bots to fail media/video-controls-rendering-toggle-display-none.html and media/video-controls-toggling.html, I've skipped them for now. It looks like the commit has caused failures for GTK+ as well. See bug 86328

(In reply to comment #24) > This patch caused the EFL bots to fail media/video-controls-rendering-toggle-display-none.html and media/video-controls-toggling.html, I've skipped them for now. It looks like the commit has caused failures for GTK+ as well. This is, unfortunately, expected; that's why I have marked the tests as failing in Mac and Chromium at this time. Seems that after toggling "display:none", localToAbsolute returns different coordinates. I'm trying to find out what's the cause of this. Thanks for marking them accordingly Raphael, Philippe.

(In reply to comment #26) > (In reply to comment #24) > > This patch caused the EFL bots to fail media/video-controls-rendering-toggle-display-none.html and media/video-controls-toggling.html, I've skipped them for now. It looks like the commit has caused failures for GTK+ as well. > > This is, unfortunately, expected; that's why I have marked the tests as failing in Mac and Chromium at this time. > > Seems that after toggling "display:none", localToAbsolute returns different coordinates. I'm trying to find out what's the cause of this. I tried figuring out what's happening there, but this seems to be non-trivial. The easiest solution I could come with is to hide the volume slider each time the controls are hidden (by JS or fadeout). > Thanks for marking them accordingly Raphael, Philippe. I have un-marked them in the same patch containing the changes mentioned above (https://bugs.webkit.org/show_bug.cgi?id=87591)