[webkit-reviews] review granted: [Bug 217212] Cocoa: Make WebGLLayer not dependent on GraphicsContextGLOpenGL : [Attachment 411127] Patch

[bugzilla-daemon at webkit.org]

Dean Jackson <dino at apple.com> has granted Kimmo Kinnunen
<kkinnunen at apple.com>'s request for review:
Bug 217212: Cocoa: Make WebGLLayer not dependent on  GraphicsContextGLOpenGL
https://bugs.webkit.org/show_bug.cgi?id=217212

Attachment 411127: Patch

https://bugs.webkit.org/attachment.cgi?id=411127&action=review




--- Comment #12 from Dean Jackson <dino at apple.com> ---
Comment on attachment 411127
  --> https://bugs.webkit.org/attachment.cgi?id=411127
Patch

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

> Source/WebCore/ChangeLog:18
> +	   Move the back buffer ownership to the GraphicsContextGLOpenGL.

Nice.

> Source/WebCore/ChangeLog:21
> +	   Make the front buffer ownership explicit in WebGLLayer.
> +	   Move the EGL bindings ownerships of all buffers to
> +	   GraphicsContextGLOpenGL.

Nicer!

> Source/WebCore/platform/graphics/cocoa/WebGLLayer.h:34
> +    void* handle { nullptr }; // Client specific metadata handle (such as
EGLSurface).

Will a WebGLLayer that is getting remote buffers need a handle? If not, it
might be ok if this is specific to EGL.

> Source/WebCore/platform/graphics/cocoa/WebGLLayer.h:63
> +- (WebCore::WebGLLayerBuffer) recycleBuffer;

Nit: We don't put a space between the return type and the name.

>>>>>>> Source/WebCore/platform/graphics/opengl/GraphicsContextGLOpenGL.h:93
>>>>>>> +#endif
>>>>>> 
>>>>>> Looks like you kept this PLATFORM(COCOA). Please you use a more specific
#define rather here.
>>>>> 
>>>>> Would you be able to suggest a define that would be more appropriate and
still work with other code defined inside PLATFORM(COCOA) ifdefs and be
implemented in the file GraphicsContextGLOpenGLCocoa.mm ?
>>>> 
>>>> Sure. Maybe USE(CA) in this case? One day maybe we will have a better
PlatformLayer abstraction than what we have right now :(.
>>> 
>>> But do I rename the file GraphicsContextGLOpenGLCocoa.mm to
GraphicsContextGLOpenGLCA.mm ? Do I rename the existing ifdef PLATFORM(COCOA)
to PLATFORM(CA) ? None of that existing code will make sense if part of it is
CA and part of it is COCOA ?
>> 
>> Up to you to rename existing things. This stuff is already a mess of
different conflicting defines. If you really feel strongly about keeping this
COCOA, I guess that's ok too.
> 
> Thanks. I'll keep the COCOA in this patch. Renaming COCOA->CA should be done
in a different patch, targeting specifically that goal.

I think it is ok to leave the files named Cocoa.

I wonder if we could make WebGLLayerClient an unconditional inheritance though,
and have an empty implementation for other ports.