Bug 41010 – [chromium] Attach a GLES2Context to a WebView to enable resource sharing

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Vangelis Kokkevis]

In order for the page compositor to share resources (textures) with any hardware accelerated elements within a page (e.g. WebGL, Pepper3D) there needs to be a single GLES2Context associated with the view that's available when the accelerated elements are created (they often get created before the layer compositor).

[Vangelis Kokkevis]

Created attachment 59416 [details]
Proposed patch

[WebKit Review Bot]

Attachment 59416 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3334596

[Vangelis Kokkevis]

Created attachment 59426 [details]
Patch. Hopefully fixes the link issues

[WebKit Review Bot]

Attachment 59426 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3311607

[Vangelis Kokkevis]

Created attachment 59459 [details]
Proposed patch. Removed virtual method from WebView class.

[Vangelis Kokkevis]

Ok, this looks like it actually did compile successfully after all. Darin or Dimitri, can you please take a look when you get a chance?

[Darin Fisher (:fishd, Google)]

Comment on attachment 59459 [details]
Proposed patch. Removed virtual method from WebView class.

WebKit/chromium/src/GLES2Context.cpp:68
 +      WebKit::WebViewImpl* webView = WebKit::WebViewImpl::fromPage(page);
I recommend putting a "using namespace WebKit" at the top of this file.

WebKit/chromium/src/WebViewImpl.h:340
 +      WebGLES2Context* GLES2Context();
WebKit style suggests that this method should be named gles2Context
instead.  See examples like ResourceRequest::httpReferrer.

WebKit/chromium/src/WebViewImpl.cpp:2290
 +      if (!m_gles2Context) {
should we check if accelerated compositing is enabled before
creating the GLES2Context?

[Vangelis Kokkevis]

Created attachment 59532 [details]
Proposed patch addressing review comments

[Vangelis Kokkevis]

(In reply to comment #7)
> (From update of attachment 59459 [details])
> WebKit/chromium/src/GLES2Context.cpp:68
>  +      WebKit::WebViewImpl* webView = WebKit::WebViewImpl::fromPage(page);
> I recommend putting a "using namespace WebKit" at the top of this file.
> 
Done.

> WebKit/chromium/src/WebViewImpl.h:340
>  +      WebGLES2Context* GLES2Context();
> WebKit style suggests that this method should be named gles2Context
> instead.  See examples like ResourceRequest::httpReferrer.
> 
Done.

> WebKit/chromium/src/WebViewImpl.cpp:2290
>  +      if (!m_gles2Context) {
> should we check if accelerated compositing is enabled before
> creating the GLES2Context?

This method currently gets called from one of two places:
1. When creating a WebGL context, only if the accelerated compositing flag is enabled
2. When creating the page compositor

So, as it stands, it will only create the context if compositing is enabled.  However, one can imagine other cases where it would be useful to attach a GLES2 context to the view even when not doing accelerated compositing so I don't think it should be checking whether the feature is enabled.
I had previously made a mistake and defined the method within the USE(ACCELERATED_COMPOSTING) block which shouldn't be the case.  It's fixed in this patch.

[WebKit Review Bot]

Attachment 59532 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebKit/chromium/src/WebViewImpl.cpp:2285:  One space before end of line comments  [whitespace/comments] [5]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Fisher (:fishd, Google)]

(In reply to comment #9)
> > WebKit/chromium/src/WebViewImpl.cpp:2290
> >  +      if (!m_gles2Context) {
> > should we check if accelerated compositing is enabled before
> > creating the GLES2Context?
> 
> This method currently gets called from one of two places:
> 1. When creating a WebGL context, only if the accelerated compositing flag is 
enabled
> 2. When creating the page compositor
> 
> So, as it stands, it will only create the context if compositing is enabled.
However, one can imagine other cases where it would be useful to attach a GLES2 
context to the view even when not doing accelerated compositing so I don't think 
it should be checking whether the feature is enabled.
> I had previously made a mistake and defined the method within the 
USE(ACCELERATED_COMPOSTING) block which shouldn't be the case.  It's fixed in 
this patch.

OK.  My concern was with this being an API method, but your explanation
makes sense to me.

[Darin Fisher (:fishd, Google)]

Comment on attachment 59532 [details]
Proposed patch addressing review comments

R=me, cq- due to style issue.

[Vangelis Kokkevis]

Created attachment 59547 [details]
Proposed patch. Fixed style issue.

[Eric Seidel (no email)]

Comment on attachment 59532 [details]
Proposed patch addressing review comments

Cleared Darin Fisher's review+ from obsolete attachment 59532 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Darin Fisher (:fishd, Google)]

Landed as http://trac.webkit.org/changeset/61774