Bug 221996 – [macOS] Observe system sleep events in the UI process

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Per Arne Vollan]

System sleep events should be observed in the UI process, which should notify the WebContent process.

[Radar WebKit Bug Importer]

<rdar://problem/74406570>

[Per Arne Vollan]

Created attachment 420540 [details]
Patch

[Brent Fulgham]

Comment on attachment 420540 [details]
Patch

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

I'm worried this change is not right for iOS, since the MediaSessionManagerIOS class is a subclass of MediaSessionManager. I like the idea of the UIProcess handling sleep events, and forwarding them to the WebContent process -- I just think you might need these changes for iOS, too.

> Source/WebKit/UIProcess/WebProcessPool.h:132
> +    , private PAL::SystemSleepListener::Client

MediaSessionManagerIOS *is a* MediaSessionManagerCocoa. Are you sure we don't need this code in the iOS case?

[Per Arne Vollan]

(In reply to Brent Fulgham from comment #3)
> Comment on attachment 420540 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=420540&action=review
> 
> I'm worried this change is not right for iOS, since the
> MediaSessionManagerIOS class is a subclass of MediaSessionManager. I like
> the idea of the UIProcess handling sleep events, and forwarding them to the
> WebContent process -- I just think you might need these changes for iOS, too.
> 
> > Source/WebKit/UIProcess/WebProcessPool.h:132
> > +    , private PAL::SystemSleepListener::Client
> 
> MediaSessionManagerIOS *is a* MediaSessionManagerCocoa. Are you sure we
> don't need this code in the iOS case?

Yes, I believe you are right. I will add it for iOS as well.

Thanks for reviewing!

[youenn fablet]

Comment on attachment 420540 [details]
Patch

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

>> Source/WebKit/UIProcess/WebProcessPool.h:132
>> +    , private PAL::SystemSleepListener::Client
> 
> MediaSessionManagerIOS *is a* MediaSessionManagerCocoa. Are you sure we don't need this code in the iOS case?

I think this change is ok in practice, agreed it is a bit weird that this code is in Cocoa and not MediaSessionManagerMac, but we do not have any.
I am not sure WebProcessPool is the best choice though.
Maybe there should be a singleton that would be enabled at creation of WebProcessPool for instance and that would send it to all processes of all process pools.

Another approach would be to do this in GPUProcess, since this is only used for media.

[Peng Liu]

*** Bug 222055 has been marked as a duplicate of this bug. ***

[Chris Dumez]

Comment on attachment 420540 [details]
Patch

r=me

[Per Arne Vollan]

(In reply to Per Arne Vollan from comment #4)
> (In reply to Brent Fulgham from comment #3)
> > Comment on attachment 420540 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=420540&action=review
> > 
> > I'm worried this change is not right for iOS, since the
> > MediaSessionManagerIOS class is a subclass of MediaSessionManager. I like
> > the idea of the UIProcess handling sleep events, and forwarding them to the
> > WebContent process -- I just think you might need these changes for iOS, too.
> > 
> > > Source/WebKit/UIProcess/WebProcessPool.h:132
> > > +    , private PAL::SystemSleepListener::Client
> > 
> > MediaSessionManagerIOS *is a* MediaSessionManagerCocoa. Are you sure we
> > don't need this code in the iOS case?
> 
> Yes, I believe you are right. I will add it for iOS as well.
> 
> Thanks for reviewing!

Looking closer, it seems that the SystemSleepListener is only an empty implementation on iOS, so it does not seem like it is required.

[Per Arne Vollan]

(In reply to youenn fablet from comment #5)
> Comment on attachment 420540 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=420540&action=review
> 
> >> Source/WebKit/UIProcess/WebProcessPool.h:132
> >> +    , private PAL::SystemSleepListener::Client
> > 
> > MediaSessionManagerIOS *is a* MediaSessionManagerCocoa. Are you sure we don't need this code in the iOS case?
> 
> I think this change is ok in practice, agreed it is a bit weird that this
> code is in Cocoa and not MediaSessionManagerMac, but we do not have any.
> I am not sure WebProcessPool is the best choice though.
> Maybe there should be a singleton that would be enabled at creation of
> WebProcessPool for instance and that would send it to all processes of all
> process pools.
> 
> Another approach would be to do this in GPUProcess, since this is only used
> for media.

These are very good points. In this case, it seems we only create MediaSessionManagerCocoa in the WP process, also with the GPU process enabled, so it seems to be sufficient to only forward these events to the WP process. We may need to forward these events to other WebKit process going forward, though.

Thanks for reviewing!

[Per Arne Vollan]

(In reply to Chris Dumez from comment #7)
> Comment on attachment 420540 [details]
> Patch
> 
> r=me

Thanks for reviewing!

[EWS]

Committed r273082: <https://commits.webkit.org/r273082>

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