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

[bugzilla-daemon at webkit.org]

James Robinson <jamesr at chromium.org> has denied 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 75787: Patch
https://bugs.webkit.org/attachment.cgi?id=75787&action=review

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