[webkit-reviews] review granted: [Bug 197755] Support using ANGLE as the backend for the WebGL implementation : [Attachment 371798] 1. Add preliminary ANGLE backend to WebCore

[bugzilla-daemon at webkit.org]

Dean Jackson <dino at apple.com> has granted Kenneth Russell <kbr at google.com>'s
request for review:
Bug 197755: Support using ANGLE as the backend for the WebGL implementation
https://bugs.webkit.org/show_bug.cgi?id=197755

Attachment 371798: 1. Add preliminary ANGLE backend to WebCore

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




--- Comment #28 from Dean Jackson <dino at apple.com> ---
Comment on attachment 371798
  --> https://bugs.webkit.org/attachment.cgi?id=371798
1. Add preliminary ANGLE backend to WebCore

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

This is great. Thanks Ken.

I'm happy to land this now and start incrementally getting things working.

> Source/ThirdParty/ANGLE/ANGLE.xcodeproj/project.pbxproj:2615
> +			shellScript = "# Type a script or drag a script file
from your workspace to insert its path.\n/bin/sh
$SRCROOT/adjust-angle-include-paths.sh\n";

Could you remove the comment line and just have the part that calls the shell
script?

> Source/ThirdParty/ANGLE/adjust-angle-include-paths.sh:38
> +' $i > $i.new
> +  mv $i.new $i

You could do sed -i '' to edit the file in place and avoid the mv and .new
steps.

> Source/WebCore/SourcesCocoa.txt:225
> +platform/graphics/angle/Extensions3DANGLE.cpp @no-unify
> +platform/graphics/angle/GraphicsContext3DANGLE.cpp @no-unify
> +platform/graphics/angle/TemporaryANGLESetting.cpp @no-unify

It's probably fine to unify these sources, but that's not important for this
patch. I've tried to unify the few remaining WebGL files a few times but keep
making mistakes and getting rolled out, so I can just add these to that job.

Edit: Oh I see. You can't unify them in case they get merged with files that
use the system OpenGL. So we'll address this later.

> Source/WebCore/platform/graphics/angle/Extensions3DANGLE.cpp:89
> +	   // Enable support in ANGLE (if not enabled already)

Nit: End comment with . (same with other comments in this function)
Actually, maybe we don't even need these comments?

> Source/WebCore/platform/graphics/angle/Extensions3DANGLE.cpp:94
> +	       compiler.setResources(ANGLEResources);

Is getting/setting the resources expensive? If not, it might read easier if
this function just does something like:

ANGLEWebKitBridge& compiler = m_context->m_compiler;
ShBuiltInResources ANGLEResources = compiler.getResources();

if (name == "GL_OES_standard_derivatives")
  ANGLEResources.OES_standard_derivatives = 1;
else if ....

compiler.setResources(ANGLEResources);

> Source/WebCore/platform/graphics/angle/Extensions3DANGLE.cpp:128
> +	   ANGLEWebKitBridge& compiler = m_context->m_compiler;
> +	   return compiler.getResources().OES_standard_derivatives;

I wouldn't bother with the local variable here.

> Source/WebCore/platform/graphics/angle/Extensions3DANGLE.cpp:154
> +	   return ""; // Invalid shader type.

Use emptyString();

> Source/WebCore/platform/graphics/angle/Extensions3DANGLE.cpp:159
> +	   return "";

Use emptyString();

> Source/WebCore/platform/graphics/angle/Extensions3DANGLE.cpp:182
> +	   return "";

emptyString();

> Source/WebCore/platform/graphics/angle/Extensions3DANGLE.h:69
> +    // EXT Robustness - uses getGraphicsResetStatusARB()
> +    void readnPixelsEXT(int x, int y, GC3Dsizei width, GC3Dsizei height,
GC3Denum format, GC3Denum type, GC3Dsizei bufSize, void *data) override;
> +    void getnUniformfvEXT(GC3Duint program, int location, GC3Dsizei bufSize,
float *params) override;
> +    void getnUniformivEXT(GC3Duint program, int location, GC3Dsizei bufSize,
int *params) override;

Should we just leave these out for now since they aren't implemented?

> Source/WebCore/platform/graphics/angle/Extensions3DANGLE.h:88
> +    // Weak pointer back to GraphicsContext3D

Nit: End with .

> Source/WebCore/platform/graphics/angle/GraphicsContext3DANGLE.cpp:33
> +#include "GraphicsContext3DIOS.h"

I really need to get rid of this horrible hack. Adopting ANGLE will definitely
help!

> Source/WebCore/platform/graphics/angle/GraphicsContext3DANGLE.cpp:277
> +    // clamp the maximum to 8192, which is big enough for a 5K display.

Oops. We just announced a 6K display :(