Bug 137684 – Patch to build a wayland-only webkit/EFL(Without X11/Ecore_X)

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

[cjacker]

Here is a patch to build a wayland-only webkit/EFL(Without X11 and ecore_x).

It is applied on ewebkit-1.11.0.(It is almost ready for wayland-only, Thanks for great work.)

This patch includes:
1, Add some missing ifdefs to control X/Ecore_X related codes not be built under wayland-only environment.

2, Fix a inline func undefined issue in ViewportStyleResolver.cpp(I build it with clang 3.5, not sure whether it's a compiler problem or not.)

3, Fix a wrong func call in PlatformScreenEfl.cpp When ECore_X disabled.

4, Add a option 'ENABLE_WAYLAND_ONLY' and set some cmake options to control glx/X11 related codes not build when EGL enabled.

After patch applied, Webkit/EFL can be compiled without X/Ecore_X with "-DENABLE_WAYLAND_ONLY=ON". And the MiniBrowser works.

NOTE:
1, NETSCAPE_PLUGIN_API is disabled when ENABLE_WAYLAND_ONLY since it's related to X11/Xlib.

2, It will use cairosurface and software rendering mode, accelation will be disabled since evas_gl_new failed under wayland(there is no libGL exist.)

3, There is some other stable/unimplemented problems, but at least, it can be built now.

All this problems is related to X/ECore_X(Except No.2), and it's a small patch, So I think it's OK to submit a ALL-IN-ONE patch.

[cjacker]

Created attachment 239789 [details]
patch to apply on ewebkit2-1.11.0

[WebKit Commit Bot]

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


Total errors found: 0 in 0 files


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

[Gyuyoung Kim]

Comment on attachment 239789 [details]
patch to apply on ewebkit2-1.11.0

Oh, nice try ! It looks this patch works in progress still. Please ping me when you are ready to get review.

[Ryuan Choi]

Comment on attachment 239789 [details]
patch to apply on ewebkit2-1.11.0

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

First, thanks for the patch.
Patch looks almost fine to me.

I didn't try to prepare wayland backend yet.
I will check it more when I have it.

> ewebkitn/Source/cmake/OptionsEfl.cmake:128
> +option(ENABLE_ECORE_X "Enable Ecore_X specific usage (cursor, bell)")
> +
> +option(ENABLE_WAYLAND_ONLY "Enable wayland only build.")
> +if (ENABLE_WAYLAND_ONLY)
> +    set(ENABLE_X11_TARGET OFF)
> +    set(ENABLE_ECORE_X OFF)
> +    set(ENABLE_NETSCAPE_PLUGIN_API OFF)
> +else ()
> +    set(ENABLE_ECORE_X ON)
> +endif ()

In my opinion,
We'd better to choose window system between X or WAYLAND.

Perhaps, like below. (I am bad with naming)
set(WINDOW_SYSTEM "X" CACHE STRING "Choose window system for EWebKit (X or WAYLAND)")
...

In addition, I have one question.
Doesn't we need to use ECORE_WAYLAND?

> ewebkitn/Source/cmake/OptionsEfl.cmake:192
> +    if (ENABLE_WAYLAND_ONLY)
> +        set(ENABLE_X11_TARGET OFF)
> +    else ()
> +        set(ENABLE_X11_TARGET ON)
> +    endif ()

It looks not necessary if we checked similar logic in above statement.

> ewebkitn/Source/cmake/OptionsEfl.cmake:248
> +    if (ENABLE_WAYLAND_ONLY)
> +        set(ENABLE_GLES2 ON)
> +    endif ()

Should WAYLAND backend depend on this?

> ewebkitn/Source/cmake/OptionsEfl.cmake:264
> +    if (ENABLE_WAYLAND_ONLY)
> +        set(ENABLE_EGL ON)
> +    endif ()

Ditto.

> ewebkit2/Source/WebCore/platform/efl/EflScreenUtilities.cpp:69
> +#ifdef HAVE_ECORE_X
>  #include <Ecore_X.h>
> +#endif

Usually, we moved conditional includes to the bottom of includes with empty line.

#include ...
#include ...

#if XXX
#include ...
#endif

#if YYY
#include ..
#endif

> ewebkit2/Source/WebCore/css/ViewportStyleResolver.cpp:43
> +//for undefined renderStyle(), it's a inline function in header.
> +#include "NodeRenderStyle.h"

I think that this is not related to EFL port.
So I think that we should make different bug for this.

[cjacker]

(In reply to comment #4)
Thanks for your helpful reply.
comment in content.

I will try git codes and refind this patch.

> (From update of attachment 239789 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=239789&action=review
> 
> First, thanks for the patch.
> Patch looks almost fine to me.
> 
> I didn't try to prepare wayland backend yet.
> I will check it more when I have it.
> 
> > ewebkitn/Source/cmake/OptionsEfl.cmake:128
> > +option(ENABLE_ECORE_X "Enable Ecore_X specific usage (cursor, bell)")
> > +
> > +option(ENABLE_WAYLAND_ONLY "Enable wayland only build.")
> > +if (ENABLE_WAYLAND_ONLY)
> > +    set(ENABLE_X11_TARGET OFF)
> > +    set(ENABLE_ECORE_X OFF)
> > +    set(ENABLE_NETSCAPE_PLUGIN_API OFF)
> > +else ()
> > +    set(ENABLE_ECORE_X ON)
> > +endif ()
> 
> In my opinion,
> We'd better to choose window system between X or WAYLAND.
> 
> Perhaps, like below. (I am bad with naming)
> set(WINDOW_SYSTEM "X" CACHE STRING "Choose window system for EWebKit (X or WAYLAND)")
> ...
> 

Good suggestion.

> In addition, I have one question.
> Doesn't we need to use ECORE_WAYLAND?
> 

It seems there is no direct dependency on ECORE_WAYLAND currently.
I think it should be checked to ensure EFL wayland backend enabled.

> > ewebkitn/Source/cmake/OptionsEfl.cmake:192
> > +    if (ENABLE_WAYLAND_ONLY)
> > +        set(ENABLE_X11_TARGET OFF)
> > +    else ()
> > +        set(ENABLE_X11_TARGET ON)
> > +    endif ()
> 
> It looks not necessary if we checked similar logic in above statement.
> 
> > ewebkitn/Source/cmake/OptionsEfl.cmake:248
> > +    if (ENABLE_WAYLAND_ONLY)
> > +        set(ENABLE_GLES2 ON)
> > +    endif ()
> 
> Should WAYLAND backend depend on this?
> 
> > ewebkitn/Source/cmake/OptionsEfl.cmake:264
> > +    if (ENABLE_WAYLAND_ONLY)
> > +        set(ENABLE_EGL ON)
> > +    endif ()
> 
> Ditto.

Above two defines will be enabled when WEBGL or WTF_USE_3D_GRAPHICS set to ON, because there is no libGL(glx) under wayland-only environment, these two option should be and MUST be enabled on wayland-only build.

for example, "Source/WebKit2/UIProcess/CoordinatedGraphics/WebView.cpp" and other uiprocess related codes depend on COORDINATED_GRAPHICS macro, it is controlbed by WTF_USE_3D_GRAPHICS option.

Currently, acceleration will be disabled automatically, since evas_gl_new will fail without libGL. it will fallback to cairo surface instead of GLSurface, I guess WEBGL did not work now(untest), But the cairo rendering works under wayland-only enviroment.(Thank god, at least, it works.)

> 
> > ewebkit2/Source/WebCore/platform/efl/EflScreenUtilities.cpp:69
> > +#ifdef HAVE_ECORE_X
> >  #include <Ecore_X.h>
> > +#endif
> 
> Usually, we moved conditional includes to the bottom of includes with empty line.
> 
> #include ...
> #include ...
> 
> #if XXX
> #include ...
> #endif
> 
> #if YYY
> #include ..
> #endif
> 
OK, looks more clear.

> > ewebkit2/Source/WebCore/css/ViewportStyleResolver.cpp:43
> > +//for undefined renderStyle(), it's a inline function in header.
> > +#include "NodeRenderStyle.h"
> 
> I think that this is not related to EFL port.
> So I think that we should make different bug for this.

Yes, I found this commit in Git codes(No more bug report needed), It should be clang issue.

[cjacker]

Created attachment 239849 [details]
New patch to enable wayland build of ewebkit2

This patch include:
1, Changes to Source/WebKit2/UIProcess/efl/WebPageProxyEfl.cpp
The plugin related funcs is controlled by PLUGIN_ARCHITECTURE(X11) macro in header(Source/WebKit2/UIProcess/WebPageProxy.h).


2, Changes to Source/WebKit2/UIProcess/API/efl/EwkView.cpp
ECore_X.h should be imported only when HAVE_ECORE_X defined, It did no fail wayland build when this header exist.
But for pure wayland environment, This header did not exist at all.


3, Changes to Source/WebCore/platform/efl/EflScreenUtilities.cpp
Enable it only when HAVE_ECORE_X defined like other funcs in source.


4, Changes to Source/WebCore/platform/efl/PlatformScreenEfl.cpp
evas() function did not exist anymore, should use platformWidget() instead.
It's a bug when ECore_X disabled.

5, Changes to Tools/CMakeLists.txt
Do not build TestNetscapePlugIn when ENABLE_X11_TARGET OFF, since it's X11/Xlib related.

Above 5 changes should be safe, hope it can be commited soon.


And ...., about cmake options, It's not easy to make the logic clear:

Maybe ENABLE_ECORE_X should be removed and added two new ENABLE_X/ENABLE_WAYLAND options,
Here I still use a "ENABLE_WAYLAND" option, If it set ON:

1, ensure ENABLE_X11_TARGET/ENABLE_ECORE_X/ENABLE_NETSCAPE_PLUGIN_API set to OFF, since they depend on X.
2, check ecore-wayland to ensure EFL had wayland support.
3, set ENABLE_GLES2 and ENABLE_EGL ON to disable GL/GLX requirement.
4, do not build X11 related codes in EGL source list.
5, link WebCore/WebKit2 to GLESv2(when SHARED_CORE ON, libwebcore need link to GLESv2 also.)
6, Only link to X11 libs when ENABLE_X11_TARGET ON.

GLESv2/EGL required by WTF_USE_TILED_BACKING_STORE(-DWTF_USE_COORDINATED_GRAPHICS=1) under wayland build. (For X11 build, GL/GLX will be used by default.)

Acctually, WTF_USE_TILED_BACKING_STORE should never be disabled(even under X11), otherwise all gui related codes will be disabled(the COORDINATED_GRAPHICS macro mask all gui related codes.).

[Michael Catanzaro]

EFL has been removed from trunk