Bug 104553 – [EFL][WebGL] Minor Refactor in GraphicsContext3DEfl.

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Kalyan]

Remove unused code in PlatformSurface.
Share FBO between canvas and surface.

[Kalyan]

Created attachment 178564 [details]
patch

[Kalyan]

Created attachment 178574 [details]
patch

[WebKit Review Bot]

Attachment 178574 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/platform/graphics/efl/GraphicsContext3DPrivate.cpp:150:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 1 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Kenneth Rohde Christiansen]

Comment on attachment 178574 [details]
patch

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

> Source/WebCore/ChangeLog:9
> +
> +        With this patch we use the same FBO for canvas and backbuffer of offscreen surface.
> +        Also, removes some unused code.

You should explain why that is a good things etc. What it really means

> Source/WebCore/ChangeLog:23
> +        * platform/graphics/efl/GraphicsContext3DEfl.cpp:
> +        (WebCore::GraphicsContext3D::createGraphicsSurfaces):
> +        * platform/graphics/efl/GraphicsContext3DPrivate.cpp:
> +        (GraphicsContext3DPrivate::canvasResized):
> +        (GraphicsContext3DPrivate::copyToGraphicsSurface):
> +        * platform/graphics/efl/GraphicsContext3DPrivate.h:
> +        (GraphicsContext3DPrivate):
> +        * platform/graphics/opengl/GLDefs.h:
> +        * platform/graphics/opengl/GLPlatformContext.h:
> +        * platform/graphics/opengl/GLPlatformSurface.h:
> +        * platform/graphics/surfaces/glx/GLXSurface.h:

Please add more info about your changes here. Especially when you refactor

> Source/WebCore/platform/graphics/efl/GraphicsContext3DEfl.cpp:236
> +    m_private->canvasResized();

didResizeCanvas() seems more consistent with other webcore code

> Source/WebCore/platform/graphics/efl/GraphicsContext3DPrivate.cpp:146
> +    // We should copy the full buffer, and not respect the
> +    // current scissor bounds.

keep on one line

> Source/WebCore/platform/graphics/efl/GraphicsContext3DPrivate.cpp:147
> +    // FIXME::It would be more efficient to track the state of the scissor test.

not two ::, just "FIXME: "

>> Source/WebCore/platform/graphics/efl/GraphicsContext3DPrivate.cpp:150
>> +       // m_context->disable(GraphicsContext3D::SCISSOR_TEST);
> 
> Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]

Dont ever commit outcommented code!

> Source/WebCore/platform/graphics/opengl/GLPlatformSurface.h:56
> +    // Creates FBO used by the surface.
> +    // Buffers can be bound to this FBO.

one line!

> Source/WebCore/platform/graphics/opengl/GLPlatformSurface.h:57
> +    void initialize(GLuint *frameBufferId);

* placement!

> Source/WebCore/platform/graphics/opengl/GLPlatformSurface.h:70
> -    // a)Blits texture contents to back buffer.
> +    // a)Blits back buffer contents to front buffer.
>      // b)Calls Swap Buffers.
> -    // c)Sets current FBO as bindFboId and binds texture to bindTexture.
> -    virtual void updateContents(const uint32_t texture, const IntRect& sourceRect, const GLuint bindFboId, const uint32_t bindTexture);
> +    // c)Sets current FBO as bindFboId.

space after )

[Kalyan]

Created attachment 178581 [details]
proposedpatch

patch for review

[Kenneth Rohde Christiansen]

Comment on attachment 178581 [details]
proposedpatch

r- given my other comments

[Kalyan]

Created attachment 178628 [details]
patchv3

[Kalyan]

(In reply to comment #4)
> (From update of attachment 178574 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=178574&action=review
> 
> > Source/WebCore/ChangeLog:9
> > +
> > +        With this patch we use the same FBO for canvas and backbuffer of offscreen surface.
> > +        Also, removes some unused code.
> 
> You should explain why that is a good things etc. What it really means
> 
> > Source/WebCore/ChangeLog:23
> > +        * platform/graphics/efl/GraphicsContext3DEfl.cpp:
> > +        (WebCore::GraphicsContext3D::createGraphicsSurfaces):
> > +        * platform/graphics/efl/GraphicsContext3DPrivate.cpp:
> > +        (GraphicsContext3DPrivate::canvasResized):
> > +        (GraphicsContext3DPrivate::copyToGraphicsSurface):
> > +        * platform/graphics/efl/GraphicsContext3DPrivate.h:
> > +        (GraphicsContext3DPrivate):
> > +        * platform/graphics/opengl/GLDefs.h:
> > +        * platform/graphics/opengl/GLPlatformContext.h:
> > +        * platform/graphics/opengl/GLPlatformSurface.h:
> > +        * platform/graphics/surfaces/glx/GLXSurface.h:
> 
> Please add more info about your changes here. Especially when you refactor
> 
> > Source/WebCore/platform/graphics/efl/GraphicsContext3DEfl.cpp:236
> > +    m_private->canvasResized();
> 
> didResizeCanvas() seems more consistent with other webcore code
> 
> > Source/WebCore/platform/graphics/efl/GraphicsContext3DPrivate.cpp:146
> > +    // We should copy the full buffer, and not respect the
> > +    // current scissor bounds.
> 
> keep on one line
> 
> > Source/WebCore/platform/graphics/efl/GraphicsContext3DPrivate.cpp:147
> > +    // FIXME::It would be more efficient to track the state of the scissor test.
> 
> not two ::, just "FIXME: "
> 
> >> Source/WebCore/platform/graphics/efl/GraphicsContext3DPrivate.cpp:150
> >> +       // m_context->disable(GraphicsContext3D::SCISSOR_TEST);
> > 
> > Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
> 
> Dont ever commit outcommented code!
> 
> > Source/WebCore/platform/graphics/opengl/GLPlatformSurface.h:56
> > +    // Creates FBO used by the surface.
> > +    // Buffers can be bound to this FBO.
> 
> one line!
> 
> > Source/WebCore/platform/graphics/opengl/GLPlatformSurface.h:57
> > +    void initialize(GLuint *frameBufferId);
> 
> * placement!
> 
> > Source/WebCore/platform/graphics/opengl/GLPlatformSurface.h:70
> > -    // a)Blits texture contents to back buffer.
> > +    // a)Blits back buffer contents to front buffer.
> >      // b)Calls Swap Buffers.
> > -    // c)Sets current FBO as bindFboId and binds texture to bindTexture.
> > -    virtual void updateContents(const uint32_t texture, const IntRect& sourceRect, const GLuint bindFboId, const uint32_t bindTexture);
> > +    // c)Sets current FBO as bindFboId.
> 
> space after )

done.

[WebKit Review Bot]

Comment on attachment 178628 [details]
patchv3

Clearing flags on attachment: 178628

Committed r137211: <http://trac.webkit.org/changeset/137211>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Kenneth Rohde Christiansen]

This seems to break all of webgl for me!