[webkit-reviews] review granted: [Bug 180127] Adopt updated NSKeyed[Un]Archiver API when available : [Attachment 327834] Patch

[bugzilla-daemon at webkit.org]

Alex Christensen <achristensen at apple.com> has granted Brent Fulgham
<bfulgham at webkit.org>'s request for review:
Bug 180127: Adopt updated NSKeyed[Un]Archiver API when available
https://bugs.webkit.org/show_bug.cgi?id=180127

Attachment 327834: Patch

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




--- Comment #4 from Alex Christensen <achristensen at apple.com> ---
Comment on attachment 327834
  --> https://bugs.webkit.org/attachment.cgi?id=327834
Patch

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

r=me with comments.  Don't break anything.

> Source/WebCore/PAL/pal/spi/cocoa/NSKeyedArchiverSPI.h:34
> +#if USE(APPLE_INTERNAL_SDK) && USE(SECURE_BY_DEFAULT_ARCHIVER_API)
> +
> +#import <Foundation/NSKeyedArchiver_Private.h>
> +
> +#else

I think this would look nicer if it were arranged like this:

#if USE(SECURE_BY_DEFAULT_ARCHIVER_API)
#if USE(APPLE_INTERNAL_SDK)
#import <Foundation/NSKeyedArchiver_Private.h>
#else
...
#endif
#endif

I would also call SECURE_BY_DEFAULT_ARCHIVER_API SECURE_ARCHIVER_API

> Source/WebCore/PAL/pal/spi/cocoa/NSKeyedArchiverSPI.h:62
> +#include <wtf/RetainPtr.h>

This looks strange in the middle of the file.  The things below this are also
not SPI declarations.  Maybe they could go in a different header.

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

Would it make sense to call this insecurelyArchivedDataWithRootObject to mirror
securelyArchivedDataWithRootObject and emphasize that it's insecure?

> Source/WebCore/loader/archive/cf/LegacyWebArchiveMac.mm:49
>      @try {

Could we have this outside of the #if to have mirrored braces?