Bug 206905

Created attachment 389055 [details]
Patch

Created attachment 389067 [details]
Patch

Comment on attachment 389067 [details]
Patch

What made you choose "USE" over "HAVE" here? Typically in situations like this we want to use it wherever we have it. So I would think these would be HAVE. We reserve USE for where the decision to use is a more complex question than just "use what you have".

Comment on attachment 389067 [details]
Patch

I guess the things in-file said USE, but I think they are wrong.

(In reply to Darin Adler from comment #4)
> Comment on attachment 389067 [details]
> Patch
> 
> I guess the things in-file said USE, but I think they are wrong.

That's exactly why I implemented them as USE. I'll change them to HAVE

Created attachment 389140 [details]
Patch

Created attachment 389175 [details]
Patch

Comment on attachment 389175 [details]
Patch

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

> Source/WTF/wtf/PlatformHave.h:580
> +#if PLATFORM(COCOA)
> +#define HAVE_SECURE_ARCHIVER_API 1
> +#endif

Since this is used only in Cocoa-specific source files, we can remove the conditional entirely and enable things unconditionally. This avoids having to move it into PlatformHave.h.

> Source/WTF/wtf/PlatformHave.h:584
> +#if PLATFORM(MAC) || PLATFORM(IOS_FAMILY)
> +#define HAVE_SECURE_ARCHIVER_FOR_ATTRIBUTED_STRING 1
> +#endif

This expression is the same as PLATFORM(COCOA).

Since this is used only in Cocoa-specific source files, we can remove the conditional entirely and enable things unconditionally. This avoids having to move it into PlatformHave.h.

> Source/WTF/wtf/PlatformHave.h:588
> +#if PLATFORM(WATCHOS) || PLATFORM(APPLETV) || (PLATFORM(IOS_FAMILY) && !(defined __has_include && __has_include(<CoreFoundation/CFPriv.h>)))
> +#define HAVE_NSPROGRESS_PUBLISHING_SPI 1
> +#endif

I don’t understand why we are adding __has_include here. The old code didn’t use it. I believe we should use that only when absolutely necessary and I don’t see a comment explaining why it’s necessary here.

I think this should just be PLATFORM(IOS_FAMILY).

> Source/WebCore/PAL/pal/spi/cg/CoreGraphicsSPI.h:287
> +#if HAVE(CG_PATH_UNEVEN_CORNERS_ROUNDEDRECT)
>  void CGPathAddUnevenCornersRoundedRect(CGMutablePathRef, const CGAffineTransform *, CGRect, const CGSize corners[4]);
>  #endif

Seems to me it might be OK to just have this unconditionally in the SPI header, as long as the actual use of the function is guarded by HAVE(CG_PATH_UNEVEN_CORNERS_ROUNDEDRECT). But I guess this way is OK too.

Comment on attachment 389175 [details]
Patch

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

>> Source/WTF/wtf/PlatformHave.h:588
>> +#endif
> 
> I don’t understand why we are adding __has_include here. The old code didn’t use it. I believe we should use that only when absolutely necessary and I don’t see a comment explaining why it’s necessary here.
> 
> I think this should just be PLATFORM(IOS_FAMILY).

Because this is actually:

#if PLATFORM(WATCHOS) || PLATFORM(APPLETV) || (PLATFORM(IOS_FAMILY) && !USE(APPLE_INTERNAL_SDK))

but we don't have USE macros in the HAVE macro file. It might be better to just move this back into the file that originally contains it.

Created attachment 389259 [details]
Patch

Comment on attachment 389175 [details]
Patch

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

>>> Source/WTF/wtf/PlatformHave.h:588
>>> +#endif
>> 
>> I don’t understand why we are adding __has_include here. The old code didn’t use it. I believe we should use that only when absolutely necessary and I don’t see a comment explaining why it’s necessary here.
>> 
>> I think this should just be PLATFORM(IOS_FAMILY).
> 
> Because this is actually:
> 
> #if PLATFORM(WATCHOS) || PLATFORM(APPLETV) || (PLATFORM(IOS_FAMILY) && !USE(APPLE_INTERNAL_SDK))
> 
> but we don't have USE macros in the HAVE macro file. It might be better to just move this back into the file that originally contains it.

Yes should go to USE because we for some reason are only compiling this for people who have the internal SDK. But not a good situation. Not a normal part of how we want to do things. An anomaly that we should eventually fix. Our SPI headers are designed to allow us to avoid this situation.

Comment on attachment 389259 [details]
Patch

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

> Source/WTF/wtf/PlatformHave.h:578
> +#if PLATFORM(WATCHOS) || PLATFORM(APPLETV) || (PLATFORM(IOS_FAMILY) && !(defined __has_include && __has_include(<CoreFoundation/CFPriv.h>)))

Still not happy with this but maybe we can land it like this and improve later. I’m concerned we don’t understand this well enough. It seems that this is about using SPI vs. API and I am it sure it’s right.

> Source/WebCore/PAL/pal/spi/cocoa/NSKeyedArchiverSPI.h:55
>  inline NSData *_Nullable securelyArchivedDataWithRootObject(id _Nonnull object)

Should probably come back here and remove this from the SPI header. This is here only to keep the conditionals out of the source dukes and that is no longer a concern.

> Source/WebCore/PAL/pal/spi/cocoa/NSKeyedArchiverSPI.h:64
>  inline NSData *_Nullable insecurelyArchivedDataWithRootObject(id _Nonnull object)

Ditto.

> Source/WebCore/PAL/pal/spi/cocoa/NSKeyedArchiverSPI.h:80
>  inline id _Nullable unarchivedObjectOfClassesFromData(NSSet<Class> * _Nonnull classes, NSData * _Nonnull data)

Ditto.

> Source/WebCore/PAL/pal/spi/cocoa/NSKeyedArchiverSPI.h:89
>  inline RetainPtr<NSKeyedArchiver> secureArchiver()

Ditto.

> Source/WebCore/PAL/pal/spi/cocoa/NSKeyedArchiverSPI.h:95
>  inline RetainPtr<NSKeyedUnarchiver> secureUnarchiverFromData(NSData *_Nonnull data)

Ditto.

> Source/WebCore/PAL/pal/spi/cocoa/NSKeyedArchiverSPI.h:102
>  inline id _Nullable decodeObjectOfClassForKeyFromCoder(Class _Nonnull cls, NSString * _Nonnull key, NSCoder * _Nonnull coder)

Ditto.

> Source/WebCore/loader/archive/cf/LegacyWebArchiveMac.mm:68
>      return retainPtr((__bridge CFDataRef)archiver.get().encodedData);

Just something I noticed in passing: no need to call retainPtr here.

Comment on attachment 389259 [details]
Patch

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

>> Source/WTF/wtf/PlatformHave.h:578
>> +#if PLATFORM(WATCHOS) || PLATFORM(APPLETV) || (PLATFORM(IOS_FAMILY) && !(defined __has_include && __has_include(<CoreFoundation/CFPriv.h>)))
> 
> Still not happy with this but maybe we can land it like this and improve later. I’m concerned we don’t understand this well enough. It seems that this is about using SPI vs. API and I am it sure it’s right.

Looking through our callsites, I suspect we no longer need the Internal check, but I'll file a different bug about that, https://bugs.webkit.org/show_bug.cgi?id=207008

>> Source/WebCore/PAL/pal/spi/cocoa/NSKeyedArchiverSPI.h:55
>>  inline NSData *_Nullable securelyArchivedDataWithRootObject(id _Nonnull object)
> 
> Should probably come back here and remove this from the SPI header. This is here only to keep the conditionals out of the source dukes and that is no longer a concern.

Filed https://bugs.webkit.org/show_bug.cgi?id=207009

>> Source/WebCore/loader/archive/cf/LegacyWebArchiveMac.mm:68
>>      return retainPtr((__bridge CFDataRef)archiver.get().encodedData);
> 
> Just something I noticed in passing: no need to call retainPtr here.

Updating the patch to fix this one.

Created attachment 389269 [details]
Patch

Comment on attachment 389269 [details]
Patch

Clearing flags on attachment: 389269

Committed r255455: <https://trac.webkit.org/changeset/255455>

All reviewed patches have been landed.  Closing bug.

<rdar://problem/59039984>