System sleep events should be observed in the UI process, which should notify the WebContent process.
<rdar://problem/74406570>
Created attachment 420540 [details] Patch
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?
(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!
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.
*** Bug 222055 has been marked as a duplicate of this bug. ***
Comment on attachment 420540 [details] Patch r=me
(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.
(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!
(In reply to Chris Dumez from comment #7) > Comment on attachment 420540 [details] > Patch > > r=me Thanks for reviewing!
Committed r273082: <https://commits.webkit.org/r273082> All reviewed patches have been landed. Closing bug and clearing flags on attachment 420540 [details].