Bug 221258 – Register for power notifications in the UI process

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Per Arne Vollan]

Power notifications should be registered for in the UI process instead of the WebContent process.

[Per Arne Vollan]

Created attachment 418999 [details]
Patch

[Per Arne Vollan]

Created attachment 419001 [details]
Patch

[Per Arne Vollan]

Created attachment 419009 [details]
Patch

[Per Arne Vollan]

Created attachment 419014 [details]
Patch

[Brent Fulgham]

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?

[Brent Fulgham]

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?

[Per Arne Vollan]

(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!

[Brent Fulgham]

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.

[Per Arne Vollan]

Comment on attachment 419014 [details]
Patch

Thanks for reviewing!

[EWS]

commit-queue failed to commit attachment 419014 [details] to WebKit repository.

[EWS]

Committed r272241: <https://trac.webkit.org/changeset/272241>

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

[Radar WebKit Bug Importer]

<rdar://problem/73901378>