[Modern Media Controls] Media Controller: mute support

[Antoine Quint]

We need media controller support for the mute button and when the media is muted via the media API.

[Radar WebKit Bug Importer]

<rdar://problem/28851582>

[Antoine Quint]

Created attachment 292094 [details]
Patch

[Dean Jackson]

Comment on attachment 292094 [details]
Patch

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

> Source/WebCore/ChangeLog:11
> +        We introduce the MuteSupport class which brings support for muting the media
> +        by clicking on the mute button in the media controls and correctly reflecting
> +        the media's muted state should the media be muted via the media API.

I understand this is the overall goal, but it seems overkill that muting, which is just a toggle button, requires its own class. It only does two things:

1. this.mediaController.media.muted = !this.mediaController.media.muted
2. this.control.muted = this.mediaController.media.muted;

This is basically the same as the play button. And the skip back button only has one task.

Is there a way to simplify all this so you're not making new classes (and files) for all these basic tasks? Airplay, fullscreen, etc are more complicated, so that makes sense.

[Antoine Quint]

(In reply to comment #3)
> Comment on attachment 292094 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=292094&action=review
> 
> > Source/WebCore/ChangeLog:11
> > +        We introduce the MuteSupport class which brings support for muting the media
> > +        by clicking on the mute button in the media controls and correctly reflecting
> > +        the media's muted state should the media be muted via the media API.
> 
> I understand this is the overall goal, but it seems overkill that muting,
> which is just a toggle button, requires its own class. It only does two
> things:
> 
> 1. this.mediaController.media.muted = !this.mediaController.media.muted
> 2. this.control.muted = this.mediaController.media.muted;
> 
> This is basically the same as the play button. And the skip back button only
> has one task.
> 
> Is there a way to simplify all this so you're not making new classes (and
> files) for all these basic tasks? Airplay, fullscreen, etc are more
> complicated, so that makes sense.

It's a relatively basic task and I believe it remains basic. What got the previous codebase so convoluted is how everything was lumped together, event handlers for a given event accomplishing many different things. I don't think adding a file is a big deal… although I did forget, in this patch and the previous patch, to add the file to the WebCore project and the RenderThemeMac::mediaControlsScript() method.

[Dean Jackson]

> > I understand this is the overall goal, but it seems overkill that muting,
> > which is just a toggle button, requires its own class. It only does two
> > things:
> > 
> > 1. this.mediaController.media.muted = !this.mediaController.media.muted
> > 2. this.control.muted = this.mediaController.media.muted;
> > 
> > This is basically the same as the play button. And the skip back button only
> > has one task.
> > 
> > Is there a way to simplify all this so you're not making new classes (and
> > files) for all these basic tasks? Airplay, fullscreen, etc are more
> > complicated, so that makes sense.
> 
> It's a relatively basic task and I believe it remains basic. What got the
> previous codebase so convoluted is how everything was lumped together, event
> handlers for a given event accomplishing many different things. I don't
> think adding a file is a big deal… although I did forget, in this patch and
> the previous patch, to add the file to the WebCore project and the
> RenderThemeMac::mediaControlsScript() method.

I wonder if we can have a shortcut though. Instead of just new MuteSupport()... maybe we could have:

new ControlsSupport({
  events: "volumechange",
  handler: (event) => { this.media.muted = !this.media.muted; },
  sync: () => { this.control.muted = this.media.muted }
});

... or whatever the right names are.

[Antoine Quint]

(In reply to comment #5)
> > > I understand this is the overall goal, but it seems overkill that muting,
> > > which is just a toggle button, requires its own class. It only does two
> > > things:
> > > 
> > > 1. this.mediaController.media.muted = !this.mediaController.media.muted
> > > 2. this.control.muted = this.mediaController.media.muted;
> > > 
> > > This is basically the same as the play button. And the skip back button only
> > > has one task.
> > > 
> > > Is there a way to simplify all this so you're not making new classes (and
> > > files) for all these basic tasks? Airplay, fullscreen, etc are more
> > > complicated, so that makes sense.
> > 
> > It's a relatively basic task and I believe it remains basic. What got the
> > previous codebase so convoluted is how everything was lumped together, event
> > handlers for a given event accomplishing many different things. I don't
> > think adding a file is a big deal… although I did forget, in this patch and
> > the previous patch, to add the file to the WebCore project and the
> > RenderThemeMac::mediaControlsScript() method.
> 
> I wonder if we can have a shortcut though. Instead of just new
> MuteSupport()... maybe we could have:
> 
> new ControlsSupport({
>   events: "volumechange",
>   handler: (event) => { this.media.muted = !this.media.muted; },
>   sync: () => { this.control.muted = this.media.muted }
> });
> 
> ... or whatever the right names are.

That's fine for this specific class, the way it is now. But I don't think it's worth it. A new file keeps things simple. Where is the code related to dealing with mute? I know: the MuteSupport file and class! All features are dealt with the same way, each with their own MediaControllerSupport class and file. It's really kind of a template, there is no real added complexity. I really wouldn't like to treat different kind of media features in different ways for the media controller, it helps keeping things in a strict framework so it doesn't become a mess like the old code did.

[Dean Jackson]

(In reply to comment #6)

> That's fine for this specific class, the way it is now. But I don't think
> it's worth it. A new file keeps things simple. Where is the code related to
> dealing with mute? I know: the MuteSupport file and class! All features are
> dealt with the same way, each with their own MediaControllerSupport class
> and file. It's really kind of a template, there is no real added complexity.
> I really wouldn't like to treat different kind of media features in
> different ways for the media controller, it helps keeping things in a strict
> framework so it doesn't become a mess like the old code did.

OK. I think it should be MuteButtonSupport then.

[Antoine Quint]

Created attachment 292154 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 292154 [details]
Patch for landing

Clearing flags on attachment: 292154

Committed r207587: <http://trac.webkit.org/changeset/207587>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.