Bug 130529 – Add WebCore::IOSurface wrapper

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Tim Horton]

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.

[Tim Horton]

Renamed at Anders/Sam's request.

[Tim Horton]

Created attachment 227331 [details]
patch

[WebKit Commit Bot]

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.

[Tim Horton]

Created attachment 227333 [details]
style

[Anders Carlsson]

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?

[Simon Fraser (smfr)]

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.

[Tim Horton]

(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.

[Tim Horton]

(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!

[Simon Fraser (smfr)]

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.

[Tim Horton]

http://trac.webkit.org/changeset/166005