Bug 221096 – [macOS] Observe color preference changes in the UI process

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Per Arne Vollan]

As a step towards blocking the distributed notifications daemon in the WebContent process, color preference changes should be observed in the UI process. The UI process should notify the WebContent process about color preference changes.

[Radar WebKit Bug Importer]

<rdar://problem/73721275>

[Per Arne Vollan]

Created attachment 418666 [details]
Patch

[Brent Fulgham]

Comment on attachment 418666 [details]
Patch

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

> Source/WebKit/UIProcess/Cocoa/WebProcessPoolCocoa.mm:107
> +static CFStringRef AppleColorPreferencesChangedNotification = CFSTR("AppleColorPreferencesChangedNotification");

Should this be in one of our PAL SPI files?

> Source/WebKit/UIProcess/Cocoa/WebProcessPoolCocoa.mm:595
> +    auto* pool = reinterpret_cast<WebProcessPool*>(observer);

Could pool be null (perhaps during launch or shutdown)? Maybe this should be:
if (auto* pool = ....)
    pool->sendToAllProcesses

This might apply to the 'backlightLevelDidChangeCallback', too.

> Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:1022
> +    CFNotificationCenterPostNotification(CFNotificationCenterGetLocalCenter(), CFSTR("NSColorLocalPreferencesChangedNotification"), nullptr, nullptr, true);

Should 'NSColorLocalPreferencesChangedNotification' be something we put in PAL SPI somewhere? Is this a real thing used elsewhere in the system?

[Per Arne Vollan]

(In reply to Brent Fulgham from comment #3)
> Comment on attachment 418666 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=418666&action=review
> 
> > Source/WebKit/UIProcess/Cocoa/WebProcessPoolCocoa.mm:107
> > +static CFStringRef AppleColorPreferencesChangedNotification = CFSTR("AppleColorPreferencesChangedNotification");
> 
> Should this be in one of our PAL SPI files?
> 

The constant has not been assigned an official name in the system. Even so, should we add this to an SPI file?

> > Source/WebKit/UIProcess/Cocoa/WebProcessPoolCocoa.mm:595
> > +    auto* pool = reinterpret_cast<WebProcessPool*>(observer);
> 
> Could pool be null (perhaps during launch or shutdown)? Maybe this should be:
> if (auto* pool = ....)
>     pool->sendToAllProcesses
> 
> This might apply to the 'backlightLevelDidChangeCallback', too.
> 

That is a good point. In this particular case it seems pool cannot be null, since it is being initialized with 'this' in 'CFNotificationCenterAddObserver(CFNotificationCenterGetDistributedCenter(), this, colorPreferencesDidChangeCallback, AppleColorPreferencesChangedNotification, nullptr, CFNotificationSuspensionBehaviorCoalesce)'.

> > Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:1022
> > +    CFNotificationCenterPostNotification(CFNotificationCenterGetLocalCenter(), CFSTR("NSColorLocalPreferencesChangedNotification"), nullptr, nullptr, true);
> 
> Should 'NSColorLocalPreferencesChangedNotification' be something we put in
> PAL SPI somewhere? Is this a real thing used elsewhere in the system?

It is used in the system, but it seems it has not been assigned an official name. Would you prefer it being added to an SPI file?

Thanks for reviewing!

[Brent Fulgham]

Comment on attachment 418666 [details]
Patch

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

r=me

>>> Source/WebKit/UIProcess/Cocoa/WebProcessPoolCocoa.mm:107
>>> +static CFStringRef AppleColorPreferencesChangedNotification = CFSTR("AppleColorPreferencesChangedNotification");
>> 
>> Should this be in one of our PAL SPI files?
> 
> The constant has not been assigned an official name in the system. Even so, should we add this to an SPI file?

No, not if it's our own thing.

>>> Source/WebKit/UIProcess/Cocoa/WebProcessPoolCocoa.mm:595
>>> +    auto* pool = reinterpret_cast<WebProcessPool*>(observer);
>> 
>> Could pool be null (perhaps during launch or shutdown)? Maybe this should be:
>> if (auto* pool = ....)
>>     pool->sendToAllProcesses
>> 
>> This might apply to the 'backlightLevelDidChangeCallback', too.
> 
> That is a good point. In this particular case it seems pool cannot be null, since it is being initialized with 'this' in 'CFNotificationCenterAddObserver(CFNotificationCenterGetDistributedCenter(), this, colorPreferencesDidChangeCallback, AppleColorPreferencesChangedNotification, nullptr, CFNotificationSuspensionBehaviorCoalesce)'.

Makes sense.

[Per Arne Vollan]

Comment on attachment 418666 [details]
Patch

Thanks for reviewing!

[EWS]

Committed r272154: <https://trac.webkit.org/changeset/272154>

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