Bug 95173 - [EFL][WK2] Add necessary include directories in WebKit2
Summary: [EFL][WK2] Add necessary include directories in WebKit2
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
: 95165 (view as bug list)
Depends on:
Blocks: 95342
  Show dependency treegraph
 
Reported: 2012-08-27 23:54 PDT by Zhigang Gong
Modified: 2012-09-11 00:48 PDT (History)
6 users (show)

See Also:


Attachments
Patch (1.63 KB, patch)
2012-08-28 00:03 PDT, Zhigang Gong
no flags Details | Formatted Diff | Diff
Patch (1.68 KB, patch)
2012-08-28 00:19 PDT, Zhigang Gong
no flags Details | Formatted Diff | Diff
Patch (1.73 KB, text/plain)
2012-08-28 19:27 PDT, Zhigang Gong
no flags Details
Patch (1.72 KB, patch)
2012-09-09 20:55 PDT, Zhigang Gong
no flags Details | Formatted Diff | Diff
Patch (1.87 KB, patch)
2012-09-10 22:22 PDT, Zhigang Gong
no flags Details | Formatted Diff | Diff
Patch (1.88 KB, patch)
2012-09-10 22:39 PDT, Zhigang Gong
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Zhigang Gong 2012-08-27 23:54:17 PDT
Add necessary include directories in WebKit2
Comment 1 Zhigang Gong 2012-08-28 00:03:15 PDT
Created attachment 160918 [details]
Patch
Comment 2 WebKit Review Bot 2012-08-28 00:06:15 PDT
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.
Comment 3 Zhigang Gong 2012-08-28 00:19:16 PDT
Created attachment 160920 [details]
Patch
Comment 4 Simon Hausmann 2012-08-28 07:36:10 PDT
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.
Comment 5 Zhigang Gong 2012-08-28 09:45:06 PDT
*** Bug 95165 has been marked as a duplicate of this bug. ***
Comment 6 Zhigang Gong 2012-08-28 19:27:22 PDT
Created attachment 161118 [details]
Patch
Comment 7 Zhigang Gong 2012-08-31 02:39:35 PDT
(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?
Comment 8 Ryuan Choi 2012-09-05 21:26:53 PDT
LGTM.

I want this patch and another patch that adds `#include "WebPreferences.h"` in ewk_view.cpp.
Comment 9 Kangil Han 2012-09-07 06:57:55 PDT
zhigang: would you please ask hausmann for r+?
Comment 10 Zhigang Gong 2012-09-07 07:04:48 PDT
hausmann,
would you please help to review this patch? thx.
Comment 11 Ryuan Choi 2012-09-07 07:13:32 PDT
(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.
Comment 12 Zhigang Gong 2012-09-07 07:34:18 PDT
(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.
Comment 13 Simon Hausmann 2012-09-07 09:02:21 PDT
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?
Comment 14 Zhigang Gong 2012-09-09 20:55:32 PDT
Created attachment 163031 [details]
Patch
Comment 15 Gyuyoung Kim 2012-09-09 21:02:08 PDT
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.
Comment 16 Gyuyoung Kim 2012-09-10 17:56:49 PDT
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.
Comment 17 Zhigang Gong 2012-09-10 20:43:21 PDT
(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?
Comment 18 Gyuyoung Kim 2012-09-10 20:57:33 PDT
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
Comment 19 Zhigang Gong 2012-09-10 22:21:02 PDT
(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.
Comment 20 Zhigang Gong 2012-09-10 22:22:36 PDT
Created attachment 163279 [details]
Patch
Comment 21 Gyuyoung Kim 2012-09-10 22:23:54 PDT
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.
Comment 22 Zhigang Gong 2012-09-10 22:39:57 PDT
Created attachment 163283 [details]
Patch
Comment 23 Gyuyoung Kim 2012-09-10 22:46:08 PDT
Comment on attachment 163283 [details]
Patch

LGTM, thank. :-)
Comment 24 WebKit Review Bot 2012-09-11 00:48:35 PDT
Comment on attachment 163283 [details]
Patch

Clearing flags on attachment: 163283

Committed r128154: <http://trac.webkit.org/changeset/128154>
Comment 25 WebKit Review Bot 2012-09-11 00:48:40 PDT
All reviewed patches have been landed.  Closing bug.