Bug 225227 - Use OptionSet for MediaProducer::MediaStateFlags
Summary: Use OptionSet for MediaProducer::MediaStateFlags
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebRTC (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-04-30 05:29 PDT by youenn fablet
Modified: 2021-05-04 03:05 PDT (History)
16 users (show)

See Also:


Attachments
Patch (55.55 KB, patch)
2021-05-03 01:13 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch for landing (55.56 KB, patch)
2021-05-04 01:30 PDT, youenn fablet
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description youenn fablet 2021-04-30 05:29:12 PDT
Use OptionSet for MediaProducer::MediaStateFlags
Comment 1 youenn fablet 2021-05-03 01:13:42 PDT
Created attachment 427543 [details]
Patch
Comment 2 youenn fablet 2021-05-03 08:17:27 PDT
Comment on attachment 427543 [details]
Patch

bot failure is unrelated
Comment 3 Eric Carlson 2021-05-03 09:15:27 PDT
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.
Comment 4 youenn fablet 2021-05-04 01:28:38 PDT
(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.
Comment 5 youenn fablet 2021-05-04 01:30:56 PDT
Created attachment 427642 [details]
Patch for landing
Comment 6 EWS 2021-05-04 03:04:39 PDT
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].
Comment 7 Radar WebKit Bug Importer 2021-05-04 03:05:19 PDT
<rdar://problem/77496454>