[webkit-reviews] review granted: [Bug 40316] Implement extension entry points and remove EXTENSIONS enum : [Attachment 76142] Patch

[bugzilla-daemon at webkit.org]

James Robinson <jamesr at chromium.org> has granted Kenneth Russell
<kbr at google.com>'s request for review:
Bug 40316: Implement extension entry points and remove EXTENSIONS enum
https://bugs.webkit.org/show_bug.cgi?id=40316

Attachment 76142: Patch
https://bugs.webkit.org/attachment.cgi?id=76142&action=review

------- Additional Comments from James Robinson <jamesr at chromium.org>
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 :)