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

[bugzilla-daemon at webkit.org]

James Robinson <jamesr at chromium.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 67468: Revised patch
https://bugs.webkit.org/attachment.cgi?id=67468&action=review

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

> WebCore/platform/graphics/gpu/LoopBlinnShader.h:43
> +// FIXME: this class needs to be merged with the shader classes for
> +// the fills for rectangles, etc.
> +class LoopBlinnShader : public Noncopyable {
I would strongly prefer this be merged _before_ landing.  There's no reason to
check in code that we know needs to be changed.

It looks like the {SolidFill, Interior} shader is exactly the same as our
existing SolidFillShader.  Removing that permutation would make the rest of
this code a lot simpler.

> WebCore/platform/graphics/gpu/LoopBlinnShader.h:50
> +    long getUniformLocation(const String& name);
> +    long getAttribLocation(const String& name);
This is a different model from the other shader types.	Why do callers need to
grab arbitrary uniform/attribute locations?  This looks fragile since knowing
that something is a LoopBlinnShader is not enough to know what uniforms and
attributes are available.


r- for the redundancy.	I think merging with the existing Shader class should
be very straightforward - the only real change is that Shader::loadShader() /
loadProgram() should be changed to take strings as Strings rather than const
char*, but that is pretty simple and definitely an improvement anyway.