[webkit-reviews] review denied: [Bug 136487] [iOS] Make WebCore build with public iOS SDK : [Attachment 238066] Patch

[bugzilla-daemon at webkit.org]

Maciej Stachowiak <mjs at apple.com> has denied Daniel Bates <dbates at webkit.org>'s
request for review:
Bug 136487: [iOS] Make WebCore build with public iOS SDK
https://bugs.webkit.org/show_bug.cgi?id=136487

Attachment 238066: Patch
https://bugs.webkit.org/attachment.cgi?id=238066&action=review

------- Additional Comments from Maciej Stachowiak <mjs at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=238066&action=review


The only essential issue is explaining the IntRect.h change since it is not
obviously related. Plus there are many optional comments. r- for now but
looking good.

> Source/WebCore/platform/graphics/IntRect.h:45
> +typedef CGRect NSRect;

This change does not appear to be explained in the ChangeLog.

> Source/WebCore/platform/ios/wak/WAKAppKitStubs.h:37
> -#import <Foundation/NSGeometry.h>
> +#import <WebCore/NSGeometrySPI.h>

Why this change? NSGeometry is public API, at least on Mac. Is that not the
case on iOS?

> Source/WebCore/platform/spi/ca/CALayerSPI.h:30
> +#import <CoreGraphics/CoreGraphics.h>
> +#import <QuartzCore/QuartzCore.h>

Does CALayerPrivate.h include these headers? I don't think it does, so it might
be better to include them unconditionally, for consistency in what this header
pulls in.

> Source/WebCore/platform/spi/ca/CATiledLayerSPI.h:32
> +#import <CoreGraphics/CoreGraphics.h>
> +#import <QuartzCore/QuartzCore.h>

Does CALayerPrivate.h include these headers? I don't think it does, so it might
be better to include them unconditionally, for consistency in what this header
pulls in.

> Source/WebCore/platform/spi/cg/CGColorTransformSPI.h:40
> +#ifdef __cplusplus
> +extern "C" {
> +#endif

Maybe it would be worthwhile making an EXTERN_C macro in a WTF header
(Compiler.h maybe?) that expands to extern "C" for C++, and nothing or just
extern for C, so you could avoid these repeated ifdef blocks. This comment is
optional, it applies to a bunch of files but I won't mention it every time.

> Source/WebCore/platform/spi/cg/CGColorTransformSPI.h:43
> +extern CGColorRef CGColorTransformConvertColor(CGColorTransformRef,
CGColorRef, CGColorRenderingIntent);
> +extern CGColorTransformRef CGColorTransformCreate(CGColorSpaceRef,
CFDictionaryRef attributes);

I'm not sure the "extern" specifiers here do anything. Again, this might apply
to a bunch of files.

> Source/WebCore/platform/spi/cg/CGContextGStateSPI.h:27
> +#ifndef CGContextGStateSPI_h
> +#define CGContextGStateSPI_h

I know CGContext SPI calls in reality are split among multiple files, but maybe
we should put them all in one WebCore SPI header, since the split is not super
meaningful.

> Source/WebCore/platform/spi/cg/CGFloatSPI.h:38
> +    if (sizeof(value) == sizeof(float))

Seems like it would be better to make this an explicit #if on
CGFLOAT_IS_DOUBLE. I know the compiler will optimize out the dead path but it
seems lame to rely on that, and with the #if you would not need any of the
casts. Heck you could just #define CGRound to be round when CG_FLOAT_IS_DOUBLE
and roundf otherwise.

Similar comments apply to almost all the other functions in this file.

> Source/WebCore/platform/spi/cg/CGFloatSPI.h:45
> +#if defined(__STDC_VERSION__) && __STDC_VERSION__ >= 199901L

I strongly suspect all compilers we care about support C99, so I am not sure
the conditionality is needed.

> Source/WebCore/platform/spi/cocoa/MachVMSPI.h:33
> +#if (TARGET_OS_MAC && !TARGET_OS_IPHONE) ||
(defined(WTF_USE_APPLE_INTERNAL_SDK) && WTF_USE_APPLE_INTERNAL_SDK)

Why write it out this way instead of if PLATFORM(MAC) ||
USE(APPLE_INTERNAL_SDK), as in the previous file?

> Source/WebCore/platform/spi/cocoa/NSFileManagerSPI.h:29
> +#import <Foundation/Foundation.h>

Probably should include Foundation.h unconditionally.

> Source/WebCore/platform/spi/ios/AVPlayerControllerSPI.h:31
> +#import <AVKit/AVKit.h>
> +#import <Foundation/Foundation.h>
> +#import <UIKit/UIKit.h>

These should probably be imported unconditionally.

> Source/WebCore/platform/spi/ios/AVPlayerViewControllerSPI.h:33
> +#import <AVKit/AVKit.h>
> +#import <Foundation/Foundation.h>
> +#import <QuartzCore/QuartzCore.h>
> +#import <UIKit/UIKit.h>

Should probably import these unconditionally.

> Source/WebCore/platform/spi/ios/AVValueTimingSPI.h:29
> +#import <Foundation/Foundation.h>

Again.

> Source/WebCore/platform/spi/ios/AVVideoLayerSPI.h:31
> +#import <AVKit/AVKit.h>
> +#import <CoreGraphics/CoreGraphics.h>
> +#import <Foundation/Foundation.h>

And here.

> Source/WebCore/platform/spi/ios/CTFontDescriptorSPI.h:54
> +extern const CFStringRef kCTUIFontTextStyleShortHeadline CT_AVAILABLE(10_10,
7_0);
> +extern const CFStringRef kCTUIFontTextStyleShortBody CT_AVAILABLE(10_10,
7_0);
> +extern const CFStringRef kCTUIFontTextStyleShortSubhead CT_AVAILABLE(10_10,
7_0);
> +extern const CFStringRef kCTUIFontTextStyleShortFootnote CT_AVAILABLE(10_10,
7_0);
> +extern const CFStringRef kCTUIFontTextStyleShortCaption1 CT_AVAILABLE(10_10,
7_0);
> +extern const CFStringRef kCTUIFontTextStyleTallBody CT_AVAILABLE(10_10,
7_0);
> +
> +extern const CFStringRef kCTUIFontTextStyleHeadline CT_AVAILABLE(10_10,
7_0);
> +extern const CFStringRef kCTUIFontTextStyleBody CT_AVAILABLE(10_10, 7_0);
> +extern const CFStringRef kCTUIFontTextStyleSubhead CT_AVAILABLE(10_10, 7_0);

> +extern const CFStringRef kCTUIFontTextStyleFootnote CT_AVAILABLE(10_10,
7_0);
> +extern const CFStringRef kCTUIFontTextStyleCaption1 CT_AVAILABLE(10_10,
7_0);
> +extern const CFStringRef kCTUIFontTextStyleCaption2 CT_AVAILABLE(10_10,
7_0);
> +
> +extern const CFStringRef kCTFontDescriptorTextStyleEmphasized
CT_AVAILABLE(10_10, 7_0);

I don't think you should include the CT_AVAILABLE tags here.

> Source/WebCore/platform/spi/ios/CUICatalogSPI.h:31
> +#import <CoreGraphics/CoreGraphics.h>
> +#import <CoreText/CoreText.h>
> +#import <Foundation/Foundation.h>

These includes should probably be unconditional.

> Source/WebCore/platform/spi/ios/QLPreviewConverterSPI.h:29
> +#import <Foundation/Foundation.h>

Probably should include Foundation.h unconditionally.

> Source/WebCore/platform/spi/ios/QuickLookSPI.h:29
> +#import <Foundation/Foundation.h>

Unconditional.