Bug 62707 – [CMAKE] Remove platform/graphics/opengl/*OpenGL.cpp files in CMakeLists.txt

• Status: RESOLVED
• Resolution: FIXED
• Component: WebGL

[Hyowon Kim]

Bug 62376 makes all ports use GraphicsContext3DOpenGL, Extensions3DOpenGL when enabling WebGL.
However, some ports already have their own GC3D implementation. In QT and chromium, for example, GraphicsContext3D delegates to GraphicsContext3DInternal. So, I think it would be better to allow each port to decide whether or not to include files in platform/graphics/opengl.

[Hyowon Kim]

Created attachment 97261 [details]
Patch

[Ryuan Choi]

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

> Source/WebCore/ChangeLog:8
> +        [EFL] Make accelerated compositing build in Webkit-EFL
> +        https://bugs.webkit.org/show_bug.cgi?id=62361
> +
> +        Remove platform/graphics/opengl/*OpenGL.cpp files in CMakeLists.txt.

Bug title looks wrong.

Please add why you want to remove it in ChangeLog.

[Hyowon Kim]

Created attachment 97263 [details]
Patch

[Hyowon Kim]

(In reply to comment #2)
> View in context: https://bugs.webkit.org/attachment.cgi?id=97261&action=review
> > Source/WebCore/ChangeLog:8
> > +        [EFL] Make accelerated compositing build in Webkit-EFL
> > +        https://bugs.webkit.org/show_bug.cgi?id=62361
> > +
> > +        Remove platform/graphics/opengl/*OpenGL.cpp files in CMakeLists.txt.
> Bug title looks wrong.
> Please add why you want to remove it in ChangeLog.

Sorry, I fixed it.

[Gyuyoung Kim]

LGTM. lucas, how do you think about this ?

Hyowon, 

If lucas agrees with your patch, It looks below files need to be added to both CMakeListEfl.txt and CMakeListWinCE.txt with ENABLE_WEBGL macro.

    platform/graphics/opengl/Extensions3DOpenGL.cpp		
    platform/graphics/opengl/GraphicsContext3DOpenGL.cpp

[Ryuan Choi]

(In reply to comment #5)
> LGTM. lucas, how do you think about this ?
> 
> Hyowon, 
> 
> If lucas agrees with your patch, It looks below files need to be added to both CMakeListEfl.txt and CMakeListWinCE.txt with ENABLE_WEBGL macro.
> 
>     platform/graphics/opengl/Extensions3DOpenGL.cpp        
>     platform/graphics/opengl/GraphicsContext3DOpenGL.cpp

I think that WebKit/Efl is no needed if Bug 62709 (and others if required) landed.
But I agree that we should check WinCE.

I add patrick in CCs.

[Hyowon Kim]

(In reply to comment #6)
> (In reply to comment #5)
> > LGTM. lucas, how do you think about this ?
> > 
> > Hyowon, 
> > 
> > If lucas agrees with your patch, It looks below files need to be added to both CMakeListEfl.txt and CMakeListWinCE.txt with ENABLE_WEBGL macro.
> > 
> >     platform/graphics/opengl/Extensions3DOpenGL.cpp        
> >     platform/graphics/opengl/GraphicsContext3DOpenGL.cpp
> I think that WebKit/Efl is no needed if Bug 62709 (and others if required) landed.
> But I agree that we should check WinCE.
> I add patrick in CCs.

To do initial build for WebGL on EFL port, we have to add two files - DrawingBufferEfl.cpp & GraphicsContext3DEfl.cpp.
And as gyuyoung mentioned, ENABLE_WEBGL macro should be added in CMakeListEfl.txt.

IF (ENABLE_WEBGL)
  LIST(APPEND WebCore_SOURCES
    platform/graphics/efl/DrawingBufferEfl.cpp
    platform/graphics/efl/GraphicsContext3DEfl.cpp
  )
ENDIF()

[Patrick R. Gansterer]

Comment on attachment 97263 [details]
Patch

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

(In reply to comment #6)
> But I agree that we should check WinCE.
WinCE has no ENABLE_WEBGL support at the moment, so you can ignore WinCE without problems.

> Source/WebCore/CMakeLists.txt:-2074
> -        platform/graphics/opengl/GraphicsContext3DOpenGL.cpp

We usually have a empty line between different folders (graphics, graphics/gpu, graphics/opengl). But that more a "problem" of the previous patch. :-(

[Lucas De Marchi]

(In reply to comment #7)
> To do initial build for WebGL on EFL port, we have to add two files - DrawingBufferEfl.cpp & GraphicsContext3DEfl.cpp.
> And as gyuyoung mentioned, ENABLE_WEBGL macro should be added in CMakeListEfl.txt.
> 
> IF (ENABLE_WEBGL)
>   LIST(APPEND WebCore_SOURCES
>     platform/graphics/efl/DrawingBufferEfl.cpp
>     platform/graphics/efl/GraphicsContext3DEfl.cpp
>   )
> ENDIF()

No, you can ignore this part for now. Please, add this to CMakeListsEfl only when EFL has webgl support.

[Lucas De Marchi]

(In reply to comment #0)
> Bug 62376 makes all ports use GraphicsContext3DOpenGL, Extensions3DOpenGL when enabling WebGL.
> However, some ports already have their own GC3D implementation. In QT and chromium, for example, GraphicsContext3D delegates to GraphicsContext3DInternal. So, I think it would be better to allow each port to decide whether or not to include files in platform/graphics/opengl.

This explanation should appear in the ChangeLog.


Otherwise looks good.

[Hyowon Kim]

Created attachment 97426 [details]
Patch

[Hyowon Kim]

(In reply to comment #11)
> Created an attachment (id=97426) [details]
> Patch

I updated this patch to reflect Patrick's comment about empty lines & Lucas's one about the changelog.

[Leandro Pereira]

Comment on attachment 97426 [details]
Patch

OK.

[Gyuyoung Kim]

LGTM.

[Eric Seidel (no email)]

Comment on attachment 97426 [details]
Patch

rs=me.

[WebKit Review Bot]

Comment on attachment 97426 [details]
Patch

Clearing flags on attachment: 97426

Committed r95001: <http://trac.webkit.org/changeset/95001>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.