Summary: | Use OptionSet for MediaProducer::MediaStateFlags | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | youenn fablet <youennf> | ||||||
Component: | WebRTC | Assignee: | youenn fablet <youennf> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | calvaris, cdumez, changseok, eric.carlson, esprehn+autocc, ews-watchlist, glenn, gyuyoung.kim, hta, jer.noble, kangil.han, philipj, sergio, tommyw, webkit-bug-importer, youennf | ||||||
Priority: | P2 | Keywords: | InRadar | ||||||
Version: | WebKit Local Build | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
youenn fablet
2021-04-30 05:29:12 PDT
Created attachment 427543 [details]
Patch
Comment on attachment 427543 [details]
Patch
bot failure is unrelated
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. (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. Created attachment 427642 [details]
Patch for landing
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]. |