Bug 212411 – [Cocoa] Adopt read-only mode for preferences in the WebContent process

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Per Arne Vollan]

The WebContent process should never write preferences. Adopt read-only mode for preferences.

[Per Arne Vollan]

rdar://problem/63640788

[Per Arne Vollan]

Created attachment 400343 [details]
Patch

[Darin Adler]

Comment on attachment 400343 [details]
Patch

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

> Source/WTF/wtf/PlatformHave.h:640
> +#if (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101600) || (PLATFORM(IOS_FAMILY) && __IPHONE_OS_VERSION_MIN_REQUIRED >= 140000)
> +#define HAVE_CFPREFS_READONLY_SPI 1
> +#endif

This has the common mistake that Tim Horton alerted us to: checking IOS_FAMILY and then only checking __IPHONE_OS_VERSION_MIN_REQUIRED, which is frozen at a lower value on watchOS and tVOS. If our intent is to include only iOS itself, then please use PLATFORM(IOS), or add the relevant version checking for watchOS and tvOS. HAVE_CFNETWORK_METRICS_CONNECTION_PROPERTIES is one example of doing this correctly.

[Per Arne Vollan]

(In reply to Darin Adler from comment #3)
> Comment on attachment 400343 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=400343&action=review
> 
> > Source/WTF/wtf/PlatformHave.h:640
> > +#if (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101600) || (PLATFORM(IOS_FAMILY) && __IPHONE_OS_VERSION_MIN_REQUIRED >= 140000)
> > +#define HAVE_CFPREFS_READONLY_SPI 1
> > +#endif
> 
> This has the common mistake that Tim Horton alerted us to: checking
> IOS_FAMILY and then only checking __IPHONE_OS_VERSION_MIN_REQUIRED, which is
> frozen at a lower value on watchOS and tVOS. If our intent is to include
> only iOS itself, then please use PLATFORM(IOS), or add the relevant version
> checking for watchOS and tvOS. HAVE_CFNETWORK_METRICS_CONNECTION_PROPERTIES
> is one example of doing this correctly.

Ah, good point! I will update the patch.

Thanks for reviewing!

[Per Arne Vollan]

Created attachment 400353 [details]
Patch

[EWS]

ChangeLog entry in Source/WebCore/PAL/ChangeLog contains OOPS!.

[Per Arne Vollan]

Created attachment 401031 [details]
Patch

[Darin Adler]

Comment on attachment 401031 [details]
Patch

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

> Source/WTF/wtf/PlatformHave.h:651
> +#if (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101600) \
> +    || (PLATFORM(IOS) && __IPHONE_OS_VERSION_MIN_REQUIRED >= 140000) \
> +    || (PLATFORM(WATCHOS) && __WATCH_OS_VERSION_MIN_REQUIRED >= 70000) \
> +    || (PLATFORM(APPLETV) && __TV_OS_VERSION_MIN_REQUIRED >= 140000)
> +#define HAVE_CFPREFS_READONLY_SPI 1
> +#endif

Could we try to keep these in alphabetical order rather than adding new ones at the bottom? Maybe we’re not even close to that right now?

Normally we name these after the function rather than coining a new term, so it would be something like HAVE(CF_PREFS_SET_READ_ONLY).

[EWS]

Committed r262546: <https://trac.webkit.org/changeset/262546>

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

[Per Arne Vollan]

(In reply to Darin Adler from comment #8)
> Comment on attachment 401031 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=401031&action=review
> 
> > Source/WTF/wtf/PlatformHave.h:651
> > +#if (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101600) \
> > +    || (PLATFORM(IOS) && __IPHONE_OS_VERSION_MIN_REQUIRED >= 140000) \
> > +    || (PLATFORM(WATCHOS) && __WATCH_OS_VERSION_MIN_REQUIRED >= 70000) \
> > +    || (PLATFORM(APPLETV) && __TV_OS_VERSION_MIN_REQUIRED >= 140000)
> > +#define HAVE_CFPREFS_READONLY_SPI 1
> > +#endif
> 
> Could we try to keep these in alphabetical order rather than adding new ones
> at the bottom? Maybe we’re not even close to that right now?
> 
> Normally we name these after the function rather than coining a new term, so
> it would be something like HAVE(CF_PREFS_SET_READ_ONLY).

I will create a follow-up patch for this.

Thanks for reviewing!

[Per Arne Vollan]

Reopening to attach new patch.

[Per Arne Vollan]

Created attachment 401075 [details]
Patch

[Jacob Uphoff]

Reverted r262546 for reason:

This commit caused internal build failures

Committed r262571: <https://trac.webkit.org/changeset/262571>

[EWS]

Committed r262639: <https://trac.webkit.org/changeset/262639>

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

[Per Arne Vollan]

Landed follow-up fix in <https://trac.webkit.org/changeset/262730/webkit>.