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.
Created attachment 97261 [details] Patch
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.
Created attachment 97263 [details] Patch
(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.
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
(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.
(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()
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. :-(
(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.
(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.
Created attachment 97426 [details] Patch
(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.
Comment on attachment 97426 [details] Patch OK.
LGTM.
Comment on attachment 97426 [details] Patch rs=me.
Comment on attachment 97426 [details] Patch Clearing flags on attachment: 97426 Committed r95001: <http://trac.webkit.org/changeset/95001>
All reviewed patches have been landed. Closing bug.