We've been spreading IOSurface code around WebCore and WebKit2 which is SPI, and picky about how it's used. We should wrap it in a C++ class to hide some of the pickyness, drag some extra context around with them, and overall make surfaces easier to use.
Renamed at Anders/Sam's request.
Created attachment 227331 [details] patch
Attachment 227331 [details] did not pass style-queue: ERROR: Source/WebCore/platform/graphics/cocoa/IOSurface.h:33: Alphabetical sorting problem. [build/include_order] [4] ERROR: Source/WebKit2/Shared/mac/RemoteLayerBackingStore.h:32: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 2 in 16 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 227333 [details] style
Comment on attachment 227333 [details] style View in context: https://bugs.webkit.org/attachment.cgi?id=227333&action=review > Source/WebCore/platform/graphics/cocoa/IOSurface.h:52 > + IOSurfaceRef platformSurface() const { return m_surface.get(); } Why not call this surface(). > Source/WebCore/platform/graphics/cocoa/IOSurface.h:68 > +#if PLATFORM(IOS) || __MAC_OS_X_VERSION_MIN_REQUIRED >= 1090 > + bool isPurgeable() const; > + SurfaceState setIsPurgeable(bool); > +#endif // __MAC_OS_X_VERSION_MIN_REQUIRED >= 1090 Can't we just make these no-ops on older versions?
Comment on attachment 227333 [details] style View in context: https://bugs.webkit.org/attachment.cgi?id=227333&action=review > Source/WebCore/ChangeLog:20 > + * platform/graphics/cocoa/IOSurface.h: Added. Nope, there's a framework header with this name.
(In reply to comment #5) > (From update of attachment 227333 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=227333&action=review > > > Source/WebCore/platform/graphics/cocoa/IOSurface.h:52 > > + IOSurfaceRef platformSurface() const { return m_surface.get(); } > > Why not call this surface(). Sure. > > Source/WebCore/platform/graphics/cocoa/IOSurface.h:68 > > +#if PLATFORM(IOS) || __MAC_OS_X_VERSION_MIN_REQUIRED >= 1090 > > + bool isPurgeable() const; > > + SurfaceState setIsPurgeable(bool); > > +#endif // __MAC_OS_X_VERSION_MIN_REQUIRED >= 1090 > > Can't we just make these no-ops on older versions? I guess, I'm just afraid that people will think they're getting purgeability and have issues. But since it's internal, I guess that's OK.
(In reply to comment #6) > (From update of attachment 227333 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=227333&action=review > > > Source/WebCore/ChangeLog:20 > > + * platform/graphics/cocoa/IOSurface.h: Added. > > Nope, there's a framework header with this name. And yet!
Comment on attachment 227333 [details] style View in context: https://bugs.webkit.org/attachment.cgi?id=227333&action=review > Source/WebCore/platform/graphics/cocoa/IOSurface.mm:95 > + ASSERT(m_surface); I don't think this is very useful; it will fire if height or width is zero (maybe?), and/or on system that have run out of graphics memory. > Source/WebCore/platform/graphics/cocoa/IOSurface.mm:150 > + uint32_t previousState; Maybe initailize this to 0 in case the IOSurfaceSetPurgeable fails and we use uninitialized bits. > Source/WebCore/platform/graphics/cocoa/IOSurface.mm:151 > + IOReturn ret = IOSurfaceSetPurgeable(m_surface.get(), kIOSurfacePurgeableKeepCurrent, &previousState); Weird that a const function is calling IOSurfaceSetPurgeable(). Is that call a no-op? > Source/WebCore/platform/graphics/cocoa/IOSurface.mm:163 > + IOReturn ret = IOSurfaceSetPurgeable(m_surface.get(), kIOSurfacePurgeableKeepCurrent, &previousState); Again. > Source/WebCore/platform/graphics/cocoa/IOSurface.mm:171 > + IOReturn ret = IOSurfaceSetPurgeable(m_surface.get(), isPurgeable ? kIOSurfacePurgeableVolatile : kIOSurfacePurgeableNonVolatile, &previousState); Bored now. Oh wait, this is a real setter.
http://trac.webkit.org/changeset/166005