Power notifications should be registered for in the UI process instead of the WebContent process.
Created attachment 418999 [details] Patch
Created attachment 419001 [details] Patch
Created attachment 419009 [details] Patch
Created attachment 419014 [details] Patch
Comment on attachment 419009 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=419009&action=review It looks like tvOS is unhappy, too! > Source/WebKit/WebProcess/WebProcess.messages.in:199 > +#if PLATFORM(MAC) Is this needed for Catalyst? > Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:636 > + MainThreadSharedTimer::shouldSetupPowerObserver() = false; It seems like we place the burden of knowing not to set the power observer on the various client processes. Maybe it would be better if it defaulted to false, and the main UIProcess initialization had to set it to true?
Comment on attachment 419014 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=419014&action=review > Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:636 > + MainThreadSharedTimer::shouldSetupPowerObserver() = false; This places the burden on all aux processes. How about if we make the default false, and require this be set to true in 'Source/WebKit/Shared/WebKit2Initialize.cpp:void InitializeWebKit2()' so that only the UIProcess has to set this value?
(In reply to Brent Fulgham from comment #5) > Comment on attachment 419009 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=419009&action=review > > It looks like tvOS is unhappy, too! > > > Source/WebKit/WebProcess/WebProcess.messages.in:199 > > +#if PLATFORM(MAC) > > Is this needed for Catalyst? > It seems only Mac is using the PowerObserver, so I do not think this is needed for Catalyst. > > Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:636 > > + MainThreadSharedTimer::shouldSetupPowerObserver() = false; > > It seems like we place the burden of knowing not to set the power observer > on the various client processes. Maybe it would be better if it defaulted to > false, and the main UIProcess initialization had to set it to true? We only disable setup of the PowerObserver in the WebContent process. The WebContent process will be notified by the UI process about power events. For the other processes it will default to true, which gives the same behavior as before. I am perhaps not understanding your comment correctly, though. Does this address your comment? Thanks for reviewing!
Comment on attachment 419009 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=419009&action=review >>> Source/WebKit/WebProcess/WebProcess.messages.in:199 >>> +#if PLATFORM(MAC) >> >> Is this needed for Catalyst? > > It seems only Mac is using the PowerObserver, so I do not think this is needed for Catalyst. Okay, good. >>> Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:636 >>> + MainThreadSharedTimer::shouldSetupPowerObserver() = false; >> >> It seems like we place the burden of knowing not to set the power observer on the various client processes. Maybe it would be better if it defaulted to false, and the main UIProcess initialization had to set it to true? > > We only disable setup of the PowerObserver in the WebContent process. The WebContent process will be notified by the UI process about power events. For the other processes it will default to true, which gives the same behavior as before. I am perhaps not understanding your comment correctly, though. Does this address your comment? > > Thanks for reviewing! Oh, I didn't understand that. I thought we were blocking for all processes. In that case, I think your change makes sense.
Comment on attachment 419014 [details] Patch Thanks for reviewing!
commit-queue failed to commit attachment 419014 [details] to WebKit repository.
Committed r272241: <https://trac.webkit.org/changeset/272241> All reviewed patches have been landed. Closing bug and clearing flags on attachment 419014 [details].
<rdar://problem/73901378>