[Cocoa] CFPrefs synchronization does not always work for global preference changes

[Per Arne Vollan]

In CFPrefs direct mode, synchronization of global preference changes from the UI process to the WebContent process does not always work. This is caused by the KVO notification being sent to the incorrect NSUserDefault object, which leads us to believe a non-global preference was changed. Since a global preference change always leads to some NSUserDefaults object receiving the notification, we can work around this by checking if the preference being changed really belongs to the domain of the NSUserDefaults object, or if it is a global preference.

[Per Arne Vollan]

rdar://problem/63862153

[Per Arne Vollan]

Created attachment 401078 [details]
Patch

[Tim Horton]

Comment on attachment 401078 [details]
Patch

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

> Source/WebKit/ChangeLog:9
> +        This is caused by the KVO notification being sent to the incorrect NSUserDefault object, which leads us to believe a non-global preference

It's not incorrect! Global changes affect all domains. We're just inferring too much :D

> Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:923
> +        if (domain.isEmpty())

I think you need to merge this with my change.

[Per Arne Vollan]

(In reply to Tim Horton from comment #3)
> Comment on attachment 401078 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=401078&action=review
> 
> > Source/WebKit/ChangeLog:9
> > +        This is caused by the KVO notification being sent to the incorrect NSUserDefault object, which leads us to believe a non-global preference
> 
> It's not incorrect! Global changes affect all domains. We're just inferring
> too much :D
> 

Right :)

> > Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:923
> > +        if (domain.isEmpty())
> 
> I think you need to merge this with my change.

Ah, yes, this needs a rebase.

Thanks for reviewing!

[Per Arne Vollan]

Created attachment 401203 [details]
Patch

[Per Arne Vollan]

Created attachment 401343 [details]
Patch

[EWS]

Committed r262724: <https://trac.webkit.org/changeset/262724>

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

[Tim Horton]

*** Bug 212627 has been marked as a duplicate of this bug. ***

[Darin Adler]

Comment on attachment 401203 [details]
Patch

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

> Source/WebKit/UIProcess/Cocoa/PreferenceObserver.mm:72
> +        if (!m_observer)
> +            return;

Don’t we want "continue" here instead of return? If we want return, then why not do this outside the loop?

[Per Arne Vollan]

(In reply to Darin Adler from comment #9)
> Comment on attachment 401203 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=401203&action=review
> 
> > Source/WebKit/UIProcess/Cocoa/PreferenceObserver.mm:72
> > +        if (!m_observer)
> > +            return;
> 
> Don’t we want "continue" here instead of return? If we want return, then why
> not do this outside the loop?

You're right! This check should be moved outside the loop.