[Webkit-unassigned] [Bug 90217] [chromium] Avoid calling getUniformLocation??() in the compositor startup

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=90217


James Robinson <jamesr at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #150285|review?                     |review-
               Flag|                            |




--- Comment #10 from James Robinson <jamesr at chromium.org>  2012-06-29 18:27:55 PST ---
(From update of attachment 150285)
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

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.