Bug 62707 - [CMAKE] Remove platform/graphics/opengl/*OpenGL.cpp files in CMakeLists.txt
Summary: [CMAKE] Remove platform/graphics/opengl/*OpenGL.cpp files in CMakeLists.txt
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebGL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 62695 62709
  Show dependency treegraph
 
Reported: 2011-06-15 01:43 PDT by Hyowon Kim
Modified: 2011-09-12 20:01 PDT (History)
9 users (show)

See Also:


Attachments
Patch (1.54 KB, patch)
2011-06-15 01:58 PDT, Hyowon Kim
no flags Details | Formatted Diff | Diff
Patch (1.60 KB, patch)
2011-06-15 02:38 PDT, Hyowon Kim
no flags Details | Formatted Diff | Diff
Patch (1.98 KB, patch)
2011-06-16 02:42 PDT, Hyowon Kim
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Hyowon Kim 2011-06-15 01:43:03 PDT
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.
Comment 1 Hyowon Kim 2011-06-15 01:58:49 PDT
Created attachment 97261 [details]
Patch
Comment 2 Ryuan Choi 2011-06-15 02:23:32 PDT
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.
Comment 3 Hyowon Kim 2011-06-15 02:38:32 PDT
Created attachment 97263 [details]
Patch
Comment 4 Hyowon Kim 2011-06-15 02:42:30 PDT
(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.
Comment 5 Gyuyoung Kim 2011-06-15 02:50:16 PDT
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
Comment 6 Ryuan Choi 2011-06-15 03:07:47 PDT
(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.
Comment 7 Hyowon Kim 2011-06-15 03:09:27 PDT
(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 8 Patrick R. Gansterer 2011-06-15 04:24:33 PDT
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. :-(
Comment 9 Lucas De Marchi 2011-06-15 06:28:02 PDT
(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.
Comment 10 Lucas De Marchi 2011-06-15 06:28:58 PDT
(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.
Comment 11 Hyowon Kim 2011-06-16 02:42:34 PDT
Created attachment 97426 [details]
Patch
Comment 12 Hyowon Kim 2011-06-16 02:50:16 PDT
(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 13 Leandro Pereira 2011-06-16 07:15:59 PDT
Comment on attachment 97426 [details]
Patch

OK.
Comment 14 Gyuyoung Kim 2011-06-21 19:16:22 PDT
LGTM.
Comment 15 Eric Seidel (no email) 2011-09-12 15:37:10 PDT
Comment on attachment 97426 [details]
Patch

rs=me.
Comment 16 WebKit Review Bot 2011-09-12 20:01:09 PDT
Comment on attachment 97426 [details]
Patch

Clearing flags on attachment: 97426

Committed r95001: <http://trac.webkit.org/changeset/95001>
Comment 17 WebKit Review Bot 2011-09-12 20:01:15 PDT
All reviewed patches have been landed.  Closing bug.