Bug 87835 – Change the volume slider to horizontal rendering for Chrome video controls

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Silvia Pfeiffer]

This is a second patch for the introduction of the new Chromium video controls, see https://bugs.webkit.org/show_bug.cgi?id=84672 .

It includes the change of the volume slider from a vertical to a horizontal layout and inclusion into the video controls of Chrome.
The actual visual update is still to come in a separate patch.

Not to land separately and to land after 87683.

[Silvia Pfeiffer]

Created attachment 144962 [details]
Full patch for tests

[Silvia Pfeiffer]

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 .

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Eric Carlson]

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
> [phone]  Silvia Pfeiffer  <[e-mail]>
> +
> +        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
> [phone]  Silvia Pfeiffer  <[e-mail]>
> +
> +        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?

[Silvia Pfeiffer]

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

[Silvia Pfeiffer]

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 .

[Silvia Pfeiffer]

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

[Silvia Pfeiffer]

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

[Silvia Pfeiffer]

Created attachment 145698 [details]
Use this for review

use this for review

[Eric Carlson]

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.

[Silvia Pfeiffer]

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

[Silvia Pfeiffer]

Created attachment 147014 [details]
Patch for cq

[WebKit Review Bot]

Comment on attachment 147014 [details]
Patch for cq

Clearing flags on attachment: 147014

Committed r120072: <http://trac.webkit.org/changeset/120072>

[Silvia Pfeiffer]

Patch landed

[Csaba Osztrogonác]

(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