Bug 52833 - shaderSource needs to preserve original source
Summary: shaderSource needs to preserve original source
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebGL (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Zhenyao Mo
URL:
Keywords:
Depends on: 52390
Blocks:
  Show dependency treegraph
 
Reported: 2011-01-20 12:45 PST by Kenneth Russell
Modified: 2011-01-27 10:59 PST (History)
2 users (show)

See Also:


Attachments
Patch (14.42 KB, patch)
2011-01-26 14:04 PST, Zhenyao Mo
no flags Details | Formatted Diff | Diff
Patch (7.08 KB, patch)
2011-01-26 17:27 PST, Zhenyao Mo
kbr: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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]
Patch
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]
Patch

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
> +    ASSERT_NOT_REACHED();

Same here.
Comment 4 Zhenyao Mo 2011-01-26 17:27:19 PST
Created attachment 80276 [details]
Patch
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]
Patch

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>