[webkit-reviews] review canceled: [Bug 90217] [chromium] Avoid calling getUniformLocation??() in the compositor startup : [Attachment 150200] Patch

[bugzilla-daemon at webkit.org]

James Robinson <jamesr at chromium.org> has canceled  review:
Bug 90217: [chromium] Avoid calling getUniformLocation??() in the compositor
startup
https://bugs.webkit.org/show_bug.cgi?id=90217

Attachment 150200: Patch
https://bugs.webkit.org/attachment.cgi?id=150200&action=review

------- Additional Comments from James Robinson <jamesr at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=150200&action=review


If you update this patch to ToT it'll apply cleanly and trigger the auto-cc
behavior, etc.	The EWS bubbles are purple since they couldn't apply this
patch.

> Source/WebCore/platform/graphics/chromium/Extensions3DChromium.h:162
> +    void bindUniformLocationCHROMIUM(Platform3DObject program, GC3Dint
location, const GC3Dchar* uniform);

I think your WebKit checkout is out of data - you don't have to modify
Extensions3DChromium at all for compositor changes as of
http://trac.webkit.org/changeset/121204.  Just adding the entry point to
WebGraphicsContext3D is sufficient

> Source/WebCore/platform/graphics/chromium/ProgramBinding.cpp:64
>  void ProgramBindingBase::init(GraphicsContext3D* context, const String&
vertexShader, const String& fragmentShader)

ProgramBinding is based on WebGraphicsContext3D, not GraphicsContext3D (cuts
down on one level of indirection)

> Source/WebCore/platform/graphics/chromium/ProgramBinding.cpp:156
> +bool ProgramBindingBase::hasBindUniformsExtension(GraphicsContext3D*
context) const

In the compositor we query all the extensions we may need up front (inside
LayerRendererChromium::initialize) and then propagate them through as needed -
this avoids any unnecessary get() calls and makes sure that all of our
extensions use is centralized in one place.  All of the ProgramBinding uses and
initialization calls are inside of LayerRendererChromium, so it should be
pretty easy to plumb a bit for this through.

The extension name is bind_uniform (singular), so any functions/variables
should stick to that and not plural "uniforms"

> Source/WebCore/platform/graphics/chromium/ShaderChromium.cpp:228
> +	   "point[0]",

the example text in CHROMIUM_bind_uniform_location.txt queries the location of
an array with just "point", not "point[0]". are both syntaxes allowed in
getProgramUniform..() ?