Bug 97192 – [GTK] no volume slider in HTML5 media element controls

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Philippe Normand]

run-launcher --gtk LayoutTests/media/content/test.mp4

Should show the volume slider when clicking on the controls volume button. Right now it toggles mute state.

[Philippe Normand]

I wonder if this regression was actually introduced by Bug 82150.

Also, as a quick test to do (that I haven't tried), what happens if you make MediaPlayerPrivateGStreamer::hasAudio() always return true? Asking because MediaControlRootElement::showVolumeSlider() checks for it.

[Xabier Rodríguez Calvar]

(In reply to comment #1)
> I wonder if this regression was actually introduced by Bug 82150.
> 
> Also, as a quick test to do (that I haven't tried), what happens if you make MediaPlayerPrivateGStreamer::hasAudio() always return true? Asking because MediaControlRootElement::showVolumeSlider() checks for it.

After reverting that and trying, it looks like it is not.

[Xabier Rodríguez Calvar]

Created attachment 175000 [details]
Partially reverting r115829, new baseline and updated test expectations

There as a problem caused by r115829 that prevented the volume slider from showing. I reverted the commit partially and updated the tests.

[Philippe Normand]

Comment on attachment 175000 [details]
Partially reverting r115829, new baseline and updated test expectations

View in context: https://bugs.webkit.org/attachment.cgi?id=175000&action=review

Looks good, just a nit to fix :) We need to sync with Gustavo when landing this patch though, so no cq for this time.

> LayoutTests/platform/gtk-wk2/TestExpectations:589
> +Bug(GTK) media/track/track-cue-container-rendering-position.html [ Failure ]

It'd be nice to open a bug ASAP and mention it in the test expectation. So it's not forgotten after.

> LayoutTests/platform/gtk/TestExpectations:1394
> +Bug(GTK) media/media-document-audio-repaint.html [ Failure ]
> +Bug(GTK) media/track/track-cue-container-rendering-position.html [ Failure ]
> +Bug(GTK) media/track/track-cue-rendering-horizontal.html [ Failure ]
> +Bug(GTK) media/track/track-cue-rendering-vertical.html [ Failure ]

Ditto!

[Philippe Normand]

+Gustavo, we'll need a clean build for this patch. Be prepared! Tomorrow maybe?

[Xabier Rodríguez Calvar]

Created attachment 175185 [details]
Partially reverting r115829, new baseline and updated test expectations

(In reply to comment #4)
> (From update of attachment 175000 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=175000&action=review
> 
> Looks good, just a nit to fix :) We need to sync with Gustavo when landing this patch though, so no cq for this time.

Roger that.

> > LayoutTests/platform/gtk-wk2/TestExpectations:589
> > +Bug(GTK) media/track/track-cue-container-rendering-position.html [ Failure ]
> 
> It'd be nice to open a bug ASAP and mention it in the test expectation. So it's not forgotten after.
> 
> > LayoutTests/platform/gtk/TestExpectations:1394
> > +Bug(GTK) media/media-document-audio-repaint.html [ Failure ]
> > +Bug(GTK) media/track/track-cue-container-rendering-position.html [ Failure ]
> > +Bug(GTK) media/track/track-cue-rendering-horizontal.html [ Failure ]
> > +Bug(GTK) media/track/track-cue-rendering-vertical.html [ Failure ]
> 
> Ditto!

Done.

Something that I had forgotten to say: please, have special look at the expectations for media/video-volume-slider as  it has some subtantial changes, but I think they are logical because now it should be showing the slider.

[Philippe Normand]

Committed r135273: <http://trac.webkit.org/changeset/135273>

[Philippe Normand]

Comment on attachment 175185 [details]
Partially reverting r115829, new baseline and updated test expectations

Heh seems like I forgot to set r+ before landing, but I did the review for real :)