[webkit-reviews] review denied: [Bug 45520] Add shader generator and cache for GPU accelerated path rendering : [Attachment 67157] Patch

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied Kenneth Russell <kbr at google.com>'s
request for review:
Bug 45520: Add shader generator and cache for GPU accelerated path rendering
https://bugs.webkit.org/show_bug.cgi?id=45520

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

------- Additional Comments from Eric Seidel <eric at webkit.org>
View in context:
https://bugs.webkit.org/attachment.cgi?id=67157&action=prettypatch

A bunch of nits.  I'm not much of a graphics dude though, so I don't have much
of substance to say.

> WebCore/platform/graphics/gpu/LoopBlinnShader.cpp:43
> +long LoopBlinnShader::getUniformLocation(const String& name)
What is a long?  I think we normally use int or unsigned instead in WebCore
code.

> WebCore/platform/graphics/gpu/LoopBlinnShader.cpp:48
> +    long location = m_context->getUniformLocation(m_program, name);
We don't use "get" in names in WebCore code.  I guess we do use them for active
lookups (like hash lookukps) sometimes though.	Never for accessors.  So I
guess this is OK.  We should really clarify our stance in the style guide. :)

> WebCore/platform/graphics/gpu/LoopBlinnShader.cpp:56
> +    if (iter != m_attribLocations.end())
Can this just be if (iter)?

> WebCore/platform/graphics/gpu/LoopBlinnShader.h:29
> +#include "GraphicsContext3D.h"
Not needed, right?  Or is it needed for Platform3DObject?  Should
Platform3DObject be in its own header?

> WebCore/platform/graphics/gpu/LoopBlinnShader.h:30
> +#include "PlatformString.h"
Unlikely this is needed, StringHash includes it if nothing else does.

> WebCore/platform/graphics/gpu/LoopBlinnShader.h:44
> +    LoopBlinnShader(GraphicsContext3D* context, Platform3DObject program);
"context" is not needed here.  We only name arguments when they add meaning to
the declaration.

> WebCore/platform/graphics/gpu/LoopBlinnShader.h:46
> +    void use();
What does "use" mean here?

> WebCore/platform/graphics/gpu/LoopBlinnShaderCache.cpp:28
> +#include "config.h"
> +
> +#include "LoopBlinnShaderCache.h"
I don't think folks normally put a new line between config.h and the first
header.  Not a big deal.

> WebCore/platform/graphics/gpu/LoopBlinnShaderCache.cpp:31
> +#include "PlatformString.h"
This was included in the header, no?

> WebCore/platform/graphics/gpu/LoopBlinnShaderCache.cpp:41
> +    m_shaders.resize(NumShaderTypes * NumRegionTypes * NumAntialiasTypes);
Thanks for using constants!  I'm not sure what the correct style is. I don't
think they're capitalized though since that would be class names.

> WebCore/platform/graphics/gpu/LoopBlinnShaderCache.cpp:67
> +    int index = (shaderIndex * numRegions * numAntialiases +
Um, really?  That's one crazy datastructure!

> WebCore/platform/graphics/gpu/LoopBlinnShaderCache.cpp:70
> +    if (!m_shaders[index].get())
.get() is not needed here. There is a bool conversion for you.

> WebCore/platform/graphics/gpu/LoopBlinnShaderCache.cpp:75
> +PassOwnPtr<LoopBlinnShader> LoopBlinnShaderCache::createShader(ShaderType
shaderType, RegionType regionType, AntialiasType antialiasType)
This is a really long method... can we make it shorter? Split it up into
pieces?

> WebCore/platform/graphics/gpu/LoopBlinnShaderCache.cpp:154
> +    m_context->getProgramiv(program, GraphicsContext3D::LINK_STATUS,
&linkStatus);
what's "Programiv"?  A graphics term?

> WebCore/platform/graphics/gpu/LoopBlinnShaderCache.cpp:162
> +    m_context->deleteShader(vertexShader);
Should we use some sort of scoping object for this attach/delete pattern?

> WebCore/platform/graphics/gpu/LoopBlinnShaderCache.cpp:173
> +    m_context->getShaderiv(shader, GraphicsContext3D::COMPILE_STATUS,
&compileStatus);
Shaderiv?  Another graphics term?

> WebCore/platform/graphics/gpu/LoopBlinnShaderCache.h:65
> +    LoopBlinnShader* getShader(ShaderType shaderType, RegionType regionType,
AntialiasType antialiasType);
I wonder if lookupShader would be better than getShader().  Maybe I'm just
sensitive to "get" after all these years in WebCore...

> WebCore/platform/graphics/gpu/LoopBlinnShaderCache.h:68
> +    PassOwnPtr<LoopBlinnShader> createShader(ShaderType shaderType,
RegionType regionType, AntialiasType antialiasType);
None of these names are needed, the types say it all.