Bug 163727 - [Modern Media Controls] Media Controller: volume control support
Summary: [Modern Media Controls] Media Controller: volume control support
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: Safari 10
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Antoine Quint
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2016-10-20 07:28 PDT by Antoine Quint
Modified: 2016-10-28 16:38 PDT (History)
3 users (show)

See Also:

Attachments
Patch (26.15 KB, patch)
2016-10-20 07:30 PDT, Antoine Quint 		no flags Details | Formatted Diff | Diff
Patch for landing (71.05 KB, patch)
2016-10-28 16:01 PDT, Antoine Quint 		no flags Details | Formatted Diff | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Antoine Quint 2016-10-20 07:28:07 PDT 
We need to update the media's volume as the user interacts with the volume control and update the volume control if the volume is changed via the media API.
Comment 1 Antoine Quint 2016-10-20 07:28:33 PDT 
<rdar://problem/27989482>
Comment 2 Antoine Quint 2016-10-20 07:30:17 PDT 
Created attachment 292178 [details]
Patch
Comment 3 Antoine Quint 2016-10-20 07:31:13 PDT 
Tests aren't ready yet but the code can already be reviewed.
Comment 4 Dean Jackson 2016-10-20 19:13:37 PDT 
Comment on attachment 292178 [details]
Patch

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

> Source/WebCore/ChangeLog:14
> +        * Modules/modern-media-controls/media/volume-support.js: Copied from Source/WebCore/Modules/modern-media-controls/media/media-controller.js.

Any ideas what to do with this line?

> Source/WebCore/Modules/modern-media-controls/media/volume-support.js:44
> +    controlValueWillStartChanging(control)
> +    {
> +        this.mediaController.media.muted = false;
> +    }

I think maybe we should set muted to true if we end up with a volume of zero.
Comment 5 Antoine Quint 2016-10-21 01:17:29 PDT 
(In reply to comment #4)
> Comment on attachment 292178 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=292178&action=review
> 
> > Source/WebCore/ChangeLog:14
> > +        * Modules/modern-media-controls/media/volume-support.js: Copied from Source/WebCore/Modules/modern-media-controls/media/media-controller.js.
> 
> Any ideas what to do with this line?

I'll have to dig deeper into this issue, will get back to you within 5 business days.

> > Source/WebCore/Modules/modern-media-controls/media/volume-support.js:44
> > +    controlValueWillStartChanging(control)
> > +    {
> > +        this.mediaController.media.muted = false;
> > +    }
> 
> I think maybe we should set muted to true if we end up with a volume of zero.

I wonder. In my mind the muted state and volume are not connected. Imagine that we set muted to true when volume = 0, then clicking the muted button to unmute, which simply toggles the muted property on the media, wouldn't change much of anything.
Comment 6 Antoine Quint 2016-10-28 16:01:44 PDT 
Created attachment 293238 [details]
Patch for landing
Comment 7 WebKit Commit Bot 2016-10-28 16:38:07 PDT 
Comment on attachment 293238 [details]
Patch for landing

Clearing flags on attachment: 293238

Committed r208080: <http://trac.webkit.org/changeset/208080>
Comment 8 WebKit Commit Bot 2016-10-28 16:38:10 PDT 
All reviewed patches have been landed.  Closing bug.