[webkit-reviews] review denied: [Bug 61442] [chromium] Red and Blue channels are swapped in images with accelerated drawing : [Attachment 97643] proposed patch

[bugzilla-daemon at webkit.org]

James Robinson <jamesr at chromium.org> has denied Alok Priyadarshi
<alokp at chromium.org>'s request for review:
Bug 61442: [chromium] Red and Blue channels are swapped in images with
accelerated drawing
https://bugs.webkit.org/show_bug.cgi?id=61442

Attachment 97643: proposed patch
https://bugs.webkit.org/attachment.cgi?id=97643&action=review

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

I don't understand the extra indirection added to LayerTilerChromium.h.  Why
can't you just typedef two different ProgramBinding<>s to ProgramRGBA and
ProgramBGRA?  Otherwise this seems fairly reasonable.

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:164
> +    if (!m_skiaContext && m_contextSupportsTextureFormatBGRA &&
m_contextSupportsReadFormatBGRA) {

so what happens if these extensions are not available?	we always return NULL
from this function?

> Source/WebCore/platform/graphics/chromium/LayerTilerChromium.h:99
> -    typedef ProgramBinding<VertexShaderPosTexTransform,
FragmentShaderTexAlpha> Program;
> +    class Program {
> +    public:
> +	   virtual unsigned program() const = 0;
> +	   virtual int vertexMatrixLocation() const = 0;
> +	   virtual int vertexTexTransformLocation() const = 0;
> +	   virtual int fragmentAlphaLocation() const = 0;
> +	   virtual int fragmentSamplerLocation() const = 0;
> +    };
> +
> +    template<class VertexShader, class FragmentShader>
> +    class ProgramT : public Program {
> +    public:
> +	 explicit ProgramT(GraphicsContext3D* context) : m_binding(context) { }

> +
> +	 bool initialized() const { return m_binding.initialized(); }
> +	 void initialize() { m_binding.initialize(); }
> +
> +	 virtual unsigned program() const { return m_binding.program(); }
> +	 virtual int vertexMatrixLocation() const { return
m_binding.vertexShader().matrixLocation(); }
> +	 virtual int vertexTexTransformLocation() const { return
m_binding.vertexShader().texTransformLocation(); }
> +	 virtual int fragmentAlphaLocation() const { return
m_binding.fragmentShader().alphaLocation(); }
> +	 virtual int fragmentSamplerLocation() const { return
m_binding.fragmentShader().samplerLocation(); }
> +
> +    private:
> +	   typedef ProgramBinding<VertexShader, FragmentShader> Binding;
> +	   Binding m_binding;
> +    };

I'm confused - why do these two types exist?