HTMLMediaElement::setVolume should updateIsPlayingMedia

[Gavin Barraclough]

HTMLMediaElement::mediaState takes the volume into account when determining whether media is playing (audio is not considered to be playing if volume is 0). As such, and change to the volume may require mediaState to be recomputed.

[Gavin Barraclough]

Created attachment 295581 [details]
Fix

[Darin Adler]

Comment on attachment 295581 [details]
Fix

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

Might be OK, but what about in loadResource, mediaPlayerVolumeChanged, mediaVolumeDidChange, pageMutedStateDidChange, and setShouldDuck? Maybe this should go into the updateVolume function instead?

I’ll say r=me but please consider that.

> Source/WebCore/ChangeLog:9
> +        is playing (audio is not considered to be playing if volume is 0). As such, and change to

typo "and" for "any"

> Source/WebCore/html/HTMLMediaElement.cpp:3247
>      m_volumeInitialized = true;

Follow-up idea: We should make m_volume be std::optional<double> instead of using a double plus a boolean.

> Source/WebCore/html/HTMLMediaElement.cpp:3255
> +#if ENABLE(MEDIA_SESSION)
> +    document().updateIsPlayingMedia(m_elementID);
> +#else
> +    document().updateIsPlayingMedia();
> +#endif

Follow-up idea: We should create a helper member function to do this so we don’t have to repeat this five-line sequence in five different places in the class. Can be inline if we like.

[Gavin Barraclough]

(In reply to comment #2)
> Comment on attachment 295581 [details]
> Fix
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=295581&action=review
> 
> Might be OK, but what about in loadResource, mediaPlayerVolumeChanged,
> mediaVolumeDidChange, pageMutedStateDidChange, and setShouldDuck? Maybe this
> should go into the updateVolume function instead?
> 
> I’ll say r=me but please consider that.

Have made this change.

> > Source/WebCore/ChangeLog:9
> > +        is playing (audio is not considered to be playing if volume is 0). As such, and change to
> 
> typo "and" for "any"
> 
> > Source/WebCore/html/HTMLMediaElement.cpp:3247
> >      m_volumeInitialized = true;
> 
> Follow-up idea: We should make m_volume be std::optional<double> instead of
> using a double plus a boolean.

Will look at this as a followup.

> > Source/WebCore/html/HTMLMediaElement.cpp:3255
> > +#if ENABLE(MEDIA_SESSION)
> > +    document().updateIsPlayingMedia(m_elementID);
> > +#else
> > +    document().updateIsPlayingMedia();
> > +#endif
> 
> Follow-up idea: We should create a helper member function to do this so we
> don’t have to repeat this five-line sequence in five different places in the
> class. Can be inline if we like.

If we want to clean up here, with a plan to keep the MEDIA_SESSION code, then I think we could just remove the ifdef & keep the version that passes the argument. (In the callee I think it's unused anyway if MEDIA_SESSION is compiled out.)

Discussed with Jer & he recommends holding off on cleanup here for now - thinks MEDIA_SESSION code paths probably need to be removed, and can more cleanly be completely removed as is.

[Gavin Barraclough]

Committed revision 209084.