[webkit-reviews] review denied: [Bug 45939] Add entry points to GraphicsContext3D needed for Chromium compositor port : [Attachment 67871] Patch

[bugzilla-daemon at webkit.org]

Chris Marrin <cmarrin at apple.com> has denied  review:
Bug 45939: Add entry points to GraphicsContext3D needed for Chromium compositor
port
https://bugs.webkit.org/show_bug.cgi?id=45939

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

------- Additional Comments from Chris Marrin <cmarrin at apple.com>
View in context:
https://bugs.webkit.org/attachment.cgi?id=67871&action=prettypatch

I really think it is a mistake to commit this change in its current state.
Please consider backing it out and following my recommendations.

> WebCore/platform/graphics/GraphicsContext3D.h:772
> +

I don't like having explicit 'supports' calls like this. If you want to start
implementing an extension mechanism, you should do it the way we've defined it
in WebGL. Make a base class called Extension3D or something and add a
getExtension() call which returns one. Then make a concrete subclass for your
extension and add the appropriate calls to that. You can then cast the return
from getExtension to this subclass and use it. It would probably be best to not
keep a reference to the GC3D in this class to avoid lifetime issues. You can
just have the API pass it for each call. 

You should also be consistent about using the GL_ prefix with all your
extensions.

>>> WebCore/platform/graphics/mac/GraphicsContext3DMac.mm:103
>>> +	 UNUSED_PARAM(renderDirectlyToHostWindow);
>> 
>> instead of UNUSED_PARAM() just don't name the parameter
> 
> Will do.

I disagree. This is the right way to handle this, especially since this is a
boolean. It's better for documentation.

>>> WebCore/platform/graphics/qt/GraphicsContext3DQt.cpp:506
>>> +	 UNUSED_PARAM(renderDirectlyToHostWindow);
>> 
>> instead of using UNUSED_PARAM() here, just don't name the parameter
> 
> Will do.

Again, doing it this way is better for documentation.