Bug 110616 – [EFL][WebGL] Refactor platform surface.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit EFL

[Kalyan]

PlatformSurface has the logic to render content to shared surface. As we have a clear separation between the two, it would be better to separate them.

[Kalyan]

Created attachment 189807 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 189807 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=189807&action=review

> Source/WebCore/ChangeLog:14
> +        PlatformSurface has the logic to render content to transport surface. 
> +        This patch moves all the code related to transport surface in PlatformSurface
> +        to GLTransportSurface class. EXT_framebuffer_blit usage is replaced with 
> +        shaders to draw texture contents to the surface as we already have support 
> +        for shared context.

You forgot to write what they change makes sense.

You should "always" detault the what, why and how

> Source/WebCore/platform/graphics/surfaces/GLTransportSurface.cpp:2
> + * Copyright (C) 2012 Intel Corporation. All rights reserved.

Living in the past? :-)

[Kalyan]

Created attachment 189918 [details]
Patch

[Kalyan]

(In reply to comment #2)
> (From update of attachment 189807 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=189807&action=review

> You should "always" detault the what, why and how

done

> > Source/WebCore/platform/graphics/surfaces/GLTransportSurface.cpp:2
> > + * Copyright (C) 2012 Intel Corporation. All rights reserved.
> 
> Living in the past? :-)

fixed :)

[Kenneth Rohde Christiansen]

Comment on attachment 189918 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=189918&action=review

> Source/WebCore/platform/graphics/opengl/GLPlatformSurface.h:34
>  #include "IntRect.h"
> +#include <texmap/TextureMapperShaderProgram.h>
>  #include <wtf/Noncopyable.h>

Why is this needed in the header? I would understand if it was used by the cpp

> Source/WebCore/platform/graphics/surfaces/GLTransportSurface.cpp:32
> +
> +namespace WebCore {
> +static const GLfloat vertices[] = { 0, 0, 1, 0, 1, 1, 0, 1 };

newline after namespace

> Source/WebCore/platform/graphics/surfaces/GLTransportSurface.cpp:68
> +    m_rect = IntRect();
> +    ::glBindFramebuffer(GL_FRAMEBUFFER, 0);

i would add a newline after m_rect =

> Source/WebCore/platform/graphics/surfaces/GLTransportSurface.cpp:145
> +    TextureMapperShaderProgram::Options options = TextureMapperShaderProgram::Texture;
> +    m_shaderProgram = TextureMapperShaderProgram::create(m_context3D, options);
> +    ::glUseProgram(m_shaderProgram->programID());
> +    ::glUniform1i(m_shaderProgram->samplerLocation(), 0);
> +    ::glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER, GL_NEAREST);
> +    ::glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MAG_FILTER, GL_NEAREST);
> +    ::glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_WRAP_S, GL_CLAMP_TO_EDGE);
> +    ::glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_WRAP_T, GL_CLAMP_TO_EDGE);
> +    TransformationMatrix flipTransform;
> +    flipTransform.flipY();

I would add a newline before and after the :: group

[Kalyan]

Created attachment 189922 [details]
Patch

[Kalyan]

(In reply to comment #5)
> (From update of attachment 189918 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=189918&action=review
> 
> > Source/WebCore/platform/graphics/opengl/GLPlatformSurface.h:34
> >  #include "IntRect.h"
> > +#include <texmap/TextureMapperShaderProgram.h>
> >  #include <wtf/Noncopyable.h>
> 
not needed, removed thanks.

> > Source/WebCore/platform/graphics/surfaces/GLTransportSurface.cpp:32
> > +
> I would add a newline before and after the :: group

fixed.

[WebKit Review Bot]

Comment on attachment 189922 [details]
Patch

Clearing flags on attachment: 189922

Committed r143842: <http://trac.webkit.org/changeset/143842>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.