Bug 95173 – [EFL][WK2] Add necessary include directories in WebKit2

• Component: New Bugs
• Status: RESOLVED FIXED

[Zhigang Gong]

Add necessary include directories in WebKit2

[Zhigang Gong]

Created attachment 160918 [details]
Patch

[WebKit Review Bot]

Attachment 160918 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/CMakeLists.txt', u'Source/W..." exit_code: 1
Source/WebKit2/ChangeLog:9:  Line contains tab character.  [whitespace/tab] [5]
Source/WebKit2/ChangeLog:10:  Line contains tab character.  [whitespace/tab] [5]
Source/WebKit2/ChangeLog:11:  Line contains tab character.  [whitespace/tab] [5]
Source/WebKit2/ChangeLog:12:  Line contains tab character.  [whitespace/tab] [5]
Source/WebKit2/ChangeLog:13:  Line contains tab character.  [whitespace/tab] [5]
Source/WebKit2/ChangeLog:15:  Line contains tab character.  [whitespace/tab] [5]
Source/WebKit2/ChangeLog:16:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 7 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Zhigang Gong]

Created attachment 160920 [details]
Patch

[Simon Hausmann]

Comment on attachment 160920 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=160920&action=review

> Source/WebKit2/ChangeLog:8
> +        Commit f9d4da... modify the TextureMapperGL.h to include

I think that you may want to replace the git sha1 with the SVN revision here.

[Zhigang Gong]

*** Bug 95165 has been marked as a duplicate of this bug. ***

[Zhigang Gong]

Created attachment 161118 [details]
Patch

[Zhigang Gong]

(In reply to comment #4)
> (From update of attachment 160920 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=160920&action=review
> 
> > Source/WebKit2/ChangeLog:8
> > +        Commit f9d4da... modify the TextureMapperGL.h to include
> 
> I think that you may want to replace the git sha1 with the SVN revision here.

I modified the change log and resubmitted the patch. Any comment?

[Ryuan Choi]

LGTM.

I want this patch and another patch that adds `#include "WebPreferences.h"` in ewk_view.cpp.

[Kangil Han]

zhigang: would you please ask hausmann for r+?

[Zhigang Gong]

hausmann，
would you please help to review this patch? thx.

[Ryuan Choi]

(In reply to comment #10)
> hausmann，
> would you please help to review this patch? thx.

Hopefully, we can get r+.

Or, you can rquest cq? after changing "Reviewed by NOBODY (OOPS!)." to "Unreviewed build fix" because this is to fix build break.

[Zhigang Gong]

(In reply to comment #11)
> (In reply to comment #10)
> > hausmann，
> > would you please help to review this patch? thx.
> 
> Hopefully, we can get r+.
> 
> Or, you can rquest cq? after changing "Reviewed by NOBODY (OOPS!)." to "Unreviewed build fix" because this is to fix build break.

I will have to do that once i back to work on Mon, as I am not at my development machine now.

[Simon Hausmann]

Comment on attachment 161118 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=161118&action=review

> Source/WebKit2/CMakeLists.txt:115
> +        "${THIRDPARTY_DIR}/ANGLE/include"

I ran into this just a few days ago. Adding ANGLE/include to your include search path means that if you are compiling your code in a platform where EGL and GLES2 are the chosen system wide GL APIs, then an #include <EGL/egl.h> in WebKit2 will end up including ANGLE's EGL header file instead of your system wide one. The same applies to GLES2 inclusions.

Do you really need this include or can you limit this change to just the include/GLSLANG line below?

[Zhigang Gong]

Created attachment 163031 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 163031 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=163031&action=review

You don't need to request r? for unreviewed build fix.

> Source/WebKit2/ChangeLog:3
> +        Add necessary include directories in WebKit2

It is good to add [EFL][WK2] prefix to bug title.

[Gyuyoung Kim]

Comment on attachment 163031 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=163031&action=review

> Source/WebKit2/CMakeLists.txt:114
> +IF (WTF_USE_3D_GRAPHICS)

I think WebKit2/PlatformEfl.cmake is proper place because this build break occurs at EFL port only now.

[Zhigang Gong]

(In reply to comment #16)
> (From update of attachment 163031 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=163031&action=review
> 
> > Source/WebKit2/CMakeLists.txt:114
> > +IF (WTF_USE_3D_GRAPHICS)
> 
> I think WebKit2/PlatformEfl.cmake is proper place because this build break occurs at EFL port only now.

Hmm, IMO, it may be not a EFL port only problem even it just triggers problem on EFL platform now. The root cause is that when TEXTURE_MAPPER is enabled, some source files in WebKit2 include TextureMapperGL.h --> GraphicsContext3D -->trigger build problem. And as I know, TEXTURE_MAPPER is not EFL only， right? And those files which include TextureMapperGL.h are also not all for EFL platform only, for example the ShareableSurface.cpp.

Any further comments?

[Gyuyoung Kim]

Comment on attachment 163031 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=163031&action=review

>>> Source/WebKit2/CMakeLists.txt:114
>>> +IF (WTF_USE_3D_GRAPHICS)
>> 
>> I think WebKit2/PlatformEfl.cmake is proper place because this build break occurs at EFL port only now.
> 
> Hmm, IMO, it may be not a EFL port only problem even it just triggers problem on EFL platform now. The root cause is that when TEXTURE_MAPPER is enabled, some source files in WebKit2 include TextureMapperGL.h --> GraphicsContext3D -->trigger build problem. And as I know, TEXTURE_MAPPER is not EFL only， right? And those files which include TextureMapperGL.h are also not all for EFL platform only, for example the ShareableSurface.cpp.
> 
> Any further comments?

Of course, it seems to me all ports (EFL, blackberry and wince) will need to include "GLSLANG" directory if they need to use WEBGL(3D_GRAPHICS)
But, blackberry and wince ports don't start to support WK2 yet. So, I would remain to add this to EFL private file until they need to use the "GLSLANG". Because, there will may something stuff further.

In WK1 case, blackberry port also include this directory in PlatformBlackBerry.cmake.
http://trac.webkit.org/browser/trunk/Source/WebKit/PlatformBlackBerry.cmake#L120

[Zhigang Gong]

(In reply to comment #18)
> (From update of attachment 163031 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=163031&action=review
> 
> >>> Source/WebKit2/CMakeLists.txt:114
> >>> +IF (WTF_USE_3D_GRAPHICS)
> >> 
> >> I think WebKit2/PlatformEfl.cmake is proper place because this build break occurs at EFL port only now.
> > 
> > Hmm, IMO, it may be not a EFL port only problem even it just triggers problem on EFL platform now. The root cause is that when TEXTURE_MAPPER is enabled, some source files in WebKit2 include TextureMapperGL.h --> GraphicsContext3D -->trigger build problem. And as I know, TEXTURE_MAPPER is not EFL only， right? And those files which include TextureMapperGL.h are also not all for EFL platform only, for example the ShareableSurface.cpp.
> > 
> > Any further comments?
> 
> Of course, it seems to me all ports (EFL, blackberry and wince) will need to include "GLSLANG" directory if they need to use WEBGL(3D_GRAPHICS)
> But, blackberry and wince ports don't start to support WK2 yet. So, I would remain to add this to EFL private file until they need to use the "GLSLANG". Because, there will may something stuff further.
> 
> In WK1 case, blackberry port also include this directory in PlatformBlackBerry.cmake.
> http://trac.webkit.org/browser/trunk/Source/WebKit/PlatformBlackBerry.cmake#L120

I agree with you now. Will submit a new version soon.

[Zhigang Gong]

Created attachment 163279 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 163279 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=163279&action=review

> Source/WebKit2/ChangeLog:3
> +        Add necessary include directories in WebKit2

Please add [EFL][WK2] prefix to here as well.

[Zhigang Gong]

Created attachment 163283 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 163283 [details]
Patch

LGTM, thank. :-)

[WebKit Review Bot]

Comment on attachment 163283 [details]
Patch

Clearing flags on attachment: 163283

Committed r128154: <http://trac.webkit.org/changeset/128154>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.