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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Jun 29 18:27:56 PDT 2012


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

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

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


> Source/Platform/ChangeLog:7
> +	   This change replaces getUniformLocation calls in the chrome
compositor with bindUniformLocation
> +	   to speed up startup time.

The changelog entries should be specific to changes the particular ChangeLog
covers - so this one is just describing the changes to
Platform/chromium/public/WebGraphicsContext3D.h. You should just say here that
you are adding an entry point for bindUniformLocationCHROMIUM

> Source/Platform/ChangeLog:9
> +	   Reviewed by NOBODY (OOPS!).

in all ChangeLogs this line should go above your description of the patch.

> Source/WebCore/ChangeLog:9
> +	   Reviewed by NOBODY (OOPS!).

move this up

>> Source/WebCore/ChangeLog:11
>> +	    No new tests. (OOPS!)
> 
> You should remove the 'No new tests' and either add and list tests, or
explain why no new tests were possible.  [changelog/nonewtests] [5]

You will need to address this in some way - either by adding tests where
appropriate or documenting the test coverage that we have.  In this case, the
actual implementation guts of bindUniformLocation are in a different codebase
and (hopefully) well-tested there, so we don't really need to duplicate
coverage for that.  Since the only logic in the compositor is pretty basic
glue, and we have integration pixel tests to make sure we aren't completely
flubbing the programs so we are probably OK with just a description of which
tests cover this behavior (i.e. pretty much all of them that use any shaders)

> Source/Platform/chromium/public/WebGraphicsContext3D.h:416
> +    virtual void bindUniformLocationCHROMIUM(WebGLId program, WGC3Dint
location, const WGC3Dchar* uniform) { }

This isn't part of GL_EXT_occlusion_query, so it doesn't belong in that
section.  Give this extension a new section (i.e. separate with newlines) with
a comment naming the extension in case we ever get more entry points with this
extension. We probably won't, but it's better to be consistent with the other
extensions in this file.

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:230
> +    m_capabilities.usingBindUniform =
extensions.contains("GL_CHROMIUM_bind_uniform_location");

For this, I don't think we need to put a bit on m_capabilities since nobody
other than LayerRendererChromium needs to know if this extension exists on the
compositor's context or not. Instead, you can just store a member variable on
the LRC.  (This appears be true for some other things currently on
m_capabilities as well, things tend to just get cargo-culted, but for some
things like maxTextureSize other systems do have a legit need to know the
capability)

> Source/WebCore/platform/graphics/chromium/ProgramBinding.cpp:56
>      // If you hit these asserts, you initialized but forgot to call
cleanup().
>      ASSERT(!m_program);
> +    ASSERT(!m_vertexShader);
> +    ASSERT(!m_fragmentShader);

Did you patch cleanup to zero these guys out?  If not, please update either the
comment or the code to match the other.

> Source/WebCore/platform/graphics/chromium/ProgramBinding.cpp:106
> +    m_vertexShader = 0;
> +    m_fragmentShader = 0;

Hm, this is a bit confusing - do these two member variables just exist to store
some transient state?  If so, why are they member variables and not just
maintained on the stack?

> Source/WebCore/platform/graphics/chromium/ProgramBinding.h:86
> +	       ProgramBindingBase::link(context);

do you need the ProgramBindingBase:: qualifier here, or can you just call
link() ?

> Source/WebCore/platform/graphics/chromium/TextureCopier.h:65
> +    explicit AcceleratedTextureCopier(WebKit::WebGraphicsContext3D*, bool
usingBindUniforms);

no explicit on 2-argument constructors. the "explicit" keyword is to prevent
implicit conversion of one type to another in the case where we provide a
one-argument constructor.  I.e. the fact that we provide the "explicit
AcceleratedTextureCopier(WebKit::WebGraphicsContext3D*);" constructor does not
mean that we should be able to pass a WebKit::WebGraphicsContext3D* type to a
function expecting an AcceleratedTextureCopier and have a copier created for us
behind the scenes


More information about the webkit-reviews mailing list