The extension-related entry points getSupportedExtensions and getExtension must be implemented. The EXTENSIONS enum must be removed; passing it to getParameter must return INVALID_ENUM.
Created attachment 75588 [details] Patch
Attachment 75588 [details] did not build on chromium: Build output: http://queues.webkit.org/results/6738029
Note: in a full Chromium build, this patch depends on http://codereview.chromium.org/5626008 . I'll roll forward the Chromium dependencies in WebKit once the other changes are committed. I'll also submit a revised patch fixing the compile warning.
Created attachment 75592 [details] Patch
Created attachment 75787 [details] Patch
Comment on attachment 75787 [details] Patch Based on offline feedback from James Robinson, renamed m_availableExtensions to m_enabledExtensions in Chromium implementation to be clearer.
Attachment 75787 [details] did not pass style-queue: Failed to run "[u'git', u'reset', u'--hard', u'refs/remotes/trunk']" exit_code: 128 error: Could not write new index file. fatal: Could not reset index file to revision 'refs/remotes/trunk'. If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 75787 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=75787&action=review R- for the bindings issue, but I left a number of other comments. I'm still not sure whether I completely understand the chromium request/enable extension system so some of my comments may be totally off base. Please let me know if so :) > WebCore/bindings/js/JSWebGLRenderingContextCustom.cpp:167 > +static JSValue toJS(ExecState* exec, JSDOMGlobalObject* globalObject, WebGLExtension* extension) As with the V8 binding you have to create a hidden reference from the context on the extension object. The JavaScriptCore function is called markDOMObjectWrapper() but I don't know exactly how it works. > WebCore/bindings/v8/custom/V8WebGLRenderingContextCustom.cpp:156 > +static v8::Handle<v8::Value> toV8Object(WebGLExtension* extension) I'm afraid this binding code isn't quite enough - the WebGL spec requires that all calls to getExtension("foo") on the same context return the _same_ object. That means we have to hold the javascript wrapper object alive for the life of the context or the JS wrapper might get garbage collected and any expandos set on it will be lost. Example JS: ctx.getExtension("foo").myProperty = 2; // a bunch of javascript that triggers GC window.alert(ctx.getExtension("foo").myProperty); // should produce '2' I believe the way to do this is to create a hidden reference from the context to the extension object. See V8DOMTokenListCustom.cpp for an example of how this works (it uses V8DOMWrapper::setHiddenReference() under the hood). Could you also add a layout test for this (you can use window.GCController.collect() to force GC in a test, see fast/dom/HTMLElement/class-list-gc.html as an example). This sort of stuff is historically regression-prone. > WebCore/html/canvas/WebGLRenderingContext.cpp:3626 > + if (type == GraphicsContext3D::FLOAT && m_oesTextureFloat) seems odd to check 'type' in the default: clause of a switch statement on 'type'. maybe: case GraphicsContext3D::FLOAT: if (m_oesTextureFloat) break; instead? > WebCore/html/canvas/WebGLRenderingContext.cpp:3638 > + && type != GraphicsContext3D::FLOAT) { why doesn't this function check for the extension being enabled when it checks for ::FLOAT? > WebCore/html/canvas/WebGLRenderingContext.h:171 > + Vector<String> getSupportedExtensions(); Creating+returning a new Vector<> by value is not cheap (although it looks like NVRO will save one copy here). From what I can tell this list will never change for the lifetime of a WebGL context here, so could we cache this list on the WebGLRenderingContext and return a const Vector<String>& here instead. > WebCore/html/canvas/WebGLRenderingContext.h:466 > + // Enabled extension objects. > + RefPtr<OESTextureFloat> m_oesTextureFloat; What's the plan to scale this when we support more extensions? I wonder if something like a HashMap<String, WebGLExtension> would be a better fit (assuming that each extension is a singleton per context). > WebCore/platform/graphics/GraphicsContext3D.cpp:1089 > + default: > + ASSERT(false); > + } I know this isn't your doing, but why does this switch have a default: case? It's a switch on an enum and we should handle every case, so it seems like a good time to leave out the default: and let the compiler complain if we miss a case. Otherwise ASSERT_NOT_REACHED(). > WebCore/platform/graphics/chromium/Extensions3DChromium.h:45 > + virtual bool ensureEnabled(const String&); I'm not sure what "ensure" means here - why not just call this enable() and have it return a bool indicating whether the extension was successfully enabled? "ensure" makes me think that this function promises that the extension is turned on, but this doesn't. > WebKit/chromium/public/WebGraphicsContext3D.h:145 > + virtual void requestExtensionCHROMIUM(const char*) = 0; It's unfortunate to use const char* for a string type here. I think the best type to use would be WebString, or possibly WebCString if we want to ASCII-encode the value before passing it to the API. WebString would be more consistent with getRequestableExtensionsCHROMIUM(). Also I think getAvailableExtensionsCHROMIUM() would make a bit more sense (see my comment about m_requestableExtensions further down). > WebKit/chromium/src/GraphicsContext3DChromium.cpp:731 > +bool GraphicsContext3DInternal::ensureExtensionEnabled(const String& name) > +{ > + initializeExtensions(); > + > + if (m_enabledExtensions.contains(name)) > + return true; > + > + if (m_requestableExtensions.contains(name)) { > + m_impl->requestExtensionCHROMIUM(name.ascii().data()); > + m_enabledExtensions.clear(); > + m_requestableExtensions.clear(); > + m_initializedAvailableExtensions = false; > + } > + > + initializeExtensions(); > + return m_enabledExtensions.contains(name); I'm a bit confused - if an extension "foo" is in m_impl->getRequestableExtensionsCHROMIUM() will a call to m_impl->requestExtensionCHROMIUM("foo") not always actually enable the extension? > WebKit/chromium/src/GraphicsContext3DInternal.h:290 > - HashSet<String> m_availableExtensions; > + HashSet<String> m_enabledExtensions; > + HashSet<String> m_requestableExtensions; I think m_availableExtensions is a better name than m_requestableExtensions if it is the set of extensions that are available to be enabled. "requestable" to me implies that a request to enable a extension from this test could be denied, which isn't the case as I understand it.
Attachment 75787 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/update-webkit']" exit_code: 2 Updating OpenSource Incomplete data: Delta source ended unexpectedly at /usr/lib/git-core/git-svn line 5061 Died at WebKitTools/Scripts/update-webkit line 132. If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 76000 [details] Patch
(In reply to comment #11) > (From update of attachment 75787 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=75787&action=review > > R- for the bindings issue, but I left a number of other comments. I'm still not sure whether I completely understand the chromium request/enable extension system so some of my comments may be totally off base. Please let me know if so :) > > > WebCore/bindings/js/JSWebGLRenderingContextCustom.cpp:167 > > +static JSValue toJS(ExecState* exec, JSDOMGlobalObject* globalObject, WebGLExtension* extension) > > As with the V8 binding you have to create a hidden reference from the context on the extension object. The JavaScriptCore function is called markDOMObjectWrapper() but I don't know exactly how it works. > > > WebCore/bindings/v8/custom/V8WebGLRenderingContextCustom.cpp:156 > > +static v8::Handle<v8::Value> toV8Object(WebGLExtension* extension) > > I'm afraid this binding code isn't quite enough - the WebGL spec requires that all calls to getExtension("foo") on the same context return the _same_ object. That means we have to hold the javascript wrapper object alive for the life of the context or the JS wrapper might get garbage collected and any expandos set on it will be lost. Example JS: > > ctx.getExtension("foo").myProperty = 2; > // a bunch of javascript that triggers GC > window.alert(ctx.getExtension("foo").myProperty); // should produce '2' > > I believe the way to do this is to create a hidden reference from the context to the extension object. See V8DOMTokenListCustom.cpp for an example of how this works (it uses V8DOMWrapper::setHiddenReference() under the hood). Could you also add a layout test for this (you can use window.GCController.collect() to force GC in a test, see fast/dom/HTMLElement/class-list-gc.html as an example). This sort of stuff is historically regression-prone. Thanks for catching this. I verified that the bindings were buggy as you pointed out and have fixed them in the current patch, verifying in DRT for WebKit using the GCController and in Chromium by allocating enough memory to force a GC. (I haven't yet implemented OES_texture_float support for Chromium's in-process WebGL implementation and therefore DRT.) > > WebCore/html/canvas/WebGLRenderingContext.cpp:3626 > > + if (type == GraphicsContext3D::FLOAT && m_oesTextureFloat) > > seems odd to check 'type' in the default: clause of a switch statement on 'type'. maybe: > > case GraphicsContext3D::FLOAT: > if (m_oesTextureFloat) > break; > > instead? I anticipate adding more types (HALF_FLOAT in particular), so using fall-through doesn't work. I've changed this to just use a switch and duplicated the code which synthesizes INVALID_ENUM (not a big deal). > > WebCore/html/canvas/WebGLRenderingContext.cpp:3638 > > + && type != GraphicsContext3D::FLOAT) { > > why doesn't this function check for the extension being enabled when it checks for ::FLOAT? It's the same function. It's checked above. > > WebCore/html/canvas/WebGLRenderingContext.h:171 > > + Vector<String> getSupportedExtensions(); > > Creating+returning a new Vector<> by value is not cheap (although it looks like NVRO will save one copy here). From what I can tell this list will never change for the lifetime of a WebGL context here, so could we cache this list on the WebGLRenderingContext and return a const Vector<String>& here instead. This code isn't performance critical and I think at this point that adding a cache is more trouble than it's worth. If this turns up hot in some WebGL apps I'll change it if that's OK. > > WebCore/html/canvas/WebGLRenderingContext.h:466 > > + // Enabled extension objects. > > + RefPtr<OESTextureFloat> m_oesTextureFloat; > > What's the plan to scale this when we support more extensions? I wonder if something like a HashMap<String, WebGLExtension> would be a better fit (assuming that each extension is a singleton per context). Not 100% sure yet. As you can see there are cases in the code where we need to test (quickly, and ideally without a HashMap lookup) whether a particular extension has been enabled. > > WebCore/platform/graphics/GraphicsContext3D.cpp:1089 > > + default: > > + ASSERT(false); > > + } > > I know this isn't your doing, but why does this switch have a default: case? It's a switch on an enum and we should handle every case, so it seems like a good time to leave out the default: and let the compiler complain if we miss a case. Otherwise ASSERT_NOT_REACHED(). I took a shortcut here because this routine handles only a few out of a fairly large set of enums. Based on feedback on the WebGL public mailing list I'm probably going to have to generalize this code anyway to handle all of the cases, but I want to do that in a follow-up patch. I've changed it to use ASSERT_NOT_REACHED() for now. > > WebCore/platform/graphics/chromium/Extensions3DChromium.h:45 > > + virtual bool ensureEnabled(const String&); > > I'm not sure what "ensure" means here - why not just call this enable() and have it return a bool indicating whether the extension was successfully enabled? "ensure" makes me think that this function promises that the extension is turned on, but this doesn't. The only reason I used "ensure" rather than "enable" is because the extension might already be enabled. I've left this alone for now but will change it if you feel strongly. > > WebKit/chromium/public/WebGraphicsContext3D.h:145 > > + virtual void requestExtensionCHROMIUM(const char*) = 0; > > It's unfortunate to use const char* for a string type here. I think the best type to use would be WebString, or possibly WebCString if we want to ASCII-encode the value before passing it to the API. WebString would be more consistent with getRequestableExtensionsCHROMIUM(). All of the rest of the entry points on this class that receive (rather than return) strings take const char*, so for consistency I used const char*. > Also I think getAvailableExtensionsCHROMIUM() would make a bit more sense (see my comment about m_requestableExtensions further down). I've replied to this below. > > WebKit/chromium/src/GraphicsContext3DChromium.cpp:731 > > +bool GraphicsContext3DInternal::ensureExtensionEnabled(const String& name) > > +{ > > + initializeExtensions(); > > + > > + if (m_enabledExtensions.contains(name)) > > + return true; > > + > > + if (m_requestableExtensions.contains(name)) { > > + m_impl->requestExtensionCHROMIUM(name.ascii().data()); > > + m_enabledExtensions.clear(); > > + m_requestableExtensions.clear(); > > + m_initializedAvailableExtensions = false; > > + } > > + > > + initializeExtensions(); > > + return m_enabledExtensions.contains(name); > > I'm a bit confused - if an extension "foo" is in m_impl->getRequestableExtensionsCHROMIUM() will a call to m_impl->requestExtensionCHROMIUM("foo") not always actually enable the extension? It *should* unless something has gone wrong. I wanted to admit that possibility rather than (for example) asserting. > > WebKit/chromium/src/GraphicsContext3DInternal.h:290 > > - HashSet<String> m_availableExtensions; > > + HashSet<String> m_enabledExtensions; > > + HashSet<String> m_requestableExtensions; > > I think m_availableExtensions is a better name than m_requestableExtensions if it is the set of extensions that are available to be enabled. "requestable" to me implies that a request to enable a extension from this test could be denied, which isn't the case as I understand it. The Chromium-specific OpenGL extension APIs I added for this functionality (in src/gpu/GLES2/gl2ext.h in a Chromium tree) are glRequestExtensionCHROMIUM and glGetRequestableExtensionsCHROMIUM to have a consistent naming convention. I agreed with your suggestion to rename "available" to "enabled" but think that for consistency it is best to leave m_requestableExtensions named as is. Also, I definitely think that glGetRequestableExtensionsCHROMIUM should not be renamed to glGetAvailableExtensionsCHROMIUM (as your suggested change to WebGraphicsContext3D.h would imply) because then the APIs which request, and which find which extensions can be requested, on the current hardware would be named differently.
I did a bit more reading - turns out I didn't understanding the WebGL extension mechanism fully. This is a bit trickier than I thought :). Please let me know if the following are true: 1) In "normal" OpenGL, a context has an unchanging list of supported extensions. These can be queried by doing glGetString(GL_EXTENSIONS) and are always "on" (as in available). 2) In WebGL, no equivalent of glGetString(GL_EXTENSIONS) is supported. Instead, an author can get a list of available extensions and then "turn them on" by calling getExtension(). There is no API to ask a WebGL context which extensions are currently enabled, but it's safe to call getExtension() multiple times with the same extension name. 3) The WebGL context is required to enforce that any attempt to use an extension before enabling it throws a GL error. 4) An attempt from an author to enable an extension via getExtension() from the list of names returned by getSupportedExtensions() is required to succeed. Are there any requirements of (3) that cannot be handled purely at the WebGL binding layer? It's a bit unclear if the chromium command buffer glRequestExtensionCHROMIUM() notification will change observable behavior. I.e. will it cause errors to be generated or not generated in some cases? Can enabling an extension change the set of shaders that are accepted?
Comment on attachment 76000 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=76000&action=review > WebCore/html/canvas/WebGLRenderingContext.cpp:1475 > + if (name == "OES_texture_float" > + && m_context->getExtensions()->supports("GL_OES_texture_float")) { > + if (!m_oesTextureFloat) { > + if (!m_context->getExtensions()->ensureEnabled("GL_OES_texture_float")) > + return 0; > + m_oesTextureFloat = OESTextureFloat::create(); > + } > + return m_oesTextureFloat.get(); The WebGL spec for getSupportedExtensions() says "Any string in this list, when passed to getExtension must return a valid object", which seems inconsistent with the inner check here; if we return "OES_texture_float" in getSupportedExtensions() then a call to getExtension("OES_texture_float") must succeed. Maybe the spec needs to be updated? > WebCore/platform/graphics/Extensions3D.h:62 > + // Certain OpenGL and WebGL implementations may support enabling > + // extensions lazily. Returns true if the given extension has been > + // successfully enabled. > + virtual bool ensureEnabled(const String&) = 0; I don't think the bool return value here makes sense given WebGL extension semantics (see above). I think it's reasonable to ASSERT() if someone calls this function without checking supports() first, since that would be a clear caller error.
(In reply to comment #15) > I did a bit more reading - turns out I didn't understanding the WebGL extension mechanism fully. This is a bit trickier than I thought :). Please let me know if the following are true: > > 1) In "normal" OpenGL, a context has an unchanging list of supported extensions. These can be queried by doing glGetString(GL_EXTENSIONS) and are always "on" (as in available). Correct. > 2) In WebGL, no equivalent of glGetString(GL_EXTENSIONS) is supported. Instead, an author can get a list of available extensions and then "turn them on" by calling getExtension(). There is no API to ask a WebGL context which extensions are currently enabled, but it's safe to call getExtension() multiple times with the same extension name. Correct. > 3) The WebGL context is required to enforce that any attempt to use an extension before enabling it throws a GL error. Correct. > 4) An attempt from an author to enable an extension via getExtension() from the list of names returned by getSupportedExtensions() is required to succeed. Correct. > Are there any requirements of (3) that cannot be handled purely at the WebGL binding layer? It's a bit unclear if the chromium command buffer glRequestExtensionCHROMIUM() notification will change observable behavior. I.e. will it cause errors to be generated or not generated in some cases? Can enabling an extension change the set of shaders that are accepted? We want to use the glRequestExtensionCHROMIUM entry point to assist in the implementation of planned WebGL extensions, specifically OES_standard_derivatives. This affects the behavior of the shader validator / translator, and having the ability to dynamically turn on extensions eliminates a duplicate instance of the shader compiler.
(In reply to comment #16) > (From update of attachment 76000 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=76000&action=review > > > WebCore/html/canvas/WebGLRenderingContext.cpp:1475 > > + if (name == "OES_texture_float" > > + && m_context->getExtensions()->supports("GL_OES_texture_float")) { > > + if (!m_oesTextureFloat) { > > + if (!m_context->getExtensions()->ensureEnabled("GL_OES_texture_float")) > > + return 0; > > + m_oesTextureFloat = OESTextureFloat::create(); > > + } > > + return m_oesTextureFloat.get(); > > The WebGL spec for getSupportedExtensions() says "Any string in this list, when passed to getExtension must return a valid object", which seems inconsistent with the inner check here; if we return "OES_texture_float" in getSupportedExtensions() then a call to getExtension("OES_texture_float") must succeed. Maybe the spec needs to be updated? The spec is correct. The implementation here is just being conservative. > > WebCore/platform/graphics/Extensions3D.h:62 > > + // Certain OpenGL and WebGL implementations may support enabling > > + // extensions lazily. Returns true if the given extension has been > > + // successfully enabled. > > + virtual bool ensureEnabled(const String&) = 0; > > I don't think the bool return value here makes sense given WebGL extension semantics (see above). I think it's reasonable to ASSERT() if someone calls this function without checking supports() first, since that would be a clear caller error. After our offline discussion I'll change this to ASSERT and return void.
Created attachment 76142 [details] Patch
Comment on attachment 76142 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=76142&action=review R=me, just a few nits. Thanks for all the explanation of the system, it was very enlightening. > WebCore/html/canvas/WebGLRenderingContext.cpp:1468 > + if (name == "OES_texture_float" the WebGL spec says that extension names are case-insensitive, so this should be a case-insensitive match. WTFString.h has a handy equalIgnoringCase() utility function. > WebCore/platform/graphics/Extensions3D.h:62 > + // Certain OpenGL and WebGL implementations may support enabling > + // extensions lazily. This method may only be called with > + // extension names for which supports returns true. > + virtual void ensureEnabled(const String&) = 0; Can you file bugs on the GraphicsContext3D(OpenGL|Qt) implementations to add support for this? Currently it seems that the behavior of WebGL shaders and extensions is subtly different for the Chromium vs other implementations. > WebKit/chromium/public/WebGraphicsContext3D.h:145 > + virtual void requestExtensionCHROMIUM(const char*) = 0; There aren't many other cases in the WebKit API where we pass string data using const char*, so I'm pretty sure this is just wrong. I'll promise to fix the rest of the instances in this header if you fix this one to use a real string type :)
(In reply to comment #20) > (From update of attachment 76142 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=76142&action=review > > R=me, just a few nits. Thanks for all the explanation of the system, it was very enlightening. > > > WebCore/html/canvas/WebGLRenderingContext.cpp:1468 > > + if (name == "OES_texture_float" > > the WebGL spec says that extension names are case-insensitive, so this should be a case-insensitive match. WTFString.h has a handy equalIgnoringCase() utility function. Thanks, good catch -- will fix upon landing. > > WebCore/platform/graphics/Extensions3D.h:62 > > + // Certain OpenGL and WebGL implementations may support enabling > > + // extensions lazily. This method may only be called with > > + // extension names for which supports returns true. > > + virtual void ensureEnabled(const String&) = 0; > > Can you file bugs on the GraphicsContext3D(OpenGL|Qt) implementations to add support for this? Currently it seems that the behavior of WebGL shaders and extensions is subtly different for the Chromium vs other implementations. I don't think this is necessary. The other ports won't return any extensions in supports() that aren't already in the extensions string, so ensureEnabled() on those ports is a no-op. > > WebKit/chromium/public/WebGraphicsContext3D.h:145 > > + virtual void requestExtensionCHROMIUM(const char*) = 0; > > There aren't many other cases in the WebKit API where we pass string data using const char*, so I'm pretty sure this is just wrong. I'll promise to fix the rest of the instances in this header if you fix this one to use a real string type :) Per our offline discussion I've filed https://bugs.webkit.org/show_bug.cgi?id=50794 to clean up all instances. I don't want to hold this patch for this cleanup because it requires two-sided changes to Chromium and WebKit.
Attachment 76142 [details] did not build on mac: Build output: http://queues.webkit.org/results/6986020
Comment on attachment 76142 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=76142&action=review > WebCore/platform/graphics/opengl/Extensions3DOpenGL.cpp:90 > +void Extensions3DOpenGL::ensureEnabled(const String& name) > +{ > + ASSERT(supports(name)); Looks like this doesn't compile in release, guess you need an #ifndef NDEBUG UNUSED_PARAM(name) #endif or similar.
Committed r73806: <http://trac.webkit.org/changeset/73806>
(In reply to comment #23) > (From update of attachment 76142 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=76142&action=review > > > WebCore/platform/graphics/opengl/Extensions3DOpenGL.cpp:90 > > +void Extensions3DOpenGL::ensureEnabled(const String& name) > > +{ > > + ASSERT(supports(name)); > > Looks like this doesn't compile in release, guess you need an #ifndef NDEBUG UNUSED_PARAM(name) #endif or similar. Fixed in landed version.
http://trac.webkit.org/changeset/73806 might have broken SnowLeopard Intel Release (Build)