[webkit-reviews] review granted: [Bug 53313] Add ShareableSurface class : [Attachment 80466] Patch

[bugzilla-daemon at webkit.org]

Adam Roben (aroben) <aroben at apple.com> has granted Anders Carlsson
<andersca at apple.com>'s request for review:
Bug 53313: Add ShareableSurface class
https://bugs.webkit.org/show_bug.cgi?id=53313

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

------- Additional Comments from Adam Roben (aroben) <aroben at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=80466&action=review

> Source/WebKit2/Shared/mac/ShareableSurface.cpp:53
> +    encoder->encode(CoreIPC::MachPort(m_port, MACH_MSG_TYPE_COPY_SEND));

MOVE_SEND!

> Source/WebKit2/Shared/mac/ShareableSurface.cpp:74
> +    unsigned pixelFormat = 'BGRA';
> +    unsigned bytesPerElement = 4;
> +    int width = size.width();
> +    int height = size.height();

I'd put these closer to where they're used.

> Source/WebKit2/Shared/mac/ShareableSurface.cpp:97
> +    values[4] = CFNumberCreate(0, kCFNumberLongType, &bytesPerRow);
> +    keys[5] = kIOSurfaceAllocSize;
> +    values[5] = CFNumberCreate(0, kCFNumberLongType, &allocSize);

Should you use an unsigned long type?

> Source/WebKit2/Shared/mac/ShareableSurface.cpp:100
> +    RetainPtr<CFDictionaryRef> dictionary(AdoptCF, CFDictionaryCreate(0,
keys, values, 6, &kCFTypeDictionaryKeyCallBacks,
&kCFTypeDictionaryValueCallBacks));
> +    for (unsigned i = 0; i < 6; i++)

It's too bad we have to say "6" in 4 places overall.

> Source/WebKit2/Shared/mac/ShareableSurface.cpp:117
> +    ASSERT(handle.m_port != MACH_PORT_NULL);

Could use ASSERT_ARG.

> Source/WebKit2/Shared/mac/ShareableSurface.cpp:128
> +ShareableSurface::ShareableSurface(CGLContextObj cglContextObj, const
IntSize& size, RetainPtr<IOSurfaceRef> ioSurface)

You can just take a bare IOSurfaceRef.

> Source/WebKit2/Shared/mac/ShareableSurface.cpp:139
> +    CGLContextObj cgl_ctx = m_cglContextObj;

Please add a comment about why this is needed. Maybe you can get away with a
single comment somewhere high up in the file. Or do the magic needed to make it
use m_cglContextObj directly. Or add a SET_CGLCONTEXT(m_cglContextObj) macro
(or similar).

> Source/WebKit2/Shared/mac/ShareableSurface.cpp:152
> +    ASSERT(handle.m_port == MACH_PORT_NULL);

ASSERT_ARG

> Source/WebKit2/Shared/mac/ShareableSurface.cpp:168
> +	   glGenFramebuffersEXT(1, &m_frameBufferObjectID);

Do we need to check whether this succeeded?

> Source/WebKit2/Shared/mac/ShareableSurface.cpp:176
> +	   glBindFramebufferEXT(GL_FRAMEBUFFER_EXT, 0);
> +    }
> +
> +    glBindFramebufferEXT(GL_FRAMEBUFFER_EXT, m_frameBufferObjectID);

You could skip the unbind when we create the FBO, but it doesn't matter very
much.

> Source/WebKit2/Shared/mac/ShareableSurface.cpp:188
> +    if (!m_textureID) {

Early return!

> Source/WebKit2/Shared/mac/ShareableSurface.cpp:192
> +	   glGenTextures(1, &m_textureID);

Can this fail?

> Source/WebKit2/Shared/mac/ShareableSurface.h:47
> +    class Handle {

Seems like we could share this with SharedMemory, at least on Mac.