[webkit-reviews] review granted: [Bug 36262] Add GraphicsContext3D abstraction to WebKit API : [Attachment 51068] Revised patch

[bugzilla-daemon at webkit.org]

Darin Fisher (:fishd, Google) <fishd at chromium.org> has granted Kenneth Russell
<kbr at google.com>'s request for review:
Bug 36262: Add GraphicsContext3D abstraction to WebKit API
https://bugs.webkit.org/show_bug.cgi?id=36262

Attachment 51068: Revised patch
https://bugs.webkit.org/attachment.cgi?id=51068&action=review

------- Additional Comments from Darin Fisher (:fishd, Google)
<fishd at chromium.org>
> Index: WebKit/chromium/public/WebGraphicsContext3D.h
...
> +    struct Attributes {
> +	   Attributes()
> +		   : alpha(true)
> +		   , depth(true)
> +		   , stencil(true)
> +		   , antialias(true)
> +		   , premultipliedAlpha(true)

nit: ^^^ indent by 4 spaces


> +    // Typedef for server-side objects like OpenGL textures and program
objects.
> +    typedef unsigned int WebGLId;

nit: since this typedef has the Web prefix, I'd recommend just moving it
out of WebGraphicsContext3D and into the WebKit namespace.  anyways, it
seems like it might be convenient to not have to type WebGraphicsContext3D
in front of WebGLId in Chromium code.


> +    // Creates a "default" implementation of WebGraphicsContext3D which, in
> +    // Chromium, requires the sandbox to be disabled (strongly discouraged).

> +    static WebGraphicsContext3D* createDefault();

nit: we generally try to avoid references back to chromium specific things
since they can bit rot.  maybe it is enough to just say that this default
implementation uses OpenGL directly.


> +    // Initializes the graphics context; should be the first operation
performed
> +    // on newly-constructed instances. Returns true on success.
> +    virtual bool initialize(Attributes attributes) = 0;

nit: webkit style is to leave off the parameter name when the parameter
name matches the parameter type (i.e., the name doesn't add information).


> +    // Helper for software compositing path. Reads back the frame buffer
into
> +    // the memory region pointed to by "pixels". It is expected that the
storage
> +    // for "pixels" covers (4 * width * height) bytes.
> +    virtual void readBackFramebuffer(unsigned char* pixels) = 0;

nit: perhaps we should have the caller specify a size_t parameter here so
that it will be possible for the callee to check it at runtime.  this call
is already quite expensive so such a runtime check is not going to be an
issue.	it will help avoid memory corruption issues perhaps.


> Index: WebKit/chromium/public/WebKitClient.h
...
> +    // May return null if WebGL is not supported.
> +    // Returns newly allocated WebGraphicsContext3D instance; caller must
delete.
> +    virtual WebGraphicsContext3D* createGraphicsContext3D() { return 0; }

nit: The "createFoo" pattern implies "caller must delete", so it is okay
to leave that out of the comment.  but, redundancy is ok too.  i just wanted
to share that with you so you know the convention.


> Index: WebKit/chromium/src/GraphicsContext3D.cpp

> +// There are two levels of delegation in this file:
> +//
> +//	1. GraphicsContext3D delegates to GraphicsContext3DInternal. This is
done
> +//	   so that we have some place to store data members common among
> +//	   implementations; GraphicsContext3D only provides us the m_internal
> +//	   pointer. We always delegate to the GraphicsContext3DInternal. While
we
> +//	   could sidestep it and go directly to the WebGraphicsContext3D in
some
> +//	   cases, it is better for consistency to always delegate through it.
> +//
> +//	2. GraphicsContext3DInternal delegates to an implementation of
> +//	   WebGraphicsContext3D. This is done so we have a place to inject our
> +//	   command buffer implementation.
> +//
> +// The legacy, in-process, implementation uses
WebGraphicsContext3DDefaultImpl.
> +// The command buffer implementation will use a to-be-decided class in
Chrome.

nit: ^^^ again, it would be good to avoid comments that refer to Chrome like
this since they can get stale.	when that class is no longer to-be-decided,
will you remember to go back and revise this comment?  i would just leave off
the last sentence since point #2 basically implies the same thing.

R=me w/ nits addressed.