Bug 170959 – [GTK] Building Webkit2Gtk without OpenGL fails.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[John Ralls]

WebKitGtk 2.16.1

There's an option to turn it off in OptionsGTK.cmake, but Texture Mapping seems to require it (though by reverting 185805 one can disable that too), but...
../Source/WebKit2/WebProcess/WebPage/AcceleratedDrawingArea.cpp:49:24: error:
     member access into incomplete type 'WebKit::LayerTreeHost'
       m_layerTreeHost->invalidate();
and plenty more because in LayerTreeHost.h the class definition is guarded with
#if USE(COORDINATED_GRAPHICS) || USE(TEXTURE_MAPPER)

Some of AcceleratedDrawingArea's members are optional with that as well, but m_LayerTreeHost and m_previousLayerTreeHost aren't and there are a bunch of not-optional functions that would effectively be gutted if every use of those is made optional.

[John Ralls]

Created attachment 308015 [details]
Rearrange PlatformGTK.cmake so that OpenGL-dependent files are built only when ENABLE_OPENGL is true.

Preliminary patch that fixes the problem at least as far as building is concerned, but I'm not able to test that it actually works and I'm a bit concerned about that because I had to disable building a couple of files that appear to be the only concrete implementations of abstract classes.

[Carlos Alberto Lopez Perez]

Thanks for the patch.

Some comments:

* All patches need to have the proper Changelog entries
* Don't edit files unrelated to your patch (even if it is only removing a trailing space or extra line)
* When you think the patch is ready for review, please upload it with the flag r? (the tool webkit-patch upload will do that for you)

Check: https://webkit.org/contributing-code/

Very summarized the process for git users is the following:

1. Commit to a local git branch your patch.
2. Generate a changelog for it as follows:
$ Tools/Scripts/prepare-ChangeLog -b $bugnumer -g $sha1hashofthecommit
3. Edit the changelogs and commit them amending the previous commit
4. Upload the patch here using this:
$ Tools/Scripts/webkit-patch upload -g $sha1hashofthenewammendedcommit --suggest-reviewers --request-commit

[Michael Catanzaro]

(In reply to John Ralls from comment #1)
> Preliminary patch that fixes the problem at least as far as building is
> concerned, but I'm not able to test that it actually works and I'm a bit
> concerned about that because I had to disable building a couple of files
> that appear to be the only concrete implementations of abstract classes.

If that's the case, then go ahead and disable building the abstract classes too.

We'll make sure it doesn't break the ENABLE_OPENGL=ON case, because we test that: our EWS bot (scroll up) will turn red if that happens, and a bunch of people will complain. So no need to worry about accidentally breaking that. I expect you've ensured this fixes the ENABLE_OPENGL=OFF case.

[Michael Catanzaro]

Comment on attachment 308015 [details]
Rearrange PlatformGTK.cmake so that OpenGL-dependent files are built only when ENABLE_OPENGL is true.

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

> Source/WebCore/page/FrameView.cpp:106
> +#include <texmap/coordinated/TiledBackingStore.h>

Please don't do this, we only use <> style includes for headers from other projects. Instead please fix the include path. It looks like this directory needs to be in the include path for non-OpenGL builds.

> Source/WebCore/page/scrolling/ScrollingCoordinator.cpp:49
> +#include <coordinatedgraphics/ScrollingCoordinatorCoordinatedGraphics.h>

Ditto.

> Source/cmake/OptionsGTK.cmake:272
> +SET_AND_EXPOSE_TO_BUILD(USE_COORDINATED_GRAPHICS TRUE)

Hmm, this is concerning... I'd like another reviewer to say if this makes sense, please.

[John Ralls]

> we only use <> style includes for headers from other projects.

That's a silly and non-standard-compliant rule. The difference between #include<foo> and #include "foo" is that the latter searches the current compilation unit's directory before searching the include path. All you accomplish by using quotes instead of brackets for files in different directories is to slow down your build.

> Instead please fix the include path.

No thanks. That's how bug 167343 happened.

>> +SET_AND_EXPOSE_TO_BUILD(USE_COORDINATED_GRAPHICS TRUE)


> Hmm, this is concerning... I'd like another reviewer to say if this makes sense, please.

Yeah, concerning for me, too. But I lack the understanding of the code base--or of composting and GL, for that matter--necessary to be able to surgically separate the bits of the compositor that need GL from those that don't and to provide alternative methods where the only one currently available depends on GL.

> Check: https://webkit.org/contributing-code/

Yeah, I know. That's why I said "preliminary" and didn't set the review flag. This is a patch that works in the context of setting up a jhbuild tarball module but isn't ready for your code base; my intent was to show you the general scope of what's needed to build without GL. There are, BTW, 5 more patches for minor things like adjusting includes and macro-guarding function definitions to match the guards on the declarations in the header. I don't plan to open bugs and submit them because I don't think that fixing that stuff is a priority for you guys and I don't have the time needed to polish them up to make them acceptable for submission.

WebKit is too large a project and has too little test coverage (I tried to run `make check` and it aborted without running a single test because Xvfb isn't installed) for drive by patches. I don't have a couple of thousand hours lying around to learn your code base so I'm afraid that I've gone about as far as I can with this or anything else. I can get it to build, I'll include the rest of the patches in gtk-osx, and I've notified you of some problems. I completely understand that your focus is on supporting Gnome-on-Linux and that you don't have developer hours available to support other platforms.

[Carlos Garcia Campos]

Created attachment 309484 [details]
Patch

[Carlos Garcia Campos]

Committed r216483: <http://trac.webkit.org/changeset/216483>