Bug 52833

Summary: shaderSource needs to preserve original source
Product: WebKit Reporter: Kenneth Russell <kbr>
Component: WebGLAssignee: Zhenyao Mo <zmo>
Severity: Normal CC: cmarrin, zmo
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 52390    
Bug Blocks:    
Description Flags
Patch kbr: review+

Description Kenneth Russell 2011-01-20 12:45:16 PST
After the removal of comments implemented in bug 52390, shaderSource needs to preserve the original source so that it can be returned from getShaderSource.
Comment 1 Zhenyao Mo 2011-01-26 14:04:30 PST
Created attachment 80232 [details]
Comment 2 Zhenyao Mo 2011-01-26 14:09:06 PST
Note that shader source is cached in both WebGLRenderingContext (before comments removal) and GraphicsContext3D (before ANGLE).

In this patch the cached copy in GraphicsContext3D is released once the shader is compiled.  ASSERT_NOT_REACHED() is added in this level for getShaderSource() and getShaderiv(SHADER_SOURCE_LENGTH).  This is under the assumption that WebGLRenderingContext is the only place that calls these two functions.

Will need to do the same in chromium two ports to reduce memory usage.
Comment 3 Kenneth Russell 2011-01-26 14:41:33 PST
Comment on attachment 80232 [details]

View in context: https://bugs.webkit.org/attachment.cgi?id=80232&action=review

> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGL.cpp:1220
> +        ASSERT_NOT_REACHED();

WebGLRenderingContext isn't the only client of this code; for example, the accelerated 2D canvas code may go through it, and Chromium's accelerated compositor is built on top of GraphicsContext3D. Please restore this functionality. The required (slightly redundant) caches are negligible as shaders are tiny.

I don't think any changes are needed in the Chromium port either. We really just need the additional caching to preserve any Unicode characters in comments for WebGL content.

> Source/WebCore/platform/graphics/opengl/GraphicsContext3DOpenGL.cpp:1257

Same here.
Comment 4 Zhenyao Mo 2011-01-26 17:27:19 PST
Created attachment 80276 [details]
Comment 5 Zhenyao Mo 2011-01-26 17:31:12 PST
One question: how do we count the length if shader source is not all UTF8?

String::length() count a UTF16 char as 1.  However, do we want to count it as 2?
Comment 6 Kenneth Russell 2011-01-26 17:59:09 PST
(In reply to comment #5)
> One question: how do we count the length if shader source is not all UTF8?
> String::length() count a UTF16 char as 1.  However, do we want to count it as 2?

That's a good question. I think the queries of the shader source length and info log length are useless in this API, since the queries of the logs return DOMStrings. Do you want to raise that on the working group email list?
Comment 7 Kenneth Russell 2011-01-27 10:45:18 PST
Comment on attachment 80276 [details]

The code changes look fine. Thanks for raising the issue of removing the useless query of shader source length on the WebGL working group list.
Comment 8 Zhenyao Mo 2011-01-27 10:59:18 PST
Committed r76814: <http://trac.webkit.org/changeset/76814>