GraphicsContext3D is a useful abstraction for platforms support variants of OpenGL as well as containing WebGL specific logic. It would be great if the WebGL specific logic was split into a separate class. See discussion on https://bugs.webkit.org/show_bug.cgi?id=43210 for one use case..
GraphicsContext3D lives in platform/graphics but uses types (like WebGLProgram) defined in WebCore/html/canvas/. This is a layering violation, platform/ code is not supposed to be aware of anything in WebCore outside of platform/. It appears from a brief inspection that there are not any true dependencies on WebGL-isms from the types that GraphicsContext3D references, but it will take a bit of work to verify this.
Looking at the code, I think there are two issues: 1) ArrayBuffer and friends. These used to be called WebGL...Array, but have since been given more generic names. I think these should just be moved into WebCore/platform. 2) CanvasObject and subclasses. A cursory glance shows that these are not really necessary and the underlying Platform3DObject can be passed instead. So we should leave all these where they are and change the API to GraphicsContext3D to use Platform3DObject. I see one small issue in that all the create calls in GraphicsContext3D (e.g., createShader()) return an unsigned type. This should be Platform3DObject. I understand Ken's concerns, but I don't think making these changes now would be destabilizing to WebGL. They are mostly cosmetic.
Another issue I've seen: 3) paintRenderingResultsToCanvas() uses WebGLRenderingContext and HTMLCanvasElement. It needs to simply create the pixel array, fill it and then pass ownership to the caller. Or maybe it would be easier for the caller to create the array and pass it in. The caller also probably needs to make the paintToCanvas call.
Created attachment 63297 [details] Patch
This patch takes care of CanvasObject and friends and does the simplest thing possible for paintRenderingResultsToCanvas(). Since this patch is already over 100kb, I think it's better to land this one first and then work on the rest as separate patches lest the WebKit reviewers strangle me in my sleep. I can move the ArrayBuffer, etc, types to platform/ and take care of the functions that should return Platform3DObjects next.
Comment on attachment 63297 [details] Patch WebCore/html/canvas/WebGLRenderingContext.cpp:223 + m_context->attachShader(program ? program->object() : 0, shader ? shader->object() : 0); This null checking of program seems unnecessary given the null test done by validateWebGLObject. hmm... should validateWebGLObject be modified to also null check program->object()? same goes for the rest of the null checking in this file. WebCore/html/canvas/WebGLRenderingContext.cpp:330 + static const int kTextureWrapR = 0x8072; nit: kTextureWrapR -> textureWrapR otherwise, R=me this is very nice cleanup of GC3D. it was always bothersome that code in platform/graphics depended back on code in html/canvas.
I didn't want to depend on the validateXX() functions returning false for NULL pointers, mostly so readers do not have to check what the validate functions to do know how the function handles NULL. program->object() is just an integer and doesn't need its own check.
Committed r64582: <http://trac.webkit.org/changeset/64582>
First bit (switching from WebGLProgram/type/etc) to Platform3DObjects. Next: - move ArrayBuffer and friends to platform/graphics - audit uses of 'unsigned' to see if they really mean 'Platform3DObject'. It looks like the createFoo() and deleteFoo() functions really mean Platform3DObject
There is one small issue being caused by these changes. We're adding shader validation to WebGL (using ANGLE). This was being done in GraphicsContext3D because, in addition to validation, the shader is translated as needed for output to desktop OpenGL or D3D. Putting it in GraphicsContext3D made this transparent. But Paul was putting the results of the validation into the WebGLShader object for later return to the author on demand. Now that WebGLShader is no longer passed to GraphicsContext3D we can no longer do this. For now I think we will put the shader validator in WebGLRenderingContext which does have access to WebGLShader. This fits better with our new "don't do WebGL specific things in GraphicsContext3D" model. But it does mean that we will possibly have to parse the shader twice: once in WebGLRenderingContext to do the validation and once in GraphicsContext3D to do the translation. This may not be that bad since shader's tend not to be large. But we may want to rethink this decision later.
I think it'd be nice to create non-WebGL specific types for shader, program, texture etc that lived in platform/graphics and use those for GraphicsContext3D. That would provide a natural place to store additional information like this and also make the API slightly more type safe.
Created attachment 63401 [details] Patch (re-)introducing EXTRACT macro From the ChangeLog: Added a helper macro to extract the contents of WebGL objects to reduce duplicated code and fix a couple of potential crashes introduced in the previous refactoring.
Comment on attachment 63401 [details] Patch (re-)introducing EXTRACT macro WebCore/html/canvas/WebGLRenderingContext.cpp:58 + #define EXTRACT(x) ((x) ? (x)->object() : 0) I think this is the wrong change. Why would we generically pass a 0 to GraphicsContext3D when we don't have a proper object? It would be better to avoid the call and generate the appropriate WebGL error.
(In reply to comment #11) > I think it'd be nice to create non-WebGL specific types for shader, program, texture etc that lived in platform/graphics and use those for GraphicsContext3D. That would provide a natural place to store additional information like this and also make the API slightly more type safe. I'm not sure we should go down the road of "type safety" with GraphicsContext3D. We'd have a lot more work to do it right. I agree with your desire to make GraphicsContext3D very lightweight and let the higher level logic keep it safe.
(In reply to comment #13) > (From update of attachment 63401 [details]) > WebCore/html/canvas/WebGLRenderingContext.cpp:58 > + #define EXTRACT(x) ((x) ? (x)->object() : 0) > I think this is the wrong change. Why would we generically pass a 0 to GraphicsContext3D when we don't have a proper object? It would be better to avoid the call and generate the appropriate WebGL error. 0, or null at the JavaScript level, is a perfectly valid value to pass to many of the entry points, for example when detaching a renderbuffer from a framebuffer object. The entry points which do not accept null already have validation which synthesizes the appropriate OpenGL error. This change only simplifies uses of the ternary operator throughout this file, restoring logic that was previously inside the GraphicsContext3D implementations, and avoiding possible crashes that have been introduced in handleNPOTTextures() and restoreStatesAfterVertexAttrib0Simulation() due to dereferencing of a possibly NULL object.
(In reply to comment #15) > (In reply to comment #13) > > (From update of attachment 63401 [details] [details]) > > WebCore/html/canvas/WebGLRenderingContext.cpp:58 > > + #define EXTRACT(x) ((x) ? (x)->object() : 0) > > I think this is the wrong change. Why would we generically pass a 0 to GraphicsContext3D when we don't have a proper object? It would be better to avoid the call and generate the appropriate WebGL error. > > 0, or null at the JavaScript level, is a perfectly valid value to pass to many of the entry points, for example when detaching a renderbuffer from a framebuffer object. The entry points which do not accept null already have validation which synthesizes the appropriate OpenGL error. > > This change only simplifies uses of the ternary operator throughout this file, restoring logic that was previously inside the GraphicsContext3D implementations, and avoiding possible crashes that have been introduced in handleNPOTTextures() and restoreStatesAfterVertexAttrib0Simulation() due to dereferencing of a possibly NULL object. Ah, right, I forgot about that. Could we at least do this with an inline function rather than a macro?
... > > Ah, right, I forgot about that. Could we at least do this with an inline function rather than a macro? And a more specific name than extract would be good, too. objectOrNull() or something?
(In reply to comment #17) > ... > > > > Ah, right, I forgot about that. Could we at least do this with an inline function rather than a macro? > > And a more specific name than extract would be good, too. objectOrNull() or something? Sure. I'll upload a revised patch shortly.
Comment on attachment 63297 [details] Patch Cleared Darin Fisher's review+ from obsolete attachment 63297 [details] so that this bug does not appear in http://webkit.org/pending-commit.
Created attachment 63466 [details] Patch introducing objectOrZero helper function Addressed Chris Marrin's review feedback.
Committed r64660: <http://trac.webkit.org/changeset/64660>