Bug 225227 – Use OptionSet for MediaProducer::MediaStateFlags

• Status: RESOLVED
• Resolution: FIXED
• Component: WebRTC

[youenn fablet]

Use OptionSet for MediaProducer::MediaStateFlags

[youenn fablet]

Created attachment 427543 [details]
Patch

[youenn fablet]

Comment on attachment 427543 [details]
Patch

bot failure is unrelated

[Eric Carlson]

Comment on attachment 427543 [details]
Patch

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

> Source/WebCore/Modules/airplay/WebMediaSessionManager.cpp:278
> +    MediaProducer::MediaStateFlags updateConfigurationFlags { MediaProducer::MediaState::RequiresPlaybackTargetMonitoring, MediaProducer::MediaState::HasPlaybackTargetAvailabilityListener, MediaProducer::MediaState::HasAudioOrVideo };

Not new to this refactoring, but this could be `static constexpr`

> Source/WebCore/Modules/airplay/WebMediaSessionManager.cpp:282
> +    MediaProducer::MediaStateFlags playingToTargetFlags { MediaProducer::MediaState::IsPlayingToExternalDevice, MediaProducer::MediaState::IsPlayingVideo };

Ditto

> Source/WebCore/html/HTMLMediaElement.cpp:7696
> +        if (hasMediaStreamSrcObject() && mediaState().containsAny(MediaProducer::MediaState::IsPlayingAudio) && document().mediaState().containsAny(MediaProducer::MediaState::HasActiveAudioCaptureDevice)) {

Nit: although `OptionSet::contains()` and `OptionSet::constainsAny()` are the same, it is slightly odd all throughout this patch you sometimes use `constainsAny` and other times use 'contains` to test for just one value.

[youenn fablet]

(In reply to Eric Carlson from comment #3)
> Comment on attachment 427543 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=427543&action=review
> 
> > Source/WebCore/Modules/airplay/WebMediaSessionManager.cpp:278
> > +    MediaProducer::MediaStateFlags updateConfigurationFlags { MediaProducer::MediaState::RequiresPlaybackTargetMonitoring, MediaProducer::MediaState::HasPlaybackTargetAvailabilityListener, MediaProducer::MediaState::HasAudioOrVideo };
> 
> Not new to this refactoring, but this could be `static constexpr`

Will fix.

> > Source/WebCore/Modules/airplay/WebMediaSessionManager.cpp:282
> > +    MediaProducer::MediaStateFlags playingToTargetFlags { MediaProducer::MediaState::IsPlayingToExternalDevice, MediaProducer::MediaState::IsPlayingVideo };
> 
> Ditto

OK

> > Source/WebCore/html/HTMLMediaElement.cpp:7696
> > +        if (hasMediaStreamSrcObject() && mediaState().containsAny(MediaProducer::MediaState::IsPlayingAudio) && document().mediaState().containsAny(MediaProducer::MediaState::HasActiveAudioCaptureDevice)) {
> 
> Nit: although `OptionSet::contains()` and `OptionSet::constainsAny()` are
> the same, it is slightly odd all throughout this patch you sometimes use
> `constainsAny` and other times use 'contains` to test for just one value.

contains take a MediaState, containsAny takes a MediaStateFlags. I try to follow the pattern here.

containsAny can also take a MediaState since a MediaStateFlags can be implicitly constructed so we could go with containsAny all over the place.

[youenn fablet]

Created attachment 427642 [details]
Patch for landing

[EWS]

Committed r276952 (237285@main): <https://commits.webkit.org/237285@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 427642 [details].

[Radar WebKit Bug Importer]

<rdar://problem/77496454>