[webkit-reviews] review granted: [Bug 209244] [Cocoa] Fix launch time regression with CF prefs direct mode enabled : [Attachment 394134] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Mar 20 15:22:39 PDT 2020


Darin Adler <darin at apple.com> has granted Per Arne Vollan <pvollan at apple.com>'s
request for review:
Bug 209244: [Cocoa] Fix launch time regression with CF prefs direct mode
enabled
https://bugs.webkit.org/show_bug.cgi?id=209244

Attachment 394134: Patch

https://bugs.webkit.org/attachment.cgi?id=394134&action=review




--- Comment #14 from Darin Adler <darin at apple.com> ---
Comment on attachment 394134
  --> https://bugs.webkit.org/attachment.cgi?id=394134
Patch

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

>
Source/WebKit/Resources/SandboxProfiles/ios/com.apple.WebKit.WebContent.sb:-537
> -    (global-name "com.apple.cfprefsd.daemon")

Seems strange that this is not inside a conditional.

> Source/WebKit/UIProcess/Cocoa/PreferenceObserver.mm:87
>      std::initializer_list<NSString*> domains = {

How can we determine if we have registered the correct set of domains, and keep
this list updated over time? Do we have to notice bugs due to incorrect
behavior or is there a system for detecting mistakes in the list?

Is this a scalable approach? Will we run into performance problems again later
if we have to add more domains? Created by someone else not thinking about
WebKit when they start fetching more preferences in some component used by
WebKit?

> Source/WebKit/UIProcess/Cocoa/WebProcessPoolCocoa.mm:681
> +#if ENABLE(CFPREFS_DIRECT_MODE)
> +    m_activationObserver = [[NSNotificationCenter defaultCenter]
addObserverForName:@"UIApplicationDidBecomeActiveNotification" object:nil
queue:[NSOperationQueue currentQueue] usingBlock:^(NSNotification
*notification) {
> +	  
dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0),
^{
> +	       // Start observing preference changes.
> +	       [WKPreferenceObserver sharedInstance];
> +	   });
> +    }];
> +#endif

Can we cut down on the copied and pasted code somehow? Maybe a helper function
named startObservingPreferenceChanges?

Is there a race condition for preferences that we fetch before we call this
block and create a WKPreferenceObserver, where we can miss an early change?

> Source/WebKit/WebProcess/com.apple.WebProcess.sb.in:-680
> -    (global-name "com.apple.cfprefsd.daemon")

Seems strange that this is not inside a conditional.


More information about the webkit-reviews mailing list