[webkit-reviews] review granted: [Bug 98728] [Texmap] Swizzling BGRA to RGBA in temp buffer while USE(TEXMAP_OPENGL_ES_2) : [Attachment 169095] Patch

[bugzilla-daemon at webkit.org]

Noam Rosenthal <noam.rosenthal at nokia.com> has granted Hyungchan Kim
<hyungchan2.kim at lge.com>'s request for review:
Bug 98728: [Texmap] Swizzling BGRA to RGBA in temp buffer while
USE(TEXMAP_OPENGL_ES_2)
https://bugs.webkit.org/show_bug.cgi?id=98728

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

------- Additional Comments from Noam Rosenthal <noam.rosenthal at nokia.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=169095&action=review


Great! Please fix nits and then commit or cq?

> Source/WebCore/ChangeLog:13
> +	   BitmapTextureGL::updateContents() try to swizzle source Image if
opengl
> +	   driver doesn't support BGRA format.
> +	   In case of directly composited image, swizzling source image
> +	   should not be modified because another BitmapTexture will also try
> +	   to swizzle.
> +
> +	   BitmapTexture now provide UpdateContentsFlag to separate whether
> +	   source image can be modified or not

Some rewording:
BitmapTexture swizzles the source image if the OpenGL driver doesn't support
the BGRA extension.
In case of directly composited images, the source image should not be modified.


BitmapTexture::updateContents now accepts an UpdateContentsFlag argument to
separate the different cases.

> Source/WebCore/ChangeLog:19
> +	   No new tests.

This should be tested - there are a few current test cases for directly
composited images, do they test this?

> Source/WebCore/platform/graphics/texmap/TextureMapper.h:62
> +    enum UpdateContentsFlag {
> +	   UpdateCanModifyOriginalImageData = 0x01,
> +	   UpdateCannotModifyOriginalImageData = 0x02
> +    };

Any reason to use 0x01 and 0x02 instead of 0 and 1?

> Source/WebCore/platform/graphics/texmap/TextureMapperImageBuffer.cpp:57
> +void BitmapTextureImageBuffer::updateContents(Image* image, const IntRect&
targetRect, const IntPoint& offset, UpdateContentsFlag /*updatecontentsflag*/)

You don't need the comment, just omit the argument name.