Bug 41828 – [chromium] Allow resizing an offscreen GLES2Context's backing store and getting its texture ID for compositing

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[James Robinson]

[chromium] Allow resizing an offscreen GLES2Context's backing store and getting its texture ID for compositing

[James Robinson]

Created attachment 60829 [details]
Patch

[James Robinson]

Ken, Darin - mind taking a look?  I added an ASSERT(webContext) rather than returning a bool since currently there is no good way for a caller to handle a failure and because getOffscreenContentParentTextureId naturally returns an unsigned.

[Darin Fisher (:fishd, Google)]

Comment on attachment 60829 [details]
Patch

WebCore/platform/chromium/GLES2Context.h:65
 +      unsigned getOffscreenContentParentTextureId();
i like this name for this method.  it helps me understand its distinction better.

WebCore/platform/chromium/GLES2Context.h:62
 +      void resizeOffscreenContent(const IntSize& size);
nit: webkit style is to leave off parameter names that do not add information.
"size" is redundant with "IntSize"

WebCore/platform/chromium/GLES2Context.h:63
 +      // Returns the ID of the texture used for offscreen rendering in the context of the parent.
nit: a new line above this comment block would be nice for readability.

WebKit/chromium/public/WebGLES2Context.h:52
 +      virtual void resizeOffscreenContent(int width, int height) = 0;
nit: use WebSize in the WebKit API.

WebKit/chromium/public/WebGLES2Context.h:53
 +      virtual unsigned getOffscreenContentParentTextureId() = 0;
how about documenting these methods like you did for GLES2Context.h?
it is important to document the WebKit API methods since that helps
make the contract with the embedder clearer.

WebKit/chromium/src/GLES2Context.cpp:148
 +      webContext->resizeOffscreenContent(size.width(), size.height());
WebSize can be constructed implicitly from IntSize :)

otherwise, LGTM based on this being a faithful interpretation of
what kbr wrote on the whiteboard earlier today :-)

[James Robinson]

Created attachment 60959 [details]
Patch

[Kenneth Russell]

Comment on attachment 60959 [details]
Patch

Looks good to me. As in earlier Chromium review, uint32_t may be a better return type for getOffscreenContentParentTextureId().

[Darin Fisher (:fishd, Google)]

Comment on attachment 60959 [details]
Patch

WebKit/chromium/public/WebGLES2Context.h:54
> +      // The follow two functions are for managing a context that renders offscreen.
nit: put a new line after this comment so that it doesn't look like
this comment is only talking about the first function?

how critical is the unsigned vs uint32_t distinction?  in the webkit api,
we don't really have a uint32_t.  (well, we do have one inherited from
npruntime.h, but it would be odd to use that in this context.)

anyways, r=me modulo the nit above.

[James Robinson]

I don't think uint32_t vs unsigned will be an issue.  If on some crazy moon system 'unsigned' was 64 bits things would still work fine since an unsigned would be able to represent the contents of a uint32_t without issue.  We're using 'unsigned' for the GLuint type for texture IDs and other things elsewhere in WebKit code so I'd rather stick to that.

[Kenneth Russell]

(In reply to comment #7)
> I don't think uint32_t vs unsigned will be an issue.  If on some crazy moon system 'unsigned' was 64 bits things would still work fine since an unsigned would be able to represent the contents of a uint32_t without issue.  We're using 'unsigned' for the GLuint type for texture IDs and other things elsewhere in WebKit code so I'd rather stick to that.

OK, sounds fine.

[James Robinson]

Committed r62874: <http://trac.webkit.org/changeset/62874>

[James Robinson]

Thanks for the review.  Adjusted the space on the comments and landed.