Summary: | [GPUP] WebContent process should not pull audio session category from the GPU Process | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Peng Liu <peng.liu6> | ||||||||
Component: | Media | Assignee: | Peng Liu <peng.liu6> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | darin, eric.carlson, ews-watchlist, glenn, hta, jer.noble, philipj, sergio, tommyw, webkit-bug-importer | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | Other | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=221985 | ||||||||||
Attachments: |
|
Description
Peng Liu
2021-05-14 14:36:51 PDT
Created attachment 428691 [details]
Patch
Created attachment 428706 [details]
Fix build failures on iOS
Comment on attachment 428706 [details] Fix build failures on iOS View in context: https://bugs.webkit.org/attachment.cgi?id=428706&action=review I think on balance changing this from an enum to an enum class is making things worse. > Source/WebCore/page/DeprecatedGlobalSettings.cpp:157 > + return (unsigned)AudioSession::sharedSession().categoryOverride(); It’s peculiar to use a C-style cast here. I suggest static_cast. > Source/WebCore/platform/audio/AudioSession.cpp:116 > + return return AudioSession::CategoryType::None; This says "return return", so I don’t think it will compile. > Source/WebCore/platform/audio/AudioSession.h:63 > - enum CategoryType : uint8_t { > + enum class CategoryType : uint8_t { Not sure this is an improvement. Does make it more wordy everywhere. Does it make the code more clear? I’m OK with it, but not sure. > Source/WebCore/platform/audio/cocoa/MediaSessionManagerCocoa.mm:134 > + AudioSession::CategoryType category = AudioSession::CategoryType::None; Could use auto here to make things a little less verbose. Why write AudioSession::CategoryType twice on the same line of code unnecessarily? Comment on attachment 428706 [details] Fix build failures on iOS View in context: https://bugs.webkit.org/attachment.cgi?id=428706&action=review >> Source/WebCore/page/DeprecatedGlobalSettings.cpp:157 >> + return (unsigned)AudioSession::sharedSession().categoryOverride(); > > It’s peculiar to use a C-style cast here. I suggest static_cast. Agree! Will fix it. >> Source/WebCore/platform/audio/AudioSession.cpp:116 >> + return return AudioSession::CategoryType::None; > > This says "return return", so I don’t think it will compile. Good catch! It is surprising that the bots did not complain. >> Source/WebCore/platform/audio/AudioSession.h:63 >> + enum class CategoryType : uint8_t { > > Not sure this is an improvement. Does make it more wordy everywhere. Does it make the code more clear? I’m OK with it, but not sure. Hmm, it does catch a mistake in AudioSessionIOS.mm though. The below code won't compile when we use enum class. if (categoryOverride() && categoryOverride() != newCategory) >> Source/WebCore/platform/audio/cocoa/MediaSessionManagerCocoa.mm:134 >> + AudioSession::CategoryType category = AudioSession::CategoryType::None; > > Could use auto here to make things a little less verbose. Why write AudioSession::CategoryType twice on the same line of code unnecessarily? Right, will fix it. Comment on attachment 428706 [details] Fix build failures on iOS View in context: https://bugs.webkit.org/attachment.cgi?id=428706&action=review >>> Source/WebCore/platform/audio/AudioSession.h:63 >>> + enum class CategoryType : uint8_t { >> >> Not sure this is an improvement. Does make it more wordy everywhere. Does it make the code more clear? I’m OK with it, but not sure. > > Hmm, it does catch a mistake in AudioSessionIOS.mm though. The below code won't compile when we use enum class. > > if (categoryOverride() && categoryOverride() != newCategory) Well, that’s a pretty good argument in favor if it actually caught a mistake. Comment on attachment 428706 [details] Fix build failures on iOS View in context: https://bugs.webkit.org/attachment.cgi?id=428706&action=review >>>> Source/WebCore/platform/audio/AudioSession.h:63 >>>> + enum class CategoryType : uint8_t { >>> >>> Not sure this is an improvement. Does make it more wordy everywhere. Does it make the code more clear? I’m OK with it, but not sure. >> >> Hmm, it does catch a mistake in AudioSessionIOS.mm though. The below code won't compile when we use enum class. >> >> if (categoryOverride() && categoryOverride() != newCategory) > > Well, that’s a pretty good argument in favor if it actually caught a mistake. I noticed it when I saw multiple "None"s in different places, e.g., AudioSession, PlatformMediaSession, and AudioStreamDescription. To be honest I did not expect a simple change here will lead to changes in so many places. :-) Created attachment 428794 [details]
Patch for landing
Committed r277584 (237810@main): <https://commits.webkit.org/237810@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 428794 [details]. |