Bug 225826 – [GPUP] WebContent process should not pull audio session category from the GPU Process

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Peng Liu]

When “Media in GPU Process” is enabled, the audio session category in the GPU Process is decided based on the audio session categories from all WebContent processes. A WebContent process can push audio session category to the GPU Process, but should not read it back.

[Peng Liu]

Created attachment 428691 [details]
Patch

[Peng Liu]

Created attachment 428706 [details]
Fix build failures on iOS

[Darin Adler]

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?

[Peng Liu]

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.

[Darin Adler]

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.

[Peng Liu]

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. :-)

[Peng Liu]

Created attachment 428794 [details]
Patch for landing

[EWS]

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].

[Radar WebKit Bug Importer]

<rdar://problem/78108250>