[EFL][GTK] Volume slider only changes volume when thumb is released, not while dragging

[Hunseop Jeong]

Some media tests didn't invoke the 'volumechange' event when dragging the volume slider in built-in media controls.

[Hunseop Jeong]

'mediaControlsApple.js' already fixed bug in https://bugs.webkit.org/attachment.cgi?id=240041.

[Hunseop Jeong]

Created attachment 277224 [details]
Patch

[Xabier Rodríguez Calvar]

(In reply to comment #0)
> Some media tests didn't invoke the 'volumechange' event when dragging the
> volume slider in built-in media controls.

Could you please let us know which are the affected tests?

[Hunseop Jeong]

(In reply to comment #3)
> (In reply to comment #0)
> > Some media tests didn't invoke the 'volumechange' event when dragging the
> > volume slider in built-in media controls.
> 
> Could you please let us know which are the affected tests?

Not some, only media/video-volume-slider-drag.html.

[Xabier Rodríguez Calvar]

(In reply to comment #4)
> Not some, only media/video-volume-slider-drag.html.

Ok, let me do some tests and then I'll review.

[Xabier Rodríguez Calvar]

Comment on attachment 277224 [details]
Patch

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

> Source/WebCore/ChangeLog:3
> +        [EFL] Volume slider only changes volume when thumb is released, not while dragging

GTK is affected too so you should mark [GTK] as well.

> Source/WebCore/ChangeLog:23
> +        * Modules/mediacontrols/mediaControlsBase.js:
> +        (Controller.prototype.createControls):
> +        (Controller.prototype.handlePlayButtonClicked):
> +        (Controller.prototype.handleTimelineInput):
> +        (Controller.prototype.handleTimelineChange):
> +        (Controller.prototype.handleTimelineDown):
> +        (Controller.prototype.handleTimelineMouseUp):
> +        (Controller.prototype.handleMuteButtonClicked):
> +        (Controller.prototype.handleMaxButtonClicked):
> +        (Controller.prototype.handleVolumeSliderInput):
> +        (Controller.prototype.handleVolumeSliderChange): Deleted.

We would need a more complete general explanation or explain the details here.

> Source/WebCore/Modules/mediacontrols/mediaControlsBase.js:343
> -        this.listenFor(timeline, 'input', this.handleTimelineChange);
> +        this.listenFor(timeline, 'input', this.handleTimelineInput);
> +        this.listenFor(timeline, 'change', this.handleTimelineChange);

These changes seem unrelated to volume. Why are they here?

> Source/WebCore/Modules/mediacontrols/mediaControlsBase.js:737
> -    handleTimelineChange: function(event)
> +    handleTimelineInput: function(event)
>      {
>          this.video.fastSeek(this.controls.timeline.value);
>      },
>  
> +    handleTimelineChange: function(event)
> +    {
> +        this.video.currentTime = this.controls.timeline.value;
> +    },
> +    

Ditto.

> Source/WebCore/Modules/mediacontrols/mediaControlsBase.js:-793
> -
> -        // Do a precise seek when we lift the mouse:
> -        this.video.currentTime = this.controls.timeline.value;

Ditto

[Hunseop Jeong]

Created attachment 277438 [details]
Patch

[Hunseop Jeong]

Comment on attachment 277224 [details]
Patch

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

>> Source/WebCore/Modules/mediacontrols/mediaControlsBase.js:343
>> +        this.listenFor(timeline, 'change', this.handleTimelineChange);
> 
> These changes seem unrelated to volume. Why are they here?

You are right. That is related with timeline. I just moved the codes(https://bugs.webkit.org/show_bug.cgi?id=137805).

[Xabier Rodríguez Calvar]

Comment on attachment 277438 [details]
Patch

Good that you removed the failing expectations too. Nice!

[Hunseop Jeong]

Comment on attachment 277438 [details]
Patch

Thanks for the review.

[WebKit Commit Bot]

Comment on attachment 277438 [details]
Patch

Clearing flags on attachment: 277438

Committed r200126: <http://trac.webkit.org/changeset/200126>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.