Bug 140748 – [iOS] Fix iphoneos SDK builds for ios-ews queue

• Component: Tools / Tests
• Status: RESOLVED FIXED

[David Kilzer (:ddkilzer)]

Fix build failures for the iphoneos SDK due to missing private headers to make the ios-ews queue work.

[David Kilzer (:ddkilzer)]

Created attachment 245098 [details]
Patch v1

[WebKit Commit Bot]

Attachment 245098 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/spi/cocoa/IOTypesSPI.h:36:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/spi/cocoa/IOTypesSPI.h:40:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/spi/cocoa/IOTypesSPI.h:41:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/spi/cocoa/IOSurfaceSPI.h:42:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/spi/cocoa/IOSurfaceSPI.h:77:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/spi/cocoa/IOSurfaceSPI.h:78:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/spi/cocoa/IOSurfaceSPI.h:79:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/WebCore/platform/spi/cocoa/IOSurfaceSPI.h:80:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 8 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Daniel Bates]

Comment on attachment 245098 [details]
Patch v1

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

> Source/WebCore/platform/spi/cocoa/IOSurfaceSPI.h:35
> +#include "IOReturnSPI.h"

Is there value in having the separation between IOSurface SPI (IOSurfaceSPI.h) and IOKit SPI (IOReturnSPI.h) as opposed to grouping this functionality together with the functionality in IOTypesSPI.h into one IOKitSPI.h header? Or would it be too disingenuous to have such functionality in IOKitSPI.h?

> Source/WebCore/platform/spi/cocoa/IOSurfaceSPI.h:36
> +#include "IOTypesSPI.h"

Is this header necessary? If so, see my remark above.

[David Kilzer (:ddkilzer)]

(In reply to comment #3)
> Comment on attachment 245098 [details]
> Patch v1
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=245098&action=review
> 
> > Source/WebCore/platform/spi/cocoa/IOSurfaceSPI.h:35
> > +#include "IOReturnSPI.h"
> 
> Is there value in having the separation between IOSurface SPI
> (IOSurfaceSPI.h) and IOKit SPI (IOReturnSPI.h) as opposed to grouping this
> functionality together with the functionality in IOTypesSPI.h into one
> IOKitSPI.h header? Or would it be too disingenuous to have such
> functionality in IOKitSPI.h?

Since <IOSurface/IOSurface.h> is API on macosx, and since IOSurface.framework is a separate framework from IOKit.framework, I would prefer to leave it separate from IOKit itself.

The rule I was using was if the header existed as API in the macosx SDK, then keep it as a separate SPI header when making it work for the iphoneos SDK just in case we need to add more things later.

> > Source/WebCore/platform/spi/cocoa/IOSurfaceSPI.h:36
> > +#include "IOTypesSPI.h"
> 
> Is this header necessary? If so, see my remark above.

The kIOMapWriteCombineCache constant is used in these source files:

Source/WebCore/platform//graphics/cg/ImageBufferCG.cpp Source/WebCore/platform//graphics/cocoa/IOSurface.mm

Since these source files were previously only including <IOSurface/IOSurface.h> before, they were (likely) getting the IOTypes.h header through that header (although I didn't check explicitly).  It made sense to me to include it through IOSurfaceSPI.h as well rather than include it separately in each source file.

[Daniel Bates]

(In reply to comment #4)
> (In reply to comment #3)
> > Comment on attachment 245098 [details]
> > Patch v1
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=245098&action=review
> > 
> > > Source/WebCore/platform/spi/cocoa/IOSurfaceSPI.h:35
> > > +#include "IOReturnSPI.h"
> > 
> > Is there value in having the separation between IOSurface SPI
> > (IOSurfaceSPI.h) and IOKit SPI (IOReturnSPI.h) as opposed to grouping this
> > functionality together with the functionality in IOTypesSPI.h into one
> > IOKitSPI.h header? Or would it be too disingenuous to have such
> > functionality in IOKitSPI.h?
> 
> Since <IOSurface/IOSurface.h> is API on macosx, and since
> IOSurface.framework is a separate framework from IOKit.framework, I would
> prefer to leave it separate from IOKit itself.
> 
> The rule I was using was if the header existed as API in the macosx SDK,
> then keep it as a separate SPI header when making it work for the iphoneos
> SDK just in case we need to add more things later.
> 

OK

> > > Source/WebCore/platform/spi/cocoa/IOSurfaceSPI.h:36
> > > +#include "IOTypesSPI.h"
> > 
> > Is this header necessary? If so, see my remark above.
> 
> The kIOMapWriteCombineCache constant is used in these source files:
> 
> Source/WebCore/platform//graphics/cg/ImageBufferCG.cpp
> Source/WebCore/platform//graphics/cocoa/IOSurface.mm
> 
> Since these source files were previously only including
> <IOSurface/IOSurface.h> before, they were (likely) getting the IOTypes.h
> header through that header (although I didn't check explicitly).  It made
> sense to me to include it through IOSurfaceSPI.h as well rather than include
> it separately in each source file.

OK

[Daniel Bates]

Comment on attachment 245098 [details]
Patch v1

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

> Source/WebCore/platform/spi/cocoa/IOReturnSPI.h:35
> +typedef kern_return_t IOReturn;

We should include header mach/kern_return.h for the definition of kern_return_t.

Notice that IOPMLibSPI.h also has a typedef for IOReturn. We may want to consider consolidating this typedef into one file, say IOReturnSPI.h.

> Source/WebCore/platform/spi/cocoa/IOSurfaceSPI.h:82
> +#if USE(APPLE_INTERNAL_SDK)
> +#import <IOSurface/IOSurfacePrivate.h>
> +#else
> +enum {
> +    kIOSurfacePurgeableNonVolatile = 0,
> +    kIOSurfacePurgeableVolatile = 1,
> +    kIOSurfacePurgeableEmpty = 2,
> +    kIOSurfacePurgeableKeepCurrent = 3,
> +};
> +#endif

Nit: The style used for this #if/#else/#endif block differs from the style you used for similar blocks both in this file (above) and in other files with regards to the omission of empty lines after the #if (line 73), before and after the #else (line 75) and before the #endif (line 82). I suggest that we pick a style for writing such blocks and stick with it for consistency.

[Daniel Bates]

Comment on attachment 245098 [details]
Patch v1

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

> Source/WebCore/platform/spi/cocoa/IOSurfaceSPI.h:59
> +mach_port_t IOSurfaceCreateMachPort(IOSurfaceRef buffer);

Nit: We should include the header for the definition of mach_port_t, mach/mach_port.h.

[David Kilzer (:ddkilzer)]

Committed r178927: <http://trac.webkit.org/changeset/178927>