Bug 73559

Created attachment 117416 [details] Patch

Comment on attachment 117416 [details] Patch what about cleaning up the FindOpenGLES2 file? we prefer 4 space indent and a space after "if" and so on (most of the other c++ style guidelines can be applied to CMake too)

I agree with paroga that the find-file needs some love: it has wrong indentation, a lot of empty lines and checks for some Ogre-specific variables (plus, I don't think we support BORLAND). In the end, the checks should just boil down to: find_path(OPENGLES2_INCLUDE_DIRS ... PATHS ... HINTS ...) find_library(OPENGLES2_LIBRARIES ... PATHS .. HINTS ...) find_path(EGL_INCLUDE_DIRS ... PATHS ... HINTS ...) find_library(EGL_LIBRARIES ... PATHS ... HINTS ...) include(FindPackageHandleStandardArgs) find_package_handle_standard_args(OPENGLES2 DEFAULT_MSG OPENGLES2_INCLUDE_DIRS OPENGLES2_LIBRARIES) find_package_handle_standard_args(EGL DEFAULT_MSG EGL_INCLUDE_DIRS EGL_LIBRARIES) Note FOO_INCLUDE_DIRS is preferred over FOO_INCLUDE_DIR according to CMake's convention.

Comment on attachment 117416 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=117416&action=review > Source/cmake/OptionsCommon.cmake:19 > + FIND_PACKAGE(OpenGLES2 REQUIRED) You have changed the package being searched, but Source/WebCore/CMakeLists.txt still links to OPENGL_LIBRARIES in the IF (ENABLE_WEBGL) block.

Created attachment 117790 [details] Patch

Comment on attachment 117790 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=117790&action=review The find-file is still needlessly complicated, and has some faulty logic in it. I suggest following that structure I mentioned in my previous review. > Source/cmake/FindOpenGLES2.cmake:18 > +# EGL_FOUND - system has EGL > +# EGL_INCLUDE_DIRS - the EGL include directory > +# EGL_LIBRARIES - Link these to use EGL Are there plans to use these EGL variables too? Otherwise they could be dropped. > Source/cmake/FindOpenGLES2.cmake:22 > + FIND_LIBRARY (OPENGLES2_gl_LIBRARY libGLESv2) Shouldn't "libGLESv2" be "GLESv2"? > Source/cmake/FindOpenGLES2.cmake:27 > + create_search_paths(/Developer/Platforms) > + findpkg_framework(OpenGLES2) > + set(OPENGLES2_gl_LIBRARY "-framework OpenGLES") create_search_paths() and findpkg_framework() seem to be leftovers from the original file which are not defined in webkit. > Source/cmake/FindOpenGLES2.cmake:60 > + INCLUDE (FindX11) Shouldn't this be FIND_PACKAGE(X11)? > Source/cmake/FindOpenGLES2.cmake:65 > + IF (NOT APPLE) > + SET (OPENGLES2_LIBRARIES ${X11_LIBRARIES}) > + ENDIF (NOT APPLE) We're already inside an ELSE(APPLE) block. > Source/cmake/FindOpenGLES2.cmake:71 > +IF (OPENGLES2_gl_LIBRARY AND EGL_egl_LIBRARY) This is always going to fail on WIN32.

Created attachment 118582 [details] Patch

Comment on attachment 118582 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=118582&action=review Awesome, this one looks so much better and smaller :) One non-find-module question now: does this mean WebGL only works with opengl es 2.0 (ie. it had never worked before)? > Source/cmake/FindOpenGLES2.cmake:4 > +# For details see the accompanying COPYING-CMAKE-SCRIPTS file. You need to either provide the mentioned COPYING-CMAKE-SCRIPTS or add the license here.

Comment on attachment 118582 [details] Patch Eli, is this change desired for all cmake based builds? Shouldn't we only do it for us?

It's for all cmake builds. WebGL fundamentally depends on OpenGL ES, not desktop OpenGL. I'll get the new license up shortly.

Created attachment 118889 [details] Patch

Comment on attachment 118889 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=118889&action=review > Source/cmake/FindOpenGLES2.cmake:13 > +# 3. The name of the author may not be used to endorse or promote products > +# derived from this software without specific prior written permission. One last nitpick: the WebKit project tends to use the 2-clause BSD license, which does not have this last item. It'd be good if this file could be licensed that way too.

(In reply to comment #10) > It's for all cmake builds. WebGL fundamentally depends on OpenGL ES, not desktop OpenGL. Yes. it's true. But OpenGL ES is not suitable on Desktop. So WebKit has porting layer like GraphicsContext3D and ANGLE currently as I know. Do you want to support embeded device using GLES? If so, I think it's better way adding separated build option to choose opengl backend.

It's true that some ports use an emulation layer instead of the GLES2.0 headers directly. The real issue here is that standard desktop OpenGL is never the right choice. For BlackBerry, we use the OpenGL ES 2.0 headers directly. ANGLE also provides a GLES2/gl2.h. Anything else that wants to build with CMake would have to change the check to find whichever dependencies that port depends on. I'm happy to just remove the OpenGL check altogether in OptionsCommon.cmake, and leave it up to the port-specific OptionsXXX.cmake, if people prefer that option.

(In reply to comment #14) > It's true that some ports use an emulation layer instead of the GLES2.0 headers directly. > > The real issue here is that standard desktop OpenGL is never the right choice. I can't agree. Basically WebKit should run well on Desktop, it means that OpenGL is the best choice for Desktop environment, not OpenGL ES. > For BlackBerry, we use the OpenGL ES 2.0 headers directly. ANGLE also provides a GLES2/gl2.h. Anything else that wants to build with CMake would have to change the check to find whichever dependencies that port depends on. > > I'm happy to just remove the OpenGL check altogether in OptionsCommon.cmake, and leave it up to the port-specific OptionsXXX.cmake, if people prefer that option. That's what i suggested. :) we need to add a new build-option to choose gl-backend. As your patch proposed, you just replace OpenGL to OpenGL ES. It makes some problem on Desktop while checking dependency, because almost desktop environment doesn't have OpenGL ES header and library.

Comment on attachment 118889 [details] Patch Clearly needs an update, at least for the license if nothing else.

Created attachment 121501 [details] Patch

So the FindOpenGLES2.cmake file and its associated changes are not needed anymore?

It's not needed for BlackBerry, and apparently not for EFL (based on the existing code). If another port needs it, they can add it.

Alright then, that's even better :)

Comment on attachment 121501 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=121501&action=review > ChangeLog:8 > + Apparently the EFL port uses desktop OpenGL to implement WebGL, but other ports such as BlackBerry use other libraries like OpenGL ES 2.0 or ANGLE. > + > + Reviewed by NOBODY (OOPS!). Nit: These lines should be swapped. Also, we wrap lines to around 80 columns in change log entries.

Created attachment 122022 [details] Patch

Comment on attachment 122022 [details] Patch Clearing flags on attachment: 122022 Committed r104722: <http://trac.webkit.org/changeset/104722>

All reviewed patches have been landed. Closing bug.