Bug 87835

Created attachment 144962 [details] Full patch for tests

Created attachment 144964 [details] Review this for the differences to the first patch Review this for the differences to the first patch. This is the one that needs to be committed ultimately after the patch at https://bugs.webkit.org/show_bug.cgi?id=87683 .

Comment on attachment 144964 [details] Review this for the differences to the first patch Attachment 144964 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12861195 New failing tests: media/media-document-audio-repaint.html fullscreen/full-screen-stacking-context.html media/video-no-audio.html media/controls-strict.html media/controls-styling.html media/video-volume-slider.html media/video-display-toggle.html media/audio-repaint.html media/media-volume-slider-rendered-below.html media/audio-controls-rendering.html media/video-zoom-controls.html http/tests/media/video-buffered-range-contains-currentTime.html media/video-controls-rendering.html media/media-volume-slider-rendered-normal.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-layout-direction.html media/controls-after-reload.html

Created attachment 145001 [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 144962 [details] Full patch for tests Attachment 144962 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12862257 New failing tests: http/tests/media/video-buffered-range-contains-currentTime.html media/track/track-cue-rendering-snap-to-lines-not-set.html media/media-volume-slider-rendered-normal.html media/media-volume-slider-rendered-below.html

Created attachment 145053 [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 144962 [details] Full patch for tests View in context: https://bugs.webkit.org/attachment.cgi?id=144962&action=review This looks fine, modulo the nits noted. > Source/WebCore/ChangeLog:14 > +2012-05-29 Silvia Pfeiffer <silviapf@chromium.org> > + > + Change the volume slider to horizontal rendering for Chrome video controls. > + https://bugs.webkit.org/show_bug.cgi?id=87835 > + > + Reviewed by NOBODY (OOPS!). > + > + No new tests. (visual update will contain rebaselined tests) > + > + The Chrome video controls are receiving a visual update. The volume slider is included into > + the controls with horizontal rendering, the volume slider container is removed. The visual > + update itself is in a separate patch. > + > + * css/mediaControlsChromium.css: You have... > Source/WebCore/ChangeLog:70 > +2012-05-28 Silvia Pfeiffer <silviapf@chromium.org> > + > + Introduce an Enclosure Element for Chromium video controls. > + https://bugs.webkit.org/show_bug.cgi?id=87683 > + > + Reviewed by NOBODY (OOPS!). > + > + Updated tests. > + > + The Chrome video controls are receiving a visual update. A new enclosure div is required > + to provide for a offset space from the video's boundaries. The visual update itself is in > + a separate patch. ... two ChangeLog entries in this patch. > LayoutTests/ChangeLog:12 > + Fail video controls tests in test expectations. Don't you mean you are temporarily skipping the tests?

Created attachment 145210 [details] Fix and ignore some further tests

Created attachment 145211 [details] Use this for review Review this for the difference to the patch in https://bugs.webkit.org/show_bug.cgi?id=87683 .

(In reply to comment #7) > > You have... > > ... two ChangeLog entries in this patch. Yes, that's the full patch that includes the patch from https://bugs.webkit.org/show_bug.cgi?id=87683 . I have the ChangeLog entry from there plus the one from here. How do you envision this landing? As one integrated patch or each one separately? I thought the latter, which is why I'm preparing a CL for each. > > LayoutTests/ChangeLog:12 > > + Fail video controls tests in test expectations. > > Don't you mean you are temporarily skipping the tests? That's a much better wording! :-) I'm pretty sure I will have to update all the patches again with the latest codebase before cq+, so will add then. For now I'd just like to get feedback on the code changes. Thanks!

Created attachment 145697 [details] Fix changelog comment and remove volume hiding when panel already does the hiding

Created attachment 145698 [details] Use this for review use this for review

Comment on attachment 145698 [details] Use this for review View in context: https://bugs.webkit.org/attachment.cgi?id=145698&action=review > Source/WebCore/ChangeLog:8 > + No new tests. (visual update will contain rebaselined tests) Nit: the parenthetical comment should be part of the sentence (move the period to the end of the line). > Source/WebCore/ChangeLog:11 > + The Chrome video controls are receiving a visual update. The volume slider is included into > + the controls with horizontal rendering, the volume slider container is removed. The visual Nit: "... is included into the controls" -> "... is included in the controls" or "... is moved into the controls" depending on what exactly you mean.

(In reply to comment #13) > (From update of attachment 145698 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=145698&action=review > > > Source/WebCore/ChangeLog:8 > > + No new tests. (visual update will contain rebaselined tests) > > Nit: the parenthetical comment should be part of the sentence (move the period to the end of the line). > > > Source/WebCore/ChangeLog:11 > > + The Chrome video controls are receiving a visual update. The volume slider is included into > > + the controls with horizontal rendering, the volume slider container is removed. The visual > > Nit: "... is included into the controls" -> "... is included in the controls" or "... is moved into the controls" depending on what exactly you mean. Thanks, I'll fix these in a forthcoming patch for landing rebased on latest codebase.

Created attachment 147014 [details] Patch for cq

Comment on attachment 147014 [details] Patch for cq Clearing flags on attachment: 147014 Committed r120072: <http://trac.webkit.org/changeset/120072>

Patch landed

(In reply to comment #17) > Patch landed It broke all !ENABLE(VIDEO) builds, new bug filed on it: https://bugs.webkit.org/show_bug.cgi?id=88881