Bug 221258

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>