Minibrowser didn't work after r185725 because minibrowser didn't find the define of HAVE_ECORE_X.
Created attachment 255167 [details] Patch
Comment on attachment 255167 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=255167&action=review > Tools/MiniBrowser/efl/CMakeLists.txt:15 > + ${CMAKE_BINARY_DIR} We should not include any binary output directory from application side.
(In reply to comment #2) > Comment on attachment 255167 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=255167&action=review > > > Tools/MiniBrowser/efl/CMakeLists.txt:15 > > + ${CMAKE_BINARY_DIR} > > We should not include any binary output directory from application side. How can I find the "cmakeconfig.h" in main.c without adding the CMAKE_BINARY_DIR? GTK+port also added the CMAKE_BINARY_DIR at MiniBrowser_INCLUDE_DIRECTORIES.
You can check this problem when executing the minibrowser with gl backend after r185725.
Comment on attachment 255167 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=255167&action=review >>> Tools/MiniBrowser/efl/CMakeLists.txt:15 >>> + ${CMAKE_BINARY_DIR} >> >> We should not include any binary output directory from application side. > > How can I find the "cmakeconfig.h" in main.c without adding the CMAKE_BINARY_DIR? > GTK+port also added the CMAKE_BINARY_DIR at MiniBrowser_INCLUDE_DIRECTORIES. I think we need to check if generated "cmakeconfig.h" can be moved to ${DERIVED_SOURCES_WEBKIT2_DIR}/include, then we may make EwebKit2.h to include the cmakeconfig.h. If so, we don't need to modify MiniBrowser not at all.
(In reply to comment #5) > Comment on attachment 255167 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=255167&action=review > > >>> Tools/MiniBrowser/efl/CMakeLists.txt:15 > >>> + ${CMAKE_BINARY_DIR} > >> > >> We should not include any binary output directory from application side. > > > > How can I find the "cmakeconfig.h" in main.c without adding the CMAKE_BINARY_DIR? > > GTK+port also added the CMAKE_BINARY_DIR at MiniBrowser_INCLUDE_DIRECTORIES. > > I think we need to check if generated "cmakeconfig.h" can be moved to > ${DERIVED_SOURCES_WEBKIT2_DIR}/include, then we may make EwebKit2.h to > include the cmakeconfig.h. If so, we don't need to modify MiniBrowser not at > all. I'm not 100% sure if we should open the cmakeconfig.h yet.
You can check this problem when executing the minibrowser with gl backend after r185725.(In reply to comment #5) > Comment on attachment 255167 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=255167&action=review > > >>> Tools/MiniBrowser/efl/CMakeLists.txt:15 > >>> + ${CMAKE_BINARY_DIR} > >> > >> We should not include any binary output directory from application side. > > > > How can I find the "cmakeconfig.h" in main.c without adding the CMAKE_BINARY_DIR? > > GTK+port also added the CMAKE_BINARY_DIR at MiniBrowser_INCLUDE_DIRECTORIES. > > I think we need to check if generated "cmakeconfig.h" can be moved to > ${DERIVED_SOURCES_WEBKIT2_DIR}/include, then we may make EwebKit2.h to > include the cmakeconfig.h. If so, we don't need to modify MiniBrowser not at > all. Is it okay to move the cmakeconfig.h to ${DERIVED_SOURCES_WEBKIT2_DIR}/include? I have some doubt why we move the cmakeconfig.h to ${DERIVED_SOURCES_WEBKIT2_DIR}/include? Is cmakeconfig.h only used by files in ${DERIVED_SOURCES_WEBKIT2_DIR}/include? Already cmake based projects used the cmakeconfig.h in ${CMAKE_BINARY_DIR}. If moving the cmakeconfig.h is right, I have to change the many files.
(In reply to comment #7) > You can check this problem when executing the minibrowser with gl backend > after r185725.(In reply to comment #5) > > Comment on attachment 255167 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=255167&action=review > > > > >>> Tools/MiniBrowser/efl/CMakeLists.txt:15 > > >>> + ${CMAKE_BINARY_DIR} > > >> > > >> We should not include any binary output directory from application side. > > > > > > How can I find the "cmakeconfig.h" in main.c without adding the CMAKE_BINARY_DIR? > > > GTK+port also added the CMAKE_BINARY_DIR at MiniBrowser_INCLUDE_DIRECTORIES. > > > > I think we need to check if generated "cmakeconfig.h" can be moved to > > ${DERIVED_SOURCES_WEBKIT2_DIR}/include, then we may make EwebKit2.h to > > include the cmakeconfig.h. If so, we don't need to modify MiniBrowser not at > > all. > > Is it okay to move the cmakeconfig.h to > ${DERIVED_SOURCES_WEBKIT2_DIR}/include? > > I have some doubt why we move the cmakeconfig.h to > ${DERIVED_SOURCES_WEBKIT2_DIR}/include? > Is cmakeconfig.h only used by files in > ${DERIVED_SOURCES_WEBKIT2_DIR}/include? > > Already cmake based projects used the cmakeconfig.h in ${CMAKE_BINARY_DIR}. > If moving the cmakeconfig.h is right, I have to change the many files. I meant copy, not move :) Anyway, as I said, I'm not sure if we have to open cmakeconfig.h to third party application. We may let MiniBrowser to include CMAKE_BINARY_DIR. However I don't think this is correct use. It looks my recommendation is not correct use too. I think correct solution is that we define it to ewebkit.pc, application(e.g. MiniBrowser) reads it, then it works based on the definition. How about just defining HAVE_ECORE_X in main.c as below ? #define HAVE_ECORE_X 1 In other words, I think it is too excessive work that MiniBrowser includes cmakeconfig.h only in order to use HAVE_ECORE_X.
Comment on attachment 255167 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=255167&action=review >>>>>> Tools/MiniBrowser/efl/CMakeLists.txt:15 >>>>>> + ${CMAKE_BINARY_DIR} >>>>> >>>>> We should not include any binary output directory from application side. >>>> >>>> How can I find the "cmakeconfig.h" in main.c without adding the CMAKE_BINARY_DIR? >>>> GTK+port also added the CMAKE_BINARY_DIR at MiniBrowser_INCLUDE_DIRECTORIES. >>> >>> I think we need to check if generated "cmakeconfig.h" can be moved to ${DERIVED_SOURCES_WEBKIT2_DIR}/include, then we may make EwebKit2.h to include the cmakeconfig.h. If so, we don't need to modify MiniBrowser not at all. >> >> I'm not 100% sure if we should open the cmakeconfig.h yet. > > Is it okay to move the cmakeconfig.h to ${DERIVED_SOURCES_WEBKIT2_DIR}/include? > > I have some doubt why we move the cmakeconfig.h to ${DERIVED_SOURCES_WEBKIT2_DIR}/include? > Is cmakeconfig.h only used by files in ${DERIVED_SOURCES_WEBKIT2_DIR}/include? > > Already cmake based projects used the cmakeconfig.h in ${CMAKE_BINARY_DIR}. > If moving the cmakeconfig.h is right, I have to change the many files. No! In this case, We should not use HAVE_ECORE_X in MiniBrowser. HAVE_ECORE_X guard was introduced to set opengl_x11 as preferred engine but now we set opengl. I am not sure whether we should keep the macro.
(In reply to comment #9) > Comment on attachment 255167 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=255167&action=review > > >>>>>> Tools/MiniBrowser/efl/CMakeLists.txt:15 > >>>>>> + ${CMAKE_BINARY_DIR} > >>>>> > >>>>> We should not include any binary output directory from application side. > >>>> > >>>> How can I find the "cmakeconfig.h" in main.c without adding the CMAKE_BINARY_DIR? > >>>> GTK+port also added the CMAKE_BINARY_DIR at MiniBrowser_INCLUDE_DIRECTORIES. > >>> > >>> I think we need to check if generated "cmakeconfig.h" can be moved to ${DERIVED_SOURCES_WEBKIT2_DIR}/include, then we may make EwebKit2.h to include the cmakeconfig.h. If so, we don't need to modify MiniBrowser not at all. > >> > >> I'm not 100% sure if we should open the cmakeconfig.h yet. > > > > Is it okay to move the cmakeconfig.h to ${DERIVED_SOURCES_WEBKIT2_DIR}/include? > > > > I have some doubt why we move the cmakeconfig.h to ${DERIVED_SOURCES_WEBKIT2_DIR}/include? > > Is cmakeconfig.h only used by files in ${DERIVED_SOURCES_WEBKIT2_DIR}/include? > > > > Already cmake based projects used the cmakeconfig.h in ${CMAKE_BINARY_DIR}. > > If moving the cmakeconfig.h is right, I have to change the many files. > > No! > > In this case, > We should not use HAVE_ECORE_X in MiniBrowser. > > HAVE_ECORE_X guard was introduced to set opengl_x11 as preferred engine but > now we set opengl. > I am not sure whether we should keep the macro. +1 I think so ! @Hunseop, just define HAVE_ECORE_X in main.c or remove it !
(In reply to comment #10) > (In reply to comment #9) > > Comment on attachment 255167 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=255167&action=review > > > > >>>>>> Tools/MiniBrowser/efl/CMakeLists.txt:15 > > >>>>>> + ${CMAKE_BINARY_DIR} > > >>>>> > > >>>>> We should not include any binary output directory from application side. > > >>>> > > >>>> How can I find the "cmakeconfig.h" in main.c without adding the CMAKE_BINARY_DIR? > > >>>> GTK+port also added the CMAKE_BINARY_DIR at MiniBrowser_INCLUDE_DIRECTORIES. > > >>> > > >>> I think we need to check if generated "cmakeconfig.h" can be moved to ${DERIVED_SOURCES_WEBKIT2_DIR}/include, then we may make EwebKit2.h to include the cmakeconfig.h. If so, we don't need to modify MiniBrowser not at all. > > >> > > >> I'm not 100% sure if we should open the cmakeconfig.h yet. > > > > > > Is it okay to move the cmakeconfig.h to ${DERIVED_SOURCES_WEBKIT2_DIR}/include? > > > > > > I have some doubt why we move the cmakeconfig.h to ${DERIVED_SOURCES_WEBKIT2_DIR}/include? > > > Is cmakeconfig.h only used by files in ${DERIVED_SOURCES_WEBKIT2_DIR}/include? > > > > > > Already cmake based projects used the cmakeconfig.h in ${CMAKE_BINARY_DIR}. > > > If moving the cmakeconfig.h is right, I have to change the many files. > > > > No! > > > > In this case, > > We should not use HAVE_ECORE_X in MiniBrowser. > > > > HAVE_ECORE_X guard was introduced to set opengl_x11 as preferred engine but > > now we set opengl. > > I am not sure whether we should keep the macro. > > +1 I think so ! > > @Hunseop, just define HAVE_ECORE_X in main.c or remove it ! Okay, I will remove it! Thanks.
Created attachment 255181 [details] Patch
Comment on attachment 255181 [details] Patch Clearing flags on attachment: 255181 Committed r185740: <http://trac.webkit.org/changeset/185740>
All reviewed patches have been landed. Closing bug.