Bug 144998

Summary: [GTK] Fix Wayland-related header inclusions in GLContext, GLContextEGL, PlatformDisplay
Product: WebKit Reporter: Zan Dobersek <zan>
Component: New BugsAssignee: Zan Dobersek <zan>
Status: NEW ---    
Severity: Normal CC: bugs-noreply
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch mcatanzaro: review+

Description Zan Dobersek 2015-05-14 04:26:27 PDT
[GTK] Fix Wayland-related header inclusions in GLContext, GLContextEGL, PlatformDisplay
Comment 1 Zan Dobersek 2015-05-14 04:41:53 PDT
Created attachment 253118 [details]
Patch
Comment 2 Carlos Garcia Campos 2015-05-14 05:12:45 PDT
Comment on attachment 253118 [details]
Patch

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

Would it be possible to use forward declarations in the headers and do the includes in the cpp files?

> Source/WebCore/platform/graphics/GLContext.h:30
> +#include <wayland-egl.h>

This is not specific to GTK, no?

> Source/WebCore/platform/graphics/PlatformDisplay.cpp:51
>  #if PLATFORM(GTK)
> +#if PLATFORM(X11)
>  #include <gdk/gdkx.h>
>  #endif
> +#if PLATFORM(WAYLAND)
> +#include <gdk/gdkwayland.h>
> +#endif
> +#endif

I think the nested ifs makes this more difficult to read, what about something like?

#if PLATFORM(GTK) && PLATFORM(X11)
#include <gdk/gdkx.h>
#endif

#if PLATFORM(GTK) && PLATFORM(WAYLAND)
#include <gdk/gdkwayland.h>
#endif
Comment 3 Zan Dobersek 2015-05-14 09:39:50 PDT
Comment on attachment 253118 [details]
Patch

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

>> Source/WebCore/platform/graphics/GLContext.h:30
>> +#include <wayland-egl.h>
> 
> This is not specific to GTK, no?

Right, this shouldn't be. <EGL/eglplatform.h> should.

This also breaks EFL build. Oops.

>> Source/WebCore/platform/graphics/PlatformDisplay.cpp:51
>> +#endif
> 
> I think the nested ifs makes this more difficult to read, what about something like?
> 
> #if PLATFORM(GTK) && PLATFORM(X11)
> #include <gdk/gdkx.h>
> #endif
> 
> #if PLATFORM(GTK) && PLATFORM(WAYLAND)
> #include <gdk/gdkwayland.h>
> #endif

Ugh ... It does look nicer, but I'll ping you as soon as someone uploads a patch that folds the guarded #include statements back under one #if PLATFORM(GTK) guard.
Comment 4 Zan Dobersek 2015-05-14 09:51:26 PDT
Created attachment 253124 [details]
Patch