Use 3D graphics context shareResources flag to decide whether a context should share resources.

Use 3D graphics context shareResources flag to decide whether a context should share resources.

Before, it abused the noExtensions flag to determine to disable share groups for WebGL, which was awful.

This patch is dependent on https://bugs.webkit.org/show_bug.cgi?id=66516.

See also http://codereview.chromium.org/7669072 where I perpetrated the aforementioned hack.

BUG=92356
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=98527

[apatrick_chromium, 2011-08-19 01:18:17]

This is for your reference for now so you can see the other side of the webkit
patch.

[apatrick_chromium, 2011-08-25 22:53:21]

Ready for review. PTAL.

[apatrick_chromium, 2011-08-26 18:25:19]

+jbates

jamesr is unavailable. Please review, thanks.

[jbates, 2011-08-26 19:04:35]

http://codereview.chromium.org/7669072/diff/1/content/renderer/gpu/renderer_g...
File content/renderer/gpu/renderer_gl_context.cc (right):

http://codereview.chromium.org/7669072/diff/1/content/renderer/gpu/renderer_g...
content/renderer/gpu/renderer_gl_context.cc:334:
attribs.push_back(*attrib_list++);
I know this was already here, but should this check for null *attrib_list before
incrementing in case the caller provided bad arguments?

http://codereview.chromium.org/7669072/diff/1/content/renderer/gpu/renderer_g...
content/renderer/gpu/renderer_gl_context.cc:337: share_resources =
!!(*attrib_list++);
(same here)

http://codereview.chromium.org/7669072/diff/1/content/renderer/gpu/renderer_g...
content/renderer/gpu/renderer_gl_context.cc:340: bind_generates_resources =
!!(*attrib_list++);
(same here)

[apatrick_chromium, 2011-08-26 19:21:13]

http://codereview.chromium.org/7669072/diff/1/content/renderer/gpu/renderer_g...
File content/renderer/gpu/renderer_gl_context.cc (right):

http://codereview.chromium.org/7669072/diff/1/content/renderer/gpu/renderer_g...
content/renderer/gpu/renderer_gl_context.cc:321: while (attrib_list) {
If attrib_list is null then it want get beyond the while condition.

[jbates, 2011-08-26 21:31:39]

LGTM

[apatrick_chromium, 2011-08-26 22:51:21]

+piman for owners for render_widget_fullscreen_pepper.cc.

[piman, 2011-08-26 23:18:57]

http://codereview.chromium.org/7669072/diff/1/content/renderer/gpu/renderer_g...
File content/renderer/gpu/renderer_gl_context.cc (right):

http://codereview.chromium.org/7669072/diff/1/content/renderer/gpu/renderer_g...
content/renderer/gpu/renderer_gl_context.cc:319: bool bind_generates_resources =
true;
The default was false before (see callers). Is it the right thing to do to
change it to true ?
Then again, the fact that it was false seems wrong for Pepper3D (if my
understanding is correct, i.e. true is the spec behavior), so maybe true is
better anyway.
In any case, is the change deliberate ?

http://codereview.chromium.org/7669072/diff/1/content/renderer/render_widget_...
File content/renderer/render_widget_fullscreen_pepper.cc (right):

http://codereview.chromium.org/7669072/diff/1/content/renderer/render_widget_...
content/renderer/render_widget_fullscreen_pepper.cc:352:
RendererGLContext::BIND_GENERATES_RESOURCES, 1,
I think this was false before (the default), do we want to set it to 0 ?
We don't rely on the behavior either way, so whatever is most efficient is good.

[apatrick_chromium, 2011-08-26 23:40:01]

http://codereview.chromium.org/7669072/diff/1/content/renderer/gpu/renderer_g...
File content/renderer/gpu/renderer_gl_context.cc (right):

http://codereview.chromium.org/7669072/diff/1/content/renderer/gpu/renderer_g...
content/renderer/gpu/renderer_gl_context.cc:319: bool bind_generates_resources =
true;
I was deliberate. I want the default semantics to be the same as GLES2.

http://codereview.chromium.org/7669072/diff/1/content/renderer/render_widget_...
File content/renderer/render_widget_fullscreen_pepper.cc (right):

http://codereview.chromium.org/7669072/diff/1/content/renderer/render_widget_...
content/renderer/render_widget_fullscreen_pepper.cc:352:
RendererGLContext::BIND_GENERATES_RESOURCES, 1,
False referred to whether resource sharing was enabled. It is still false. See
SHARE_RESOURCES above. BIND_GENERATES_RESOURCES is a new option and is true for
Pepper, and corresponds to regular GLES2 semantics, which I think is what you
want.

[piman, 2011-08-27 00:40:42]

LGTM

http://codereview.chromium.org/7669072/diff/1/content/renderer/render_widget_...
File content/renderer/render_widget_fullscreen_pepper.cc (right):

http://codereview.chromium.org/7669072/diff/1/content/renderer/render_widget_...
content/renderer/render_widget_fullscreen_pepper.cc:352:
RendererGLContext::BIND_GENERATES_RESOURCES, 1,
On 2011/08/26 23:40:01, apatrick_chromium wrote:
> False referred to whether resource sharing was enabled. It is still false. See
> SHARE_RESOURCES above. BIND_GENERATES_RESOURCES is a new option and is true
for
> Pepper, and corresponds to regular GLES2 semantics, which I think is what you
> want.

This is for the full-screen "compositor" (which scales and draws the Pepper
content which has its own separate context). So it's actually ok to not have the
GLES2 semantics here, if it's more efficient. But it's not necessarily
important.