[webkit-reviews] review granted: [Bug 77148] [Qt][Texmap] Refactor TextureMapper API to use ImageBuffers when possible. : [Attachment 125206] Revised patch based on Martin Robinson's comments.

[bugzilla-daemon at webkit.org]

Martin Robinson <mrobinson at webkit.org> has granted Noam Rosenthal
<noam.rosenthal at nokia.com>'s request for review:
Bug 77148: [Qt][Texmap] Refactor TextureMapper API to use ImageBuffers when
possible.
https://bugs.webkit.org/show_bug.cgi?id=77148

Attachment 125206: Revised patch based on Martin Robinson's comments.
https://bugs.webkit.org/attachment.cgi?id=125206&action=review

------- Additional Comments from Martin Robinson <mrobinson at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=125206&action=review


Okay. I think I understand enough of this to give an r+ now. Very nice. The
only change I request is to move the ImageBuffer implementations to another
file (perhaps TextureMapperImageBuffer.cpp/h), to reduce the number of classes
per-file. Eventually I think it would be nice to move the BitmapTexture classes
to their own files too.

> Source/WebCore/platform/graphics/qt/GraphicsContextQt.cpp:1338
> +#if ENABLE(3D_RENDERING)

I think you are missing a && USE(TEXTURE_MAPPER) here.

> Source/WebCore/platform/graphics/texmap/TextureMapper.cpp:80
> +class BitmapTextureImageBuffer : public BitmapTexture {
> +    friend class TextureMapperImageBuffer;
> +public:

I think it makes sense to split out the ImageBuffer implementation into a new
file.

> Source/WebCore/platform/graphics/texmap/TextureMapper.cpp:194
> +    context->save();
> +    context->setAlpha(opacity);
> +#if ENABLE(3D_RENDERING)
> +    context->concat3DTransform(matrix);
> +#else
> +    context->concatCTM(matrix);
> +#endif
> +    context->drawImageBuffer(image, ColorSpaceDeviceRGB, targetRect);

It seems that if alpha == 1 and the transform is just a 2D translation we could
avoid a copy? Perhaps not, but if so it might be a good fast path to add for
the future!