Bug 73111

Created attachment 116564 [details] This patch makes TextureMapper build on EFL port.

Comment on attachment 116564 [details] This patch makes TextureMapper build on EFL port. The changes to TextureMapper are OK with me. I don't mind reviewing this whole patch, but I'm not well versed in the EFL port to know if that is welcome.

Comment on attachment 116564 [details] This patch makes TextureMapper build on EFL port. View in context: https://bugs.webkit.org/attachment.cgi?id=116564&action=review LGTM aside from the nitpicks below. > ChangeLog:10 > + No new tests, this is a buildsystem changes. changes -> change. > ChangeLog:12 > + * Source/cmake/OptionsEfl.cmake: I'd move the "Add WTF_USE_TEXTURE_MAPPER [...]" paragraph here. > Source/WebCore/ChangeLog:17 > + * platform/graphics/efl/GraphicsLayerEfl.cpp: Removed. > + * platform/graphics/efl/GraphicsLayerEfl.h: Removed. It's weird that these files do not show up in the patch.

Comment on attachment 116564 [details] This patch makes TextureMapper build on EFL port. Attachment 116564 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/10646401

Comment on attachment 116564 [details] This patch makes TextureMapper build on EFL port. Seems to not compile.

Comment on attachment 116564 [details] This patch makes TextureMapper build on EFL port. View in context: https://bugs.webkit.org/attachment.cgi?id=116564&action=review It looks this patch needs to use new GL library. But, it looks this patch misses library dependency. In order to avoid build break in EWS, we have to install needed library to EWS server first. Please add gl library dependency to cmake files, and then we have to install gl library to EWS and build bot server together. /mnt/eflews/git/webkit/Source/WebCore/platform/graphics/opengl/TextureMapperGL.cpp:46: error: variable or field 'glUniform1f' declared void /mnt/eflews/git/webkit/Source/WebCore/platform/graphics/opengl/TextureMapperGL.cpp:46: error: 'GLint' was not declared in this scope /mnt/eflews/git/webkit/Source/WebCore/platform/graphics/opengl/TextureMapperGL.cpp:46: error: 'GLfloat' was not declared in this scope /mnt/eflews/git/webkit/Source/WebCore/platform/graphics/opengl/TextureMapperGL.cpp:47: error: variable or field 'glUniform1i' declared void /mnt/eflews/git/webkit/Source/WebCore/platform/graphics/opengl/TextureMapperGL.cpp:47: error: 'GLint' was not declared in this scope /mnt/eflews/git/webkit/Source/WebCore/platform/graphics/opengl/TextureMapperGL.cpp:47: error: 'GLint' was not declared in this scope /mnt/eflews/git/webkit/Source/WebCore/platform/graphics/opengl/TextureMapperGL.cpp:48: error: variable or field 'glVertexAttribPointer' declared void /mnt/eflews/git/webkit/Source/WebCore/platform/graphics/opengl/TextureMapperGL.cpp:48: error: 'GLuint' was not declared in this scope /mnt/eflews/git/webkit/Source/WebCore/platform/graphics/opengl/TextureMapperGL.cpp:48: error: 'GLint' was not declared in this scope /mnt/eflews/git/webkit/Source/WebCore/platform/graphics/opengl/TextureMapperGL.cpp:48: error: 'GLenum' was not declared in this scope /mnt/eflews/git/webkit/Source/WebCore/platform/graphics/opengl/TextureMapperGL.cpp:48: error: 'GLboolean' was not declared in this scope /mnt/eflews/git/webkit/Source/WebCore/platform/graphics/opengl/TextureMapperGL.cpp:48: error: 'GLsizei' was not declared in this scope /mnt/eflews/git/webkit/Source/WebCore/platform/graphics/opengl/TextureMapperGL.cpp:48: error: expected primary-expression before 'const' /mnt/eflews/git/webkit/Source/WebCore/platform/graphics/opengl/TextureMapperGL.cpp:49: error: variable or field 'glUniform4f' declared void /mnt/eflews/git/webkit/Source/WebCore/platform/graphics/opengl/TextureMapperGL.cpp:49: error: 'GLint' was not declared in t > Source/WebCore/ChangeLog:11 > + No new tests. Covered by existing tests. Would you mention existing test cases you tested?

Created attachment 116716 [details] Patch

Comment on attachment 116716 [details] Patch Attachment 116716 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/10679112

Comment on attachment 116716 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=116716&action=review Ok. The needed gl library is just installed on EWS server. :-) EWS build error will not occur in next submit. > Source/cmake/OptionsEfl.cmake:39 > +FIND_PACKAGE(GL REQUIRED) I wonder if we don't need to check GL version. Is old GL library Ok ?

Hi, It still does not compile due to that there is missing FindOpenGL.cmake file. What about adding something like this: Source/cmake/FindOpenGL.cmake include(LibFindMacros) libfind_pkg_check_modules(OpenGL_PKGCONF gl) find_path(OpenGL_INCLUDE_DIR NAMES GL/gl.h PATHS ${OpenGL_PKGCONF_INCLUDE_DIRS} ) find_library(OpenGL_LIBRARY NAMES GL PATHS ${OpenGL_PKGCONF_LIBRARY_DIRS} ) set(OpenGL_PROCESS_INCLUDES OpenGL_INCLUDE_DIR) set(OpenGL_PROCESS_LIBS OpenGL_LIBRARY) libfind_process(OpenGL) It could be used as: FIND_PACKAGE(OpenGL REQUIRED)

Created attachment 116728 [details] Patch

Comment on attachment 116728 [details] Patch Attachment 116728 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/10668682

(In reply to comment #11) > Created an attachment (id=116728) [details] > Patch The ${GL_LIBRARY} is not valid now. Please update it according to my FindOpenGL file that I have posted in comment 10.

(In reply to comment #13) > (In reply to comment #11) > > Created an attachment (id=116728) [details] [details] > > Patch > > The ${GL_LIBRARY} is not valid now. Please update it according to my FindOpenGL file that I have posted in comment 10. Please don't add FindOpenGL.cmake, CMake already has one. See CMake's FindOpenGL.cmake and adjust the buildsystem accordingly.

(In reply to comment #14) > (In reply to comment #13) > > (In reply to comment #11) > > > Created an attachment (id=116728) [details] [details] [details] > > > Patch > > > > The ${GL_LIBRARY} is not valid now. Please update it according to my FindOpenGL file that I have posted in comment 10. > > Please don't add FindOpenGL.cmake, CMake already has one. See CMake's FindOpenGL.cmake and adjust the buildsystem accordingly. It should be good if you change gl's varables name to: OPENGL_INCLUDE_DIR - the GL include directory OPENGL_LIBRARIES - Link these to use OpenGL and GLU

(In reply to comment #14) > (In reply to comment #13) > > (In reply to comment #11) > > > Created an attachment (id=116728) [details] [details] [details] > > > Patch > > > > The ${GL_LIBRARY} is not valid now. Please update it according to my FindOpenGL file that I have posted in comment 10. > > Please don't add FindOpenGL.cmake, CMake already has one. See CMake's FindOpenGL.cmake and adjust the buildsystem accordingly. +dbates.

(In reply to comment #14) > (In reply to comment #13) > > (In reply to comment #11) > > > Created an attachment (id=116728) [details] [details] [details] > > > Patch > > > > The ${GL_LIBRARY} is not valid now. Please update it according to my FindOpenGL file that I have posted in comment 10. > > Please don't add FindOpenGL.cmake, CMake already has one. See CMake's FindOpenGL.cmake and adjust the buildsystem accordingly. I found that CMake has support for finding the OpenGL package. (/usr/share/cmake-2.8/Moduels/Find*.cmake) Thanks for letting me know.

Created attachment 116901 [details] Patch

(In reply to comment #2) > (From update of attachment 116564 [details]) > The changes to TextureMapper are OK with me. > I don't mind reviewing this whole patch, but I'm not well versed in the EFL port to know if that is welcome. Your review can help me a lot~! Thanks!

Comment on attachment 116901 [details] Patch LGTM. Please have a committer from EFL commit when you can watch your bots/EWS.

Comment on attachment 116901 [details] Patch Rejecting attachment 116901 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 Last 500 characters of output: hird_party/libwebp --revision 111575 --non-interactive --force --accept theirs-conflict --ignore-externals returned non-zero exit status 1 in /mnt/git/webkit-commit-queue/Source/WebKit/chromium Error: 'depot_tools/gclient sync --force --reset --delete_unversioned_trees' failed 3 tries and returned 256 at Tools/Scripts/update-webkit-chromium line 109. Re-trying 'depot_tools/gclient sync --force --reset --delete_unversioned_trees' No such file or directory at Tools/Scripts/update-webkit line 112. Full output: http://queues.webkit.org/results/10679563

Created attachment 116920 [details] Patch

Comment on attachment 116920 [details] Patch Clearing flags on attachment: 116920 Committed r101347: <http://trac.webkit.org/changeset/101347>

All reviewed patches have been landed. Closing bug.

Hmm, I wonder why the EWS did not fail with this patch (perhaps it fails when SHARED_CORE=On?). The build always fails here when linking the Tools programs (EWebLauncher, DumpRenderTree and ImageDiff): ../../../Source/WebCore/libwebcore_efl.so.0.1.0: undefined reference to `WebCore::uidForImage(WebCore::Image*)' ../../../Source/WebCore/libwebcore_efl.so.0.1.0: undefined reference to `WebCore::BGRA32PremultimpliedBuffer::create()' collect2: ld returned 1 exit status And indeed, these two are only implemented by TextureMapperQt.cpp. I think the patch as-is should be rolled out.

Created https://bugs.webkit.org/show_bug.cgi?id=73580 manually, as the rollout had conflicts and sheriffbot failed.

Hyowon, when would you have time to continue upstreaming on this topic?

(In reply to comment #27) > Hyowon, when would you have time to continue upstreaming on this topic? This patch should apply without any problems now, none of those methods exists anymore - reopen

(In reply to comment #28) > (In reply to comment #27) > > Hyowon, when would you have time to continue upstreaming on this topic? > > This patch should apply without any problems now, none of those methods exists anymore - reopen This patch more or less seems to be in trunk, minus that the old files are also still there.

Created attachment 165850 [details] Patch

(In reply to comment #30) > Created an attachment (id=165850) [details] > Patch Unnecessary files :) Thanks.

Comment on attachment 165850 [details] Patch Clearing flags on attachment: 165850 Committed r129720: <http://trac.webkit.org/changeset/129720>

All reviewed patches have been landed. Closing bug.