Bug 42203 – [chromium] GLES2Context is in platform/ and shouldn't depend on Page

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

[James Robinson]

[chromium] GLES2Context is in platform/ and shouldn't depend on Page

[James Robinson]

Created attachment 61428 [details]
Patch

[James Robinson]

My plan for creating GLES2Contexts from WebCore is to have two functions on ChromeClient:

// Returns a GLES2Context used for rendering directly to the screen
PassOwnPtr<GLES2Context> getOnscreenGLES2Context();
// Returns a GLES2Context for rendering offscreen that is capable of sharing
// resources with the compositor's context
PassOwnPtr<GLES2Context> getOffscreenGLES2Context();

The lifetime of GLES2Contexts is still kind of awkward.  Making two calls to getOnscreenGLES2Context() will return two OwnPtr<>s that each wrap the same underlying object, but deleting a GLES2Context does not destroy the actual underlying object.  Calling GLES2Context::destroy() does do something destructive.  I think it's still valid to have this be an OwnPtr<> to make sure we don't leak the wrapper object (which I believe we do currently).

I can post that as a followup or as a separate patch.  Adding stuff to ChromeClient involves a loooot of plumbing so it's a rather noisy patch.

[James Robinson]

Created attachment 61581 [details]
Patch

[James Robinson]

This patch adds in the onscreen/offscreen getters to ChromeClientChromium.  They are currently uncalled, but that won't be true for long.

One unfortunate thing is that this exposes two types of WebCore::GLES2Context's - those that own their underlying WebKit::WebGLES2Context and those that don't.  When getting the GLES2Context for compositing, the caller doesn't want to have ownership.  When getting an offscreen context the caller does want ownership.  I added a flag to the GLES2ContextInternal constructor to allow for both kinds but it's kind of ugly.  We don't need the ChromeClientChromium::getOnscreenGLES2Context() function yet so we could delete it for now and figure it out later.

[Darin Fisher (:fishd, Google)]

Comment on attachment 61581 [details]
Patch

WebKit/chromium/public/WebGLES2Context.h:49
 +      WEBKIT_API virtual bool initialize(WebView*, WebGLES2Context* parent) = 0;
no need for WEBKIT_API on virtual methods.  no need to export
these from WebKit when it is built as a DLL.
WebKit/chromium/src/GLES2ContextInternal.h:55
 +  }
} // namespace WebCore

WebKit/chromium/src/WebViewImpl.cpp:2139
 +      return WebCore::GLES2Context::create(WebCore::GLES2ContextInternal::create(gles2Context(), false));
no need for the WebCore:: prefix in this .cpp file given the 
using declaration at the top of the file.

WebKit/chromium/src/WebViewImpl.cpp:2142
 +  PassOwnPtr<WebCore::GLES2Context> WebViewImpl::getOffscreenGLES2Context()
ditto about the WebCore:: prefix

otherwise, R=me with these nits addressed.

[James Robinson]

Committed r63383: <http://trac.webkit.org/changeset/63383>

[WebKit Review Bot]

http://trac.webkit.org/changeset/63383 might have broken Chromium Mac Release
The following changes are on the blame list:
http://trac.webkit.org/changeset/63384
http://trac.webkit.org/changeset/63385
http://trac.webkit.org/changeset/63386
http://trac.webkit.org/changeset/63389
http://trac.webkit.org/changeset/63383

[James Robinson]

Fixed the compile error in http://trac.webkit.org/changeset/63392