[webkit-reviews] review denied: [Bug 87738] Make TextureMapper work with GraphicsSurface. : [Attachment 145279] patch for review. - adding a second GraphicSurface as a buffer.

[bugzilla-daemon at webkit.org]

Noam Rosenthal <noam.rosenthal at nokia.com> has denied Zeno Albisser
<zeno at webkit.org>'s request for review:
Bug 87738: Make TextureMapper work with GraphicsSurface.
https://bugs.webkit.org/show_bug.cgi?id=87738

Attachment 145279: patch for review. - adding a second GraphicSurface as a
buffer.
https://bugs.webkit.org/attachment.cgi?id=145279&action=review

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


Good! Just a few nitpicks.

> Source/WebCore/platform/graphics/texmap/TextureMapperBackingStore.cpp:34
> +    if (graphicsSurfaceToken != m_backbufferGraphicsSurfaceToken) {

backbuffer -> backBuffer

> Source/WebCore/platform/graphics/texmap/TextureMapperBackingStore.cpp:36
> +					       |
GraphicsSurface::SupportsSharing;

It actually needs TextureTarget and not CopyFromTexture... but I guess we can
deal with that when we deal with hardware that makes that distinction.

> Source/WebCore/platform/graphics/texmap/TextureMapperBackingStore.cpp:52
> +    // Swap front and backbuffer GraphicsSurface.
> +    RefPtr<GraphicsSurface> temp =  m_currentGraphicsSurface;
> +    m_currentGraphicsSurface = m_backbufferGraphicsSurface;
> +    m_backbufferGraphicsSurface = temp;
> +    uint32_t tempToken = m_currentGraphicsSurfaceToken;
> +    m_currentGraphicsSurfaceToken = m_backbufferGraphicsSurfaceToken;
> +    m_backbufferGraphicsSurfaceToken = tempToken;
> +    uint32_t tempTextureId = m_currentTextureId;
> +    m_currentTextureId = m_backbufferTextureId;
> +    m_backbufferTextureId = tempTextureId;

Would be better to have these 3 members in a struct and call std::swap

> Source/WebCore/platform/graphics/texmap/TextureMapperBackingStore.cpp:55
> +PassRefPtr<BitmapTexture> TextureMapperDirectBackingStore::texture() const

I think this is not pure virtual, you should be able to simply not override it.


> Source/WebCore/platform/graphics/texmap/TextureMapperBackingStore.h:43
> +class TextureMapperDirectBackingStore : public TextureMapperBackingStore {

How about TextureMapperSurfaceBackingStore? It's a bit more of a direct name :)


> Source/WebCore/platform/graphics/texmap/TextureMapperShaderManager.cpp:253
> +    program->initialize(); // Avoid calling virtual functions in
constructor.

Put comment in previous line