Bug 221096

Summary: [macOS] Observe color preference changes in the UI process
Product: WebKit Reporter: Per Arne Vollan <pvollan>
Component: WebKit Misc.Assignee: Per Arne Vollan <pvollan>
Status: RESOLVED FIXED    
Severity: Normal CC: bfulgham, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch none

Description Per Arne Vollan 2021-01-28 13:09:34 PST 
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.
Comment 1 Radar WebKit Bug Importer 2021-01-28 13:11:02 PST 
<rdar://problem/73721275>
Comment 2 Per Arne Vollan 2021-01-28 13:22:38 PST 
Created attachment 418666 [details]
Patch
Comment 3 Brent Fulgham 2021-01-29 10:05:38 PST 
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?
Comment 4 Per Arne Vollan 2021-01-29 13:19:43 PST 
(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!
Comment 5 Brent Fulgham 2021-02-01 09:47:34 PST 
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.
Comment 6 Per Arne Vollan 2021-02-01 09:54:23 PST 
Comment on attachment 418666 [details]
Patch

Thanks for reviewing!
Comment 7 EWS 2021-02-01 12:06:32 PST 
Committed r272154: <https://trac.webkit.org/changeset/272154>

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