Bug 146135 – [EFL] Fix the minibrowser after r185725

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit EFL

[Hunseop Jeong]

Minibrowser didn't work after r185725 because minibrowser didn't find the define of HAVE_ECORE_X.

[Hunseop Jeong]

Created attachment 255167 [details]
Patch

[Gyuyoung Kim]

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.

[Hunseop Jeong]

(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.

[Hunseop Jeong]

You can check this problem when executing the minibrowser with gl backend after r185725.

[Gyuyoung Kim]

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.

[Gyuyoung Kim]

(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.

[Hunseop Jeong]

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.

[Gyuyoung Kim]

(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.

[Ryuan Choi]

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.

[Gyuyoung Kim]

(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 !

[Hunseop Jeong]

(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.

[Hunseop Jeong]

Created attachment 255181 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 255181 [details]
Patch

Clearing flags on attachment: 255181

Committed r185740: <http://trac.webkit.org/changeset/185740>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.