[webkit-reviews] review granted: [Bug 217374] Mac: Remove OpenGL and OpenGL ES backends : [Attachment 410745] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Kimmo Kinnunen
<kkinnunen at apple.com>'s request for review:
Bug 217374: Mac: Remove OpenGL and OpenGL ES backends
https://bugs.webkit.org/show_bug.cgi?id=217374

Attachment 410745: Patch

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




--- Comment #2 from Darin Adler <darin at apple.com> ---
Comment on attachment 410745
  --> https://bugs.webkit.org/attachment.cgi?id=410745
Patch

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

Looks great.

> Source/WebCore/platform/graphics/GraphicsContextGL.h:780
> +

Should not leave this extra blank line.

> Source/WebCore/platform/graphics/cocoa/WebGLLayer.h:38
> +

Should not add this extra blank line here.

> Source/WebCore/platform/graphics/cv/VideoTextureCopierCV.cpp:52
> +namespace {

We mostly still use static rather than anonymous namespace for this kind of
thing in WebKit.

> Source/WebCore/platform/graphics/cv/VideoTextureCopierCV.cpp:53
> +// TODO: Fix style to allow multi-line raw strings.

Not an important follow-up so I would omit this comment. Also. WebKit usies
FIXME, not TODO.

> Source/WebCore/platform/graphics/cv/VideoTextureCopierCV.cpp:54
> +#define SHADER(text) #text##_s

Why do we need this macro at all? It does not seem to add any helpful
abstraction.

> Source/WebCore/platform/graphics/cv/VideoTextureCopierCV.cpp:56
> +const ASCIILiteral s_rgbVertexShader { SHADER(

constexpr auto or static constexpr auto?