Bug 42203 - [chromium] GLES2Context is in platform/ and shouldn't depend on Page
Summary: [chromium] GLES2Context is in platform/ and shouldn't depend on Page
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: James Robinson
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-07-13 14:43 PDT by James Robinson
Modified: 2010-07-14 19:27 PDT (History)
6 users (show)

See Also:


Attachments
Patch (16.87 KB, patch)
2010-07-13 16:25 PDT, James Robinson
no flags Details | Formatted Diff | Diff
Patch (23.42 KB, patch)
2010-07-14 16:11 PDT, James Robinson
fishd: review+
fishd: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description James Robinson 2010-07-13 14:43:49 PDT
[chromium] GLES2Context is in platform/ and shouldn't depend on Page
Comment 1 James Robinson 2010-07-13 16:25:25 PDT
Created attachment 61428 [details]
Patch
Comment 2 James Robinson 2010-07-13 18:50:07 PDT
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.
Comment 3 James Robinson 2010-07-14 16:11:08 PDT
Created attachment 61581 [details]
Patch
Comment 4 James Robinson 2010-07-14 16:16:54 PDT
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.
Comment 5 Darin Fisher (:fishd, Google) 2010-07-14 16:52:54 PDT
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.
Comment 6 James Robinson 2010-07-14 17:36:52 PDT
Committed r63383: <http://trac.webkit.org/changeset/63383>
Comment 8 James Robinson 2010-07-14 19:27:12 PDT
Fixed the compile error in http://trac.webkit.org/changeset/63392